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

Jump to bottom

Improve test coverage for bucket and metric aggregations #22278

Closed
63 tasks done
martijnvg opened this issue Dec 20, 2016 · 3 comments
Closed
63 tasks done

Improve test coverage for bucket and metric aggregations #22278

martijnvg opened this issue Dec 20, 2016 · 3 comments
Assignees
@nik9000 @colings86 @jpountz @martijnvg @tlrx @polyfractal @cbuescher @jimczi
Labels
:Analytics/Aggregations Aggregations Meta >test Issues or PRs that are addressing/adding tests

Comments

@martijnvg
Copy link
Member

martijnvg commented Dec 20, 2016 •
edited by markharwood

Today there is very little unit test coverage for bucket and metric aggregations. This meta issue aim is to significantly improve that. For each aggregation we should add more unit tests for the aggregator (how it interacts with the Lucene index via org.apache.lucene.search.Collector that each aggregation implements), the reduce logic and serialization of the aggregation results.

We should add unit tests for the following Aggregator implementations:

We should also add tests for the following InternalAggregation implementations:
(to test the reduce and result serialization logic)

I may have forgotten some classes, so please update this issue if that is the case :)

I listed the InternalAggregation implementations separately from Aggregator implementations as unit tests for each can be written in parallel by different devs. However I think for the less complex aggregations unit tests for both the InternalAggregation implementation and Aggregator implementations can be added in the same PR.

I think at least the unit tests should be added to the master branch. Backporting to 5.x branch is best effort and should only be considered if it is low hanging fruit.

I suggest that we work in the following way in order to avoid accidentally doing work twice:

  • Add your name the a task you like to work on before starting to write the unit tests.
  • When you open a pr then add the PR number to task.
  • Once the pr is merged, then check off the task.

Note that aggregations are tested, however due to how before the code was structured, unit testing was really difficult.
@martijnvg martijnvg mentioned this issue Dec 20, 2016
Merged
@martijnvg martijnvg added the >test Issues or PRs that are addressing/adding tests label Dec 20, 2016
nik9000 added a commit to nik9000/elasticsearch that referenced this issue Jan 17, 2017
@nik9000
Improve unit test coverage of aggs
dd0c180 
Add tests for `GlobalAggregator`, `MaxAggregator`, and `InternalMax`.

Relates to elastic#22278
@nik9000 nik9000 mentioned this issue Jan 17, 2017
Merged
@jimczi jimczi mentioned this issue Jan 18, 2017
Merged
nik9000 added a commit that referenced this issue Jan 18, 2017
@nik9000
Improve unit test coverage of aggs (#22668)
e71b26f 
Add tests for `GlobalAggregator`, `MaxAggregator`, and `InternalMax`.

Relates to #22278
jimczi added a commit that referenced this issue Jan 19, 2017
@jimczi
Add unit tests for FiltersAggregator (#22678)
b781a4a 
Adds unit tests for the `filters` aggregation.
This change also adds an helper to search and reduce any aggregator in a unit test.
This is done by dividing a single searcher in sub-searcher, one for each segment.

Relates #22278
jimczi added a commit that referenced this issue Jan 19, 2017
@jimczi
Add unit tests for FiltersAggregator (#22678)
ac4e1bb 
Adds unit tests for the `filters` aggregation.
This change also adds an helper to search and reduce any aggregator in a unit test.
This is done by dividing a single searcher in sub-searcher, one for each segment.

Relates #22278
tlrx added a commit to tlrx/elasticsearch that referenced this issue Jan 20, 2017
@tlrx
Add unit tests for DateHistogramAggregator
e6584be 
Adds unit tests for the date histogram aggregator.

Relates elastic#22278
@tlrx tlrx mentioned this issue Jan 20, 2017
Merged
tlrx added a commit that referenced this issue Jan 20, 2017
@tlrx
Add unit tests for DateHistogramAggregator (#22714)
239ed0c 
Adds unit tests for the date histogram aggregator.

Relates #22278
tlrx added a commit that referenced this issue Jan 20, 2017
@tlrx
Add unit tests for DateHistogramAggregator (#22714)
f7dc7e2 
Adds unit tests for the date histogram aggregator.

Relates #22278
tlrx added a commit to tlrx/elasticsearch that referenced this issue Jan 23, 2017
@tlrx
Add unit tests for ValueCountAggregator and InternalValueCount
eafe4c1 
Adds unit tests for the value count aggregator.

Relates elastic#22278
@tlrx tlrx mentioned this issue Jan 23, 2017
Merged
@markharwood markharwood mentioned this issue Jan 23, 2017
Closed
tlrx added a commit that referenced this issue Jan 23, 2017
@tlrx
Add unit tests for ValueCountAggregator and InternalValueCount (#22741)
11164b3 
Adds unit tests for the value count aggregator.

Relates #22278
tlrx added a commit that referenced this issue Jan 23, 2017
@tlrx
Add unit tests for ValueCountAggregator and InternalValueCount (#22741)
12982e9 
Adds unit tests for the value count aggregator.

Relates #22278
nik9000 added a commit to nik9000/elasticsearch that referenced this issue Jan 23, 2017
@nik9000
Add tests for top_hits aggregation
4ff7144 
Add unit tests for `TopHitsAggregator` and convert some snippets in
docs for `top_hits` aggregation to `// CONSOLE`.

Relates to elastic#22278
Relates to elastic#18160
@nik9000 nik9000 mentioned this issue Jan 23, 2017
Merged
@tlrx tlrx mentioned this issue Apr 18, 2017
Merged
tlrx added a commit that referenced this issue Apr 19, 2017
@tlrx
[Test] Add unit tests for InternalHDRPercentilesTests (#24157)
741c031 
Related to #22278
@martijnvg martijnvg mentioned this issue Apr 19, 2017
Merged
@tlrx tlrx mentioned this issue Apr 21, 2017
Merged
martijnvg added a commit that referenced this issue Apr 24, 2017
@martijnvg
[TEST] Added unittests for InternalGeoCentroid
dabbf5d 
Relates to #22278
tlrx added a commit that referenced this issue May 2, 2017
@tlrx
[Test] Add unit tests for HDR/TDigest PercentilesAggregators (#24245)
ad3c042 
Relates to #22278
@martijnvg martijnvg mentioned this issue May 9, 2017
Merged
tlrx added a commit to tlrx/elasticsearch that referenced this issue May 9, 2017
@tlrx
[Test] Add unit tests for Range aggregations
1492d6a 
Related to elastic#22278
@tlrx tlrx mentioned this issue May 9, 2017
Merged
martijnvg added a commit that referenced this issue May 10, 2017
@martijnvg
Added unit tests for InternalMatrixStats.
51c74ce 
Also moved InternalAggregationTestCase to test-framework module in order to make use of it from other modules than core.

Relates to #22278
@cbuescher cbuescher mentioned this issue May 11, 2017
Merged
cbuescher added a commit that referenced this issue May 12, 2017
@cbuescher
[Tests] Add unit tests for InternalFilters (#24624)
f06467b 
Relates to #22278
@cbuescher cbuescher mentioned this issue May 16, 2017
Merged
cbuescher added a commit that referenced this issue May 16, 2017
@cbuescher
[Tests] Add unit test for InternalAdjecencyMatrix aggregation (#24698)
8c6b5a9 
Adding a unit test to InternalAdjecencyMatrix that extends the shared InternalAggregationTestCase
 that we use for testing aggregations.
Relates to #22278
@martijnvg martijnvg mentioned this issue May 23, 2017
Merged
@javanna javanna mentioned this issue May 23, 2017
Merged
@markharwood markharwood mentioned this issue May 26, 2017
Merged
@martijnvg martijnvg mentioned this issue May 30, 2017
Merged
markharwood added a commit to markharwood/elasticsearch that referenced this issue Jun 23, 2017
@markharwood
Added unit test coverage for GlobalOrdinalsSignificantTermsAggregator…
7112faa 
…, GlobalOrdinalsSignificantTermsAggregator.WithHash, SignificantLongTermsAggregator and SignificantStringTermsAggregator

Removed integration test

Relates elastic#22278
markharwood added a commit that referenced this issue Jun 23, 2017
@markharwood
Added unit test coverage for SignificantTerms (#24904)
973530f 
Added unit test coverage for GlobalOrdinalsSignificantTermsAggregator, GlobalOrdinalsSignificantTermsAggregator.WithHash, SignificantLongTermsAggregator and SignificantStringTermsAggregator.
Removed integration test.

Relates #22278
@colings86
Copy link
Contributor

All tasks for this ticket have now been completed 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nik9000 nik9000

@colings86 colings86

@jpountz jpountz

@martijnvg martijnvg

@tlrx tlrx

@polyfractal polyfractal

@cbuescher cbuescher

@jimczi jimczi

Labels
:Analytics/Aggregations Aggregations Meta >test Issues or PRs that are addressing/adding tests
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
9 participants
@MaineC @nik9000 @colings86 @jpountz @martijnvg @tlrx @polyfractal @cbuescher @jimczi