XContentTests : insert random fields at random positions #30867

olcbean · 2018-05-25T12:36:46Z

Currently AbstractXContentTestCase#testFromXContent appends random fields. This PR shuffled all fields after the random fields have been appended, hence the random fields are actually added to random positions.

elasticmachine · 2018-05-25T15:15:10Z

Pinging @elastic/es-search-aggs

javanna · 2018-05-28T09:48:26Z

thanks for the PR @olcbean I never realized that we would be adding random fields always in the same position. Out of curiosity, did you encounter problems with this? Would you mind adding a test to XContentTestUtilsTests?

javanna · 2018-05-28T09:49:45Z

sorry, adding a test where I suggested won't cut it as the change is in AbstractXContentTestCase :) I wonder if there is a way to test this though to make sure that this behaviour is maintained over time.

olcbean · 2018-05-30T12:58:14Z

I did not encounter a problem, just some confusion that an element is always 'appended'. So I decided to open a PR and see if it was intended ;)
Tomorrow I should have some time to add a test.

javanna · 2018-06-21T11:55:22Z

what do you think about this one @cbuescher ?

cbuescher

@olcbean @javanna looks good to me in general, I took the libery of merging in master locally which I will push to the branch to resolve the current merge conflicts.
I want to make sure we run this with some higher number of repetition before merging it though so we can see if we uncover some previously undetected issues. I will do so locally first, maybe we also want to increase AbstractXContentTestCase#NUMBER_OF_TEST_RUNS on CI once to see if/what breaks.

Conflicts: test/framework/src/main/java/org/elasticsearch/test/AbstractXContentTestCase.java

cbuescher · 2018-06-21T13:48:21Z

Local testing hit some potential problem. I can reproduce this:

2> REPRODUCE WITH: ./gradlew :server:test -Dtests.seed=58C3B75CE8CB49CA -Dtests.class=org.elasticsearch.cluster.health.ClusterIndexHealthTests -Dtests.method="testFromXContent" -Dtests.security.manager=true -Dtests.locale=hy -Dtests.timezone=America/Bogota
FAILURE 0.24s | ClusterIndexHealthTests.testFromXContent <<< FAILURES!
   > Throwable #1: java.lang.AssertionError: expected:<ClusterIndexHealth{index='jjblZrf', numberOfShards=329, numberOfReplicas=44, activeShards=31, relocatingShards=673, initializingShards=914, unassignedShards=143, activePrimaryShards=564, status=YELLOW, shards.size=1}> but was:<ClusterIndexHealth{index='jjblZrf', numberOfShards=329, numberOfReplicas=44, activeShards=31, relocatingShards=673, initializingShards=914, unassignedShards=143, activePrimaryShards=564, status=YELLOW, shards.size=0}>
   >    at __randomizedtesting.SeedInfo.seed([58C3B75CE8CB49CA:79AB8244616CA910]:0)
   >    at org.elasticsearch.test.AbstractWireTestCase.assertEqualInstances(AbstractWireTestCase.java:90)
   >    at org.elasticsearch.test.AbstractSerializingTestCase.lambda$testFromXContent$0(AbstractSerializingTestCase.java:39)
   >    at org.elasticsearch.test.AbstractXContentTestCase.testFromXContent(AbstractXContentTestCase.java:69)
   >    at org.elasticsearch.test.AbstractSerializingTestCase.testFromXContent(AbstractSerializingTestCase.java:37)
   >    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
   >    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
   >    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
   >    at java.base/java.lang.reflect.Method.invoke(Method.java:564)
   >    at java.base/java.lang.Thread.run(Thread.java:844)

I assume we have some issues in ClusterIndexHealthTests that this new shuffling uncovers. Will take a quick look, but in any case this shows we should run this extensively locally and in CI before merging.

cbuescher · 2018-06-21T13:48:33Z

Sorry, wrong button.

cbuescher · 2018-06-21T14:22:26Z

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

@@ -54,21 +54,21 @@
        for (int runs = 0; runs < numberOfTestRuns; runs++) {
            T testInstance = instanceSupplier.get();
            XContentType xContentType = randomFrom(XContentType.values());
-            BytesReference shuffled = toShuffledXContent(testInstance, xContentType, toXContentParams,false,
-                createParserFunction, shuffleFieldsExceptions);
+            BytesReference xContent = XContentHelper.toXContent(testInstance, xContentType, ToXContent.EMPTY_PARAMS, false);


The error mentioned is most likely cause by not using the "toXContentParams" here as before mit using the EMPTY_PARAMS. I think this needs to be changed.

cbuescher · 2018-06-21T14:23:26Z

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

            T parsed = parseFunction.apply(parser);
            assertEqualsConsumer.accept(testInstance, parsed);
            if (assertToXContentEquivalence) {
-                assertToXContentEquivalent(shuffled, XContentHelper.toXContent(parsed, xContentType, toXContentParams, false),
-                    xContentType);
+                assertToXContentEquivalent(xContent, XContentHelper.toXContent(parsed, xContentType, false), xContentType);


I think the toXContentParams also need to be re-added here.

cbuescher · 2018-06-21T14:26:08Z

@olcbean the error I mentioned above seems to be fixed with the two changes I suggested.

cbuescher · 2018-06-21T15:47:37Z

I gave this a bit of testing locally with the changes mentioned above and didn't run into any test failures again yet.

I wonder if there is a way to test this though

@javanna I wonder if a test for this change is really necessary. Of cource one could test AbstractXContentTestCase#testFromXContent, but where would that even go? AbstractXContentTestCaseTests? Sounds a bit meta to me to be honest. I know we test hour test infra sometimes, but in this case I don't think it adds much tbh.

javanna · 2018-06-21T16:17:14Z

@cbuescher the test is useful to guarantee that this behaviour is maintained in the future or consciously modified. Otherwise we may go and change this, even without realizing, and have no tests fail.

cbuescher · 2018-06-21T16:24:22Z

I wasn't saying its not useful, I was just wondering how far we want to go with testing out test infra.

rather than at the last position ( as till now )

olcbean · 2018-06-21T21:23:45Z

I am not really sure where to add the test of the test ;) I kept on changing between the AbstractXContentTestCase and the XContentTestUtils, and somehow I end up with the same pros and cons for either ... Any ideas are welcomed ( especially the one to remove the test all together ;) )

cbuescher

@olcbean thanks for adding the test anyway, I took a look and left some concerns of mine. I'm still not convinced we need that test, especially since it seems to be tricky to trigger the "random insert" in a reproducible way which would be needed for a simple unit test, but I'm happy to be convinced otherwise.

cbuescher · 2018-06-22T07:30:35Z

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

            XContentParser parser = createParserFunction.apply(XContentFactory.xContent(xContentType), shuffledContent);
            T parsed = parseFunction.apply(parser);
            assertEqualsConsumer.accept(testInstance, parsed);
            if (assertToXContentEquivalence) {
-                assertToXContentEquivalent(xContent, XContentHelper.toXContent(parsed, xContentType, false), xContentType);
+                assertToXContentEquivalent(
+                        XContentHelper.toXContent(testInstance, xContentType, false),


I have a hunch that this will again need to be changed using the variant that takes the toXContentParams argument. Will dig.

cbuescher · 2018-06-22T07:32:41Z

test/framework/src/test/java/org/elasticsearch/test/AbstractXContentTestCaseTests.java

+
+    private static final int NUMBER_OF_TEST_RUNS = 10;
+
+    public void testSomethign() throws IOException {


nit: typo (but it looks like this is going to be renamed anyway?)

cbuescher · 2018-06-22T07:36:00Z

test/framework/src/test/java/org/elasticsearch/test/AbstractXContentTestCaseTests.java

+    }
+}
+
+class TestInstance implements ToXContentObject {


Fancy, I didn't know you can habe two classes on the same level in one file. It's kind of uncommon though, I would make it an inner class if we keep it.

cbuescher · 2018-06-22T07:38:59Z

test/framework/src/test/java/org/elasticsearch/test/AbstractXContentTestCaseTests.java

+    private static final int NUMBER_OF_TEST_RUNS = 10;
+
+    public void testSomethign() throws IOException {
+        for (int runs = 0; runs < NUMBER_OF_TEST_RUNS; runs++) {


I don't think we need repetition here. I would keep from randomizing too much when testing test methods otherwise this game never ends ;-)

cbuescher · 2018-06-22T07:41:21Z

test/framework/src/test/java/org/elasticsearch/test/AbstractXContentTestCaseTests.java

+        for (int runs = 0; runs < NUMBER_OF_TEST_RUNS; runs++) {
+            TestInstance t = new TestInstance();
+            boolean atRandomPosition = false;
+            for (int i = 0; i < 5; i++) {


My guess is these repetitions are here so the likelihood for a random shuffle that inserts something in the 1st position that is not the original "field" field is increased? In that case, I think relying on "we just need to do enough repetitions" for a test to pass is trappy and should be avoided.

Yup, I need at least on of the iterations to make sure that something is inserted at the 1st position.

I also do not like the test itself as it is rather 'ugly' and testing the test but at the same time it make sense to have it to detect if the behavior is changed ...
I am fine either way ( and removing a test is easy ;) )

javanna

LGTM thanks @olcbean . I am good with the current test as long as it fails without the change made as part of this PR.

cbuescher

I think unfortunately the test still needs changes, but I left some ideas in my comment.

cbuescher · 2018-07-09T19:12:39Z

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

     */
    protected ToXContent.Params getToXContentParams() {
        return ToXContent.EMPTY_PARAMS;
    }
+
+    public static BytesReference insertRandomFieldsAndShuffle(ToXContent testInstance, XContentType xContentType,


Can this be package private please?

cbuescher · 2018-07-09T19:34:47Z

test/framework/src/test/java/org/elasticsearch/test/AbstractXContentTestCaseTests.java

+    public void testInsertRandomFieldsAndShuffle() throws IOException {
+        TestInstance t = new TestInstance();
+        boolean randomFieldAtFirstPosition = false;
+        for (int i = 0; i < 5; i++) {


Five repetitions is unfortunately not enough to always hit a case where the randomization kicks in. I ran the test with 100 repetitions and it failed 2 times out of that. Too much for out CI. I also wouldn't say the number should be increased. However, it would be great to "fix" the randomness in this particular case, since we are testing a test, but want it to behave in a specific way (do the shuffling). I did some digging in LuceneTestCase and fortunately found this little gem: RandomizedContext#runWithPrivateRandomness(). This seems to overwrite the test suite source of randomness for the time of invocation of a Callable, which in our case would be enough to make the shuffling always have some effect. We could fix the seed like this and avoid the loop alltogether:

// we need to "fix" randomness in order to be sure the shuffling takes effect BytesReference insertRandomFieldsAndShuffle = RandomizedContext.current().runWithPrivateRandomness(1, () -> AbstractXContentTestCase.insertRandomFieldsAndShuffle(t, XContentType.JSON, true, new String[] {}, null, this::createParser, ToXContent.EMPTY_PARAMS));

Note that with differnt seed (like e.g. 0) the test consistently fails, so we should use a "good" one.
I haven't used this before and also would like the opinion of @javanna if this makes sense. I would much prefer this to increasing the current loop count, which also doesn't really makes certain the test succeeds.

sounds good to me

@cbuescher thanks for digging into this ;)
I did not know of the runWithPrivateRandomness which works great in this case !

…eRandomness instead

cbuescher

@olcbean thanks, LGTM. I will kick of the CI tests.

cbuescher · 2018-07-11T13:28:17Z

@elasticachine test this please

cbuescher · 2018-07-11T15:17:38Z

@olcbean thanks for checking in on the CI build and fixing, second round...
@elasticmachine test this please

olcbean · 2018-07-12T07:52:50Z

Hey @cbuescher! Thanks for all your support.
I just checked the CI and saw that the build timed out. I am not really sure what the problem is; it seems like MasterDisruptionIT.testMasterNodeGCs never terminated :

21:58:25 HEARTBEAT J2 PID([email protected]): 2018-07-11T21:58:25, stalled for 23044s at: MasterDisruptionIT.testMasterNodeGCs
21:58:28 Build timed out (after 400 minutes). Marking the build as failed.

Do you have an idea what is going wrong?

cbuescher · 2018-07-12T08:56:54Z

I am not really sure what the problem is; it seems like MasterDisruptionIT.testMasterNodeGCs never terminated

Yes, that looks like it. Unfortunately those timeouts are hard to analyze, and I doubt it has anything to do with this PR. Best/Fastest option is to merge in master and run CI again.

cbuescher · 2018-07-12T09:13:38Z

@olcbean I took the liberty to merge in master
@elasticmachine test this again please

cbuescher · 2018-07-12T09:14:36Z

D'oh, wrong incantation. @elasticmachine test this please

Currently AbstractXContentTestCase#testFromXContent appends random fields, but in a fixed position. This PR shuffles all fields after the random fields have been appended, hence the random fields are actually added to random positions.

cbuescher · 2018-07-12T17:12:25Z

Finally green. I merged this to master and 6.x. Thanks again a lot @olcbean for sticking with this.

* master: [TEST] Mute SlackMessageTests.testTemplateRender Docs: Explain closing the high level client [ML] Re-enable memory limit integration tests (#31328) [test] disable packaging tests for suse boxes Add nio transport to security plugin (#31942) XContentTests : Insert random fields at random positions (#30867) Force execution of fetch tasks (#31974) Fix unreachable error condition in AmazonS3Fixture (#32005) Tests: Fix SearchFieldsIT.testDocValueFields (#31995) Add Expected Reciprocal Rank metric (#31891) [ML] Get ForecastRequestStats doc in RestoreModelSnapshotIT (#31973) SQL: Add support for single parameter text manipulating functions (#31874) [ML] Ensure immutability of MlMetadata (#31957) Tests: Mute SearchFieldsIT.testDocValueFields() muted tests due to #31940 Work around reported problem in eclipse (#31960) Move build integration tests out of :buildSrc project (#31961) Tests: Remove use of joda time in some tests (#31922) [Test] Reactive 3rd party tests on CI (#31919) SQL: Support for escape sequences (#31884) SQL: HAVING clause should accept only aggregates (#31872) Docs: fix typo in datehistogram (#31972) Switch url repository rest tests to new style requests (#31944) Switch reindex tests to new style requests (#31941) Docs: Added note about cloud service to installation and getting started [DOCS] Removes alternative docker pull example (#31934) Add Snapshots Status API to High Level Rest Client (#31515) ingest: date_index_name processor template resolution (#31841) Test: fix null failure in watcher test (#31968) Switch test framework to new style requests (#31939) Switch low level rest tests to new style Requests (#31938) Switch high level rest tests to new style requests (#31937) [ML] Mute test failing due to Java 11 date time format parsing bug (#31899) [TEST] Mute SlackMessageTests.testTemplateRender Fix assertIngestDocument wrongfully passing (#31913) Remove unused reference to filePermissionsCache (#31923) rolling upgrade should use a replica to prevent relocations while running a scroll HLREST: Bundle the x-pack protocol project (#31904) Increase logging level for testStressMaybeFlush Added lenient flag for synonym token filter (#31484) [X-Pack] Beats centralized management: security role + licensing (#30520) HLRest: Move xPackInfo() to xPack().info() (#31905) Docs: add security delete role to api call table (#31907) [test] port archive distribution packaging tests (#31314) Watcher: Slack message empty text (#31596) [ML] Mute failing DetectionRulesIT.testCondition() test Fix broken NaN check in MovingFunctions#stdDev() (#31888) Date: Add DateFormatters class that uses java.time (#31856) [ML] Switch native QA tests to a 3 node cluster (#31757) Change trappy float comparison (#31889) Fix building AD URL from domain name (#31849) Add opaque_id to audit logging (#31878) re-enable backcompat tests add support for is_write_index in put-alias body parsing (#31674) Improve release notes script (#31833) [DOCS] Fix broken link in painless example Handle missing values in painless (#30975) Remove the ability to index or query context suggestions without context (#31007) Ingest: Enable Templated Fieldnames in Rename (#31690) [Docs] Fix typo in the Rollup API Quick Reference (#31855) Ingest: Add ignore_missing option to RemoveProc (#31693) Add template config for Beat state to X-Pack Monitoring (#31809) Watcher: Add ssl.trust email account setting (#31684) Remove link to oss-MSI (#31844) Painless: Restructure Definition/Whitelist (#31879) HLREST: Add x-pack-info API (#31870)

* 6.x: Force execution of fetch tasks (#31974) [TEST] Mute SlackMessageTests.testTemplateRender Docs: Explain closing the high level client [test] disable java packaging tests for suse XContentTests : Insert random fields at random positions (#30867) Add Get Snapshots High Level REST API (#31980) Fix unreachable error condition in AmazonS3Fixture (#32005) [6.x][ML] Ensure immutability of MlMetadata (#31994) Add Expected Reciprocal Rank metric (#31891) SQL: Add support for single parameter text manipulating functions (#31874) muted tests due to #31940 Work around reported problem in eclipse (#31960) Move build integration tests out of :buildSrc project (#31961) [Test] Reactive 3rd party tests on CI (#31919) Fix assertIngestDocument wrongfully passing (#31913) (#31951) SQL: Support for escape sequences (#31884) SQL: HAVING clause should accept only aggregates (#31872) Docs: fix typo in datehistogram (#31972) Switch url repository rest tests to new style requests (#31944) Switch reindex tests to new style requests (#31941) Switch test framework to new style requests (#31939) Docs: Added note about cloud service to installation and getting started [DOCS] Removes alternative docker pull example (#31934) ingest: date_index_name processor template resolution (#31841) Test: fix null failure in watcher test (#31968) Watcher: Slack message empty text (#31596) Switch low level rest tests to new style Requests (#31938) Switch high level rest tests to new style requests (#31937) HLREST: Bundle the x-pack protocol project (#31904) [ML] Mute test failing due to Java 11 date time format parsing bug (#31899) Increase logging level for testStressMaybeFlush rolling upgrade should use a replica to prevent relocations while running a scroll [test] port archive distribution packaging tests (#31314) HLRest: Move xPackInfo() to xPack().info() (#31905) Increase logging level for :qa:rolling-upgrade Backport: Add template config for Beat state to X-Pack Monitoring (#31809) (#31893) Fix building AD URL from domain name (#31849) Fix broken NaN check in MovingFunctions#stdDev() (#31888) Change trappy float comparison (#31889) Add opaque_id to audit logging (#31878) add support for is_write_index in put-alias body parsing (#31674) Ingest: Enable Templated Fieldnames in Rename (#31690) (#31896) Ingest: Add ignore_missing option to RemoveProc (#31693) (#31892) [Docs] Fix typo in the Rollup API Quick Reference (#31855) Watcher: Add ssl.trust email account setting (#31684) [PkiRealm] Invalidate cache on role mappings change (#31510) [Security] Check auth scheme case insensitively (#31490) HLREST: Add x-pack-info API (#31870) Remove link to oss-MSI (#31844) Painless: Restructure Definition/Whitelist (#31879)

XContentTests : insert random fields at random positions

690a878

cbuescher added >test Issues or PRs that are addressing/adding tests review :Search/Search Search-related issues that do not fall into other categories labels May 25, 2018

javanna self-requested a review May 25, 2018 15:26

cbuescher self-assigned this Jun 21, 2018

cbuescher reviewed Jun 21, 2018

View reviewed changes

Merge branch 'master' into modify_XContentTest

c8d83ac

Conflicts: test/framework/src/main/java/org/elasticsearch/test/AbstractXContentTestCase.java

cbuescher closed this Jun 21, 2018

cbuescher reopened this Jun 21, 2018

cbuescher reviewed Jun 21, 2018

View reviewed changes

adding a test that a random field will be inserted at a random position

31de42c

rather than at the last position ( as till now )

cbuescher requested changes Jun 22, 2018

View reviewed changes

integrate reviewers comments

d6d28f5

javanna approved these changes Jul 9, 2018

View reviewed changes

cbuescher requested changes Jul 9, 2018

View reviewed changes

removing the unreliable loop by using RandomizedContext#runWithPrivat…

85b9b32

…eRandomness instead

cbuescher approved these changes Jul 11, 2018

View reviewed changes

adding license header

a854b23

Merge branch 'master' into modify_XContentTest

ed83d40

cbuescher added >enhancement v7.0.0 v6.4.0 labels Jul 12, 2018

cbuescher merged commit 334c255 into elastic:master Jul 12, 2018

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


		private static final int NUMBER_OF_TEST_RUNS = 10;

		public void testSomethign() throws IOException {

XContentTests : insert random fields at random positions #30867

XContentTests : insert random fields at random positions #30867

Conversation

olcbean commented May 25, 2018

elasticmachine commented May 25, 2018

javanna commented May 28, 2018

javanna commented May 28, 2018

olcbean commented May 30, 2018

javanna commented Jun 21, 2018

cbuescher left a comment

Choose a reason for hiding this comment

cbuescher commented Jun 21, 2018

cbuescher commented Jun 21, 2018

cbuescher Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbuescher commented Jun 21, 2018

cbuescher commented Jun 21, 2018

javanna commented Jun 21, 2018

cbuescher commented Jun 21, 2018

olcbean commented Jun 21, 2018

cbuescher 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

javanna left a comment

Choose a reason for hiding this comment

cbuescher 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

cbuescher left a comment

Choose a reason for hiding this comment

cbuescher commented Jul 11, 2018

cbuescher commented Jul 11, 2018

olcbean commented Jul 12, 2018

cbuescher commented Jul 12, 2018

cbuescher commented Jul 12, 2018

cbuescher commented Jul 12, 2018

cbuescher commented Jul 12, 2018

cbuescher Jun 21, 2018 •

edited

Loading