-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Bazel branch coverage ignores Jacoco filtering #12696
Comments
From what I have seen from Jacoco's and Bazel's source: In Jacoco's Jacoco uses ClassProbesAdapter with ClassAnalyzer in https://github.com/jacoco/jacoco/blob/6bcce6972b8939d7925c4b4d3df785d9a7b00007/org.jacoco.core/src/org/jacoco/core/analysis/Analyzer.java#L102. Bazel in contrast uses its own ClassProbesMapper ( bazel/src/java_tools/junitrunner/java/com/google/testing/coverage/ClassProbesMapper.java Line 24 in 88a313d
bazel/src/java_tools/junitrunner/java/com/google/testing/coverage/BranchDetailAnalyzer.java Line 119 in 88a313d
From what I see the reason could be, that Jacoco's implementation that uses filtering is not re-used by Bazel. I don't yet see where it should be added and would be grateful for any guidance (and happy to contribute). I also don't understand how it's possible that filtering works for line coverage and doesn't work for branch coverage. |
I think I found the reason. bazel/src/java_tools/junitrunner/java/com/google/testing/coverage/JacocoCoverageRunner.java Lines 125 to 128 in c5f87ea
Bazel creates coverage data with Jacoco for lines (and functions), but uses it's own logic for branch coverage (that is in fact older than Jacoco's filtering functionality: 2016 vs. 2017). That explains why line and function coverage respects Jacoco filtering, while branch coverage not. Data is then processed here: bazel/src/java_tools/junitrunner/java/com/google/testing/coverage/JacocoLCOVFormatter.java Lines 131 to 179 in 88a313d
Branch data is processed at: 145-166. We can use For my case this change fixes the issue. |
After thinking about it once more, emitting branch coverage data should be done basing on what Jacoco has provided us (not just checking filtering), as it may happen that Jacoco filters out just part of the branches for a single line. Updated PR #12710 according to this. |
Jacoco's branch coverage output reports only the total number of branches in a line, and the number then taken. It does not give any details to individual branches in a line. Since Bazel attempts to merge coverage reports, we need to be able to identify when individual branches so they can be merged successfully to give a meaningful report. Bazel uses its own The only "quick" solution I can think of is to omit branches on a line if Jacoco's analyzer says there are zero branches in total, otherwise report all branches. Since |
I also considered this solution in #12710 and dropped it for the same reason.
I think that Bazel is already quite much dependent on Jacoco, e.g. updating to Jacoco 0.8.5 (or higher) will need some changes in Bazel itself. So if Bazel is trying to provide an alternate implementation for branch coverage, and already uses internal Jacoco classes like |
I've done some more investigation into this and have made promising progress.
I don't grok what's going on Note that Filters have three potential output calls to
It isn't clear to me yet what needs to be done to support more than just I have a WIP branch here: https://github.com/c-mita/bazel/tree/jacoco_filter Non-Scala/Kotlin demonstrations of the problem of extraneous branches can be demonstrated in Java with string switches:
The "expected" number of branches here is 3, however the java compiler will emit more than double this number. Jacoco normally filters these extra branches. |
Jacoco applies a set of filters during its coverage analysis to remove various compiler constructs that could lead to surprising outputs in coverage results, leaving behind coverage data that closer fits the code given to the compiler. A simple java example is the function: ``` public int foo(String input) { switch (input) { case "A": return 1; case "B": return 2; case "C": return 3; default: return -1 } } ``` This would generate at least double the number of expected branches as the compiler generates a second set of branches to operate on hashCode. Jacoco would normally ignore the first set of branches, reducing the number to the expected 4. Because Bazel applies its own branch analysis step, Bazel's branch coverage output does not benefit from this filtering. This change adapts the custom analyzer to record the necessary information during the ASM Tree walk in the same manner as Jacoco's regular analyzer in order to apply the filters. The filters have three output operations: * ignore - ignore a range of instructions * merge - merges the coverage information of two instructions * replaceBranches - retarget the output branches of an instruction The first of these, ignore, is by far the simplest to implement and covers the vast majority of use cases in Java. By mapping the AbstractInsnNode objects recorded during the ASM Tree walk to the Instruction objects used during branch analysis, we can simply skip over Instruction objects that have had their associated AbstractInsnNode object ignored by a filter. The remaining operations, merge and replaceBranches, are left unimplemeted for now; these require more care to handle, are harder to test, and affect only a minority of cases, specifically: * merge is only used to handle duplication of finally blocks * replaceBranches is used by Kotlin and Eclipse compiler filters Part of #12696 Closes #13896. PiperOrigin-RevId: 395235432
Instructions that are indicated to be merged must have their corresponding coverage expressions (`CovExp`) combined in a pairwise fashion. e.g. A pair of branch expressions that should be merged: `BranchExp(a, b)`, `BranchExp(c, d)` Becomes: `BranchExp(a | c, b | d)` This is distinct from the previous merge operation on BranchExp which simply joined the branches of the two objects (this has been renamed to concatenate). Part of #12696. Closes #13972. PiperOrigin-RevId: 396573429
These were added during bazelbuild#12696. The integration tests in question depend on the pre-built version of java_tools and so would have started to fail when the tests were updated for the new functionality added. Now java_tools has been updated (several times), it is safe to allow automated running of them once again.
These were added during #12696. The integration tests in question depend on the pre-built version of java_tools and so would have started to fail when the tests were updated for the new functionality added. Now java_tools has been updated (several times), it is safe to allow automated running of them once again. Closes #15100. PiperOrigin-RevId: 436702004
Upgraded jacoco package name for Bazel 5.0+. Upgraded jacoco from 0.8.3 to 0.8.6. Jacoco upgrade to 0.8.6 was made in Bazel in: bazelbuild/bazel#11674 bazelbuild/bazel@cb7c1a2 Removed options for workarounds where those have been fixed in the meantime: Bazel's handling for branch coverage was fixed in: bazelbuild/bazel#12696
Upgraded jacoco package name for Bazel 5.0+. Upgraded jacoco from 0.8.3 to 0.8.6. Jacoco upgrade to 0.8.6 was made in Bazel in: bazelbuild/bazel#11674 bazelbuild/bazel@cb7c1a2 Removed options for workarounds where those have been fixed in the meantime: Bazel's handling for branch coverage was fixed in: bazelbuild/bazel#12696 Instead of changing the existing script for building Jacoco, added a new one that can be used for Bazel 5.0.
* JacocoRunner script: update for Bazel 5.0+ Upgraded jacoco package name for Bazel 5.0+. Upgraded jacoco from 0.8.3 to 0.8.6. Jacoco upgrade to 0.8.6 was made in Bazel in: bazelbuild/bazel#11674 bazelbuild/bazel@cb7c1a2 Removed options for workarounds where those have been fixed in the meantime: Bazel's handling for branch coverage was fixed in: bazelbuild/bazel#12696 Instead of changing the existing script for building Jacoco, added a new one that can be used for Bazel 5.0. * build_jacocorunner_bazel_5.0+: update bazel tag to avoid error for Java 17 bazelbuild/bazel#15081 fixed a bug when using coverage with Scala targets on Java 17. Use the tag 6.0.0-pre.20220520.1 where this was fixed.
Description of the problem / feature request:
Bazel counts branch coverage also for code that would be filtered out by Jacoco.
Function and line coverage (when looking at reports generated by genhtml) seem to be respecting Jacoco filtering.
Jacoco is constantly improving filtering (https://www.jacoco.org/jacoco/trunk/doc/changes.html), but these changes (e.g. filtering out generated bytecode for Java and Kotlin) aren't then taken by Bazel branch coverage.
Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.
Note: These lines are not covered because of a bug in Jacoco 0.8.3 (that Bazel currently uses), it was fixed in Jacoco 0.8.5.
When in contrast I follow the instructions to upgrade Jacoco in #11674 (comment) (to Jacoco 0.8.3 with the backported Scala 2.12 lambda fix) and then run the above steps the line coverage changes (the above lines 23-27 are now covered) and the branch coverage stays the same for example-lib/src/main/scala/mypackage/Foo.scala (5/30).
Compare with plain Jacoco:
What operating system are you running Bazel on?
Ubuntu 20.04.
What's the output of
bazel info release
?release 3.7.1
If
bazel info release
returns "development version" or "(@non-git)", tell us how you built Bazel.What's the output of
git remote get-url origin ; git rev-parse master ; git rev-parse HEAD
?https://github.com/gergelyfabian/bazel-scala-example
8a9b58a48b86c456a0593427fbe217d6f193b404
aa8039f593df6628ef749f0c7bcb462976d45419
Have you found anything relevant by searching the web?
No.
Any other information, logs, or outputs that you want to share?
genhtml output segment for Foo.scala with Bazel 3.7.1 default Jacoco (0.8.3):
With Jacoco upgraded to 0.8.3 with Scala 2.12 lambda fix patch:
The text was updated successfully, but these errors were encountered: