-
Notifications
You must be signed in to change notification settings - Fork 313
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
Improve isolation in race control tests #1172
Improve isolation in race control tests #1172
Conversation
With this commit we rely on pytest fixtures for proper cleanup even when a test case fails. Previously, cleanup was only working when a test succeeded leading to unwanted side-effects between tests.
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.
Good catch. LGTM, left two suggestions.
tests/racecontrol_test.py
Outdated
with pytest.raises(exceptions.SystemSetupError) as ex: | ||
racecontrol.run(cfg) | ||
|
||
assert re.match(r"Unknown pipeline \[invalid]. List the available pipelines with [\S]+? list pipelines.", |
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.
We could also use the modern feature match feature of pytest
and simply:
with pytest.raises(
exceptions.SystemSetupError,
match=r"Unknown pipeline \[invalid]. List the available pipelines with [\S]+? list pipelines."):
racecontrol.run(cfg)
thus removing the need to use a named resource for the exception and a separate assert (and more importantly having to know which property of the exception object holds the message :) )
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.
Nice, thanks for the improvement! I've pushed this in dff0ee0.
tests/racecontrol_test.py
Outdated
racecontrol.run(cfg) | ||
|
||
mock_pipeline.assert_called_once_with(cfg) | ||
assert ex.value.message == "Only the [benchmark-only] pipeline is supported by the Rally Docker image.\n" \ |
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.
Same suggestion as above, for leveraging match, and using re.escape()
since this is a literal match i.e.
with pytest.raises(
exceptions.SystemSetupError,
match=re.escape(
"Only the [benchmark-only] pipeline is supported by the Rally Docker image.\n"
"Add --pipeline=benchmark-only in your Rally arguments and try again.\n"
"For more details read the docs for the benchmark-only pipeline in "
"https://esrally.readthedocs.io/en/latest/pipelines.html#benchmark-only\n"
)):
racecontrol.run(cfg)
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.
Nice, thanks for the improvement! I've pushed this in dff0ee0.
With this commit we rely on pytest fixtures for proper cleanup even when
a test case fails. Previously, cleanup was only working when a test
succeeded leading to unwanted side-effects between tests.