Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

limit > 1 breaks rule config #783

Closed
skywalker01101 opened this issue Nov 5, 2018 · 5 comments · Fixed by #931
Closed

limit > 1 breaks rule config #783

skywalker01101 opened this issue Nov 5, 2018 · 5 comments · Fixed by #931

Comments

@skywalker01101
Copy link

skywalker01101 commented Nov 5, 2018

Steps to reproduce

I created a rule

<rule implementation="org.jacoco.maven.RuleConfiguration">
                                    <element>PACKAGE</element>
                                    <limits>
                                        <limit implementation="org.jacoco.report.check.Limit">
                                            <counter>INSTRUCTION</counter>
                                            <value>COVEREDRATIO</value>
                                            <!-- Note: if you put in a value > 1 it will report 0% coverage! I'm reporting this bug to Jacoco. -->
                                            <minimum>90</minimum>
                                        </limit>
                                    </limits>
                                </rule>

This blows up saying every package in the project has 0% coverage. When I look at the html reports they still have real looking coverage info. This caused me to waste a lot of time and great frustration. I almost gave up.

JaCoCo version: jacoco-maven-plugin:0.8.2
Operating system: MacOS
Tool integration: Maven

Expected behaviour

If a value greater than 1 is entered it should either report an invalid limit or just show the package level as being below the impossible value of 1.

Actual behaviour

Currently blows up saying 0% coverage even though the html report shows the accurate value.

@Godin
Copy link
Member

Godin commented Nov 5, 2018

In absence of complete example (please consider providing such in future), I created jacoco-783.zip for which mvn verify demonstrates behavior.

After reading documentation 😉 could be noted that description of "rules" in it clearly shows that value 0.80 sets 80%.

@Godin
Copy link
Member

Godin commented Nov 5, 2018

Message produced for the above example

instructions covered ratio is 0, but expected minimum is 90

doesn't have word "percentage" and uses term instructions covered ratio - number of covered instructions dived by total number of instructions, which can't be be greater than one 😉

It is shown as 0 in the above message, because of rounding of 4 / 7 ≈ 0.5714 to the precision specified by 90 - see #449 (comment)

Specification of 0.58 instead of 90 will show

instructions covered ratio is 0.57, but expected minimum is 0.58

and specification of 0.6

instructions covered ratio is 0.5, but expected minimum is 0.6

@marchof
Copy link
Member

marchof commented Nov 7, 2018

Also documentation is very clear about this:

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 %.

https://www.jacoco.org/jacoco/trunk/doc/check-mojo.html

@skywalker01101
Copy link
Author

skywalker01101 commented Nov 7, 2018

Perhaps the issue was not understood. This did not fix the issue. There is a bug in the reporting of unit test coverage. The amount of code covered by unit test should not change based on the value of a rule. That is the bug.
If your code is 35% (or 0.35) covered it should ALWAYS report that no matter what limit you set in a rule. Please fix this. If I ask for 90% coverage it should still report the 35%. If I ask for 9000% (obviously a bad value) it should still report 35% coverage. The amount of code covered by unit test should not change based on the value of a rule.
@Godin @marchof

@Godin
Copy link
Member

Godin commented Nov 7, 2018

@skywalker01101 if you ask for 90%, then on the same example provided by me:

instructions covered ratio is 0.57, but expected minimum is 0.90

if you ask for 9000%, then:

instructions covered ratio is 0.57, but expected minimum is 90.00

0.57 in both cases.

Given that clearly documented as a feature that

number of decimal places will also determine the precision in error messages

and please consider that there are users who rely on this, for whom important to see same precision in message as they specify in configuration - this is not a bug.

Your initial problem is misuse / misconfiguration - absence of %.

What I can imagine - is an enhancement of error message:

in case when ratio is greater than 1 and has no %

Invalid ratio specified - "90", please read documentation, to specify percentage add "%".

in general case

instructions covered ratio is 0.57 ( covered = 4 , total = 7 ), but expected minimum is 0.90

But not change of mechanism of rounding, just because you want so. Because again - this will break configurations and expectations of users who rely on this. Highly likely that it won't be you, who will be dealing with questions about change of behavior. Rather than screaming "ALWAYS" please try to wear shoes of maintainers, who need to take into account existence of users with needs that can be different from yours. And who do this absolutely for free in their spare time. Fair enough?

Thank you for your understanding.

@Godin Godin reopened this Nov 7, 2018
@marchof marchof self-assigned this Aug 30, 2019
@marchof marchof added this to Candidates in Current work items via automation Aug 30, 2019
@marchof marchof added this to the 0.8.5 milestone Aug 30, 2019
@Godin Godin moved this from Candidates to Implementation in Current work items Aug 30, 2019
@marchof marchof moved this from Implementation to Review in Current work items Aug 30, 2019
Current work items automation moved this from Review to Done Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging a pull request may close this issue.

3 participants