Cannot skip problematic classes during coverage instrumentation

[jmmv]

Description of the feature request:

We have a few methods that become too large after coverage instrumentation. These cause the Jacoco instrumeter to fail with Method too large.

I've been playing with the --instrumentation_filter flag but it does not help avoid the problem. The reason it doesn't help is that the problematic classes are part of really large targets that contain many other classes. I cannot move the problematic classes to their own targets for various reasons and I cannot skip instrumenting the large targets altogether because then I'd have massive gaps in the coverage data.

Digging on how this whole thing works, the first thing I found was that JavaCompileActionBuilder.java passes -*Test and -*TestCase arguments to the Jacoco processor. These seemed to be filters for things to not instrument and I thought it was only a matter of adding extra patterns there. But that didn't work.

So, digging further, I ended up reaching the following comment in JacocoInstrumentationProcessor.java:

            // TODO(bazel-team): filter with coverage_instrumentation_filter?
            // It's not clear whether there is any advantage in not instrumenting *Test classes,
            // apart from lowering the covered percentage in the aggregate statistics.

It turns out that the Jacoco processor seems to have had a feature earlier on to actually skip files to instrument but either that feature was never fully implemented or was lost in some refactoring because what's left behind doesn't work.

To answer the question in the to-do, the answer is yes: having the ability to filter out individual classes during instrumentation would be helpful. We might need a separate flag from --instrumentation_filter though, given that this flag seems to specify Bazel patterns and we need to specify filters on file names.

Which category does this issue belong to?

Java Rules

What underlying problem are you trying to solve with this feature?

Skip classes that are too large for instrumentation and cannot be easily refactored.

Which operating system are you running Bazel on?

N/A

What is the output of bazel info release?

release 6.5.0 but also applies to 7.0.2

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse HEAD ?

No response

Have you found anything relevant by searching the web?

No response

Any other information, logs, or outputs that you want to share?

No response

[jmmv]

I have a POC of what I was looking for in https://github.com/bazelbuild/bazel/pull/21521

[jmmv]

https://github.com/bazelbuild/bazel/pull/21559 contains a different take on the fix, which is simpler: just ignore classes that fail instrumentation and emit a warning for them.

[cushon]

For posterity there was some discussion about the issue and possible solutions in https://github.com/bazelbuild/bazel/pull/21521

I would be nice if JaCoCo could recognize this case and handle it instead of emitting invalid bytecode. I see some related discussion in https://github.com/jacoco/jacoco/issues/983, where the recommendation was to have smaller methods or else exclude the large methods from coverage instrumentation.

[jmmv]

@cushon Yes, we should be refactoring our code to avoid the large methods but it isn't that simple... One case is easy: we have a large parameters class with lots of constants so the class initializer grows too big. I could workaround this one by splitting the class in two and making one inherit from the other; ugly, but fine. However, another case is trickier: ANTLR is generating the too-long code, so we are at the mercy of that code generation tool.

Agree it'd be nice if JaCoCo didn't fail here and skipped these methods.

In any case, what's your opinion on the best path forward here? Are #21521 or #21559 patches you'd be willing to take?

[hvadehra]

Finally got around to looking at this, sorry for the delay.

https://github.com/bazelbuild/bazel/pull/21559 might be simpler, but in it's current state seems like the worse option to me.

At the very least, I think we should: a. not ignore exceptions by default - this behaviour should need an explicit opt in b. as @cushon suggested on the PR, only catch a minimal set of specific exceptions and revisit expanding the set later on if needed.

If we're going to do (a), I'd prefer making it a list of classes/packages which would have the nice property of giving users an allowlist of sorts to burn down. This is in contrast to a boolean flag, which once set becomes as good as not having it at all.

[cushon]

I'm leaning towards (b).

(a) would be OK, the main drawback is just needing to create another configuration mechanism and plumb it through to JavaBuilder.

If we're going to do (a), I'd prefer making it a list of classes/packages which would have the nice property of giving users an allowlist of sorts to burn down.

I like this kind of approach in general, but in this specific case I'd be inclined to wait and see if anyone wants that before building it. If there's some generated code that's close enough to the method size limit to trigger this issue, I suspect the coverage information isn't going to be that useful anyways (it's generated code), and that people may not be motivated to refactor to avoid the issue instead of just allowing coverage instrumentation to skip those methods.

[hvadehra]

Sorry I wasn't clearer. I was actually suggesting doing both.

I think (a) is especially important if the exception(s) we choose to ignore with (b) are more general than we'd like. If we can exactly ignore only failures like the method size limit, I guess that's okay. But if not, we risk (almost) silently skipping easy-to-fix failures by default.

[hvadehra]

@jmmv WDYT?

[github-actions[bot]]

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 90 days unless any other activity occurs. If you think this issue is still relevant and should stay open, please post any comment here and the issue will no longer be marked as stale.

[jmmv]

Inactivity doesn't mean the bug is gone.