Add parameter object to Aggregator Test Case#90320
Closed
not-napoleon wants to merge 18 commits intoelastic:mainfrom
Closed
Add parameter object to Aggregator Test Case#90320not-napoleon wants to merge 18 commits intoelastic:mainfrom
not-napoleon wants to merge 18 commits intoelastic:mainfrom
Conversation
Collaborator
|
Pinging @elastic/es-analytics-geo (Team:Analytics) |
nik9000
requested changes
Sep 23, 2022
|
|
||
| public record AggTestConfig( | ||
| public record AggTestConfig<V extends InternalAggregation> ( | ||
| IndexSearcher searcher, |
Member
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); | ||
| } |
Member
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.
| * 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 { |
Member
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 | ||
| ); |
Member
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.....
Member
Author
|
Upon reflection, this doesn't seem like it will be worth the work at this point. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.