Add support for switching distribution for all integration tests #30874

alpar-t · 2018-05-25T14:41:24Z

Allow using the tests.distribution property for telling integration tests which distribution to use.
If the property is not present, integration tests for plugins will use integ-test-zip, qa integration tests will default to oss-zip, and x-pack qa to zip.
When the property is present, integration tests will always honor it, meaning that integration tests will be skipped if they cannot run against the specific distribution. e.x. we won't run x-pack/qa with -Dtests.distribution=oss-zip because we can't honor that, ignoring the property and running them with zip would just duplicate the work.

TODO:

make use of tests.distribution for plugins to switch distribution these run against
add an info level log to mention which distribution was used when starting up the cluster
skip the x-pack/plugins/build.gradle ( top level ) integration tests if distribution is not zip
skip xpack-qa tests if tests.distribution and it's not zip
what happens if we pass integ-test-zip to QA ? Tests should probably be skipped.
test the whole build without tests.distribution and with each of zip, oss-zip
fix plugin and module tests so they work with different dsitributions

elasticmachine · 2018-05-25T14:41:26Z

Pinging @elastic/es-core-infra

alpar-t · 2018-05-25T14:42:36Z

@nik9000 @rjernst did I start down the right path ?

nik9000 · 2018-05-25T14:48:20Z

plugins/discovery-file/build.gradle

@@ -38,7 +38,7 @@ task setupSeedNodeAndUnicastHostsFile(type: DefaultTask) {
 // setup the initial cluster with one node that will serve as the seed node
 // for unicast discovery
 ClusterConfiguration config = new ClusterConfiguration(project)
-config.distribution = 'integ-test-zip'
+config.distribution = System.getProperty('tests.distribution', 'integ-test-zip')


I'm not sure we need this line at all given your change to PluginBuildPlugin.

This would take precedence and override that. So it would be set back to integ-test-zip regardless of the property.

I am concerned this is fragile. I don't want to see this logic copied around to other projects. Can you please create an issue for making creating additional cluster fixtures easier, so that the default can be set by PluginBuildPlugin for this project and reused the we create this cluster?

++ on this being fragile. Fixing it in a followup is fine with me though.

nik9000 · 2018-05-25T14:49:33Z

qa/smoke-test-rank-eval-with-mustache/build.gradle

@@ -24,10 +24,4 @@ apply plugin: 'elasticsearch.rest-test'
 dependencies {
    testCompile project(path: ':modules:rank-eval', configuration: 'runtime')
    testCompile project(path: ':modules:lang-mustache', configuration: 'runtime')
-}
-
-/*


++. This is closed. If you want to just push a commit that removes that comment to the repo without waiting on a PR that'd be awesome.

nik9000 · 2018-05-25T14:52:19Z

plugins/examples/painless-whitelist/build.gradle

@@ -30,8 +30,12 @@ dependencies {
  compileOnly project(':modules:lang-painless')
 }

-integTestCluster {
-  distribution = 'zip'
+if (System.getProperty('tests.distribution') == null) {


This seems a bit complicated to add to every project. I'm not 100% sure we need to support -Dtests.distribution=integ-test-zip. Maybe integ-test-zip is a thing we should only use when you don't override the property.

It'd be fairly simple to fail the build if someone sets distribution to integ-test-zip. If we did that then this project could do distribution = System.getProperty('tests.distribution', 'zip).

I might be cleaner to move the PluginBuildPlugin change to a gradle.projectsEvaluated and have it handle the logic all in one place.

setting distro integ-test-zip would offer a way to run some integration tests in a build that would be much quicker as it would skip a lot of the heavier integration tests (probably most of QA). I'm not sure how useful that would be during development, so can't decide if it's worth doing.

I don't think that is generally useful. Plugins and modules are the projects which use integ-test-zip, and you can already cd into the plugins or modules dir and run tests from there.

Yeah. I don't think we should support setting -Dtests.distribution=integ-test-zip. It should be what you get when you run without setting it.

alpar-t · 2018-05-25T15:01:24Z

@nik9000 just want to make sure that you looked at the todo list as well and share your thoughts before I start implementing

rjernst

I have some general comments

rjernst · 2018-05-25T15:55:16Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/test/ClusterFormationTasks.groovy

@@ -532,9 +532,6 @@ class ClusterFormationTasks {
    }

    static Task configureInstallModuleTask(String name, Project project, Task setup, NodeInfo node, Project module) {
-        if (node.config.distribution != 'integ-test-zip') {


If using a non integ-test-zip distribution, modules need to not be installed at all. This looks like it allows installing them over what the distribution installs, which could create craziness. I think we need to skip setting up install module tasks if the distribution is not integ-test-zip.

rjernst · 2018-05-25T15:58:05Z

plugins/examples/painless-whitelist/build.gradle

@@ -30,8 +30,12 @@ dependencies {
  compileOnly project(':modules:lang-painless')
 }

-integTestCluster {
-  distribution = 'zip'
+if (System.getProperty('tests.distribution') == null) {


I don't think that is generally useful. Plugins and modules are the projects which use integ-test-zip, and you can already cd into the plugins or modules dir and run tests from there.

rjernst · 2018-05-25T16:01:21Z

plugins/discovery-file/build.gradle

@@ -38,7 +38,7 @@ task setupSeedNodeAndUnicastHostsFile(type: DefaultTask) {
 // setup the initial cluster with one node that will serve as the seed node
 // for unicast discovery
 ClusterConfiguration config = new ClusterConfiguration(project)
-config.distribution = 'integ-test-zip'
+config.distribution = System.getProperty('tests.distribution', 'integ-test-zip')


I am concerned this is fragile. I don't want to see this logic copied around to other projects. Can you please create an issue for making creating additional cluster fixtures easier, so that the default can be set by PluginBuildPlugin for this project and reused the we create this cluster?

alpar-t · 2018-05-28T12:19:17Z

@nik9000 @rjernst I adressed hopefully all comments. opened #30903 and #30904.

alpar-t · 2018-05-28T19:22:40Z

Some tests seem to be written specifically for the integ test distro ./gradlew clean check -Dtests.jvms=6 -Dtests.distribution=zip -x :plugins:ingest-attachment:integTestRunner -x :plugins:ingest-geoip:integTestRunner -x :plugins:ingest-user-agent:integTestRunner -x :modules:lang-mustache:integTestRunner -x :modules:transport-netty4:test -x :modules:lang-painless:integTestRunner ( not a complete list )

Makes it possible to have a key that points to a list and assert that a certain object is present in the list. All keys have to be present and values have to match. The objects in the source list may have additional fields. example: ``` match: { 'nodes.$master.plugins': { name: ingest-attachment } } ```

Some of the tests expected that the integration tests will always be ran with the `integ-test-zip` distribution so that there will be no other plugins loaded. With this change, we check for the presence of the plugin without assuming exclusivity.

alpar-t · 2018-05-29T13:00:44Z

The following still fails:
./gradlew clean ':modules:transport-netty4:check' -Dtests.distribution=zip
but it only does so with the zip distribution and not others.

To match the behavior of tets.distributions

nik9000 · 2018-05-30T12:41:18Z

test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/MatchAssertion.java

+        if(expectedValue instanceof Map && actualValue instanceof List) {
+            Map<String, Object> expectedMap = (Map<String, Object>) expectedValue;
+            List<Object> actualList = (List<Object>) actualValue;
+            assertTrue(


I'm not sure we want match to be contains in some cases. If we want this we should make a new construct.

The semantics seemed to fit with me, but now that I think of it, match is in fact an exact match so it should be an exact match when dealing with lists as well. I'll add a new construct and call it contains.

Replaces the previus changes that caused `match` to do a partial match.

alpar-t · 2018-05-31T13:34:10Z

test this please

alpar-t · 2018-06-14T13:07:43Z

@nik9000 can you take another look please.

nik9000

I think it this is still a good thing to do. I've left a few requests to change things. Thanks for pinging me again on it!

nik9000 · 2018-06-20T15:05:57Z

modules/lang-expression/src/test/resources/rest-api-spec/test/lang_expression/10_basic.yml

@@ -1,4 +1,4 @@
-# Integration tests for Expression scripts
+# Integration tests for Expression scriptsmatch


Did you mean to add this?

no, I'll have to be more careful with my fat fingers :) thanks !

nik9000 · 2018-06-20T15:06:46Z

plugins/discovery-file/build.gradle

@@ -38,7 +38,7 @@ task setupSeedNodeAndUnicastHostsFile(type: DefaultTask) {
 // setup the initial cluster with one node that will serve as the seed node
 // for unicast discovery
 ClusterConfiguration config = new ClusterConfiguration(project)
-config.distribution = 'integ-test-zip'
+config.distribution = System.getProperty('tests.distribution', 'integ-test-zip')


++ on this being fragile. Fixing it in a followup is fine with me though.

nik9000 · 2018-06-20T15:08:07Z

plugins/examples/painless-whitelist/build.gradle

+  integTestCluster.distribution = 'oss-zip'
+}
+
+if (integTestCluster.distribution == 'integ-test-zip') {


I think we can remove this line because we don't support setting the setting to this.

We can add something to cluster formation tasks to make sure we don't set the system property to integ-test-zip.

nik9000 · 2018-06-20T15:09:54Z

qa/build.gradle

@@ -5,6 +5,9 @@ subprojects { Project subproj ->
  subproj.tasks.withType(RestIntegTestTask) {
    subproj.extensions.configure("${it.name}Cluster") { cluster ->
      cluster.distribution = System.getProperty('tests.distribution', 'oss-zip')
+      if (cluster.distribution == 'integ-test-zip') {


I think we're better off checking if the system property is integ-test-zip in the cluster formation tasks and failing there. Basically, the only way you can get integ-test-zip is to not set the property.

nik9000 · 2018-06-20T15:40:45Z

test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/ContainsAssertion.java

+            Map<String, Object> expectedMap = (Map<String, Object>) expectedValue;
+            List<Object> actualList = (List<Object>) actualValue;
+            List<Map<String, Object>> actualValues = actualList.stream()
+                .filter(each -> each instanceof Map)


I think we should fail the assertion if the thing we're testing isn't a map.

As implemented now, we can check that a list has an object that has all the keys at the specific values. In theory we could have a list having a mix of lists and objects. The filter only rules out anything that's not a map at this point when looking for actualValues. The subsequent check on actual values not empty would fail the assertion in case of a list of lists for example.
The only case we don't fail if we have a list that has both a list and an object that has the keys we are interested in, but I don't think we should in this case, because we did find that the list contains what we are looking for.

I see! Got it.

nik9000 · 2018-06-20T15:42:18Z

test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/MatchAssertion.java

 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertTrue;


These look like leftovers.

nik9000 · 2018-06-20T15:42:39Z

x-pack/plugin/src/test/resources/rest-api-spec/test/xpack/10_basic.yml

@@ -1,4 +1,4 @@
-# Integration tests for monitoring
+# Integration tests for monitoring.23.


nik9000 · 2018-06-20T15:42:53Z

x-pack/plugin/src/test/resources/rest-api-spec/test/xpack/10_basic.yml

-    - match:  { nodes.$master.modules.21.name: x-pack-sql  }
-    - match:  { nodes.$master.modules.22.name: x-pack-upgrade  }
-    - match:  { nodes.$master.modules.23.name: x-pack-watcher  }
+    - contains:  { nodes.$master.modules: { name: x-pack-core } }


This seems nice!

nik9000

Thanks for working with me on this one! LGTM.

nik9000 · 2018-06-26T13:22:26Z

test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/ContainsAssertion.java

+            Map<String, Object> expectedMap = (Map<String, Object>) expectedValue;
+            List<Object> actualList = (List<Object>) actualValue;
+            List<Map<String, Object>> actualValues = actualList.stream()
+                .filter(each -> each instanceof Map)


I see! Got it.

* master: ingest: Add ignore_missing property to foreach filter (#22147) (#31578) Fix a formatting issue in the docvalue_fields documentation. (#31563) reduce log level at gradle configuration time [TEST] Close additional clients created while running yaml tests (#31575) Docs: Clarify sensitive fields watcher encryption (#31551) Watcher: Remove never executed code (#31135) Add support for switching distribution for all integration tests (#30874) Improve robustness of geo shape parser for malformed shapes (#31449) QA: Create xpack yaml features (#31403) Improve test times for tests using `RandomObjects::addFields` (#31556) [Test] Add full cluster restart test for Rollup (#31533) Enhance thread context uniqueness assertion [DOCS] Fix heading format errors (#31483) fix writeIndex evaluation for aliases (#31562) Add x-opaque-id to search slow logs (#31539) Watcher: Fix put watch action (#31524) Add package pre-install check for java binary (#31343) Reduce number of raw types warnings (#31523) Migrate scripted metric aggregation scripts to ScriptContext design (#30111) turn GetFieldMappingsResponse to ToXContentObject (#31544) Close xcontent parsers (partial) (#31513) Ingest Attachment: Upgrade Tika to 1.18 (#31252) TEST: Correct the assertion arguments order (#31540)

* elastic/master: (57 commits) HLRest: Fix test for explain API [TEST] Fix RemoteClusterConnectionTests Add Create Snapshot to High-Level Rest Client (elastic#31215) Remove legacy MetaDataStateFormat (elastic#31603) Add explain API to high-level REST client (elastic#31387) Preserve thread context when connecting to remote cluster (elastic#31574) Unify headers for full text queries Remove redundant 'minimum_should_match' JDBC driver prepared statement set* methods (elastic#31494) [TEST] call yaml client close method from test suite (elastic#31591) ingest: Add ignore_missing property to foreach filter (elastic#22147) (elastic#31578) Fix a formatting issue in the docvalue_fields documentation. (elastic#31563) reduce log level at gradle configuration time [TEST] Close additional clients created while running yaml tests (elastic#31575) Docs: Clarify sensitive fields watcher encryption (elastic#31551) Watcher: Remove never executed code (elastic#31135) Add support for switching distribution for all integration tests (elastic#30874) Improve robustness of geo shape parser for malformed shapes (elastic#31449) QA: Create xpack yaml features (elastic#31403) Improve test times for tests using `RandomObjects::addFields` (elastic#31556) ...

Makes it possible to have a key that points to a list and assert that a certain object is present in the list. All keys have to be present and values have to match. The objects in the source list may have additional fields. example: ``` contains: { 'nodes.$master.plugins': { name: ingest-attachment } } ``` Backported from elastic#30874 (e8b8d11) Co-authored-by: Alpar Torok <torokalpar@gmail.com>

The contains syntax was added in elastic#30874 but the skips were not properly put in place. The java runner has the feature so the tests will run as part of the build, but language clients will be able to support it at their own pace.

The contains syntax was added in #30874 but the skips were not properly put in place. The java runner has the feature so the tests will run as part of the build, but language clients will be able to support it at their own pace.

Makes it possible to have a key that points to a list and assert that a certain object is present in the list. All keys have to be present and values have to match. The objects in the source list may have additional fields. example: ``` contains: { 'nodes.$master.plugins': { name: ingest-attachment } } ``` Backported from #30874 (e8b8d11) Co-authored-by: Alpar Torok <torokalpar@gmail.com>

alpar-t added 2 commits May 25, 2018 16:22

remove left-over comment

ed0f95c

make sure of the property for plugins

677cce5

alpar-t added >non-issue :Delivery/Build Build or test infrastructure labels May 25, 2018

alpar-t added the WIP label May 25, 2018

nik9000 reviewed May 25, 2018

View reviewed changes

rjernst reviewed May 25, 2018

View reviewed changes

alpar-t added 4 commits May 28, 2018 12:15

skip installing modules if these exist in the distribution

73fa278

Log the distrbution being ran

3364a43

Don't allow running with integ-tests-zip passed externally

9e457bd

top level x-pack/qa can't run with oss distro

fef4e22

alpar-t mentioned this pull request May 28, 2018

Provide a way for individual projects to specify which distributions they can run/test against #30903

Closed

alpar-t removed the WIP label May 28, 2018

Merge branch 'master' into test/distributions

e9579aa

alpar-t mentioned this pull request May 28, 2018

Provide a better way to declare nodes/clusters/cluster formation during the build #30904

Closed

7 tasks

alpar-t added 2 commits May 29, 2018 13:28

Allow modules to run on other distros as well

2b4d2ca

To match the behavior of tets.distributions

nik9000 requested changes May 30, 2018

View reviewed changes

alpar-t added 2 commits May 31, 2018 10:33

Add and use a new contains assertion

8631413

Replaces the previus changes that caused `match` to do a partial match.

Merge commit '8e4ab82' into test/distributions

7ba9499

Merge commit 'fcf1e41' into test/distributions

e0fa121

Merge remote-tracking branch 'origin/master' into test/distributions

cc454ec

nik9000 requested changes Jun 20, 2018

View reviewed changes

alpar-t added 2 commits June 22, 2018 15:14

Merge commit '86423f9' into test/distributions

b9faa69

Implement PR review comments

80ffc38

nik9000 approved these changes Jun 26, 2018

View reviewed changes

alpar-t merged commit 08b8d11 into elastic:master Jun 26, 2018

alpar-t deleted the test/distributions branch June 26, 2018 13:50

tvernum mentioned this pull request Oct 22, 2018

Rest Tests - Support for matching objects in lists #34693

Merged

alpar-t mentioned this pull request Oct 23, 2018

Add a new "contains" feature to yml test #34738

Merged

mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020

		@@ -1,4 +1,4 @@
		# Integration tests for Expression scripts
		# Integration tests for Expression scriptsmatch

		@@ -1,4 +1,4 @@
		# Integration tests for monitoring
		# Integration tests for monitoring.23.

Add support for switching distribution for all integration tests #30874

Add support for switching distribution for all integration tests #30874

Conversation

alpar-t commented May 25, 2018 • edited Loading

elasticmachine commented May 25, 2018

alpar-t commented May 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpar-t May 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpar-t commented May 25, 2018

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpar-t commented May 28, 2018

alpar-t commented May 28, 2018

alpar-t commented May 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpar-t commented May 31, 2018

alpar-t commented Jun 14, 2018

nik9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpar-t commented May 25, 2018 •

edited

Loading

alpar-t May 25, 2018 •

edited

Loading