commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alex Herbert <alex.d.herb...@gmail.com>
Subject Re: False coverage decrease accusations by Coveralls
Date Wed, 03 Jul 2019 10:32:23 GMT
On 03/07/2019 10:35, Heinrich Bohne wrote:
> But the detailed report you linked to is exactly where I got the
> information about what existing lines have (purportedly) been uncovered
> from. It's true that the master branch changed in the meantime, but
> those commits only concerned formatting and changing the field
> serialVersionUID in BigFraction and Fraction. I don't understand exactly
> what this variable is for, but I doubt that it has something to do with
> the respective lines now being uncovered. In fact, the corresponding
> build https://coveralls.io/builds/24338122 lists the two files
> BigFraction.java and Fraction.java under the menu "COVERAGE CHANGED",
> but it doesn't actually report any change in coverage, so maybe this has
> something to do with the bug.

Your changes made:

commons-numbers-fraction: 88.31% to 88.41%

Overall: 2673 of 2836 (94.252%) to 2692 of 2857 (94.22%)

This issue is that although you increased coverage in 
commons-numbers-fraction because the increase is lower than the average 
across all modules you get an overall lowering of total coverage.

Your new changes have 40/42 (95.24%) coverage. This is below the 
previous overall average so the score is lower.

The two missed lines are in your new functions toFloatingPointBits and 
roundAndRightShift which have an uncovered IllegalArgumentException edge 
case. Are these ever possible to hit? If not then you should put in a 
message for the exception, for example "Internal error: Please file a 
bug report".

This should make it clear that the exception is not meant to be possible 
but the assertion it makes is a requirement for the rest of the function 
to work.


>
> On 7/3/19 11:19 AM, Alex Herbert wrote:
>>
>>> On 3 Jul 2019, at 09:38, Heinrich Bohne <heinrich.bohne@gmx.at> wrote:
>>>
>>> So this is the second time this happens to me. I've submitted a pull
>>> request ( https://github.com/apache/commons-numbers/pull/63 ), and the
>>> Coveralls reports says that several existing lines have been uncovered,
>>> which is a lie, because the lines purportedly "uncovered" were already
>>> not covered in the master branch (specifically the method
>>> BigFraction.toString(), and, in the class Fraction, some lines in
>>> addSub(Fraction, boolean), toString(), zero(), one() and 
>>> parse(String)).
>>> Something should probably be done about this, but I don't know the 
>>> right
>>> place where to report this.
>>>
>> You can click on the Coveralls badge on Github to get the detailed 
>> report of what changed:
>>
>> https://coveralls.io/builds/24338717 
>> <https://coveralls.io/builds/24338717>
>>
>> This requires a bit of digesting. It seems to have been confused by 
>> the removal of lots of lines and addition of lines to the same file. 
>> It thinks you haveĀ  19 new lines covered and 2 extra lines missed in 
>> BigFraction.
>>
>> Did you rebase your change against master? Perhaps the reference 
>> master it is comparing to is slightly different.
>>
>> If you care then you could run locally with Jacoco.
>>
>> What my inspection does show is that a lot of edge cases are not 
>> being covered by tests (divide by zero, addition of zero, etc). This 
>> is more important to fix.
>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>
>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message