CLI: adding filtering support into instrumentation and into reporting

[mathjeff]

No description provided.

[marchof]

@mathjeff Thanks for starting working on this! However, before implementing a solution for the CLI we need to clarify the filtering semantics for all our tools. We currently have a bad and confusing mixture (see #34) and I don't want to make it worse by making the same mistakes for the CLI. The design should also consider:

• Includes/excludes options for Ant report task #17
• Add class includes/excludes attributes for instrumentation #187
• Add includes/excludes attributes to dump goal/task #188
• Add includes/excludes attributes to merge goal/task #189

I'm not saying this all has to be implemented. But before we add new filters we need a clear design how we distinguish between class name and file name filtering in future.

[mathjeff]

Is your concern more about the semantics of filtering class files deeply nested within .jar files that are themselves nested within other .jar files, or more about the (backwards compatibility problem) fact that the arguments slightly change (filename -> classname) when a caller wants to filter a subset of class files in a directory (or do you have a different concern entirely)?

My assumption was (eventually after consolidating all the integrations (Maven, Ant...) to use RecursiveInstrumenter and ClassnameMatcher) that only filtering by classname would be supported, and that the list of file paths to filter would be a parameter.

Who would know what the requirements for the design would be? Would that be you?

Thanks

[marchof]

@mathjeff Change of semantics and backward compatibility both are concerns. I would definitely like to move to class name filtering for all tools in the future (independent of packaging).

We can change the API (probably with a compatibility mode for some releases), but as said before we need a consistent target design and migration path before we start implementing new filtering locally. Me and @Godin are the contacts for design decisions.

Regarding internal implementation: I would prefer a filter callback interface instead of subclassing the Instrumenter class. The same callback should then also be used in Analyzer.

[mathjeff]

@marchof

I see a few ways we could do the transition. It seems to me that the properties that we wish be true about the transition are:

1. It's easy for users to detect that the transition is starting and that they have to make a change to their usage
2. The new usage is concise
3. If a user declines to change their usage but updates to a new version of Jacoco that no longer supports the old usage, the error is clear.

I see a test where a Maven pom specifies an exclude of '**/DoNotInstrument.class'. I also see a test where an Ant build xml specifies an include of 'includes="**/*.class" '

My thought about the transition period is that the Maven and Ant plugins (which don't necessarily have to transition at the same times as each other) would provide two sets of filters: the current "includes" and "excludes" that they have now, and also a second set of filters that applies on top of the other filters: "includeClasses" and "excludeClasses". During the transition period, a warning would be shown (with upgrade instructions) if the "excludes" property is nonempty. After the transition period, the "excludes" property would be removed. However, I think we want to keep the "includes" property (with its current meaning) because there needs to be some way of specifying which files should be read in the first place (I expect reading all files in some default location would be rather slow if there exist many directories that have no class files).

Yeah, after I redid my filtering patch on top of yours, I replaced mathjeff@0e0c5ec#diff-b62366bdae367404470726a7f2a30c1aR26 with 5ce8fc2#diff-4dc4f1c858db7bb2270b3de33ec79aa8R43 since I wasn't adding more filtering on top of a patch that already had filtering. My approach this time was to extract the recursion logic into a separate class from the instrumentation logic, and for the filter itself to be a third class. I could do a callback instead if you'd rather keep the recursion logic in the same class.

If you have other, unrelated things you'd rather be working on, though, it's not particularly important for me to get this pull request merged. I imagine whatever syntax Jacoco does eventually end up officially supporting won't be particularly difficult to use, and that it won't be much effort for Android to update to a newer Jacoco at that time. Alternatively if you do have interest then I should still have some more time to keep updating things based on feedback.

If you do have interest, then I'm curious about whether there are any other difficulties that you expect the design to need to cover other than the upgrade process, or whether we've covered all of the concerns.

Thanks

[mathjeff]

Also, when you do come back to this pull request, it'd be good to know your opinions about adding a dependency like Guava or apache-commons that already contains an interface like this, as opposed to redefining an interface here.

It happens to be slightly more convenient for Android if we avoid the dependency, although I imagine that whichever approach is best for Jacoco itself would be the one we'd want to actually do.

[Godin]

@mathjeff For our artifacts that themselve can be used as dependencies (i.e. core - everything except Maven Plugin and CLI) I would prefer to avoid any dependency on any third-party artifact except ASM to avoid any questions about compatibility and transitive dependencies. And so especially avoid dependency on Guava (https://dev.eclipse.org/mhonarc/lists/cross-project-issues-dev/msg14431.html). Also recall that JaCoCo can still be executed under Java 5, while latest versions of Guava and Apache commons-collections, commons-lang, commons-io can't. Also recall that for integration into EclEmma any JaCoCo dependency should be in Eclipe Orbit. Shading of dependencies raises licensing/redistribution questions. So all in all - IMO a way simpler to just avoid them. A single interface definitely not worth the efforts.

[mathjeff]

@Godin Hey thanks for the response. That sounds great to me.

[lhotari]

Filtering support (excludes) would be a useful addition to the CLI interface. The CLI works, but since there's no way to exclude classes inside jar files, the solution requires some hacks to process jar files before reporting.

The Maven plugin is unusable for creating a code coverage solution for Apache Pulsar CI pipeline since the Maven plugin's report-aggregate requires dependent projects to be part of the Maven reactor. In addition, the includeCurrentProject feature (#1007) isn't available in a released version.

The Apache Pulsar CI build is split into about 25 builds that all collect coverage information. It was pretty painful to get the solution in place and it required a lot of bash scripting. If anyone is interested, the PR in Apache Pulsar is #19264 . Perhaps some day, it will be easier to get consistent code coverage metrics implemented on a large project.
The solution also covers collecting coverage from integration tests that run in docker containers with Testcontainers.