Skip to content

Conversation

@martijnvg
Copy link
Member

Also adds some initial tests for some aggregations.

First PR for #22278.

@martijnvg martijnvg added review >test Issues or PRs that are addressing/adding tests v5.2.0 v6.0.0-alpha1 labels Dec 20, 2016
Copy link
Member

Choose a reason for hiding this comment

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

Aggregator

@jpountz jpountz changed the title Add base class for writing unit test for aggragations Add base class for writing unit test for aggregations Dec 20, 2016
Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

I left some comments, but +1 to start from there.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the generics on this method are buying anything since we do a blind cast here?

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason I did the cast here is that we don't have to do the cast in all tests that need an aggregator.

Copy link
Contributor

Choose a reason for hiding this comment

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

can you also test what happens when no docs match the query?

Copy link
Contributor

Choose a reason for hiding this comment

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

or when some docs match the query but do not have a value

Copy link
Contributor

Choose a reason for hiding this comment

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

can you call it aggregationBuilder rather than aggregatorBuilder to be more consistent with the class name?

Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

This looks like a good start! Thanks for working on that!

@martijnvg martijnvg force-pushed the aggregator_unit_tests branch from 7b847a0 to 0346583 Compare December 21, 2016 07:42
@martijnvg martijnvg force-pushed the aggregator_unit_tests branch from 0346583 to 417746c Compare December 21, 2016 07:44
@martijnvg martijnvg merged commit 417746c into elastic:master Dec 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>test Issues or PRs that are addressing/adding tests v5.2.0 v6.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants