Skip to content

Migrate tests from MaxIT to MaxAggregatorTests #45030

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
Merged
merged 3 commits into from
Aug 20, 2019
Merged

Migrate tests from MaxIT to MaxAggregatorTests #45030

merged 3 commits into from
Aug 20, 2019

Conversation

csoulios
Copy link
Contributor

This PR migrates tests from MaxIT integration test to MaxAggregatorTests, as described in #42893

@csoulios csoulios added >test Issues or PRs that are addressing/adding tests :Analytics/Aggregations Aggregations >refactoring v8.0.0 v7.4.0 labels Jul 31, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo

@csoulios csoulios requested review from polyfractal and jimczi July 31, 2019 10:03
jimczi
jimczi reviewed Jul 31, 2019
View reviewed changes
Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

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

The move of the tests look good to me but I left one comment regarding the remaining integration test.

server/src/test/java/org/elasticsearch/search/aggregations/metrics/MaxIT.java Outdated

public class MaxIT extends AbstractNumericTestCase {
public class MaxIT extends ESIntegTestCase {

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's ok to remove this test entirely. testDontCacheScripts is tested on a lot of aggregations (metrics, top_hits, terms, ...) but we don't really need an integration test for this. For instance AbstractQueryTestCase#testToQuery checks QueryShardContext#isCacheable after building the query, we could have the same kind of unit test in MaxAggregatorTests. Does this makes sense ?

Copy link
Contributor

Choose a reason for hiding this comment

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

++ Christos and I chatted about this last week and agreed it's doable in the framework. I'll make the change and merge (Christos was on support ramp and wasn't able to get to it before PTO). Thanks!

Copy link
Contributor Author

@csoulios csoulios Aug 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Removed integration test MaxIT and implemented caching checks in MaxAggregatorTests#testCacheAggregation (without script -> cached) and MaxAggregatorTests#testDontCacheScripts (with script -> not cached)

@csoulios csoulios requested a review from jimczi August 20, 2019 06:21
jimczi
jimczi approved these changes Aug 20, 2019
View reviewed changes
Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

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

LGTM

@csoulios csoulios mentioned this pull request Aug 20, 2019
Merged
polyfractal
polyfractal approved these changes Aug 20, 2019
View reviewed changes
Copy link
Contributor

@polyfractal polyfractal left a comment

Choose a reason for hiding this comment

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

Also LGTM! :)

@csoulios csoulios merged commit 09d8384 into elastic:master Aug 20, 2019
@csoulios csoulios deleted the refactor_maxit_test branch August 20, 2019 14:00
csoulios added a commit to csoulios/elasticsearch that referenced this pull request Aug 20, 2019
@csoulios
Migrate tests from MaxIT to MaxAggregatorTests (elastic#45030)
49cb460 
This PR migrates tests from MaxIT integration test to MaxAggregatorTests, as described in elastic#42893
@csoulios csoulios mentioned this pull request Aug 20, 2019
Merged
csoulios added a commit that referenced this pull request Aug 20, 2019
@csoulios
Implemented AvgAggregatorTests#testDontCacheScripts and removed AvgIT…
0f024c1 
… tests (#45737)

Similar to PR #45030 integration test testDontCacheScripts() was moved to unit test AvgAggregatorTests#testDontCacheScripts.

AvgIT class was removed.
csoulios added a commit to csoulios/elasticsearch that referenced this pull request Aug 20, 2019
@csoulios
Implemented AvgAggregatorTests#testDontCacheScripts and removed AvgIT…
221de22 
… tests (elastic#45737)

Similar to PR elastic#45030 integration test testDontCacheScripts() was moved to unit test AvgAggregatorTests#testDontCacheScripts.

AvgIT class was removed.
@csoulios csoulios mentioned this pull request Aug 20, 2019
Merged
csoulios added a commit that referenced this pull request Aug 20, 2019
@csoulios
[7.x] Migrate tests from MaxIT to MaxAggregatorTests (#45030) #45742
96a40ac 
Backports PR #45030 to 7.x:

    This PR migrates tests from MaxIT integration test to MaxAggregatorTests, as described in #42893
csoulios added a commit that referenced this pull request Aug 20, 2019
@csoulios
[7.x] Implement AvgAggregatorTests#testDontCacheScripts and remove Av…
2a0c7c4 
…gIT #45746

Backports PR #45737:

    Similar to PR #45030 integration test testDontCacheScripts() was moved to unit test AvgAggregatorTests#testDontCacheScripts.

    AvgIT class was removed.
@csoulios csoulios mentioned this pull request Nov 11, 2021
Open
46 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@polyfractal polyfractal polyfractal approved these changes

@jimczi jimczi jimczi approved these changes

Assignees
No one assigned
Labels
:Analytics/Aggregations Aggregations >refactoring >test Issues or PRs that are addressing/adding tests v7.4.0 v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@csoulios @elasticmachine @polyfractal @jimczi @jakelandis