-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
Aggregations/HL Rest client fix: missing scores #32774
Merged
Merged
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ops :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it maybe be possible to recreate this in a unit test as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any examples of existing unit tests for "toXContent -> fromXContent" serialization on aggregation buckets I can base this on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as far as I can see BaseAggregationTestCase has a testFromXContent method. So SignficantTermsTests sounds like the appropriate test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In which case we might be missing some test infrastructure for aggs here. That method is for testing agg requests but we have no equivalent for testing responses. I guess
toXcontent->fromXContent
is a transformation of response objects that didn't exist before high level rest client.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, that is for requests, my bad, but we have tests for responses too, for instance
InternalTermsTestCase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a quick chat with @colings86 and it looks like we need a base class for aggs with an abstract
assertParsedResponse(InternalX, ParsedX)
. InternalX here is the object used to hold the response on the server and ParsedX is the client-side equivalent.That work's beyond the scope of this fix so I suggest we open another issue to address that broader set of work and put this PR to bed as-is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just looked and actually
InternalAggregationTestCase
already has a method of ensuringthe serialisation is correct in it'stestFromXContent()
method. So I think this might be a bug inInternalSignificantTermsTestCase which is not producing random instance that exercise this. We can however produce a new test method to make sure this is tested using
parseAndAssert()and
assertFromXContent()` even if we can't make the randomised test instances exercise the code path directlyThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, @colings86. The buckets need
updateScore
calling on them in the test code to derive scores from all the frequency stats. Otherwise tests were always testing for score=0 conditions and failing to highlight the bug.When I remove
fromXContent
fix and run with the beefed up tests it now reproduces the original error so it looks like the test infra is there.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
glad to hear, am a bit rusty on this topic but I kind of knew/hoped that we have added tests when adding parsing for the high-level REST client.