-
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
Migrate more integration tests #966
Migrate more integration tests #966
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.
yay! one thing i would be fine with is using single quotes in those esrally invocation lines to protect us against any bash substitution. I assume they are all regular strings tho.
Example
"esrally \"$(some foo junk)\" "
vs
"esrally '$(some foo junk)' "
will be evalutated differently on the command line
@@ -27,6 +27,8 @@ | |||
from esrally.utils import process, io | |||
|
|||
CONFIG_NAMES = ["in-memory-it", "es-it"] | |||
DISTRIBUTIONS = ["2.4.6", "5.6.16", "6.8.0", "7.1.1"] |
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 this something that should be in the .ci
folder for externalization, vs in some python 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.
I don't think we should put this in .ci
as nothing except our integration test code is using this.
Thanks for your review!
I think we can address this separately but that's a good point. |
With this commit we migrate a few more integration tests from the shell script approach to the new approach based on
py.test
.