Report invalid coverage ratio configuration #931
Conversation
To avoid common misconfigurations of coverage limits where users provide ratio as percentage value (without "%") we now issue a rule violation in such situations. * Update documentation to make clear that the range for ratios is [0,1] * Use new naming conventions in corresponding unit tests * Remove unnecessary config elements from Ant tests * Fix local variable name
|
While the rules could be validated once I decided to include it in the existing check methods. This leads to repeated execution but on the other hand allows to keep the existin |
| @@ -161,8 +161,8 @@ private boolean canCheckCoverage() { | |||
| } | |||
|
|
|||
| @Override | |||
| public void executeMojo() throws MojoExecutionException, | |||
| MojoExecutionException { | |||
There was a problem hiding this comment.
Duplicate in throws clause - fixed in master.
|
@marchof we have really nice statement on Development Conventions page
So I cherry-picked non-functional changes as separate commits into master - 02682c9 and 9f1efb2 This really helps to focus on functional changes 😉 |
org.jacoco.doc/docroot/doc/ant.html
Outdated
| 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> |
There was a problem hiding this comment.
This and other HTML files use spaces for indentation, so for consistency tab characters should be removed.
| @@ -80,10 +80,11 @@ public CounterEntity getEntity() { | |||
|
|
|||
| /** | |||
| * Sets the counter entity to check. | |||
| * | |||
| * TODO: use CounterEntity directly once Maven 3 is required. | |||
There was a problem hiding this comment.
Not only this Maven-ism affected API, but also this TODO-comment is visible in documentation 😞 And while we already require Maven 3 (#821), I propose to just move this TODO-comment outside of javadoc.
| @@ -98,10 +99,11 @@ public CounterValue getValue() { | |||
|
|
|||
| /** | |||
| * Sets the value to check. | |||
| * | |||
| * TODO: use CounterValue directly once Maven 3 is required. | |||
There was a problem hiding this comment.
Same as above - let's move this TODO-comment outside of javadoc.
| <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. |
There was a problem hiding this comment.
There is already dot after reference to ticket as like in all existing places, so this one should be removed for consistency.
|
|
||
| private String checkRatioLimit(final String minmax, final BigDecimal v) { | ||
| if (v != null && (v.compareTo(BigDecimal.ZERO) == -1 | ||
| || v.compareTo(BigDecimal.ONE) == 1)) { |
There was a problem hiding this comment.
While BigDecimal.compareTo clearly specifies return values as -1, 0, or 1,
< 0 (less than zero) and > 0 (greater than zero) look clearer to my taste,
also Comparable.compareTo specifies return values as a negative integer, zero, or a positive.
|
@marchof merged! Nice trick to not change API 👍Hope you don't mind that I fixed little things mentioned above. Also given that people do not read documentation wanna hope that such message will be enough without |
|
@Godin Many thanks for cleaning up my mess! 😺 |
Fixes #783
To avoid common misconfigurations of coverage limits where users provide
ratio as percentage value (without "%") we now issue a rule violation in
such situations.