-
Notifications
You must be signed in to change notification settings - Fork 308
Allow test case filtering and other unittest arguments #3653
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some files could not be reviewed due to errors:
./test/easyblocks/easyblock_specific.py:363:18: B008: Do not perform function...
./test/easyblocks/easyblock_specific.py:363:18: B008: Do not perform function calls in argument defaults. The call is performed only once at function definition time. All calls to your function will reuse the result of that definition-time function call. If this is intended, assign the function call to a module-level variable and use that variable as a default value.
./test/easyblocks/general.py:184:18: B008: Do not perform function calls in a...
./test/easyblocks/general.py:184:18: B008: Do not perform function calls in argument defaults. The call is performed only once at function definition time. All calls to your function will reuse the result of that definition-time function call. If this is intended, assign the function call to a module-level variable and use that variable as a default value.
./test/easyblocks/init_easyblocks.py:208:18: B008: Do not perform function ca...
./test/easyblocks/init_easyblocks.py:208:18: B008: Do not perform function calls in argument defaults. The call is performed only once at function definition time. All calls to your function will reuse the result of that definition-time function call. If this is intended, assign the function call to a module-level variable and use that variable as a default value.
|
Can this be merged @boegel ? Working on a test issue in a specific test but can only run all tests without this |
The test suite always runs all tests without a way to filter. Use the unittest loader to handle all unittest arguments, especially `-k testpattern`
|
@Crivella Could you take a look at this? This would be really useful for developing and debugging specific tests |
| if not res.wasSuccessful(): | ||
| sys.stderr.write("ERROR: Not all tests were successful.\n") | ||
| print("Log available at %s" % log_fn) | ||
| sys.exit(2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason that the behavior of sys.exit(2) is not kept in the changes?
I think without this the test command will return 1 on failure, not sure if this matters for anything
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is so that unittest can to the reporting and handling based on the returned test result. But yes it might change the exit code from 2 to:
By default main calls sys.exit() with an exit code indicating success (0) or failure (1) of the tests run. An exit code of 5 indicates that no tests were run or skipped.
I don't think it matters though
|
I think the This might be dependent on how framework, not sure if this would be the preferred approach or if we should use test filtering in the same way it is being done in framework |
|
It isn't just test case filtering but also all other options Also the unittest test filtering is much more powerful as it allows fine grained selection/deselection. I have a similar PR open for framework: easybuilders/easybuild-framework#3790 Additionally options like Parsing cmdline options as arguments to EasyBuild is IMO a bug that should be fixed. Currently no options were supported so this wasn't causing issues yet as no arguments were ever passed. I added a commit that disables that and |
Crivella
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
- Test behavior in CI is exactly the same
- CI test command also works with checked out PR branch locally
- Have not seen anywhere where the value of the return code is actually being checked beyond not being zero (so allowing
unittest.mainto callsys.exitshould not be a problem)
|
Going in, thanks @Flamefire! |
The test suite always runs all tests without a way to filter. Use the unittest loader to handle all unittest arguments, especially
-k testpattern