Convert InternalAggTestCase to AbstractNamedWriteableTestCase#55250
Convert InternalAggTestCase to AbstractNamedWriteableTestCase#55250polyfractal merged 7 commits intoelastic:masterfrom
Conversation
Some aggregations, such as the Terms* family, will use an alternate class to represent unmapped shard results (while the rest of the aggs use the same object but with some form of "empty" or "nullish" values to represent unmapped). This was problematic with AbstractWireSerializingTestCase because it expects the instanceReader to always match the original class. Instead, we need to use the NamedWriteable version so that the registry can be consulted for the proper deserialization reader.
|
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
test/framework/src/main/java/org/elasticsearch/test/InternalAggregationTestCase.java
Show resolved
Hide resolved
| import static org.hamcrest.Matchers.lessThanOrEqualTo; | ||
|
|
||
| public abstract class InternalAggregationTestCase<T extends InternalAggregation> extends AbstractWireSerializingTestCase<T> { | ||
| public abstract class InternalAggregationTestCase<T extends InternalAggregation> extends AbstractNamedWriteableTestCase<T> { |
There was a problem hiding this comment.
I think it is worth adding a comment that subclasses need to make sure that xContentRegistry includes the agg we're going to parse. Built in aggs are included automatically but other aggs won't be.
There was a problem hiding this comment.
Buncha xpack tests needed this treatment, and it was non-trivial for tests to convert aggspecs into namedwriteables, so I did a bit of refactoring to make it easier in the general case.
| */ | ||
| @Override | ||
| @SuppressWarnings("unchecked") | ||
| protected Class<T> categoryClass() { |
There was a problem hiding this comment.
It could be. I left it non-final in case an implementor needs to override it for some reason. But I suppose with the generic bounds on <InternalAggregation> it's pretty definitive what the class should be...
There was a problem hiding this comment.
I do think this should be final then. I guess the suppresswarnings means that the bounds on AbstractNamedWriteableTestCase are wrong.
There was a problem hiding this comment.
Yeah to be honest I really have no idea what's going on with the generic... scratched my head over it for a while and just gave up :/
I'll make final 👍
| */ | ||
| protected List<NamedWriteableRegistry.Entry> getNamedWriteables() { | ||
| List<NamedWriteableRegistry.Entry> entries | ||
| = new ArrayList<>(new SearchModule(Settings.EMPTY, emptyList()).getNamedWriteables()); |
There was a problem hiding this comment.
If you pass the plugins in instead of emptyList() then I think SearchModule will do this stuff for you.
|
Thanks @nik9000! |
Some aggregations, such as the Terms* family, will use an alternate class to represent unmapped shard results (while the rest of the aggs use the same object but with some form of "empty" or "nullish" values to represent unmapped). This was problematic with AbstractWireSerializingTestCase because it expects the instanceReader to always match the original class. Instead, we need to use the NamedWriteable version so that the registry can be consulted for the proper deserialization reader.
Some aggregations, such as the Terms* family, will use an alternate class to represent unmapped shard results (while the rest of the aggs use the same object but with some form of "empty" or "nullish" values to represent unmapped).
This was problematic with AbstractWireSerializingTestCase because it expects the instanceReader to always match the original class. Instead, we need to use the NamedWriteable version so that the registry can be consulted for the proper deserialization reader.
Fixes #55149