SONAR-5095 Catch validation exceptions to provide a clearer message

2 files changed, 91 insertions, 52 deletions

diff --git a/sonar-batch/src/main/java/org/sonar/batch/qualitygate/ConditionUtils.java b/sonar-batch/src/main/java/org/sonar/batch/qualitygate/ConditionUtils.java
index f82259d62af..18494255960 100644
--- a/sonar-batch/src/main/java/org/sonar/batch/qualitygate/ConditionUtils.java
+++ b/sonar-batch/src/main/java/org/sonar/batch/qualitygate/ConditionUtils.java
@@ -92,22 +92,25 @@ class ConditionUtils {
   }
 
   private static Comparable getValueForComparison(Metric metric, String value) {
-    if (isADouble(metric)) {
-      return Double.parseDouble(value);
-    }
-    if (isAInteger(metric)) {
-      return parseInteger(value);
-    }
-    if (isAString(metric)) {
-      return value;
-    }
-    if (isABoolean(metric)) {
-      return Integer.parseInt(value);
-    }
-    if (isAWorkDuration(metric)) {
-      return Long.parseLong(value);
-    }
-    throw new NotImplementedException(metric.getType().toString());
+    Comparable valueToCompare = null;
+    try {
+      if (isADouble(metric)) {
+        valueToCompare = Double.parseDouble(value);
+      } else if (isAInteger(metric)) {
+        valueToCompare = parseInteger(value);
+      } else if (isAString(metric)) {
+        valueToCompare = value;
+      } else if (isABoolean(metric)) {
+        valueToCompare = Integer.parseInt(value);
+      } else if (isAWorkDuration(metric)) {
+        valueToCompare = Long.parseLong(value);
+      } else {
+        throw new NotImplementedException(metric.getType().toString());
+      }
+    } catch (NumberFormatException badValueFormat) {
+      throw new IllegalArgumentException(String.format("Unable to parse value '%s' to compare against %s", value, metric.getName()));
+    }
+    return valueToCompare;
   }
 
   private static Comparable<Integer> parseInteger(String value) {
@@ -176,16 +179,17 @@ class ConditionUtils {
   }
 
   private static Double getValue(ResolvedCondition condition, Measure measure) {
-    if (condition.period() == null) {
+    Integer period = condition.period();
+    if (period == null) {
       return measure.getValue();
-    } else if (condition.period() == 1) {
+    } else if (period == 1) {
       return measure.getVariation1();
-    } else if (condition.period() == 2) {
+    } else if (period == 2) {
       return measure.getVariation2();
-    } else if (condition.period() == 3) {
+    } else if (period == 3) {
       return measure.getVariation3();
     } else {
-      throw new IllegalStateException("Following index period is not allowed : " + Double.toString(condition.period()));
+      throw new IllegalStateException("Following index period is not allowed : " + Double.toString(period));
     }
   }
 }
diff --git a/sonar-batch/src/test/java/org/sonar/batch/qualitygate/ConditionUtilsTest.java b/sonar-batch/src/test/java/org/sonar/batch/qualitygate/ConditionUtilsTest.java
index 5b1c0136e42..6ea5bee366f 100644
--- a/sonar-batch/src/test/java/org/sonar/batch/qualitygate/ConditionUtilsTest.java
+++ b/sonar-batch/src/test/java/org/sonar/batch/qualitygate/ConditionUtilsTest.java
@@ -20,13 +20,15 @@
 
 package org.sonar.batch.qualitygate;
 
-import org.junit.Assert;
+import org.apache.commons.lang.NotImplementedException;
 import org.junit.Before;
 import org.junit.Test;
 import org.sonar.api.measures.Measure;
 import org.sonar.api.measures.Metric;
 import org.sonar.core.qualitygate.db.QualityGateConditionDto;
 
+import static org.fest.assertions.Assertions.assertThat;
+import static org.fest.assertions.Fail.fail;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
@@ -57,7 +59,7 @@ public class ConditionUtilsTest {
       when(condition.errorThreshold()).thenReturn("20");
       ConditionUtils.getLevel(condition, measure);
     } catch (NumberFormatException ex) {
-      Assert.fail();
+      fail();
     }
 
     try {
@@ -65,7 +67,7 @@ public class ConditionUtilsTest {
       when(condition.errorThreshold()).thenReturn("20.1");
       ConditionUtils.getLevel(condition, measure);
     } catch (NumberFormatException ex) {
-      Assert.fail();
+      fail();
     }
 
     try {
@@ -73,7 +75,7 @@ public class ConditionUtilsTest {
       when(condition.errorThreshold()).thenReturn("20.1");
       ConditionUtils.getLevel(condition, measure);
     } catch (NumberFormatException ex) {
-      Assert.fail();
+      fail();
     }
   }
 
@@ -86,20 +88,20 @@ public class ConditionUtilsTest {
     when(condition.metric()).thenReturn(metric);
 
     when(condition.errorThreshold()).thenReturn("10.2");
-    Assert.assertEquals(Metric.Level.ERROR, ConditionUtils.getLevel(condition, measure));
+    assertThat(ConditionUtils.getLevel(condition, measure)).isEqualTo(Metric.Level.ERROR);
 
     when(condition.errorThreshold()).thenReturn("10.1");
-    Assert.assertEquals(Metric.Level.OK, ConditionUtils.getLevel(condition, measure));
+    assertThat(ConditionUtils.getLevel(condition, measure)).isEqualTo(Metric.Level.OK);
 
     metric.setType(Metric.ValueType.STRING);
     measure.setData("TEST");
     measure.setValue(null);
 
     when(condition.errorThreshold()).thenReturn("TEST");
-    Assert.assertEquals(Metric.Level.ERROR, ConditionUtils.getLevel(condition, measure));
+    assertThat(ConditionUtils.getLevel(condition, measure)).isEqualTo(Metric.Level.ERROR);
 
     when(condition.errorThreshold()).thenReturn("TEST2");
-    Assert.assertEquals(Metric.Level.OK, ConditionUtils.getLevel(condition, measure));
+    assertThat(ConditionUtils.getLevel(condition, measure)).isEqualTo(Metric.Level.OK);
 
   }
 
@@ -112,20 +114,20 @@ public class ConditionUtilsTest {
     when(condition.metric()).thenReturn(metric);
 
     when(condition.errorThreshold()).thenReturn("10.2");
-    Assert.assertEquals(Metric.Level.OK, ConditionUtils.getLevel(condition, measure));
+    assertThat(ConditionUtils.getLevel(condition, measure)).isEqualTo(Metric.Level.OK);
 
     when(condition.errorThreshold()).thenReturn("10.1");
-    Assert.assertEquals(Metric.Level.ERROR, ConditionUtils.getLevel(condition, measure));
+    assertThat(ConditionUtils.getLevel(condition, measure)).isEqualTo(Metric.Level.ERROR);
 
     metric.setType(Metric.ValueType.STRING);
     measure.setData("TEST");
     measure.setValue(null);
 
     when(condition.errorThreshold()).thenReturn("TEST");
-    Assert.assertEquals(Metric.Level.OK, ConditionUtils.getLevel(condition, measure));
+    assertThat(ConditionUtils.getLevel(condition, measure)).isEqualTo(Metric.Level.OK);
 
     when(condition.errorThreshold()).thenReturn("TEST2");
-    Assert.assertEquals(Metric.Level.ERROR, ConditionUtils.getLevel(condition, measure));
+    assertThat(ConditionUtils.getLevel(condition, measure)).isEqualTo(Metric.Level.ERROR);
 
   }
 
@@ -137,10 +139,10 @@ public class ConditionUtilsTest {
     when(condition.metric()).thenReturn(metric);
 
     when(condition.errorThreshold()).thenReturn("10.1");
-    Assert.assertEquals(Metric.Level.ERROR, ConditionUtils.getLevel(condition, measure));
+    assertThat(ConditionUtils.getLevel(condition, measure)).isEqualTo(Metric.Level.ERROR);
 
     when(condition.errorThreshold()).thenReturn("10.3");
-    Assert.assertEquals(Metric.Level.OK, ConditionUtils.getLevel(condition, measure));
+    assertThat(ConditionUtils.getLevel(condition, measure)).isEqualTo(Metric.Level.OK);
   }
 
   @Test
@@ -151,10 +153,10 @@ public class ConditionUtilsTest {
     when(condition.metric()).thenReturn(metric);
 
     when(condition.errorThreshold()).thenReturn("10.1");
-    Assert.assertEquals(Metric.Level.OK, ConditionUtils.getLevel(condition, measure));
+    assertThat(ConditionUtils.getLevel(condition, measure)).isEqualTo(Metric.Level.OK);
 
     when(condition.errorThreshold()).thenReturn("10.3");
-    Assert.assertEquals(Metric.Level.ERROR, ConditionUtils.getLevel(condition, measure));
+    assertThat(ConditionUtils.getLevel(condition, measure)).isEqualTo(Metric.Level.ERROR);
   }
 
   @Test
@@ -165,7 +167,7 @@ public class ConditionUtilsTest {
     when(condition.metric()).thenReturn(metric);
 
     when(condition.errorThreshold()).thenReturn("10.2");
-    Assert.assertEquals(Metric.Level.ERROR, ConditionUtils.getLevel(condition, measure));
+    assertThat(ConditionUtils.getLevel(condition, measure)).isEqualTo(Metric.Level.ERROR);
   }
 
   @Test
@@ -176,7 +178,7 @@ public class ConditionUtilsTest {
     when(condition.metric()).thenReturn(metric);
 
     when(condition.errorThreshold()).thenReturn("10.2");
-    Assert.assertEquals(Metric.Level.ERROR, ConditionUtils.getLevel(condition, measure));
+    assertThat(ConditionUtils.getLevel(condition, measure)).isEqualTo(Metric.Level.ERROR);
   }
 
   @Test
@@ -187,10 +189,10 @@ public class ConditionUtilsTest {
     when(condition.metric()).thenReturn(metric);
 
     when(condition.errorThreshold()).thenReturn("10");
-    Assert.assertEquals(Metric.Level.ERROR, ConditionUtils.getLevel(condition, measure));
+    assertThat(ConditionUtils.getLevel(condition, measure)).isEqualTo(Metric.Level.ERROR);
 
     when(condition.errorThreshold()).thenReturn("10.2");
-    Assert.assertEquals(Metric.Level.ERROR, ConditionUtils.getLevel(condition, measure));
+    assertThat(ConditionUtils.getLevel(condition, measure)).isEqualTo(Metric.Level.ERROR);
   }
 
   @Test
@@ -201,13 +203,13 @@ public class ConditionUtilsTest {
     when(condition.metric()).thenReturn(metric);
 
     when(condition.errorThreshold()).thenReturn(Metric.Level.ERROR.toString());
-    Assert.assertEquals(Metric.Level.ERROR, ConditionUtils.getLevel(condition, measure));
+    assertThat(ConditionUtils.getLevel(condition, measure)).isEqualTo(Metric.Level.ERROR);
 
     when(condition.errorThreshold()).thenReturn(Metric.Level.OK.toString());
-    Assert.assertEquals(Metric.Level.OK, ConditionUtils.getLevel(condition, measure));
+    assertThat(ConditionUtils.getLevel(condition, measure)).isEqualTo(Metric.Level.OK);
 
     when(condition.operator()).thenReturn(QualityGateConditionDto.OPERATOR_NOT_EQUALS);
-    Assert.assertEquals(Metric.Level.ERROR, ConditionUtils.getLevel(condition, measure));
+    assertThat(ConditionUtils.getLevel(condition, measure)).isEqualTo(Metric.Level.ERROR);
   }
 
   @Test
@@ -218,17 +220,25 @@ public class ConditionUtilsTest {
     when(condition.metric()).thenReturn(metric);
 
     when(condition.errorThreshold()).thenReturn("1");
-    Assert.assertEquals(Metric.Level.OK, ConditionUtils.getLevel(condition, measure));
+    assertThat(ConditionUtils.getLevel(condition, measure)).isEqualTo(Metric.Level.OK);
 
     when(condition.errorThreshold()).thenReturn("0");
-    Assert.assertEquals(Metric.Level.ERROR, ConditionUtils.getLevel(condition, measure));
+    assertThat(ConditionUtils.getLevel(condition, measure)).isEqualTo(Metric.Level.ERROR);
 
     when(condition.operator()).thenReturn(QualityGateConditionDto.OPERATOR_NOT_EQUALS);
     when(condition.errorThreshold()).thenReturn("1");
-    Assert.assertEquals(Metric.Level.ERROR, ConditionUtils.getLevel(condition, measure));
+    assertThat(ConditionUtils.getLevel(condition, measure)).isEqualTo(Metric.Level.ERROR);
 
     when(condition.errorThreshold()).thenReturn("0");
-    Assert.assertEquals(Metric.Level.OK, ConditionUtils.getLevel(condition, measure));
+    assertThat(ConditionUtils.getLevel(condition, measure)).isEqualTo(Metric.Level.OK);
+
+    when(condition.errorThreshold()).thenReturn("polop");
+    try {
+      ConditionUtils.getLevel(condition, measure);
+      fail();
+    } catch(Exception expected) {
+      assertThat(expected).isInstanceOf(IllegalArgumentException.class).hasMessage("Unable to parse value 'polop' to compare against name");
+    }
   }
 
   @Test
@@ -239,7 +249,15 @@ public class ConditionUtilsTest {
     when(condition.metric()).thenReturn(metric);
 
     when(condition.errorThreshold()).thenReturn("60");
-    Assert.assertEquals(Metric.Level.ERROR, ConditionUtils.getLevel(condition, measure));
+    assertThat(ConditionUtils.getLevel(condition, measure)).isEqualTo(Metric.Level.ERROR);
+
+    when(condition.errorThreshold()).thenReturn("polop");
+    try {
+      ConditionUtils.getLevel(condition, measure);
+      fail();
+    } catch(Exception expected) {
+      assertThat(expected).isInstanceOf(IllegalArgumentException.class).hasMessage("Unable to parse value 'polop' to compare against name");
+    }
   }
 
   @Test
@@ -250,13 +268,30 @@ public class ConditionUtilsTest {
     when(condition.metric()).thenReturn(metric);
 
     when(condition.errorThreshold()).thenReturn("10.2");
-    Assert.assertEquals(Metric.Level.ERROR, ConditionUtils.getLevel(condition, measure));
+    assertThat(ConditionUtils.getLevel(condition, measure)).isEqualTo(Metric.Level.ERROR);
 
     when(condition.errorThreshold()).thenReturn("10.1");
-    Assert.assertEquals(Metric.Level.OK, ConditionUtils.getLevel(condition, measure));
+    assertThat(ConditionUtils.getLevel(condition, measure)).isEqualTo(Metric.Level.OK);
 
     when(condition.errorThreshold()).thenReturn("10.3");
     when(condition.warningThreshold()).thenReturn("10.2");
-    Assert.assertEquals(Metric.Level.WARN, ConditionUtils.getLevel(condition, measure));
+    assertThat(ConditionUtils.getLevel(condition, measure)).isEqualTo(Metric.Level.WARN);
+  }
+
+  @Test
+  public void testUnsupportedType() {
+    metric.setType(Metric.ValueType.DATA);
+    measure.setValue(3.14159265358);
+    when(condition.operator()).thenReturn(QualityGateConditionDto.OPERATOR_EQUALS);
+    when(condition.metric()).thenReturn(metric);
+
+    when(condition.errorThreshold()).thenReturn("1.60217657");
+    try {
+      ConditionUtils.getLevel(condition, measure);
+      fail();
+    } catch (Exception expected) {
+      assertThat(expected).isInstanceOf(NotImplementedException.class);
+    }
   }
+
 }