Report invalid coverage ratio configuration (bazel-contrib#931)

To avoid common misconfigurations where
users forget to specify percent sign.

Author: marchof

jacoco-maven-plugin/src/org/jacoco/maven/CheckMojo.java

@@ -48,10 +48,10 @@ public class CheckMojo extends AbstractJacocoMojo implements IViolationsOutput {
 	 * applies to a certain counter (INSTRUCTION, LINE, BRANCH, COMPLEXITY,
 	 * METHOD, CLASS) and defines a minimum or maximum for the corresponding
 	 * value (TOTALCOUNT, COVEREDCOUNT, MISSEDCOUNT, COVEREDRATIO, MISSEDRATIO).
-	 * If a limit refers to a ratio the range is from 0.0 to 1.0 where the
-	 * number of decimal places will also determine the precision in error
-	 * messages. A limit ratio may optionally be declared as a percentage
-	 * where 0.80 and 80% represent the same value, the value must end with %.
+	 * If a limit refers to a ratio it must be in the range from 0.0 to 1.0
+	 * where the number of decimal places will also determine the precision in
+	 * error messages. A limit ratio may optionally be declared as a percentage
+	 * where 0.80 and 80% represent the same value.
 	 * </p>
 	 * 
 	 * <p>
@@ -150,8 +150,8 @@ private boolean canCheckCoverage() {
 			getLog().info(MSG_SKIPPING + dataFile);
 			return false;
 		}
-		final File classesDirectory = new File(getProject().getBuild()
-				.getOutputDirectory());
+		final File classesDirectory = new File(
+				getProject().getBuild().getOutputDirectory());
 		if (!classesDirectory.exists()) {
 			getLog().info(
 					"Skipping JaCoCo execution due to missing classes directory:"

org.jacoco.ant.test/src/org/jacoco/ant/ReportTaskTest.xml

@@ -464,6 +464,24 @@
 		<au:assertLogContains level="error" text="instructions covered ratio is 0.00, but expected minimum is 0.90"/>
 	</target>
 
+	<target name="testReportInvalidConfiguration">
+		<au:expectfailure expectedMessage="Coverage check failed due to violated rules.">
+			<jacoco:report>
+				<structure name="Test">
+					<classfiles>
+						<fileset dir="${org.jacoco.ant.reportTaskTest.classes.dir}" includes="**/*.class"/>
+					</classfiles>
+				</structure>
+				<check>
+					<rule element="BUNDLE">
+						<limit counter="INSTRUCTION" value="COVEREDRATIO" minimum="80"/>
+					</rule>
+				</check>
+			</jacoco:report>
+		</au:expectfailure>
+		<au:assertLogContains level="error" text="given minimum ratio is 80, but must be between 0.0 and 1.0"/>
+	</target>
+	
 	<target name="testReportCheckSetPropertyOnly">
 		<jacoco:report>
 			<structure name="Test">

org.jacoco.doc/docroot/doc/ant.html

@@ -857,11 +857,11 @@ <h3>Element <code>check</code></h3>
     </tr>
     <tr>
       <td><code>minimum</code></td>
-      <td>Expected minimum value. If the minimum refers to a ratio the range is
-          from 0.0 to 1.0 where the number of decimal places will also determine
-          the precision in error messages. A limit ratio may optionally be
-          declared as a percentage where 0.80 and 80% represent the same value,
-          the value must end with %.</td>
+      <td>Expected minimum value. If the minimum refers to a ratio it must be
+          in the range from 0.0 to 1.0 where the number of decimal places will
+          also determine the precision in error messages. A limit ratio may
+          optionally be declared as a percentage where 0.80 and 80% represent
+          the same value.</td>
       <td><i>none</i></td>
     </tr>
     <tr>

org.jacoco.doc/docroot/doc/changes.html

@@ -62,6 +62,14 @@ <h3>Non-functional Changes</h3>
       (GitHub <a href="https://github.com/jacoco/jacoco/issues/910">#910</a>).</li>
 </ul>
 
+<h3>API Changes</h3>
+<ul>
+    <li>The coverage check API and tools (Ant, Maven) now report an error, when
+        a coverage ratio limit is configured outside the range [0,1] to avoid
+        common configuration mistakes
+        (GitHub <a href="https://github.com/jacoco/jacoco/issues/783">#783</a>).</li>
+</ul>
+
 <h2>Release 0.8.4 (2019/05/08)</h2>
 
 <h3>New Features</h3>

org.jacoco.report.test/src/org/jacoco/report/check/LimitTest.java

@@ -79,27 +79,27 @@ public void check_should_fail_on_value_coveredcount() {
 	@Test
 	public void check_should_fail_on_value_missedratio() {
 		limit.setValue(CounterValue.MISSEDRATIO.name());
-		limit.setMaximum("-1");
+		limit.setMaximum("0.5");
 		assertEquals(CounterValue.MISSEDRATIO, limit.getValue());
 		assertEquals(
-				"instructions missed ratio is 0, but expected maximum is -1",
+				"instructions missed ratio is 1.0, but expected maximum is 0.5",
 				limit.check(new TestNode() {
 					{
-						instructionCounter = CounterImpl.COUNTER_0_1;
+						instructionCounter = CounterImpl.COUNTER_1_0;
 					}
 				}));
 	}
 
 	@Test
 	public void check_should_fail_on_value_coveredratio() {
 		limit.setValue(CounterValue.COVEREDRATIO.name());
-		limit.setMaximum("-1");
+		limit.setMaximum("0.5");
 		assertEquals(CounterValue.COVEREDRATIO, limit.getValue());
 		assertEquals(
-				"instructions covered ratio is 0, but expected maximum is -1",
+				"instructions covered ratio is 1.0, but expected maximum is 0.5",
 				limit.check(new TestNode() {
 					{
-						instructionCounter = CounterImpl.COUNTER_1_0;
+						instructionCounter = CounterImpl.COUNTER_0_1;
 					}
 				}));
 	}
@@ -242,6 +242,24 @@ public void check_should_report_counter_with_given_precision() {
 				}));
 	}
 
+	@Test
+	public void check_should_fail_when_minimum_ratio_is_smaller_than_0() {
+		limit.setMinimum("-3");
+		assertEquals("-3", limit.getMinimum());
+		assertEquals(
+				"given minimum ratio is -3, but must be between 0.0 and 1.0",
+				limit.check(new TestNode()));
+	}
+
+	@Test
+	public void check_should_fail_when_minimum_ratio_is_bigger_than_1() {
+		limit.setMinimum("80");
+		assertEquals("80", limit.getMinimum());
+		assertEquals(
+				"given minimum ratio is 80, but must be between 0.0 and 1.0",
+				limit.check(new TestNode()));
+	}
+
 	@Test
 	public void setMinimum_should_accept_percentage_string() {
 		limit.setMinimum("1.55%");
@@ -314,6 +332,24 @@ public void check_should_report_actual_ratio_rounded_up_when_maximum_is_not_met(
 				}));
 	}
 
+	@Test
+	public void check_should_fail_when_maximum_ratio_is_smaller_than_0() {
+		limit.setMaximum("-3");
+		assertEquals("-3", limit.getMaximum());
+		assertEquals(
+				"given maximum ratio is -3, but must be between 0.0 and 1.0",
+				limit.check(new TestNode()));
+	}
+
+	@Test
+	public void check_should_fail_when_maximum_ratio_is_bigger_than_1() {
+		limit.setMaximum("80");
+		assertEquals("80", limit.getMaximum());
+		assertEquals(
+				"given maximum ratio is 80, but must be between 0.0 and 1.0",
+				limit.check(new TestNode()));
+	}
+
 	@Test
 	public void setMaximum_should_accept_percentage_string() {
 		limit.setMaximum("1.55%");

org.jacoco.report/src/org/jacoco/report/check/BundleChecker.java

@@ -139,11 +139,11 @@ private void checkRules(final ICoverageNode node,
 	}
 
 	private void checkLimit(final ICoverageNode node, final String elementtype,
-			final String typename, final Rule rule, final Limit limit) {
+			final String elementname, final Rule rule, final Limit limit) {
 		final String message = limit.check(node);
 		if (message != null) {
 			output.onViolation(node, rule, limit, String.format(
-					"Rule violated for %s %s: %s", elementtype, typename,
+					"Rule violated for %s %s: %s", elementtype, elementname,
 					message));
 		}
 	}

org.jacoco.report/src/org/jacoco/report/check/Limit.java

@@ -83,8 +83,8 @@ public CounterEntity getEntity() {
 	 *
 	 * @param entity
 	 *            counter entity to check
-	 * TODO: use CounterEntity directly once Maven 3 is required.
 	 */
+	// TODO: use CounterEntity directly once Maven 3 is required.
 	public void setCounter(final String entity) {
 		this.entity = CounterEntity.valueOf(entity);
 	}
@@ -101,8 +101,8 @@ public CounterValue getValue() {
 	 *
 	 * @param value
 	 *            value to check
-	 * TODO: use CounterValue directly once Maven 3 is required.
 	 */
+	// TODO: use CounterValue directly once Maven 3 is required.
 	public void setValue(final String value) {
 		this.value = CounterValue.valueOf(value);
 	}
@@ -116,9 +116,11 @@ public String getMinimum() {
 	}
 
 	/**
-	 * Sets allowed maximum value as decimal string or percent representation.
-	 * The given precision is also considered in error messages. Coverage ratios
-	 * are given in the range from 0.0 to 1.0.
+	 * Sets the expected minimum value. If the minimum refers to a ratio it must
+	 * be in the range from 0.0 to 1.0 where the number of decimal places will
+	 * also determine the precision in error messages. A limit ratio may
+	 * optionally be declared as a percentage where 0.80 and 80% represent the
+	 * same value.
 	 * 
 	 * @param minimum
 	 *            allowed minimum or <code>null</code>, if no minimum should be
@@ -137,9 +139,11 @@ public String getMaximum() {
 	}
 
 	/**
-	 * Sets allowed maximum value as decimal string or percent representation.
-	 * The given precision is also considered in error messages. Coverage ratios
-	 * are given in the range from 0.0 to 1.0.
+	 * Sets the expected maximum value. If the maximum refers to a ratio it must
+	 * be in the range from 0.0 to 1.0 where the number of decimal places will
+	 * also determine the precision in error messages. A limit ratio may
+	 * optionally be declared as a percentage where 0.80 and 80% represent the
+	 * same value.
 	 * 
 	 * @param maximum
 	 *            allowed maximum or <code>null</code>, if no maximum should be
@@ -153,17 +157,22 @@ private static BigDecimal parseValue(final String value) {
 		if (value == null) {
 			return null;
 		}
-		
+
 		final String trimmedValue = value.trim();
 		if (trimmedValue.endsWith("%")) {
-			final String percent = trimmedValue.substring(0, trimmedValue.length() - 1);
+			final String percent = trimmedValue.substring(0,
+					trimmedValue.length() - 1);
 			return new BigDecimal(percent).movePointLeft(2);
 		}
 
 		return new BigDecimal(trimmedValue);
 	}
 
 	String check(final ICoverageNode node) {
+		final String msg = checkRatioLimit();
+		if (msg != null) {
+			return msg;
+		}
 		final double d = node.getCounter(entity).getValue(value);
 		if (Double.isNaN(d)) {
 			return null;
@@ -186,4 +195,29 @@ private String message(final String minmax, final BigDecimal v,
 				rounded.toPlainString(), minmax, ref.toPlainString());
 	}
 
+	private String checkRatioLimit() {
+		if (CounterValue.MISSEDRATIO.equals(value)
+				|| CounterValue.COVEREDRATIO.equals(value)) {
+			final String minmsg = checkRatioLimit("minimum", minimum);
+			if (minmsg != null) {
+				return minmsg;
+			}
+			final String maxmsg = checkRatioLimit("maximum", maximum);
+			if (maxmsg != null) {
+				return maxmsg;
+			}
+		}
+		return null;
+	}
+
+	private String checkRatioLimit(final String minmax, final BigDecimal v) {
+		if (v != null && (v.compareTo(BigDecimal.ZERO) < 0
+				|| v.compareTo(BigDecimal.ONE) > 0)) {
+			return String.format(
+					"given %s ratio is %s, but must be between 0.0 and 1.0",
+					minmax, v);
+		}
+		return null;
+	}
+
 }