Migrate tests from MaxIT to MaxAggregatorTests#45030
Migrate tests from MaxIT to MaxAggregatorTests#45030csoulios merged 3 commits intoelastic:masterfrom
Conversation
|
Pinging @elastic/es-analytics-geo |
jimczi
left a comment
There was a problem hiding this comment.
The move of the tests look good to me but I left one comment regarding the remaining integration test.
|
|
||
| public class MaxIT extends AbstractNumericTestCase { | ||
| public class MaxIT extends ESIntegTestCase { | ||
|
|
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
++ 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!
There was a problem hiding this comment.
Removed integration test MaxIT and implemented caching checks in MaxAggregatorTests#testCacheAggregation (without script -> cached) and MaxAggregatorTests#testDontCacheScripts (with script -> not cached)
This PR migrates tests from MaxIT integration test to MaxAggregatorTests, as described in elastic#42893
… tests (elastic#45737) Similar to PR elastic#45030 integration test testDontCacheScripts() was moved to unit test AvgAggregatorTests#testDontCacheScripts. AvgIT class was removed.
This PR migrates tests from MaxIT integration test to MaxAggregatorTests, as described in #42893