You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Heinrich Bohne <he...@gmx.at> on 2019/07/03 08:38:04 UTC
False coverage decrease accusations by Coveralls
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.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: False coverage decrease accusations by Coveralls
Posted by Heinrich Bohne <he...@gmx.at>.
> so my pull request did
> uncover these lines in BigFraction.toString(), contrary to what the
> Coverall report says.
Excuse me, of course it should be "my pull request did *not*
uncover these lines"
On 7/3/19 1:00 PM, Heinrich Bohne wrote:
> I think we are talking about two completely different issues here. I am
> aware that 2 of my newly introduced lines (the IllegalArgumentException
> cases you mentioned) are not covered. These are argument validations
> inside private methods, so they should never be reached, as you
> correctly assumed.
>
> What I meant, however, is, for example, the method
> BigFraction.toString(). According to Coveralls, my pull request caused
> several lines inside this method that were previously covered to be
> uncovered. But both https://coveralls.io/builds/24307694 (which seems to
> be the version of the master branch upon which the comparison in the
> report for my pull request is based) and
> https://coveralls.io/builds/24338122 (which is the current version of
> master) already list these lines as uncovered, so my pull request did
> uncover these lines in BigFraction.toString(), contrary to what the
> Coverall report says.
>
> Here are the links that directly point to the relevant lines in the
> report for BigFraction:
>
> https://coveralls.io/builds/24338717/source?filename=commons-numbers-fraction%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fcommons%2Fnumbers%2Ffraction%2FBigFraction.java#L1273
>
> (my pull request)
>
> https://coveralls.io/builds/24307694/source?filename=commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java#L1128
>
> (the master version against which my pull request was compared)
>
> https://coveralls.io/builds/24338122/source?filename=commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java#L1127
>
> (the current master version)
>
> On 7/3/19 12:32 PM, Alex Herbert wrote:
>> 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 <he...@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
>>
>>
>
>
> ---------------------------------------------------------------------
> 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
Re: False coverage decrease accusations by Coveralls
Posted by Gilles Sadowski <gi...@gmail.com>.
Hello.
Le mer. 3 juil. 2019 à 13:56, Heinrich Bohne <he...@gmx.at> a écrit :
>
> It is very strange indeed, because the last time this happened, the
> reported change in coverage percentage was also wrong, so I initially
> assumed that the -0.03% report for this pull request time was false too.
> Actually, I'm still not entirely convinced that it isn't false, because
> in the summary above, it says "40 of 42 new or added lines in 1 file
> covered.", but in the menu "SOURCE CHANGED", it only says that 21
> relevant lines have been added to "BigFraction", 19 of which are
> covered. Anyway, thanks for taking the time to look into this.
>
>
> On 7/3/19 1:24 PM, Alex Herbert wrote:
> >
> > On 03/07/2019 12:00, Heinrich Bohne wrote:
> >> I think we are talking about two completely different issues here. I am
> >> aware that 2 of my newly introduced lines (the IllegalArgumentException
> >> cases you mentioned) are not covered. These are argument validations
> >> inside private methods, so they should never be reached, as you
> >> correctly assumed.
> >>
> >> What I meant, however, is, for example, the method
> >> BigFraction.toString(). According to Coveralls, my pull request caused
> >> several lines inside this method that were previously covered to be
> >> uncovered. But both https://coveralls.io/builds/24307694 (which seems to
> >> be the version of the master branch upon which the comparison in the
> >> report for my pull request is based) and
> >> https://coveralls.io/builds/24338122 (which is the current version of
> >> master) already list these lines as uncovered, so my pull request did
> >> uncover these lines in BigFraction.toString(), contrary to what the
> >> Coverall report says.
> >>
> >> Here are the links that directly point to the relevant lines in the
> >> report for BigFraction:
> >>
> >> https://coveralls.io/builds/24338717/source?filename=commons-numbers-fraction%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fcommons%2Fnumbers%2Ffraction%2FBigFraction.java#L1273
> >>
> >> (my pull request)
> >>
> >> https://coveralls.io/builds/24307694/source?filename=commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java#L1128
> >>
> >> (the master version against which my pull request was compared)
> >>
> >> https://coveralls.io/builds/24338122/source?filename=commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java#L1127
> >>
> >> (the current master version)
> >
> > OK I see. You are not disputing the overall coverage but the report on
> > what has been uncovered. So Coveralls cannot link back to the previous
> > coverage report where they were also not covered. It is a bug to raise
> > with Coveralls; or not worry about it.
IIUC, Coveralls merely displays[1][2] what it gets from Jacoco
executed by Travis.[3]
Gilles
[1] https://docs.coveralls.io/java
[2] https://github.com/trautonen/coveralls-maven-plugin
[3] https://gitbox.apache.org/repos/asf?p=commons-numbers.git;a=blob;f=.travis.yml;h=88452cecde83de241397600b3e2a9b3f9e9b9c46;hb=HEAD
> >
> >>
> >> On 7/3/19 12:32 PM, Alex Herbert wrote:
> >>> 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 <he...@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
Re: False coverage decrease accusations by Coveralls
Posted by Heinrich Bohne <he...@gmx.at>.
It is very strange indeed, because the last time this happened, the
reported change in coverage percentage was also wrong, so I initially
assumed that the -0.03% report for this pull request time was false too.
Actually, I'm still not entirely convinced that it isn't false, because
in the summary above, it says "40 of 42 new or added lines in 1 file
covered.", but in the menu "SOURCE CHANGED", it only says that 21
relevant lines have been added to "BigFraction", 19 of which are
covered. Anyway, thanks for taking the time to look into this.
On 7/3/19 1:24 PM, Alex Herbert wrote:
>
> On 03/07/2019 12:00, Heinrich Bohne wrote:
>> I think we are talking about two completely different issues here. I am
>> aware that 2 of my newly introduced lines (the IllegalArgumentException
>> cases you mentioned) are not covered. These are argument validations
>> inside private methods, so they should never be reached, as you
>> correctly assumed.
>>
>> What I meant, however, is, for example, the method
>> BigFraction.toString(). According to Coveralls, my pull request caused
>> several lines inside this method that were previously covered to be
>> uncovered. But both https://coveralls.io/builds/24307694 (which seems to
>> be the version of the master branch upon which the comparison in the
>> report for my pull request is based) and
>> https://coveralls.io/builds/24338122 (which is the current version of
>> master) already list these lines as uncovered, so my pull request did
>> uncover these lines in BigFraction.toString(), contrary to what the
>> Coverall report says.
>>
>> Here are the links that directly point to the relevant lines in the
>> report for BigFraction:
>>
>> https://coveralls.io/builds/24338717/source?filename=commons-numbers-fraction%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fcommons%2Fnumbers%2Ffraction%2FBigFraction.java#L1273
>>
>> (my pull request)
>>
>> https://coveralls.io/builds/24307694/source?filename=commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java#L1128
>>
>> (the master version against which my pull request was compared)
>>
>> https://coveralls.io/builds/24338122/source?filename=commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java#L1127
>>
>> (the current master version)
>
> OK I see. You are not disputing the overall coverage but the report on
> what has been uncovered. So Coveralls cannot link back to the previous
> coverage report where they were also not covered. It is a bug to raise
> with Coveralls; or not worry about it.
>
>>
>> On 7/3/19 12:32 PM, Alex Herbert wrote:
>>> 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 <he...@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
>>>
>>>
>>
>>
>> ---------------------------------------------------------------------
>> 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
Re: False coverage decrease accusations by Coveralls
Posted by Alex Herbert <al...@gmail.com>.
On 03/07/2019 12:00, Heinrich Bohne wrote:
> I think we are talking about two completely different issues here. I am
> aware that 2 of my newly introduced lines (the IllegalArgumentException
> cases you mentioned) are not covered. These are argument validations
> inside private methods, so they should never be reached, as you
> correctly assumed.
>
> What I meant, however, is, for example, the method
> BigFraction.toString(). According to Coveralls, my pull request caused
> several lines inside this method that were previously covered to be
> uncovered. But both https://coveralls.io/builds/24307694 (which seems to
> be the version of the master branch upon which the comparison in the
> report for my pull request is based) and
> https://coveralls.io/builds/24338122 (which is the current version of
> master) already list these lines as uncovered, so my pull request did
> uncover these lines in BigFraction.toString(), contrary to what the
> Coverall report says.
>
> Here are the links that directly point to the relevant lines in the
> report for BigFraction:
>
> https://coveralls.io/builds/24338717/source?filename=commons-numbers-fraction%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fcommons%2Fnumbers%2Ffraction%2FBigFraction.java#L1273
>
> (my pull request)
>
> https://coveralls.io/builds/24307694/source?filename=commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java#L1128
>
> (the master version against which my pull request was compared)
>
> https://coveralls.io/builds/24338122/source?filename=commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java#L1127
>
> (the current master version)
OK I see. You are not disputing the overall coverage but the report on
what has been uncovered. So Coveralls cannot link back to the previous
coverage report where they were also not covered. It is a bug to raise
with Coveralls; or not worry about it.
>
> On 7/3/19 12:32 PM, Alex Herbert wrote:
>> 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 <he...@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
>>
>>
>
>
> ---------------------------------------------------------------------
> 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
Re: False coverage decrease accusations by Coveralls
Posted by Heinrich Bohne <he...@gmx.at>.
I think we are talking about two completely different issues here. I am
aware that 2 of my newly introduced lines (the IllegalArgumentException
cases you mentioned) are not covered. These are argument validations
inside private methods, so they should never be reached, as you
correctly assumed.
What I meant, however, is, for example, the method
BigFraction.toString(). According to Coveralls, my pull request caused
several lines inside this method that were previously covered to be
uncovered. But both https://coveralls.io/builds/24307694 (which seems to
be the version of the master branch upon which the comparison in the
report for my pull request is based) and
https://coveralls.io/builds/24338122 (which is the current version of
master) already list these lines as uncovered, so my pull request did
uncover these lines in BigFraction.toString(), contrary to what the
Coverall report says.
Here are the links that directly point to the relevant lines in the
report for BigFraction:
https://coveralls.io/builds/24338717/source?filename=commons-numbers-fraction%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fcommons%2Fnumbers%2Ffraction%2FBigFraction.java#L1273
(my pull request)
https://coveralls.io/builds/24307694/source?filename=commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java#L1128
(the master version against which my pull request was compared)
https://coveralls.io/builds/24338122/source?filename=commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java#L1127
(the current master version)
On 7/3/19 12:32 PM, Alex Herbert wrote:
> 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 <he...@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
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: False coverage decrease accusations by Coveralls
Posted by Alex Herbert <al...@gmail.com>.
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 <he...@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
Re: False coverage decrease accusations by Coveralls
Posted by Heinrich Bohne <he...@gmx.at>.
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.
On 7/3/19 11:19 AM, Alex Herbert wrote:
>
>> On 3 Jul 2019, at 09:38, Heinrich Bohne <he...@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
Re: False coverage decrease accusations by Coveralls
Posted by Alex Herbert <al...@gmail.com>.
> On 3 Jul 2019, at 09:38, Heinrich Bohne <he...@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
>