Hello. Le mer. 3 juil. 2019 à 13:56, Heinrich Bohne <heinrich.bo...@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 <heinrich.bo...@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-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
