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

Clean up some aggregation tests #66044

Merged
merged 3 commits into from
Dec 9, 2020
Merged

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Dec 8, 2020

This rewrites two tests for aggregations to use AggregatorTestCase's
simpler way of making Aggregators, allowing us to remove a ctor on
ProductionAggregationContext that we weren't happy about. Now there is
only a single test call to ProductionAggregationContext and we can
remove that soon.

This rewrites two tests for aggregations to use `AggregatorTestCase`'s
simpler way of making `Aggregator`s, allowing us to remove a ctor on
`ProductionAggregationContext` that we weren't happy about. Now there is
only a single test call to `ProductionAggregationContext` and we can
remove that soon.
@nik9000 nik9000 requested a review from not-napoleon December 8, 2020 16:50
@elasticmachine elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Dec 8, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (Team:Analytics)

Copy link
Member

@not-napoleon not-napoleon left a comment

Choose a reason for hiding this comment

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

LGTM. Left a nit about test naming, but it's a matter of taste.


public class AggregationCollectorTests extends ESSingleNodeTestCase {
public class AggregationCollectorTests extends AggregatorTestCase {
Copy link
Member

Choose a reason for hiding this comment

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

Oh, hey, moving this off of ESSingleNodeTestCase is a win!

IndexService index = createIndex("idx");
client().prepareIndex("idx").setId("1").setSource("f", 5).execute().get();
client().admin().indices().prepareRefresh("idx").get();
public void testSubScriptDoesntNeedScoresIfSubScriptDoesntNeedScores() throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

I am not a fan of this method of documenting tests. CamelCase sentences are hard to read, and with many similar names it's easy to miss the differences between two of them. Maybe just name this something like testSubScriptNoScore and add a javadoc noting what it's intended to test? Same for the other very long test names.

Copy link
Member Author

Choose a reason for hiding this comment

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

I like having a test for each thing. I could go either way on the long names. I'll change them.

@nik9000 nik9000 merged commit 1585078 into elastic:master Dec 9, 2020
nik9000 added a commit to nik9000/elasticsearch that referenced this pull request Dec 9, 2020
This rewrites two tests for aggregations to use `AggregatorTestCase`'s
simpler way of making `Aggregator`s, allowing us to remove a ctor on
`ProductionAggregationContext` that we weren't happy about. Now there is
only a single test call to `ProductionAggregationContext` and we can
remove that soon.
nik9000 added a commit that referenced this pull request Dec 9, 2020
This rewrites two tests for aggregations to use `AggregatorTestCase`'s
simpler way of making `Aggregator`s, allowing us to remove a ctor on
`ProductionAggregationContext` that we weren't happy about. Now there is
only a single test call to `ProductionAggregationContext` and we can
remove that soon.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Aggregations Aggregations >refactoring Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v7.11.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants