Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Migrate configure test to new IT infrastructure #1023

Merged
merged 4 commits into from
Jul 2, 2020
Merged

Migrate configure test to new IT infrastructure #1023

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
11d8cfc
Migrate configure test to new IT infrastructure
hub-cap Jun 29, 2020
cce8d1f
Fixing test to actually be useful
hub-cap Jul 1, 2020
67ecdc3
Removing test configure from the it test
hub-cap Jul 1, 2020
5cdfe73
One final pass to fix the dup config
hub-cap Jul 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions integration-test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,12 +250,6 @@ function random_build_type {
eval "$1='${BUILD_TYPES[$((RANDOM%num_build_types))]}'"
}

function test_configure {
Copy link
Contributor

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

rally/integration-test.sh

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).

Copy link
Contributor Author

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.

info "test configure()"
# just run to test the configuration procedure, don't use this configuration in other tests.
esrally configure --assume-defaults --configuration-name="config-integration-test"
}

function test_proxy_connection {
local cfg

Expand Down
24 changes: 24 additions & 0 deletions it/configuration_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# Licensed to Elasticsearch B.V. under one or more contributor
# license agreements. See the NOTICE file distributed with
# this work for additional information regarding copyright
# ownership. Elasticsearch B.V. licenses this file to you under
# the Apache License, Version 2.0 (the "License"); you may
# not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing,
# software distributed under the License is distributed on an
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.

import it


@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'")
Copy link
Contributor

@dliappis dliappis Jun 30, 2020 •
edited
Loading

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():

rally/it/__init__.py

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])
+

-@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'")