Fix some quality flaws

2 files changed, 201 insertions, 15 deletions

diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/batch/sensor/test/internal/DefaultCoverage.java b/sonar-plugin-api/src/main/java/org/sonar/api/batch/sensor/test/internal/DefaultCoverage.java
index 4671dab8620..451ec6d912e 100644
--- a/sonar-plugin-api/src/main/java/org/sonar/api/batch/sensor/test/internal/DefaultCoverage.java
+++ b/sonar-plugin-api/src/main/java/org/sonar/api/batch/sensor/test/internal/DefaultCoverage.java
@@ -41,7 +41,7 @@ public final class DefaultCoverage implements Coverage {
   private SortedMap<Integer, Integer> conditionsByLine = Maps.newTreeMap();
   private SortedMap<Integer, Integer> coveredConditionsByLine = Maps.newTreeMap();
 
-  protected transient final SensorStorage storage;
+  protected final transient SensorStorage storage;
   private transient boolean saved = false;
 
   public DefaultCoverage() {
@@ -54,23 +54,27 @@ public final class DefaultCoverage implements Coverage {
 
   @Override
   public DefaultCoverage lineHits(int lineId, int hits) {
-    if (!hitsByLine.containsKey(lineId)) {
-      hitsByLine.put(lineId, hits);
-      if (hits > 0) {
-        totalCoveredLines += 1;
-      }
+    Preconditions.checkArgument(lineId >= 1, "Line number should be positive and non zero [" + file.relativePath() + ":" + lineId + "]");
+    Preconditions.checkArgument(hits >= 0, "Hits should be positive [" + file.relativePath() + ":" + lineId + "]");
+    Preconditions.checkArgument(!hitsByLine.containsKey(lineId), "Hits already saved on line [" + file.relativePath() + ":" + lineId + "]");
+    hitsByLine.put(lineId, hits);
+    if (hits > 0) {
+      totalCoveredLines += 1;
     }
     return this;
   }
 
   @Override
   public DefaultCoverage conditions(int lineId, int conditions, int coveredConditions) {
-    if (conditions > 0 && !conditionsByLine.containsKey(lineId)) {
-      totalConditions += conditions;
-      totalCoveredConditions += coveredConditions;
-      conditionsByLine.put(lineId, conditions);
-      coveredConditionsByLine.put(lineId, coveredConditions);
-    }
+    Preconditions.checkArgument(lineId >= 1, "Line number should be positive and non zero [" + file.relativePath() + ":" + lineId + "]");
+    Preconditions.checkArgument(conditions >= 0, "Number of conditions should be positive [" + file.relativePath() + ":" + lineId + "]");
+    Preconditions.checkArgument(coveredConditions >= 0, "Number of covered conditions should be positive [" + file.relativePath() + ":" + lineId + "]");
+    Preconditions.checkArgument(conditions >= coveredConditions, "Number of covered conditions can't exceed conditions [" + file.relativePath() + ":" + lineId + "]");
+    Preconditions.checkArgument(!conditionsByLine.containsKey(lineId), "Conditions already saved on line [" + file.relativePath() + ":" + lineId + "]");
+    totalConditions += conditions;
+    totalCoveredConditions += coveredConditions;
+    conditionsByLine.put(lineId, conditions);
+    coveredConditionsByLine.put(lineId, coveredConditions);
     return this;
   }
 
@@ -81,7 +85,7 @@ public final class DefaultCoverage implements Coverage {
   @Override
   public DefaultCoverage onFile(InputFile inputFile) {
     Preconditions.checkNotNull(inputFile, INPUT_FILE_SHOULD_BE_NON_NULL);
-    Preconditions.checkArgument(inputFile.type() == Type.MAIN, "Coverage is only supported on main files");
+    Preconditions.checkArgument(inputFile.type() == Type.MAIN, "Coverage is only supported on main files [" + inputFile.relativePath() + "]");
     this.file = inputFile;
     return this;
   }
@@ -97,13 +101,14 @@ public final class DefaultCoverage implements Coverage {
     return this;
   }
 
+  @Override
   public void save() {
     Preconditions.checkNotNull(this.storage, "No persister on this object");
     Preconditions.checkState(!saved, "This object was already saved");
     Preconditions.checkNotNull(this.file, "File is mandatory on Coverage");
     Preconditions.checkNotNull(this.type, "Type is mandatory on Coverage");
 
-    if (hitsByLine.size() > 0) {
+    if (!hitsByLine.isEmpty()) {
       new DefaultMeasure<Integer>(storage)
         .onFile(file)
         .forMetric(type.linesToCover())
@@ -142,6 +147,7 @@ public final class DefaultCoverage implements Coverage {
         .withValue(KeyValueFormat.format(conditionsByLine))
         .save();
     }
+    this.saved = true;
   }
 
 }
diff --git a/sonar-plugin-api/src/test/java/org/sonar/api/batch/sensor/test/internal/DefaultCoverageTest.java b/sonar-plugin-api/src/test/java/org/sonar/api/batch/sensor/test/internal/DefaultCoverageTest.java
index d89707b3aab..1ff6e4bd6fe 100644
--- a/sonar-plugin-api/src/test/java/org/sonar/api/batch/sensor/test/internal/DefaultCoverageTest.java
+++ b/sonar-plugin-api/src/test/java/org/sonar/api/batch/sensor/test/internal/DefaultCoverageTest.java
@@ -19,7 +19,9 @@
  */
 package org.sonar.api.batch.sensor.test.internal;
 
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 import org.sonar.api.batch.fs.InputFile;
 import org.sonar.api.batch.fs.internal.DefaultInputFile;
 import org.sonar.api.batch.sensor.SensorStorage;
@@ -30,9 +32,13 @@ import org.sonar.api.measures.CoreMetrics;
 import static org.fest.assertions.Assertions.assertThat;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
 
 public class DefaultCoverageTest {
 
+  @Rule
+  public ExpectedException thrown = ExpectedException.none();
+
   private InputFile main = new DefaultInputFile("foo", "src/Foo.php").setType(InputFile.Type.MAIN);
 
   @Test
@@ -49,7 +55,62 @@ public class DefaultCoverageTest {
   }
 
   @Test
-  public void testSaveUnitTests() {
+  public void testSaveLines() {
+    SensorStorage storage = mock(SensorStorage.class);
+    new DefaultCoverage(storage)
+      .onFile(main)
+      .ofType(CoverageType.UNIT)
+      .lineHits(1, 2)
+      .lineHits(2, 5)
+      .lineHits(3, 0)
+      .lineHits(4, 0)
+      .save();
+
+    verify(storage).store(new DefaultMeasure<Integer>()
+      .onFile(main)
+      .forMetric(CoreMetrics.LINES_TO_COVER)
+      .withValue(4));
+    verify(storage).store(new DefaultMeasure<Integer>()
+      .onFile(main)
+      .forMetric(CoreMetrics.UNCOVERED_LINES)
+      .withValue(2));
+    verify(storage).store(new DefaultMeasure<String>()
+      .onFile(main)
+      .forMetric(CoreMetrics.COVERAGE_LINE_HITS_DATA)
+      .withValue("1=2;2=5;3=0;4=0"));
+    verifyNoMoreInteractions(storage);
+  }
+
+  @Test
+  public void testSaveConditions() {
+    SensorStorage storage = mock(SensorStorage.class);
+    new DefaultCoverage(storage)
+      .onFile(main)
+      .ofType(CoverageType.UNIT)
+      .conditions(1, 2, 1)
+      .save();
+
+    verify(storage).store(new DefaultMeasure<Integer>()
+      .onFile(main)
+      .forMetric(CoreMetrics.CONDITIONS_TO_COVER)
+      .withValue(2));
+    verify(storage).store(new DefaultMeasure<Integer>()
+      .onFile(main)
+      .forMetric(CoreMetrics.UNCOVERED_CONDITIONS)
+      .withValue(1));
+    verify(storage).store(new DefaultMeasure<String>()
+      .onFile(main)
+      .forMetric(CoreMetrics.COVERED_CONDITIONS_BY_LINE)
+      .withValue("1=1"));
+    verify(storage).store(new DefaultMeasure<String>()
+      .onFile(main)
+      .forMetric(CoreMetrics.CONDITIONS_BY_LINE)
+      .withValue("1=2"));
+    verifyNoMoreInteractions(storage);
+  }
+
+  @Test
+  public void testSaveLinesAndConditions() {
     SensorStorage storage = mock(SensorStorage.class);
     new DefaultCoverage(storage)
       .onFile(main)
@@ -89,6 +150,125 @@ public class DefaultCoverageTest {
       .onFile(main)
       .forMetric(CoreMetrics.CONDITIONS_BY_LINE)
       .withValue("1=2"));
+    verifyNoMoreInteractions(storage);
+  }
+
+  @Test
+  public void dontSaveTwice() {
+    SensorStorage storage = mock(SensorStorage.class);
+    DefaultCoverage coverage = new DefaultCoverage(storage)
+      .onFile(main)
+      .ofType(CoverageType.UNIT)
+      .lineHits(1, 2)
+      .lineHits(2, 5)
+      .lineHits(3, 0)
+      .lineHits(4, 0);
+    coverage.save();
+
+    thrown.expect(IllegalStateException.class);
+    thrown.expectMessage("This object was already saved");
+
+    coverage.save();
+  }
+
+  @Test
+  public void fileIsMain() {
+    thrown.expect(IllegalArgumentException.class);
+    thrown.expectMessage("Coverage is only supported on main files [test/FooTest.php]");
+
+    new DefaultCoverage()
+      .onFile(new DefaultInputFile("foo", "test/FooTest.php").setType(InputFile.Type.TEST))
+      .ofType(CoverageType.UNIT);
+  }
+
+  @Test
+  public void lineHitsValidation() {
+    thrown.expect(IllegalArgumentException.class);
+    thrown.expectMessage("Line number should be positive and non zero [src/Foo.php:0]");
+
+    new DefaultCoverage()
+      .onFile(main)
+      .ofType(CoverageType.UNIT)
+      .lineHits(0, 2);
+  }
+
+  @Test
+  public void hitsPositive() {
+    thrown.expect(IllegalArgumentException.class);
+    thrown.expectMessage("Hits should be positive [src/Foo.php:1]");
+
+    new DefaultCoverage()
+      .onFile(main)
+      .ofType(CoverageType.UNIT)
+      .lineHits(1, -1);
+  }
+
+  @Test
+  public void hitsNoDuplicate() {
+    thrown.expect(IllegalArgumentException.class);
+    thrown.expectMessage("Hits already saved on line [src/Foo.php:1]");
+
+    new DefaultCoverage()
+      .onFile(main)
+      .ofType(CoverageType.UNIT)
+      .lineHits(1, 2)
+      .lineHits(1, 1);
+  }
+
+  @Test
+  public void lineConditionValidation() {
+    thrown.expect(IllegalArgumentException.class);
+    thrown.expectMessage("Line number should be positive and non zero [src/Foo.php:0]");
+
+    new DefaultCoverage()
+      .onFile(main)
+      .ofType(CoverageType.UNIT)
+      .conditions(0, 2, 2);
+  }
+
+  @Test
+  public void conditionsPositive() {
+    thrown.expect(IllegalArgumentException.class);
+    thrown.expectMessage("Number of conditions should be positive [src/Foo.php:1]");
+
+    new DefaultCoverage()
+      .onFile(main)
+      .ofType(CoverageType.UNIT)
+      .conditions(1, -1, 0);
+  }
+
+  @Test
+  public void coveredConditionsPositive() {
+    thrown.expect(IllegalArgumentException.class);
+    thrown.expectMessage("Number of covered conditions should be positive [src/Foo.php:1]");
+
+    new DefaultCoverage()
+      .onFile(main)
+      .ofType(CoverageType.UNIT)
+      .conditions(1, 1, -1);
+  }
+
+  @Test
+  public void coveredConditionsVsConditions() {
+    thrown.expect(IllegalArgumentException.class);
+    thrown.expectMessage("Number of covered conditions can't exceed conditions [src/Foo.php:1]");
+
+    new DefaultCoverage()
+      .onFile(main)
+      .ofType(CoverageType.UNIT)
+      .conditions(1, 2, 3);
+  }
+
+  @Test
+  public void conditionsNoDuplicate() {
+    thrown.expect(IllegalArgumentException.class);
+    thrown.expectMessage("Conditions already saved on line [src/Foo.php:1]");
+
+    new DefaultCoverage()
+      .onFile(main)
+      .ofType(CoverageType.UNIT)
+      .conditions(1, 4, 3)
+      .conditions(1, 4, 2);
   }
 
 }