Do not exit if pytest version check fails#498
Conversation
Fixes failing CI builds for dashing, eloquent, and foxy. Signed-off-by: Jacob Perron <jacob@openrobotics.org>
|
@j-rivero can you comment on your intention here? If I had to guess I'd say that exit_on_error was added without realizing it would halt the build since what you're trying to capture is whether pytest >=6.0.0 is present not assert that it is. If that's the case then this PR looks like it exhibits the originally intended behavior. |
nuclearsandwich
left a comment
There was a problem hiding this comment.
Looks good to me but I'd like @j-rivero to confirm.
That was the intention originally, yes. Not sure why it was not catch during my initial tests in pull request #471. Thanks @jacobperron, @nuclearsandwich for the fix. |
| 'sys.exit(StrictVersion(pytest.__version__) >= StrictVersion("6.0.0"))' | ||
| "'"]) | ||
| "'"], | ||
| exit_on_error=False) |
There was a problem hiding this comment.
Using the return value for both the version comparison result as well as potential errors looks fragile to me. I would suggest to refactor this logic to:
- either print the version comparison result (and interpret the output) and ensure that the command has a return code of zero
- or just print
pytest.__version__and perform the comparison in the calling Python code rather than within the invoked command.
|
This patch does not work as it appears the result |
I'm testing a fix for the problem, based on Dirk's suggestion: master...shell_pytest6 |
Fixes failing CI builds for dashing, eloquent, and foxy.
Addresses #471 (comment)
Before:
After: