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

XContentTests : insert random fields at random positions #30867

Merged
merged 7 commits into from
Jul 12, 2018
Merged

XContentTests : insert random fields at random positions #30867

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
690a878
XContentTests : insert random fields at random positions
olcbean May 23, 2018
c8d83ac
Merge branch 'master' into modify_XContentTest
Jun 21, 2018
31de42c
adding a test that a random field will be inserted at a random position
olcbean Jun 21, 2018
d6d28f5
integrate reviewers comments
olcbean Jun 22, 2018
85b9b32
removing the unreliable loop by using RandomizedContext#runWithPrivat…
olcbean Jul 11, 2018
a854b23
adding license header
olcbean Jul 11, 2018
ed83d40
Merge branch 'master' into modify_XContentTest
Jul 12, 2018
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
38 changes: 25 additions & 13 deletions test/framework/src/main/java/org/elasticsearch/test/AbstractXContentTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,21 +54,16 @@ public static <T extends ToXContent> void testFromXContent(int numberOfTestRuns,
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 withRandomFields;
if (supportsUnknownFields) {
// we add a few random fields to check that parser is lenient on new fields
withRandomFields = XContentTestUtils.insertRandomFields(xContentType, shuffled, randomFieldsExcludeFilter, random());
} else {
withRandomFields = shuffled;
}
XContentParser parser = createParserFunction.apply(XContentFactory.xContent(xContentType), withRandomFields);
BytesReference shuffledContent = insertRandomFieldsAndShuffle(testInstance, xContentType, supportsUnknownFields,
shuffleFieldsExceptions, randomFieldsExcludeFilter, createParserFunction, toXContentParams);
XContentParser parser = createParserFunction.apply(XContentFactory.xContent(xContentType), shuffledContent);
T parsed = parseFunction.apply(parser);
assertEqualsConsumer.accept(testInstance, parsed);
if (assertToXContentEquivalence) {
assertToXContentEquivalent(shuffled, XContentHelper.toXContent(parsed, xContentType, toXContentParams, false),
xContentType);
assertToXContentEquivalent(
XContentHelper.toXContent(testInstance, xContentType, toXContentParams, false),
XContentHelper.toXContent(parsed, xContentType, toXContentParams, false),
xContentType);
}
}
}
Expand Down Expand Up @@ -132,9 +127,26 @@ protected String[] getShuffleFieldsExceptions() {
}

/**
* Params that have to be provided when calling calling {@link ToXContent#toXContent(XContentBuilder, ToXContent.Params)}
* Params that have to be provided when calling {@link ToXContent#toXContent(XContentBuilder, ToXContent.Params)}
*/
protected ToXContent.Params getToXContentParams() {
return ToXContent.EMPTY_PARAMS;
}

public static BytesReference insertRandomFieldsAndShuffle(ToXContent testInstance, XContentType xContentType,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be package private please?

boolean supportsUnknownFields, String[] shuffleFieldsExceptions, Predicate<String> randomFieldsExcludeFilter,
CheckedBiFunction<XContent, BytesReference, XContentParser, IOException> createParserFunction,
ToXContent.Params toXContentParams) throws IOException {
BytesReference xContent = XContentHelper.toXContent(testInstance, xContentType, toXContentParams, false);
BytesReference withRandomFields;
if (supportsUnknownFields) {
// add a few random fields to check that the parser is lenient on new fields
withRandomFields = XContentTestUtils.insertRandomFields(xContentType, xContent, randomFieldsExcludeFilter, random());
} else {
withRandomFields = xContent;
}
XContentParser parserWithRandonFields = createParserFunction.apply(XContentFactory.xContent(xContentType), withRandomFields);
return BytesReference.bytes(ESTestCase.shuffleXContent(parserWithRandonFields, false, shuffleFieldsExceptions));
}

}
48 changes: 48 additions & 0 deletions test/framework/src/test/java/org/elasticsearch/test/AbstractXContentTestCaseTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
package org.elasticsearch.test;

import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.xcontent.ToXContent;
import org.elasticsearch.common.xcontent.ToXContentObject;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.XContentType;

import java.io.IOException;
import java.util.Map;

import static org.hamcrest.Matchers.equalTo;

public class AbstractXContentTestCaseTests extends ESTestCase {

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good to me

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

BytesReference insertRandomFieldsAndShuffle = AbstractXContentTestCase.insertRandomFieldsAndShuffle(t, XContentType.JSON, true,
new String[] {}, null, this::createParser, ToXContent.EMPTY_PARAMS);
try (XContentParser parser = createParser(XContentType.JSON.xContent(), insertRandomFieldsAndShuffle)) {
Map<String, Object> mapOrdered = parser.mapOrdered();
assertThat(mapOrdered.size(), equalTo(2));
if (false == "field".equals(mapOrdered.keySet().iterator().next())) {
randomFieldAtFirstPosition = true;
}
}
}
assertThat(randomFieldAtFirstPosition, equalTo(true));
}

private class TestInstance implements ToXContentObject {

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject();
{
builder.field("field", 1);
}
builder.endObject();
return builder;
}

}

}