Tightened testing on metric maker errors

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<St
         int actualCount = dependentMetrics.size();
         if (actualCount != requiredCount) {
             String message = String.format(
-                    "%s got %d of %d dependent metrics",
+                    INCORRECT_NUMBER_OF_DEPS_FORMAT,
                     dictionaryName,
                     actualCount,
                     requiredCount
@@ -98,7 +101,7 @@ protected void assertDependentMetricsExist(List<String> 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);
         }

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,15 +26,14 @@ 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.
  */
 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<String> 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<String> 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<String> 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<String> 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<String> 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]