-
Notifications
You must be signed in to change notification settings - Fork 24.2k
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
Migrate CardinalityIT test to AggregatorTestCase #80951
Migrate CardinalityIT test to AggregatorTestCase #80951
Conversation
💚 CLA has been signed |
@elasticmachine run CLA |
Replace integration tests with unit tests for cardinality aggregations. Unit tests are generally faster and easier to test against.
1cf48ab
to
2c9d2f9
Compare
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.
Very good first effort. Perhaps you should override the getSupportedValuesSourceTypes()
method so that we test the matrix of supported/unsupported field types.
Also, I think that some aggregation tests from CardinalityIT
have not been ported to CardinalityAggregatorTests
(testAsSubAgg
, testScriptCaching
, testPartiallyUnmapped
and the "value script" tests) (check MaxAggregatorTests
on how to implement some of them)
.../src/test/java/org/elasticsearch/search/aggregations/metrics/CardinalityAggregatorTests.java
Show resolved
Hide resolved
Overriding getSupportedValuesSourceTypes and createAggBuilderForTypeTest results in testing all the supported value types for the cardinality aggregator. The cardinality aggregator supports all core values types.
This test uses a terms aggregation as a top level aggregation and a cardinality aggregation as a sub-aggretation.
@elasticmachine run elasticsearch-ci/rest-compatibility |
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.
Add couple more comments.
.../src/test/java/org/elasticsearch/search/aggregations/metrics/CardinalityAggregatorTests.java
Outdated
Show resolved
Hide resolved
.../src/test/java/org/elasticsearch/search/aggregations/metrics/CardinalityAggregatorTests.java
Outdated
Show resolved
Hide resolved
.../src/test/java/org/elasticsearch/search/aggregations/metrics/CardinalityAggregatorTests.java
Outdated
Show resolved
Hide resolved
.../src/test/java/org/elasticsearch/search/aggregations/metrics/CardinalityAggregatorTests.java
Outdated
Show resolved
Hide resolved
.../src/test/java/org/elasticsearch/search/aggregations/metrics/CardinalityAggregatorTests.java
Outdated
Show resolved
Hide resolved
.../src/test/java/org/elasticsearch/search/aggregations/metrics/CardinalityAggregatorTests.java
Outdated
Show resolved
Hide resolved
@elasticmachine update branch |
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.
LGTM
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.
LGTM
@elasticmachine update branch |
Congrats on your first PR getting merged! :) |
Replace integration tests with unit tests for cardinality aggregations.
Unit tests are generally faster and easier to test against.
This is part of the transition from integration to unit tests described by #42893