[Rollup] Replace RollupIT with a ESRestTestCase version #31977

polyfractal · 2018-07-11T19:58:51Z

The old RollupIT was a node IT, and flaky for a number of reasons. This new version is an ESRestTestCase and should be a little more robust. It shoould also be easier to add more varied tests in the future with this setup.

This was added to the multi-node QA tests as that seemed like the most appropriate location. It didn't seem necessary to create a whole new QA module.

Note: The only test that was ported was the "Big" test for validating a larger dataset. The rest of the tests are represented in existing yaml tests.

Closes #31258
Closes #30232
Related to #30290

The old RollupIT was a node IT, an flaky for a number of reasons. This new version is an ESRestTestCase and should be a little more robust. This was added to the multi-node QA tests as that seemed like the most appropriate location. It didn't seem necessary to create a whole new QA module. Note: The only test that was ported was the "Big" test for validating a larger dataset. The rest of the tests are represented in existing yaml tests. Closes elastic#31258 Closes elastic#30232 Related to elastic#30290

elasticmachine · 2018-07-11T19:58:53Z

Pinging @elastic/es-search-aggs

jimczi

Thanks @polyfractal ! I left some comments

jimczi · 2018-07-12T09:04:16Z

x-pack/qa/multi-node/src/test/java/org/elasticsearch/multi_node/RollupIT.java

+        Map<String, Object> getRollupJobResponse = toMap(client().performRequest(getRollupJobRequest));
+        Map<String, Object> job = getJob(getRollupJobResponse, rollupJob);
+        if (job != null) {
+            assertThat(ObjectPath.eval("status.job_state", job), expectedStates);


this fails the test compilation, should it be:
assertThat(ObjectPath.eval("status.job_state", job), equalTo(expectedStates));?

It fails compilation for you? Looks ok on my side... I pilfered this from FullClusterRestartIT tests that Tanguy added recently. expectedStates is defined above a few lines:

final Matcher<?> expectedStates = anyOf(equalTo("indexing"), equalTo("started"));

jimczi · 2018-07-12T09:04:23Z

x-pack/qa/multi-node/src/test/java/org/elasticsearch/multi_node/RollupIT.java

+        Map<String, Object> taskResponseNode = (Map<String, Object>) taskResponseNodes.values().iterator().next();
+        Map<String, Object> taskResponseTasks = (Map<String, Object>) taskResponseNode.get("tasks");
+        Map<String, Object> taskResponseStatus = (Map<String, Object>) taskResponseTasks.values().iterator().next();
+        assertThat(ObjectPath.eval("status.job_state", taskResponseStatus), expectedStates);


jimczi · 2018-07-12T09:04:33Z

x-pack/qa/multi-node/src/test/java/org/elasticsearch/multi_node/RollupIT.java

+                hasRollupTask = true;
+
+                final String jobStateField = "task.xpack/rollup/job.state.job_state";
+                assertThat("Expected field [" + jobStateField + "] to be started or indexing in " + task.get("id"),


and here ;)

jimczi · 2018-07-12T09:04:43Z

x-pack/qa/multi-node/src/test/java/org/elasticsearch/multi_node/RollupIT.java

+
+            Map<String, Object> job = getJob(getRollupJobResponse, rollupJob);
+            if (job != null) {
+                assertThat(ObjectPath.eval("status.job_state", job), expectedStates);


here as well ?

here expectedStates is passed in as a final Matcher<?> expectedStates

It fails the compilation:
method Assert.<T#1>assertThat(T#1,Matcher<? super T#1>) is not applicable
Can you check the states differently ? With an array of strings and anyOf ?

jimczi · 2018-07-12T09:05:57Z

x-pack/qa/multi-node/src/test/java/org/elasticsearch/multi_node/RollupIT.java

+            + "\"index_pattern\":\"rollup-*\","
+            + "\"rollup_index\":\"results-rollup\","
+            + "\"cron\":\"*/1 * * * * ?\","             // fast cron and big page size so test runs quickly
+            + "\"page_size\":200,"


We should test multiple pages, how slow is it if you set the page_size to 20 for instance ?

++ I dont think multiple pages will be bad. The major slowdown is waiting for the cron to tick over (e.g. if it's 30s and you get unlucky you may be waiting the full time).

polyfractal · 2018-07-12T16:53:17Z

Jenkins, run gradle build tests

jimczi

CI still fails due to a compilation error with assertThat. I left a comment where it happens but no need for another review, we just need a green CI.

jimczi · 2018-07-13T08:27:36Z

x-pack/qa/multi-node/src/test/java/org/elasticsearch/multi_node/RollupIT.java

+
+            Map<String, Object> job = getJob(getRollupJobResponse, rollupJob);
+            if (job != null) {
+                assertThat(ObjectPath.eval("status.job_state", job), expectedStates);


It fails the compilation:
method Assert.<T#1>assertThat(T#1,Matcher<? super T#1>) is not applicable
Can you check the states differently ? With an array of strings and anyOf ?

polyfractal · 2018-07-13T13:57:46Z

Just pushed a new derivation using isOneOf() with a string array. 🤞

No longer needed since the Rollup tests are all unit

…upit

The old RollupIT was a node IT, an flaky for a number of reasons. This new version is an ESRestTestCase and should be a little more robust. This was added to the multi-node QA tests as that seemed like the most appropriate location. It didn't seem necessary to create a whole new QA module. Note: The only test that was ported was the "Big" test for validating a larger dataset. The rest of the tests are represented in existing yaml tests. Closes #31258 Closes #30232 Related to #30290

* es/master: (21 commits) Tweaked Elasticsearch Service links for SEO Watcher: Store username on watch execution (#31873) Use correct formatting for links (#29460) Painless: Separate PainlessLookup into PainlessLookup and PainlessLookupBuilder (#32054) Scripting: Remove dead code from painless module (#32064) [Rollup] Replace RollupIT with a ESRestTestCase version (#31977) [TEST] Consistent algorithm usage (#32077) [Rollup] Fix duplicate field names in test (#32075) Ensure only parent breaker trips in unit test Unmute field collapsing rest tests Fix BWC check after backport [Tests] Fix failure due to changes exception message (#32036) Remove unused params from SSource and Walker (#31935) [Test] Mute MlJobIT#testDeleteJobAfterMissingAliases Turn off real-mem breaker in REST tests Turn off real-mem breaker in single node tests Fix broken OpenLDAP Vagrant QA test Cleanup Duplication in `PainlessScriptEngine` (#31991) SCRIPTING: Remove unused MultiSearchTemplateRequestBuilder (#32049) Fix compile issues introduced by merge (#32058) ...

* es/6.x: Use correct formatting for links (#29460) Revert "Adds a new auto-interval date histogram (#28993)" Revert "fix typo" fix typo Adds a new auto-interval date histogram (#28993) [Rollup] Replace RollupIT with a ESRestTestCase version (#31977) [Rollup] Fix duplicate field names in test (#32075) [Tests] Fix failure due to changes exception message (#32036) [Test] Mute MlJobIT#testDeleteJobAfterMissingAliases Replace Ingest ScriptContext with Custom Interface (#32003) (#32060) Cleanup Duplication in `PainlessScriptEngine` (#31991) (#32061) HLRC: Add xpack usage api (#31975) Clean Up Snapshot Create Rest API (#31779)

$polyfractal$

$@polyfractal$ polyfractal added >test Issues or PRs that are addressing/adding tests review :StorageEngine/Rollup Turn fine-grained time-based data into coarser-grained data labels Jul 11, 2018

$@polyfractal$ polyfractal requested a review from jimczi July 11, 2018 19:58

jimczi reviewed Jul 12, 2018

View reviewed changes

polyfractal added 2 commits July 12, 2018 13:02

$@polyfractal$

Reduce page size to 20

32ef176

$@polyfractal$

Hardcode the matchers to see if the build likes it better

bb5c529

jimczi approved these changes Jul 13, 2018

View reviewed changes

colings86 mentioned this pull request Jul 13, 2018

[CI] RollupIT#testBig seems flaky #32032

Closed

$@polyfractal$

Try to use isOneOf instead of anyOf(equals())

f7ff73b

polyfractal added 3 commits July 13, 2018 11:44

$@polyfractal$

remove internalClusterTest details from build file

e7c03fa

No longer needed since the Rollup tests are all unit

$@polyfractal$

Gradle tweak

0637040

$@polyfractal$

Merge remote-tracking branch 'origin/master' into rollup_replace_roll…

760355b

…upit

$@polyfractal$ polyfractal merged commit 59191b4 into elastic:master Jul 16, 2018

$@polyfractal$ polyfractal mentioned this pull request Jul 16, 2018

Fix rollup on date fields that don't support epoch_millis #31890

Merged

$@polyfractal$ polyfractal added the v7.0.0 label Jul 16, 2018

$@polyfractal$ polyfractal added the v6.4.0 label Jul 16, 2018

jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

talevy mentioned this pull request Feb 14, 2019

ArrayCompareConditionSearchTests thread leak suite failure #38875

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Rollup] Replace RollupIT with a ESRestTestCase version #31977

[Rollup] Replace RollupIT with a ESRestTestCase version #31977

$@polyfractal$ polyfractal commented Jul 11, 2018

elasticmachine commented Jul 11, 2018

jimczi left a comment

jimczi Jul 12, 2018

$@polyfractal$ polyfractal Jul 12, 2018

jimczi Jul 12, 2018

jimczi Jul 12, 2018

jimczi Jul 12, 2018

$@polyfractal$ polyfractal Jul 12, 2018

jimczi Jul 13, 2018

jimczi Jul 12, 2018

$@polyfractal$ polyfractal Jul 12, 2018

polyfractal commented Jul 12, 2018

jimczi left a comment

jimczi Jul 13, 2018

polyfractal commented Jul 13, 2018

[Rollup] Replace RollupIT with a ESRestTestCase version #31977

[Rollup] Replace RollupIT with a ESRestTestCase version #31977

Conversation

polyfractal commented Jul 11, 2018

elasticmachine commented Jul 11, 2018

jimczi 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

polyfractal commented Jul 12, 2018

jimczi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polyfractal commented Jul 13, 2018

$@polyfractal$ polyfractal commented Jul 11, 2018