Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
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, false),
Copy link
Member

Choose a reason for hiding this comment

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

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

XContentHelper.toXContent(parsed, xContentType, 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));
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
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 {

private static final int NUMBER_OF_TEST_RUNS = 10;

public void testSomethign() throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

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

for (int runs = 0; runs < NUMBER_OF_TEST_RUNS; runs++) {
Copy link
Member

Choose a reason for hiding this comment

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

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

TestInstance t = new TestInstance();
boolean atRandomPosition = 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.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ;) )

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())) {
atRandomPosition = true;
break;
}
}
}
assertThat(atRandomPosition, equalTo(true));
}
}
}

class TestInstance implements ToXContentObject {
Copy link
Member

Choose a reason for hiding this comment

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

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.


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

}