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

Add unit tests for NestedAggregator #24054

Merged
merged 2 commits into from
Apr 12, 2017

Conversation

polyfractal
Copy link
Contributor

I wasn't sure what to do with the existing NestedAggregatorTests test, which is based on ESSingleNodeTestCase. Partly because I wasn't really sure what it was testing...so I just left it alone :)

Sorry for the delay on getting this done!

Relates to #22278

@polyfractal polyfractal added :Analytics/Aggregations Aggregations review >test Issues or PRs that are addressing/adding tests labels Apr 11, 2017
@martijnvg
Copy link
Member

@polyfractal The tests look good. I think you can port the test in NestedAggregatorTests to this class and then remove that file and rename this file to NestedAggregatorTests?

Also while you're at it can you reduce the visibility of the NestedAggregator, NestedAggregatorFactory and InternalNested? The constructors of these classes can be made package protected. Also NestedAggregator and NestedAggregatorFactory classes can be made package protected too.

@polyfractal
Copy link
Contributor Author

Updated! :)

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

LGTM

@polyfractal polyfractal merged commit 1fd50bc into elastic:master Apr 12, 2017
@polyfractal polyfractal deleted the nestedAggregatorTests branch April 12, 2017 20:00
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Apr 13, 2017
* master:
  Remove more hidden file leniency from plugins
  Register error listener in evil logger tests
  Detect using logging before configuration
  [DOCS] Added note about Elastic Cloud to improve 'elastic aws' SERP results.
  Add version constant for 5.5 (elastic#24075)
  Add unit tests for NestedAggregator (elastic#24054)
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Apr 13, 2017
* master: (41 commits)
  Remove awaits fix from evil JNA native tests
  Correct handling of default and array settings
  Build: Switch jna dependency to an elastic version (elastic#24081)
  fix CategoryContextMappingTests compilation bugs
  testConcurrentGetAndSetOnPrimary - fix a race condition between indexing and updating value map
  Allow different data types for category in Context suggester (elastic#23491)
  Restrict build info loading to ES jar, not any jar (elastic#24049)
  Remove more hidden file leniency from plugins
  Register error listener in evil logger tests
  Detect using logging before configuration
  [DOCS] Added note about Elastic Cloud to improve 'elastic aws' SERP results.
  Add version constant for 5.5 (elastic#24075)
  Add unit tests for NestedAggregator (elastic#24054)
  Add more debugging information to rethrottles
  Tests: Use random analyzer only on string fields in Match/MultiMatchBuilderTests
  Cleanup outdated comments for fixing up pom dependencies (elastic#24056)
  S3 Repository: Eagerly load static settings (elastic#23910)
  Reject duplicate settings on the command line
  Wildcard cluster names for cross cluster search (elastic#23985)
  Update scripts/security docs for sandboxed world (elastic#23977)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Aggregations Aggregations >test Issues or PRs that are addressing/adding tests v6.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants