linux-test-project / lcov

LCOV
GNU General Public License v2.0
904 stars 242 forks source link

Problems with differential reporting #315

Closed ziad-fernride closed 3 weeks ago

ziad-fernride commented 2 months ago

Hi, thanks to @henry2cox 's help in: https://github.com/linux-test-project/lcov/issues/314, I managed to get something a lot better running ... But after a couple of days of data collection, I am seeing these issues:

Example 1: incorrect diff Base commit coverage report:

Screenshot 2024-08-14 at 13 49 19

Differential coverage report:

Screenshot 2024-08-14 at 13 51 15

Example 2: incorrect ECB, UIC values I am seeing a lot of these values:

Screenshot 2024-08-14 at 13 54 14 Screenshot 2024-08-14 at 13 51 15

And then when I step into the file, there are actually no UIC nor ECB.

Ok, now my workflow is:

Any idea why the differential report is still unreliable ? Notice that I am not using a --version-script ... would that fix this ? what does it do on top of the diff anyways ?

Thanks a lot 🙏

henry2cox commented 2 months ago

With respect to the major question here ("why does the differential categorization look wrong here?"), I very strongly suspect that the reason is that the 'diff' file is wrong and/or the source code versions do not match the corresponding coverage DB.

From a UI perspective: the number in the 'file summary' header table in the directory view should be a hyperlink - to take you to the first coverpoint in the corresponding category in the corresponding file. The hyperlink does not work if you use --frames (I think, a bug in both chrome and firefox...but I could not make it work).

With respect to the --version-script question:

Minor point: your first exclusion regexp could be '^\s*};?\s*$' - or you could put optional space before the semicolon, if you think somebody might write code that way. You could also combine the regexps into one that deals with lone open/close brace.

shabanzd commented 2 months ago

Sorry for my late reply 😊

Regarding the more important problem of the failing criteria, I realized the change actually moves the (.) into an (arrow) operators in the lines caught as NUC (since the developer technically removed a line and inserted a new one) .. so all good there 👍 that was my bad actually!

Regarding the other less important false positive ECB and UIC, they do show up often even with no code diff changes. I can show some real life diffs, although I think there is little value in that since they are empty from a code's perspective...

One interesting thing about the number hyperlink is that it is indeed there ... but when clicking on it, it takes me to lines that are not marked in the case of false positives. For example, a file would show ECB of 1, but none of the lines are marked ECB.

I will try to find a more concrete example tomorrow ! Cheers, Henry !

henry2cox commented 2 months ago

Glad the changed code was resolved.

With respect to the ECB and UIC code:

It should be possible to check the data associated with one or more of these lines to see what is happening. Note that the corresponding lines may have different line numbers between baseline and current due to edits. The 'diff' data is used to track this.

It is always possible that there is a bug such that your coverage data is miscategorized - so it is worth to track down the issue. The version you are using is a bit old (TOT is more than a year and many, many changes newer) - but I don't recall fixing anything in this area, so I don't think that moving to latest will change anything.

I have seen "miscategorization" caused by version mismatches and data inconsistency - which was the genesis of the version callback and associated error checking. I recently added some additional consistency checks to try to catch more such situations (not yet pushed/not externally available).

To debug this case, it may be helpful to tell the tool to look at just one file (one of the ones where you see apparently wrong categorization):

If not: this is definitely a bug. I probably need more data to debug (the subset described below will not be enough).

If so:

ziad-fernride commented 2 months ago

Hey again, for feedback, I am now using version 2.1 ... will wait and see what issues regarding diff coverage show up :)

But I have some interesting stuff to report (will open separate issues for those)

henry2cox commented 3 weeks ago

I think that this issue is probably addressed by the consistency checking added in the SHA mentioned above. If nothing else: the checks should make debugging/diagnosing any problems a bit easier.

If there are no updates, then I will close this issue in a couple of days (or you can close it). Thanks.

ziad-fernride commented 3 weeks ago

Thanks, Henry ! Agreed, will close the issue