AbstractQueryTestCase should run without type less often #28936

jimczi · 2018-03-08T11:08:49Z

AbstractQueryTestCase is instanciated without mapping type one third
of the time. Since most of the tests are disabled when there is no type
this commit modifies the probability to use rarely() instead of a
fixed probability.

AbstractQueryTestCase is instanciated without mapping type one third of the time. Since most of the tests are disabled when there is no type this commit modifies the probability to use rarely() instead of a fixed probability.

cbuescher

I left a small suggestion, other than that I agree with this change. I would also port the part about the frequency to the 6.x branches, the part about randomTypes might not work because I think we should still test for multiple types there.

cbuescher · 2018-03-08T11:30:17Z

test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java

-        // Set a single type in the index
-        switch (random().nextInt(3)) {
-        case 0:
+        if (rarely()) {


cbuescher · 2018-03-08T11:33:10Z

test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java

            currentTypes = new String[0]; // no types
-            break;
-        default:
+        } else {
            currentTypes = new String[] { "_doc" };


If we only have one allowed type on master now, we could also look into making currentTypes a String, not an array. also changing it to "currentType" might be better then.

cbuescher · 2018-03-08T11:33:51Z

test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java

+            case 0:
+                return new String[0];
+            case 1:
+                return currentTypes;


If we change currentTypes to be a String constant, we could also use this here.

cbuescher · 2018-03-08T11:38:38Z

test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java

@@ -858,21 +854,17 @@ protected static String getRandomRewriteMethod() {
    }

    private static String[] getRandomTypes() {
-        String[] types;


I just saw this is also tagged with 6.3, I think this part of the change shouldn't be backported because in 6.x we should still test for multiple types (support for 5.x indices)

This commit changes the randomization to always create an index with a type. It also adds a way to create a query shard context that maps to an index with no type registered in order to explicitely test cases where there is no type.

jimczi · 2018-03-09T10:58:34Z

Thanks for looking @cbuescher . I've changed the pr to always create a type for the main index (_doc) and added a way to create a query shard context that maps to an index with no type. This way we can explicitly test cases where no type is registered. This allows to always test all methods defined in a concrete class and make the no-type test more explicit. I've also removed the v6.x tag since this should only goes to master. Can you take another look ?

javanna · 2018-03-13T09:59:52Z

heya @jimczi this is a good change, thanks! I was wondering, where do we use no types now? There were way too many "assumes" around that before, on the other hand I think that we introduced it in the first place to test that toQuery works ok also when no types are registered, or maybe @cbuescher remembers other reasons for that?

cbuescher · 2018-03-13T10:28:23Z

we introduced it in the first place to test that toQuery works ok also when no types are registered

No, I think that was the main reason. @javanna maybe time to get rid of this case on master altogether, at least from the existing tests I don't see a compelling reason to keep it (except for the special case in GeoBoundingBoxQueryBuilderTests), wdyt? I'm okay with removing it.

@jimczi I took a look at the last update, great to get rid of all the "assumes".

javanna · 2018-03-13T10:57:57Z

mmm, not sure about removing this completely, I think it proved useful in the past to find bugs when calling toQuery and helped increasing code coverage, on the other hand it is annoying that it requires so many exceptions. Maybe we should have a specific testToQuery run against the index with no types? But then we would still have to conditionally run it only for the queries that support it.

jimczi · 2018-03-19T09:45:22Z

Maybe we should have a specific testToQuery run against the index with no types? But then we would still have to conditionally run it only for the queries that support it.

I think it's reasonable, I prefer to add special cases for the rare queries that need it rather than adding this assumeTrue on every test. I'll update the pr shortly.

elasticmachine · 2018-06-25T13:38:45Z

Pinging @elastic/es-search-aggs

cbuescher · 2018-07-23T18:42:31Z

@jimczi just stumbled upon this while going through old PRs on my list. This looks reade after merge conflicts are resolved and would be good to get in I think.

jimczi · 2018-07-25T08:42:12Z

Thanks for the ping @cbuescher . I merged the conflicts and adapted the tests to cope with the new alias fields. I'll merge if the CI finishes without error.

jimczi · 2018-07-25T14:15:46Z

test this please

jimczi · 2018-07-26T12:20:37Z

test this please

@throws

* master: Remove reference to non-existent store type (#32418) [TEST] Mute failing FlushIT test Fix ordering of bootstrap checks in docs (#32417) [TEST] Mute failing InternalEngineTests#testSeqNoAndCheckpoints [TEST] Mute failing testConvertLongHexError bump lucene version after backport Upgrade to Lucene-7.5.0-snapshot-608f0277b0 (#32390) [Kerberos] Avoid vagrant update on precommit (#32416) TESTS: Move netty leak detection to paranoid level (#32354) [DOCS] Fixes formatting of scope object in job resource Copy missing segment attributes in getSegmentInfo (#32396) AbstractQueryTestCase should run without type less often (#28936) INGEST: Fix Deprecation Warning in Script Proc. (#32407) Switch x-pack/plugin to new style Requests (#32327) Docs: Correcting a typo in tophits (#32359) Build: Stop double generating buildSrc pom (#32408) TEST: Avoid triggering merges in FlushIT Fix missing JavaDoc for @throws in several places in KerberosTicketValidator. Switch x-pack full restart to new style Requests (#32294) Release requests in cors handler (#32364) Painless: Clean Up PainlessClass Variables (#32380) Docs: Fix callouts in put license HL REST docs (#32363) [ML] Consistent pattern for strict/lenient parser names (#32399) Update update-settings.asciidoc (#31378) Remove some dead code (#31993) Introduce index store plugins (#32375) Rank-Eval: Reduce scope of an unchecked supression Make sure _forcemerge respects `max_num_segments`. (#32291) TESTS: Fix Buf Leaks in HttpReadWriteHandlerTests (#32377) Only enforce password hashing check if FIPS enabled (#32383)

AbstractQueryTestCase should run without type less often

c2a0a09

AbstractQueryTestCase is instanciated without mapping type one third of the time. Since most of the tests are disabled when there is no type this commit modifies the probability to use rarely() instead of a fixed probability.

jimczi added >non-issue >test Issues or PRs that are addressing/adding tests v7.0.0 v6.3.0 labels Mar 8, 2018

jimczi requested a review from cbuescher March 8, 2018 11:08

cbuescher approved these changes Mar 8, 2018

View reviewed changes

cbuescher reviewed Mar 8, 2018

View reviewed changes

cbuescher self-assigned this Mar 8, 2018

Always create _doc type for the main index

cc8d1b7

This commit changes the randomization to always create an index with a type. It also adds a way to create a query shard context that maps to an index with no type registered in order to explicitely test cases where there is no type.

jimczi removed the v6.3.0 label Mar 9, 2018

colings86 added the :Search/Search Search-related issues that do not fall into other categories label Jun 25, 2018

jimczi added 2 commits July 25, 2018 09:11

Merge branch 'master' into abstract_query_case_with_type

f73e541

fix tests after merge

a5fc4f5

fix style checks

97529a6

Merge branch 'master' into abstract_query_case_with_type

f09ed59

jimczi merged commit 8e5f281 into elastic:master Jul 26, 2018

jimczi deleted the abstract_query_case_with_type branch July 26, 2018 18:29

colings86 added the v7.0.0-beta1 label Feb 7, 2019

colings86 removed the v7.0.0 label Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AbstractQueryTestCase should run without type less often #28936

AbstractQueryTestCase should run without type less often #28936

jimczi commented Mar 8, 2018

cbuescher left a comment

cbuescher Mar 8, 2018

cbuescher Mar 8, 2018

cbuescher Mar 8, 2018

cbuescher Mar 8, 2018

jimczi commented Mar 9, 2018

javanna commented Mar 13, 2018

cbuescher commented Mar 13, 2018

javanna commented Mar 13, 2018

jimczi commented Mar 19, 2018

elasticmachine commented Jun 25, 2018

cbuescher commented Jul 23, 2018

jimczi commented Jul 25, 2018

jimczi commented Jul 25, 2018

jimczi commented Jul 26, 2018

AbstractQueryTestCase should run without type less often #28936

AbstractQueryTestCase should run without type less often #28936

Conversation

jimczi commented Mar 8, 2018

cbuescher left a comment

Choose a reason for hiding this comment

cbuescher Mar 8, 2018

Choose a reason for hiding this comment

cbuescher Mar 8, 2018

Choose a reason for hiding this comment

cbuescher Mar 8, 2018

Choose a reason for hiding this comment

cbuescher Mar 8, 2018

Choose a reason for hiding this comment

jimczi commented Mar 9, 2018

javanna commented Mar 13, 2018

cbuescher commented Mar 13, 2018

javanna commented Mar 13, 2018

jimczi commented Mar 19, 2018

elasticmachine commented Jun 25, 2018

cbuescher commented Jul 23, 2018

jimczi commented Jul 25, 2018

jimczi commented Jul 25, 2018

jimczi commented Jul 26, 2018