Bazel: Default `--instrumentation_filter` when 2 targets share a prefix is incorrect

[keith]

The instrumentation_filter flag appears to to have a few different default cases:

1. If you're testing a single target like: //Modules/ResourceSupport:ResourceSupportTests it will default to //Modules/ResourceSupport[/:]
2. If you're testing 2 targets like: //Modules/ResourceSupport:ResourceSupportTests and
   //Modules/LyftKit:LyftKitTests it will default to their common
   prefix: //Modules[/:]
3. The unexpected case is if you test too targets like
   //Modules/TransitNearby:TransitNearbyTests and
   //Modules/TransitNearbyFlow:TransitNearbyFlowTests where they have
   a common prefix, it defaults to //Modules/TransitNearby[/:] which
   ends up excluding the TransitNearbyFlow files from the coverage manifest.

Am I correct in thinking that this is a bug and in that case it should either be //Modules[/:] or //Modules/TransitNearby without the [/:]

What operating system are you running Bazel on?

macOS

What's the output of bazel info release?

release 2.2.0

[limdor]

I just came across this issue today. @lberki could we add the coverage label?
@caiyulun I saw that you created a PR some time ago, are you planning to continue it? I understand that the missing part are only tests right?

[lberki]

sure can!

[limdor]

@lberki I would like to fix this issue, taking the open PR that @caiyulun created. In the PR @caiyulun mentioned 4 months ago that was working on tests but I assume that something prevented to continue on the topic. However I have some question about the code base, would it be here, in the PR, or in bazel-dev the place to ask? For the moment I will put it here in case you can answer right a way and if not I can move it to some other channel.

What is missing in the PR is a test case but InstrumentationFilterSupport has directly no tests. I was checking if the test should derive from BuildIntegrationTestCase or AnalysisTestCase but I cannot get an answer. I thought that these instrumentation filters are computed during the analysis phase but the file InstrumentationFilterSupport.java is inside buildtool folder and not analysis. There is no test in buildtool that derive from AnalysisTestCase.

I wanted to derive from AnalysisTestCase because the method under test is:
public static String computeInstrumentationFilter(EventHandler eventHandler, Collection<Target> testTargets)
And to get the target collection I was considering to use Target getTarget(String label) that it is only in AnalysisTestCase

Here my question: Can the new test derive from AnalysisTestCase and stay under the buildtool folder? Will it be enough or we really need to build some target to compute the instrumentation filters? I assume that BuildIntegrationTestCase would work but I thought that maybe is too much overkill and AnalysisTestCase should sufficient.

Bonus: I was also thinking if indeed InstrumentationFilterSupport.java should be moved from buildtool to analysis but I saw that is used in AnalysisPhaseRunner that is also under buildtool. In addition I saw that needsInstrumentationFilter is in BuildRequest.java that is also inside buildtool

[c-mita]

The internal InstrumentationFilterTest class derives BuildViewTestCase, which is similar to AnalysisTestCase for the purposes of this discussion.

Since we have internal tests, I would not worry too much about _where_ you implement your test case, since we don't need to pull in the test class itself and can instead, transfer the cases you write into the existing test class.

With a little bit of inlining, the existing tests look kind of like this:

@Test
private void testFoo() throws Exception {
  EventCollector events = new EventCollector(EventKind.INFO);
  scratch.file("foo/BUILD", "sh_test(name='t', srcs=['t.sh']")
  Collection<Target> targets = Collections.singletonList(getTarget("//foo:t");
  String expectedFilter = "^//foo[/:]";
  assertThat(InstrumentationFilterSupport.computeInstrumentationFilter(events, targets))
      .isEqualTo(expectedFilter);
}

(Caveat, I might have got the expectedFilter wrong, I haven't checked it).

It's not ideal, and it would be much nicer if we could get the test code open sourced, but I don't know where we are with that effort at the moment.

[limdor]

Thanks for the hints @c-mita, I created a PR with the changes of the original PR, adding the missing include and adding some tests.
Just tell me if I got something wrong, last time I wrote Java code was more than 10 years ago :-)
https://github.com/bazelbuild/bazel/pull/12607

[keith]

Does that PR fix this one? If so I can close this

[limdor]

Yes, and I would also recommend to close https://github.com/bazelbuild/bazel/pull/11690