From daaf5a3dcc6e1fc4890b78b2250944f87b9423d5 Mon Sep 17 00:00:00 2001 From: Zachary Tong Date: Mon, 1 Jun 2020 12:10:05 -0400 Subject: [PATCH] Fix assertion catching in aggregation supported type test (#56466) (#57382) At some point, we changed the supported-type test to also catch assertion errors. This has the side effect of also catching the `fail()` call inside the try-catch, which silently smothered some failures. This modifies the test to throw at the end of the try-catch block to prevent from accidentally catching itself. Catching the AssertionError is convenient because there are other locations that do throw an assertion in tests (due to hitting an assertion before the exception is thrown) so I think we should keep it around. Also includes a variety of fixes to other tests which were failing but being silently smothered. --- .../terms/SignificantTextAggregatorTests.java | 2 +- .../metrics/AvgAggregatorTests.java | 4 +- .../metrics/MaxAggregatorTests.java | 4 +- .../metrics/StatsAggregatorTests.java | 7 ++-- .../metrics/SumAggregatorTests.java | 4 +- .../metrics/ValueCountAggregatorTests.java | 5 ++- .../aggregations/AggregatorTestCase.java | 25 +++++++----- .../HistoBackedAvgAggregatorTests.java | 2 +- .../HistoBackedSumAggregatorTests.java | 2 +- .../HistoBackedValueCountAggregatorTests.java | 3 ++ .../analytics/ttest/TTestAggregatorTests.java | 40 ++++++++++++++++++- 11 files changed, 72 insertions(+), 26 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/SignificantTextAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/SignificantTextAggregatorTests.java index a1a135ea58f..0a04c508f2d 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/SignificantTextAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/SignificantTextAggregatorTests.java @@ -84,7 +84,7 @@ public class SignificantTextAggregatorTests extends AggregatorTestCase { sigAgg.sourceFieldNames(Arrays.asList(new String [] {"json_only_field"})); } SamplerAggregationBuilder aggBuilder = new SamplerAggregationBuilder("sampler") - .subAggregation(sigAgg); + .subAggregation(sigAgg); try (IndexReader reader = DirectoryReader.open(w)) { assertEquals("test expects a single segment", 1, reader.leaves().size()); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/AvgAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/AvgAggregatorTests.java index 783967e2cfa..fee3e8a03db 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/AvgAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/AvgAggregatorTests.java @@ -704,8 +704,8 @@ public class AvgAggregatorTests extends AggregatorTestCase { protected List getSupportedValuesSourceTypes() { return Arrays.asList( CoreValuesSourceType.NUMERIC, - CoreValuesSourceType.DATE, - CoreValuesSourceType.BOOLEAN); + CoreValuesSourceType.BOOLEAN, + CoreValuesSourceType.DATE); } @Override diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/MaxAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/MaxAggregatorTests.java index f0fe490365d..2ced446413d 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/MaxAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/MaxAggregatorTests.java @@ -164,8 +164,8 @@ public class MaxAggregatorTests extends AggregatorTestCase { protected List getSupportedValuesSourceTypes() { return Arrays.asList( CoreValuesSourceType.NUMERIC, - CoreValuesSourceType.DATE, - CoreValuesSourceType.BOOLEAN + CoreValuesSourceType.BOOLEAN, + CoreValuesSourceType.DATE ); } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/StatsAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/StatsAggregatorTests.java index 97d548ebda9..21e298fa3c3 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/StatsAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/StatsAggregatorTests.java @@ -475,10 +475,9 @@ public class StatsAggregatorTests extends AggregatorTestCase { @Override protected List getSupportedValuesSourceTypes() { - return Arrays.asList( - CoreValuesSourceType.NUMERIC, - CoreValuesSourceType.DATE, - CoreValuesSourceType.BOOLEAN); + return Arrays.asList(CoreValuesSourceType.NUMERIC, + CoreValuesSourceType.BOOLEAN, + CoreValuesSourceType.DATE); } @Override diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/SumAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/SumAggregatorTests.java index 8acb5b3cfd8..16eee09952c 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/SumAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/SumAggregatorTests.java @@ -403,8 +403,8 @@ public class SumAggregatorTests extends AggregatorTestCase { protected List getSupportedValuesSourceTypes() { return Arrays.asList( CoreValuesSourceType.NUMERIC, - CoreValuesSourceType.DATE, - CoreValuesSourceType.BOOLEAN); + CoreValuesSourceType.BOOLEAN, + CoreValuesSourceType.DATE); } @Override diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregatorTests.java index 4debbbde4f5..bfd567b7ce5 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregatorTests.java @@ -93,7 +93,10 @@ public class ValueCountAggregatorTests extends AggregatorTestCase { CoreValuesSourceType.BYTES, CoreValuesSourceType.IP, CoreValuesSourceType.GEOPOINT, - CoreValuesSourceType.RANGE + CoreValuesSourceType.RANGE, + CoreValuesSourceType.BOOLEAN, + CoreValuesSourceType.DATE, + CoreValuesSourceType.IP ); } diff --git a/test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java b/test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java index 4e29b1418cf..2ebb6fc84ac 100644 --- a/test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java @@ -188,7 +188,7 @@ public abstract class AggregatorTestCase extends ESTestCase { // Make this @Before instead of @BeforeClass so it can call the non-static getSearchPlugins method @Before public void initValuesSourceRegistry() { - SearchModule searchModule = new SearchModule(Settings.EMPTY, false, getSearchPlugins()); + SearchModule searchModule = new SearchModule(Settings.EMPTY, false, this.getSearchPlugins()); valuesSourceRegistry = searchModule.getValuesSourceRegistry(); } @@ -327,7 +327,8 @@ public abstract class AggregatorTestCase extends ESTestCase { .collect(Collectors.toMap(MappedFieldType::name, Function.identity()))); fieldNameToType.putAll(getFieldAliases(fieldTypes)); - registerFieldTypes(searchContext, mapperService, fieldNameToType); + registerFieldTypes(searchContext, mapperService, + fieldNameToType); doAnswer(invocation -> { /* Store the release-ables so we can release them at the end of the test case. This is important because aggregations don't * close their sub-aggregations. This is fairly similar to what the production code does. */ @@ -721,18 +722,23 @@ public abstract class AggregatorTestCase extends ESTestCase { ValuesSourceType vst = fieldType.getValuesSourceType(); // TODO in the future we can make this more explicit with expectThrows(), when the exceptions are standardized + AssertionError failure = null; try { searchAndReduce(indexSearcher, new MatchAllDocsQuery(), aggregationBuilder, fieldType); if (supportedVSTypes.contains(vst) == false || unsupportedMappedFieldTypes.contains(fieldType.typeName())) { - fail("Aggregator [" + aggregationBuilder.getType() + "] should not support field type [" + failure = new AssertionError("Aggregator [" + aggregationBuilder.getType() + "] should not support field type [" + fieldType.typeName() + "] but executing against the field did not throw an exception"); } - } catch (Exception e) { + } catch (Exception | AssertionError e) { if (supportedVSTypes.contains(vst) && unsupportedMappedFieldTypes.contains(fieldType.typeName()) == false) { - fail("Aggregator [" + aggregationBuilder.getType() + "] supports field type [" - + fieldType.typeName() + "] but executing against the field threw an exception: [" + e.getMessage() + "]"); + failure = new AssertionError("Aggregator [" + aggregationBuilder.getType() + "] supports field type [" + + fieldType.typeName() + "] but executing against the field threw an exception: [" + e.getMessage() + "]", + e); } } + if (failure != null) { + throw failure; + } } } } @@ -768,8 +774,8 @@ public abstract class AggregatorTestCase extends ESTestCase { json = "{ \"" + fieldName + "\" : \"" + f + "\" }"; } else { // smallest numeric is a byte so we select the smallest - v = Math.abs(randomByte()); - json = "{ \"" + fieldName + "\" : \"" + v + "\" }"; + v = Math.abs(randomByte()); + json = "{ \"" + fieldName + "\" : \"" + v + "\" }"; } doc.add(new SortedNumericDocValuesField(fieldName, v)); @@ -836,8 +842,7 @@ public abstract class AggregatorTestCase extends ESTestCase { " \"gte\" : \"" + start + "\",\n" + " \"lte\" : \"" + end + "\"\n" + " }}"; - - } else if (fieldType.getValuesSourceType().equals(CoreValuesSourceType.GEOPOINT)) { + } else if (vst.equals(CoreValuesSourceType.GEOPOINT)) { double lat = randomDouble(); double lon = randomDouble(); doc.add(new LatLonDocValuesField(fieldName, lat, lon)); diff --git a/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/aggregations/metrics/HistoBackedAvgAggregatorTests.java b/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/aggregations/metrics/HistoBackedAvgAggregatorTests.java index 737e73903c6..8d679df9235 100644 --- a/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/aggregations/metrics/HistoBackedAvgAggregatorTests.java +++ b/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/aggregations/metrics/HistoBackedAvgAggregatorTests.java @@ -137,8 +137,8 @@ public class HistoBackedAvgAggregatorTests extends AggregatorTestCase { // Note: this is the same list as Core, plus Analytics return org.elasticsearch.common.collect.List.of( CoreValuesSourceType.NUMERIC, - CoreValuesSourceType.DATE, CoreValuesSourceType.BOOLEAN, + CoreValuesSourceType.DATE, AnalyticsValuesSourceType.HISTOGRAM ); } diff --git a/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/aggregations/metrics/HistoBackedSumAggregatorTests.java b/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/aggregations/metrics/HistoBackedSumAggregatorTests.java index 2e84fa7d261..01fc080b0d1 100644 --- a/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/aggregations/metrics/HistoBackedSumAggregatorTests.java +++ b/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/aggregations/metrics/HistoBackedSumAggregatorTests.java @@ -137,8 +137,8 @@ public class HistoBackedSumAggregatorTests extends AggregatorTestCase { // Note: this is the same list as Core, plus Analytics return org.elasticsearch.common.collect.List.of( CoreValuesSourceType.NUMERIC, - CoreValuesSourceType.DATE, CoreValuesSourceType.BOOLEAN, + CoreValuesSourceType.DATE, AnalyticsValuesSourceType.HISTOGRAM ); } diff --git a/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/aggregations/metrics/HistoBackedValueCountAggregatorTests.java b/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/aggregations/metrics/HistoBackedValueCountAggregatorTests.java index 0ff62e0861f..f6bab3768fb 100644 --- a/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/aggregations/metrics/HistoBackedValueCountAggregatorTests.java +++ b/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/aggregations/metrics/HistoBackedValueCountAggregatorTests.java @@ -144,6 +144,9 @@ public class HistoBackedValueCountAggregatorTests extends AggregatorTestCase { CoreValuesSourceType.IP, CoreValuesSourceType.GEOPOINT, CoreValuesSourceType.RANGE, + CoreValuesSourceType.BOOLEAN, + CoreValuesSourceType.DATE, + CoreValuesSourceType.IP, AnalyticsValuesSourceType.HISTOGRAM ); } diff --git a/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/ttest/TTestAggregatorTests.java b/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/ttest/TTestAggregatorTests.java index 5980146ed78..d0f7682e3e4 100644 --- a/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/ttest/TTestAggregatorTests.java +++ b/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/ttest/TTestAggregatorTests.java @@ -14,7 +14,10 @@ import org.apache.lucene.search.MatchAllDocsQuery; import org.apache.lucene.search.Query; import org.elasticsearch.common.CheckedConsumer; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.time.DateUtils; import org.elasticsearch.index.fielddata.ScriptDocValues; +import org.elasticsearch.index.mapper.BooleanFieldMapper; +import org.elasticsearch.index.mapper.DateFieldMapper; import org.elasticsearch.index.mapper.KeywordFieldMapper; import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.mapper.NumberFieldMapper; @@ -34,11 +37,14 @@ import org.elasticsearch.search.aggregations.bucket.global.InternalGlobal; import org.elasticsearch.search.aggregations.bucket.histogram.HistogramAggregationBuilder; import org.elasticsearch.search.aggregations.bucket.histogram.InternalHistogram; import org.elasticsearch.search.aggregations.support.AggregationInspectionHelper; +import org.elasticsearch.search.aggregations.support.CoreValuesSourceType; import org.elasticsearch.search.aggregations.support.MultiValuesSourceFieldConfig; +import org.elasticsearch.search.aggregations.support.ValuesSourceType; import org.elasticsearch.search.lookup.LeafDocLookup; import org.elasticsearch.xpack.analytics.AnalyticsPlugin; import java.io.IOException; +import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.List; @@ -60,13 +66,43 @@ public class TTestAggregatorTests extends AggregatorTestCase { @Override protected AggregationBuilder createAggBuilderForTypeTest(MappedFieldType fieldType, String fieldName) { + if (fieldType instanceof NumberFieldMapper.NumberFieldType) { + return new TTestAggregationBuilder("foo") + .a(new MultiValuesSourceFieldConfig.Builder().setFieldName(fieldName) + .setFilter(QueryBuilders.rangeQuery(fieldName).lt(10)).build()) + .b(new MultiValuesSourceFieldConfig.Builder().setFieldName(fieldName) + .setFilter(QueryBuilders.rangeQuery(fieldName).gte(10)).build()); + } else if (fieldType.typeName().equals(DateFieldMapper.CONTENT_TYPE) + || fieldType.typeName().equals(DateFieldMapper.DATE_NANOS_CONTENT_TYPE)) { + + return new TTestAggregationBuilder("foo") + .a(new MultiValuesSourceFieldConfig.Builder().setFieldName(fieldName) + .setFilter(QueryBuilders.rangeQuery(fieldName).lt(DateUtils.toInstant(10))).build()) + .b(new MultiValuesSourceFieldConfig.Builder().setFieldName(fieldName) + .setFilter(QueryBuilders.rangeQuery(fieldName).gte(DateUtils.toInstant(10))).build()); + } else if (fieldType.typeName().equals(BooleanFieldMapper.CONTENT_TYPE)) { + return new TTestAggregationBuilder("foo") + .a(new MultiValuesSourceFieldConfig.Builder().setFieldName(fieldName) + .setFilter(QueryBuilders.rangeQuery(fieldName).lt("true")).build()) + .b(new MultiValuesSourceFieldConfig.Builder().setFieldName(fieldName) + .setFilter(QueryBuilders.rangeQuery(fieldName).gte("false")).build()); + } + // if it's "unsupported" just use matchall filters to avoid parsing issues return new TTestAggregationBuilder("foo") .a(new MultiValuesSourceFieldConfig.Builder().setFieldName(fieldName) - .setFilter(QueryBuilders.rangeQuery(fieldName).lt(10)).build()) + .setFilter(QueryBuilders.matchAllQuery()).build()) .b(new MultiValuesSourceFieldConfig.Builder().setFieldName(fieldName) - .setFilter(QueryBuilders.rangeQuery(fieldName).gte(10)).build()); + .setFilter(QueryBuilders.matchAllQuery()).build()); } + @Override + protected List getSupportedValuesSourceTypes() { + return Arrays.asList(CoreValuesSourceType.NUMERIC, + CoreValuesSourceType.BOOLEAN, + CoreValuesSourceType.DATE); + } + + @Override protected ScriptService getMockScriptService() { Map, Object>> scripts = new HashMap<>();