Add parameter object to Aggregator Test Case #90320
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
not-napoleon
added
>non-issue
:Analytics/Aggregations
elasticsearchmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytics-geo (Team:Analytics) |
nik9000
requested changes
Sep 23, 2022
| @@ -1470,37 +1475,102 @@ public CircuitBreakerStats stats(String name) { | |||
| } | |||
| } | |||
|
|
|||
| public record AggTestConfig( | |||
| public record AggTestConfig<V extends InternalAggregation> ( | |||
| IndexSearcher searcher, | |||
There was a problem hiding this comment.
Can you make this into a CheckedSupplier<Searcher, IOException> and remove the buildIndex - instead we can have a withBuildIndex or something that converts a buildIndex into Supplier<....>.
| } | ||
|
|
||
| public AggTestConfig(IndexSearcher searcher, Query query, AggregationBuilder builder, MappedFieldType... fieldTypes) { | ||
| this(searcher, query, builder, DEFAULT_MAX_BUCKETS, randomBoolean(), true, fieldTypes); | ||
| this(searcher, query, null, null, builder, DEFAULT_MAX_BUCKETS, randomBoolean(), true, fieldTypes); | ||
| } |
There was a problem hiding this comment.
It'd be super cool if we could have one ctor for this one day. But that doesn't seem like today.
| @@ -440,7 +450,7 @@ protected ScriptService getMockScriptService() { | |||
| * It runs the aggregation as well using a circuit breaker that randomly throws {@link CircuitBreakingException} | |||
| * in order to mak sure the implementation does not leak. | |||
| */ | |||
| protected <A extends InternalAggregation, C extends Aggregator> A searchAndReduce(AggTestConfig aggTestConfig) throws IOException { | |||
| protected <A extends InternalAggregation, C extends Aggregator> A searchAndReduce(AggTestConfig<A> aggTestConfig) throws IOException { | |||
There was a problem hiding this comment.
I think this should call verify on the context. If we're going to have a verify member on AggTestConfig we should call it in every possible place.
| splitLeavesIntoSeparateAggregators, | ||
| shouldBeCached, | ||
| fieldTypes | ||
| ); |
There was a problem hiding this comment.
Does this work?
public <V> AggTestCase withVerifier(Consumer<V> verifier) {
Consumer<InternalAggregation> = i -> verifier((V) i);
new Agg.....
|
Upon reflection, this doesn't seem like it will be worth the work at this point. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I apologize for the huge, largely automated, refactoring PR. This extends the refactoring from #90149 by applying the same parameter object to
AggregatorTestCase#testCase, and extending it to include that method's parameters, which allows removing some useless wrapper functions. By being a bit more explicit about type parameters, I was also able to remove some useless casting. This should make it easier to write tests and extend the testing framework going forward.