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.

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

[Test] Always check the XContent equivalent when parsing aggregations #24208

Conversation

Projects
None yet
3 participants
@tlrx
Copy link
Member

commented Apr 20, 2017

I mixed up things in #24183 when adding the parse() method in InternalAggregationTestCase.

This pull request fixes InternalAggregationTestCase so that it always checks that the internal aggregation and the parsed aggregation always produce the same XContent (using assertToXContentEquivalent()) even when the original internal aggregation has been shuffled.

[Test] Always check the XContent equivalent when parsing aggregations
In InternalAggregationTestCase, we can check that the internal
aggregation and the parsed aggregation always produce the same XContent
  even if the original internal aggregation has been shuffled or not.
@cbuescher
Copy link
Member

left a comment

LGTM, left one minor comment


final BytesReference parsedBytes = toXContent((ToXContent) parsedAggregation, xContentType, params, humanReadable);
assertToXContentEquivalent(originalBytes, parsedBytes, xContentType);
final Aggregation parsedAggregation = parseAndAssert(aggregation, xContentType, randomBoolean(), randomBoolean());

This comment has been minimized.

Copy link
@cbuescher

cbuescher Apr 20, 2017

Member

Nit: could this by simplified even more by randomizing the xContentType, humanReadable and shuffle flag inside of parsedAggregation? Not sure about other callers of this method...

This comment has been minimized.

Copy link
@tlrx

tlrx Apr 20, 2017

Author Member

Sure, thanks for the suggestion

@tlrx tlrx merged commit d0df1ed into elastic:feature/client_aggs_parsing Apr 20, 2017

1 check passed

CLA Commit author is a member of Elasticsearch
Details
@tlrx

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2017

Thanks @cbuescher !

@tlrx tlrx deleted the tlrx:update-internal-aggregation-test-case branch Apr 20, 2017

@javanna javanna removed the v6.0.0-alpha1 label Apr 25, 2017

javanna added a commit to javanna/elasticsearch that referenced this pull request May 23, 2017

[Test] Always check the XContent equivalent when parsing aggregations (
elastic#24208)

In InternalAggregationTestCase, we can check that the internal aggregation and the parsed aggregation always produce the same XContent even if the original internal aggregation has been shuffled or not.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.