Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

t: reorganize t1007-flux-account.t into multiple sharness tests #367

Merged
merged 5 commits into from
Jul 17, 2023

Conversation

cmoussa1
Copy link
Member

Problem

t1007-flux-account.t has grown very large because it includes tests for almost every flux account command. As a result, it is unorganized and hard to read (my fault!!).


This PR reorganizes the sharness test file and creates multiple test files according to the category of flux account commands it deals with. So, t1007-flux-account.t becomes the test file that's responsible for testing all user-related commands; it is now t1007-flux-account-users.t. New test files are created for the bank, queue, and project commands. There is also a new file that tests the permissions for the flux-accounting commands that should only be available to administrators.

This reorganization allows for an easier time to add/edit tests for the suite of flux account commands if necessary.

@cmoussa1 cmoussa1 added improvement Upgrades to an already existing feature testing issues that deal with testing labels Jul 17, 2023
@cmoussa1 cmoussa1 requested a review from ryanday36 July 17, 2023 20:11
flux account add-bank --parent-bank=root B 1 &&
flux account add-bank --parent-bank=root C 1 &&
flux account add-bank --parent-bank=root D 1 &&
flux account add-bank --parent-bank=D E 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should there be a '&&' at the end of this line?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, my understanding of the && is to signal that there are additional commands to be run on the next line, so there isn't a need to put the && at the end of the last command in the test.

Copy link
Member

@chu11 chu11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than some nits, nothing huge.

I thought I'd comment that the way that you split up the commits does have a minor side effect. Basically after the first commit, technically you have "lost tests" in the commit history. i.e. for a small moment in commit history, these tests disappeared. Then in the middle few commits you added new test files but didn't add them to the Makefile. So for a small moment in history the re-added tests probably won't run when running make check.

If you were hunting down some bug that was introduced awhile back, this could be a nuisance. i.e. make check would not run tests that were there before.

I might suggest a commit history like:

t: split bank tests out of t1007.... (add this new file to Makefile.am)
t: split queue tests out of t1007... (and add new file to Makefile.am)
...
t: rename t1007 to ...

that way the history is never lost momentarily and make check works on each commit.

It's not a huge deal, so I leave it to you if you'd like to do.

@@ -0,0 +1,114 @@
#!/bin/bash

test_description='Test flux-account commands'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

user commands?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, all of these test descriptions should be updated/made clearer. Thanks for pointing that out!

@@ -0,0 +1,128 @@
#!/bin/bash

test_description='Test flux-account commands'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bank? seems needs update in all new files :-)

@@ -0,0 +1,136 @@
#!/bin/bash

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commit message, why mv and not move like other messages? NBD, thought maybe a typo.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used mv here to fit the commit title in the (is it 50?) character limit for the title. I can update the titles for these commands so that they are all the same and fit in the character limit. Sorry if that caused any confusion!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh the 50 char limit, it's fine then ... i just thought maybe you typoed it

flux account add-bank --parent-bank=root B 1 &&
flux account add-bank --parent-bank=root C 1 &&
flux account add-bank --parent-bank=root D 1 &&
flux account add-bank --parent-bank=D E 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as last file, should this line end in '&&'?

Copy link
Contributor

@ryanday36 ryanday36 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me overall. There were a few lines that I think should have ended in '&&', but maybe not. Feel free to ignore those comments if I'm wrong.

My only other comment would be to update the 'test_description' line in the various files to describe the specific tests in the file (e.g. 'Test flux-account bank commands' rather than 'Test flux-account commands' in 'f1023-flux-account-banks.t').

flux account add-bank --parent-bank=root B 1 &&
flux account add-bank --parent-bank=root C 1 &&
flux account add-bank --parent-bank=root D 1 &&
flux account add-bank --parent-bank=D E 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

&&

'

test_expect_success 'add a queue with no optional args to the queue_table' '
flux account add-queue queue_1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

&&

flux account add-bank --parent-bank=root B 1 &&
flux account add-bank --parent-bank=root C 1 &&
flux account add-bank --parent-bank=root D 1 &&
flux account add-bank --parent-bank=D E 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

&&

cmoussa1 added 3 commits July 17, 2023 13:54
Problem: t1007-flux-account.t has grown very large because it includes
tests for almost every flux account command. As a result, it is somewhat
unorganized and hard to read.

Move the flux account tests that deal with banks to its own sharness
test file.

Edit one of the expected output files to match the commands being run in
t1023-flux-account-banks.t (i.e the users in this test don't have queues
added to their accounts, so remove the queue lists from the expected
output file).
Problem: t1007-flux-account.t has grown very large because it includes
tests for almost every flux account command. As a result, it is somewhat
unorganized and hard to read.

Move the flux account tests that deal with queues to its own sharness
test file.
Problem: t1007-flux-account.t has grown very large because it includes
tests for almost every flux account command. As a result, it is somewhat
unorganized and hard to read.

Move the flux account tests that deal with projects to its own sharness
test file.
cmoussa1 added 2 commits July 17, 2023 14:01
Problem: t1007-flux-account.t has grown very large because it includes
tests for almost every flux account command. As a result, it is somewhat
unorganized and hard to read.

Move the flux account tests that deal with permissions to its own
sharness test file.
Problem: t1007-flux-account.t has grown very large because it includes
tests for almost every flux account command. As a result, it is somewhat
unorganized and hard to read.

Remove the tests in t1007-flux-account.t that don't deal with user
accounts directly so that they can be placed in separate test files.

Rename t1007-flux-account.t to t1007-flux-account-users.t.
@codecov
Copy link

codecov bot commented Jul 17, 2023

Codecov Report

Merging #367 (f9418b2) into master (5aa89e6) will not change coverage.
The diff coverage is n/a.

❗ Current head f9418b2 differs from pull request most recent head 5480a1b. Consider uploading reports for the commit 5480a1b to get more accurate results

@@           Coverage Diff           @@
##           master     #367   +/-   ##
=======================================
  Coverage   83.73%   83.73%           
=======================================
  Files          23       23           
  Lines        1236     1236           
=======================================
  Hits         1035     1035           
  Misses        201      201           

@cmoussa1
Copy link
Member Author

OK, I've pushed up some changes:

  • I changed the commit structure so that the tests are split and the new test file is added to t/Makefile.am in the same commit instead of adding all of the new test files to Makefile.am at the very end. I also moved the commit that changes the original t1007-flux-account.t file to the very end of the commit stack.
  • I changed the test descriptions in each new test file to hopefully make it clearer what each test file is doing.

Thanks for the approval @chu11 and @ryanday36. I can set MWP on this shortly unless you have any other suggestions!! :)

@mergify mergify bot merged commit 3bc255f into flux-framework:master Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Upgrades to an already existing feature merge-when-passing testing issues that deal with testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants