-
Notifications
You must be signed in to change notification settings - Fork 314
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 configure test to new IT infrastructure #1023
Conversation
This commit moves the test_configure which validates the configure command completes properly to the new infrastructure. Relates elastic#975
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.
Thanks for marching on with this task!
I left a few comments.
it/configuration_test.py
Outdated
@it.rally_in_mem | ||
def test_configure(cfg): | ||
# just run to test the configuration procedure, don't use this configuration in other tests. | ||
it.esrally_command_line_for(cfg, "configure --assume-defaults --configuration-name='config-integration-test'") |
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.
Looking at esrally_command_line_for()
:
Line 71 in 12d7fc3
return f"esrally {command_line} --kill-running-processes --configuration-name='{cfg}'" |
Seems to be the above command will end up specifying --configuration-name=
twice and additionally it doesn't test anything, just receiving a command line string.
I think what we want to do is validate that a new config works so we should assert the return value is 0.
Additionally we'll need to remove any test config.
I came up with the following working example:
$ git diff
diff --git a/it/__init__.py b/it/__init__.py
index 3ce0a7b..efc928f 100644
--- a/it/__init__.py
+++ b/it/__init__.py
@@ -198,8 +198,9 @@ def install_integration_test_config():
copy_config(n)
-def remove_integration_test_config():
- for n in CONFIG_NAMES:
+def remove_integration_test_config(config_names=None):
+ config_names = config_names or CONFIG_NAMES
+ for n in config_names:
os.remove(ConfigFile(n).target_path)
diff --git a/it/configuration_test.py b/it/configuration_test.py
index 6a5f8f5..089ae4b 100644
--- a/it/configuration_test.py
+++ b/it/configuration_test.py
@@ -17,8 +17,16 @@
import it
+CFG_FILE = "rally-config-integration-test"
+
+
+class TestConfigure:
+ def test_configure(self):
+ # just run to test the configuration procedure, don't use this configuration in other tests.
+ assert it.esrally(CFG_FILE, "configure --assume-defaults") == 0
+
+ @classmethod
+ def teardown_class(cls):
+ it.remove_integration_test_config([CFG_FILE])
+
[email protected]_in_mem
-def test_configure(cfg):
- # just run to test the configuration procedure, don't use this configuration in other tests.
- it.esrally_command_line_for(cfg, "configure --assume-defaults --configuration-name='config-integration-test'")
@@ -250,12 +250,6 @@ function random_build_type { | |||
eval "$1='${BUILD_TYPES[$((RANDOM%num_build_types))]}'" | |||
} | |||
|
|||
function test_configure { |
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.
Shouldn't we also remove the invocation of this function from
Lines 418 to 419 in 12d7fc3
echo "**************************************** TESTING CONFIGURATION OF RALLY ****************************************" | |
test_configure |
make it
locally failed (but curious why it didn't fail in CI 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.
It failed for me as well. It is surely something environmental. I will look into it but I dont think that this commit has caused the instability.
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 thanks
This commit moves the test_configure which validates the configure
command completes properly to the new infrastructure.
Relates #975