-
Notifications
You must be signed in to change notification settings - Fork 9
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
Exitcode skips #153
Exitcode skips #153
Conversation
…ode isn't equal to the desired exit code.
src/pytest_workflow/plugin.py
Outdated
@@ -443,20 +443,21 @@ def collect(self): | |||
# collection phase. | |||
workflow = self.queue_workflow() | |||
|
|||
workflow.desired_exit_code = self.workflow_test.exit_code |
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 better to do this in the self.queue_workflow function so that the desired exit code can be set upon class instantion rather than bolted on later.
src/pytest_workflow/workflow.py
Outdated
@@ -69,6 +69,7 @@ def __init__(self, | |||
self._started = False | |||
self.errors: List[Exception] = [] | |||
self.start_lock = threading.Lock() | |||
self.desired_exit_code = 0 |
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.
This value should be set trough the __init__
parameters as it should be user configurable.
tests/test_skip_tests.py
Outdated
""") | ||
|
||
|
||
def test_skips(pytester): | ||
pytester.makefile(".yml", test=SKIP_TESTS) | ||
result = pytester.runpytest("-v").parseoutcomes() | ||
assert {"failed": 4, "passed": 3, "skipped": 3} == result | ||
assert {"failed": 5, "passed": 3, "skipped": 7} == result |
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.
By having one combined result this makes the test prone to errors later. Two test units might fail in a way that the outcomes compensate for eachother. Also it makes it harder to adopt the test as you need to count all the units together to see what the expected numbers are. That is 15 tests to keep track of in your head. It is better to test the 3 units separately.
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.
Almost there. The tests you removed however test functionality that should remain tested. Please do not remove these tests.
Nicely removed the test dependency on grep! (This has some LOCALE issues, on my Dutch machine at home the messages are different.) |
Checklist