add tests for missing aggregation #53214
Conversation
Test with unmapped fields and using the missing parameter, which isn't very useful with this aggregation but does work as expected. Also includes yaml tests For elastic#42949
andyb-elastic
added
>test
|
Pinging @elastic/es-analytics-geo (:Analytics/Geo) |
| assertTrue(AggregationInspectionHelper.hasValue(internalMissing)); | ||
| }); | ||
| }, | ||
| List.of(fieldType, anotherFieldType)); |
There was a problem hiding this comment.
Note: in some of the aggregator tests similar to this one (where there's a second field we expect to be absent sometimes), we don't pass a field type for the second field like above. If the test is missing the second field type, that field can't be read even if an aggregator was erroneously trying to collect it
There was a problem hiding this comment.
We should fix that, could you open a ticket so we don't forget please?
There was a problem hiding this comment.
I opened #53250
polyfractal
added
:Analytics/Aggregations
There was a problem hiding this comment.
Test changes look good, thanks for beefing them up!
Let's also add in the new "supported type" tests. Here's a recently-merged example for terms agg: https://github.com/elastic/elasticsearch/blob/master/server/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorTests.java#L127-L136
Missing agg supporting missing... I guess I'm not surprised but yeah, what a not-useful option. I wonder if we should deprecate that in the future....
Edit: Oh, let's add a test for scripting too, while we're at it? Here's an example from the Avg tests: https://github.com/elastic/elasticsearch/blob/master/server/src/test/java/org/elasticsearch/search/aggregations/metrics/AvgAggregatorTests.java#L341
|
Thanks, I'll add tests for scripting and supported types. For some reason I thought the supported types tests weren't being run in master yet |
Also change the test case pattern to be more flexible and more similar to how the other agg tests are written
|
@polyfractal added some scripting tests. Ended up changing the rest of the test cases to bring them more in line with the other agg tests and (I hope) make them a little more readable |
|
Thanks! |
Tests for unmapped fields, the missing parameter, scripting, and correct ValuesSource types in MissingAggregatorTests. Basic yaml tests for the missing agg For #42949
5 participants
Test with unmapped fields and using the missing parameter, which isn't
very useful with this aggregation but does work as expected. Also
includes yaml tests
For #42949