diff --git a/fili-core/src/main/java/com/yahoo/bard/webservice/data/config/metric/makers/MetricMaker.java b/fili-core/src/main/java/com/yahoo/bard/webservice/data/config/metric/makers/MetricMaker.java index 115f4aa296..dee60ddba2 100644 --- a/fili-core/src/main/java/com/yahoo/bard/webservice/data/config/metric/makers/MetricMaker.java +++ b/fili-core/src/main/java/com/yahoo/bard/webservice/data/config/metric/makers/MetricMaker.java @@ -28,6 +28,9 @@ public abstract class MetricMaker { private static final Logger LOG = LoggerFactory.getLogger(MetricMaker.class); public static final NoOpResultSetMapper NO_OP_MAPPER = new NoOpResultSetMapper(); + private static final String SKETCH_REQUIRED_FORMAT = "Field must be a sketch: %s but is: %s"; + private static final String INCORRECT_NUMBER_OF_DEPS_FORMAT = "%s got %d of %d expected metrics"; + private static final String MISSING_DEP_FORMAT = "Dependent metric %s is not in the metric dictionary"; /** * The metric dictionary from which dependent metrics will be resolved. @@ -79,7 +82,7 @@ protected void assertRequiredDependentMetricCount(String dictionaryName, List dependentMetrics) { for (String dependentMetric : dependentMetrics) { if (!metrics.containsKey(dependentMetric)) { String message = String.format( - "Dependent metric %s is not in the metric dictionary", + MISSING_DEP_FORMAT, dependentMetric ); LOG.error(message); @@ -280,7 +283,7 @@ protected static PostAggregation getSketchField(MetricField field) { // Check for sketches, since we require them after this point if (!field.isSketch()) { - String message = String.format("Field must be a sketch: %s but is: %s", field.getName(), field); + String message = String.format(SKETCH_REQUIRED_FORMAT, field.getName(), field); LOG.error(message); throw new IllegalArgumentException(message); } diff --git a/fili-core/src/test/groovy/com/yahoo/bard/webservice/data/config/metric/makers/MetricMakerSpec.groovy b/fili-core/src/test/groovy/com/yahoo/bard/webservice/data/config/metric/makers/MetricMakerSpec.groovy index e659868bab..438900ac8f 100644 --- a/fili-core/src/test/groovy/com/yahoo/bard/webservice/data/config/metric/makers/MetricMakerSpec.groovy +++ b/fili-core/src/test/groovy/com/yahoo/bard/webservice/data/config/metric/makers/MetricMakerSpec.groovy @@ -2,6 +2,7 @@ // Licensed under the terms of the Apache license. Please see LICENSE.md file distributed with this work for terms. package com.yahoo.bard.webservice.data.config.metric.makers +import static com.yahoo.bard.webservice.data.config.metric.makers.MetricMaker.INCORRECT_NUMBER_OF_DEPS_FORMAT import static com.yahoo.bard.webservice.druid.model.postaggregation.ArithmeticPostAggregation.ArithmeticPostAggregationFunction.MULTIPLY import static com.yahoo.bard.webservice.druid.model.postaggregation.SketchSetOperationPostAggFunction.UNION @@ -10,6 +11,7 @@ import com.yahoo.bard.webservice.data.metric.MetricDictionary import com.yahoo.bard.webservice.data.metric.TemplateDruidQuery import com.yahoo.bard.webservice.data.metric.mappers.NoOpResultSetMapper import com.yahoo.bard.webservice.data.metric.mappers.SketchRoundUpMapper +import com.yahoo.bard.webservice.druid.model.MetricField import com.yahoo.bard.webservice.druid.model.aggregation.Aggregation import com.yahoo.bard.webservice.druid.model.aggregation.LongSumAggregation import com.yahoo.bard.webservice.druid.model.aggregation.ThetaSketchAggregation @@ -24,7 +26,6 @@ import com.yahoo.bard.webservice.druid.util.ThetaSketchFieldConverter import spock.lang.Specification import spock.lang.Unroll - /** * Tests the code implemented by MetricMaker, using a stub for code that is meant to be implemented by the children. * Primarily tests the sad paths. The specifications of the children of MetricMaker test the various happy paths. @@ -32,7 +33,7 @@ import spock.lang.Unroll class MetricMakerSpec extends Specification { static final String METRIC_NAME = "metric name" - static final int DEPENDENT_METRICS = 1 + static final int DEPENDENT_METRICS = 3 static final FieldConverters originalConverter = FieldConverterSupplier.sketchConverter static final FieldConverters CONVERTER = new ThetaSketchFieldConverter() @@ -158,40 +159,38 @@ class MetricMakerSpec extends Specification { } } - def "When a metric has too few dependent metrics, an exception is thrown."(){ - given: "One fewer dependent metrics than this metric depends on" - List dependentMetricNames = buildDependentMetricNames(DEPENDENT_METRICS - 1) + def "When a claimed dependent metric does not exist in the maker's dictionary, an exception is thrown"(){ + given: "A list of dependent metrics, one of which doesn't exist" + List dependentMetricNames = buildDependentMetricNames(DEPENDENT_METRICS) dictionary.putAll(makeEmptyMetrics(dependentMetricNames)) + String badName = "I don't exist!" + dependentMetricNames.add(badName) when: - getMakerInstance().make(METRIC_NAME, dependentMetricNames) + maker.make(METRIC_NAME, dependentMetricNames) then: - thrown(IllegalArgumentException) + Exception exception = thrown(IllegalArgumentException) + exception.message == String.format(MetricMaker.MISSING_DEP_FORMAT, badName) } - def "When a claimed dependent metric does not exist in the maker's dictionary, an exception is thrown"(){ - given: "A list of dependent metrics, one of which doesn't exist" - List dependentMetricNames = buildDependentMetricNames(DEPENDENT_METRICS - 1) - dictionary.putAll(makeEmptyMetrics(dependentMetricNames)) - dependentMetricNames.add("I don't exist!") - - when: - maker.make(METRIC_NAME, dependentMetricNames) - - then: - thrown(IllegalArgumentException) - } - def "When a metric has too many dependent metrics, an exception is thrown"(){ - given: "One more than the number of dependent metrics this maker depends on." - List dependentMetricNames = buildDependentMetricNames(maker.getDependentMetricsRequired() + 1) - maker.metrics.putAll(makeEmptyMetrics(dependentMetricNames)) + @Unroll + def "When a metric has too #adjective dependent metrics, an exception is thrown."(){ + given: "One fewer dependent metrics than this metric depends on" + List dependentMetricNames = buildDependentMetricNames(DEPENDENT_METRICS+2) + dictionary.putAll(makeEmptyMetrics(dependentMetricNames)) when: - maker.make(METRIC_NAME, dependentMetricNames) + getMakerInstance().make(METRIC_NAME, dependentMetricNames.subList(0, DEPENDENT_METRICS + adjustment)) then: - thrown(IllegalArgumentException) + Exception exception = thrown(IllegalArgumentException) + exception.getMessage() == String.format(INCORRECT_NUMBER_OF_DEPS_FORMAT, METRIC_NAME, DEPENDENT_METRICS+adjustment, DEPENDENT_METRICS) + + where: + adjective | adjustment + "many" | 1 + "few" | -1 } @Unroll @@ -226,10 +225,12 @@ class MetricMakerSpec extends Specification { @Unroll def "getSketchField throws error when given a non sketch based field #fromMetric.name "() { when: - MetricMaker.getSketchField(fromMetric.metricField) + MetricField field = fromMetric.metricField + MetricMaker.getSketchField(field) then: - thrown(IllegalArgumentException) + Exception exception = thrown(IllegalArgumentException) + exception.getMessage() == String.format(MetricMaker.SKETCH_REQUIRED_FORMAT, field.name, field) where: fromMetric << [constantMetric, squareMetric, longSumMetric]