Filter coverage output to just the files in the source file manifest

Coverage output should be filtered to just the sources listed in the manifest because those are the sources of targets matched by --instrumentation_filter (excluding test targets unless --instrument_test_targets). Coverage output should be restricted to those files according to the documentation for those flags.

This is a particular concern for C++ because C++ source files can include header files. That is, header files are source files of the target which directly includes them, but unlike most source files, code in header files may also be compiled by that target's consumers. Thus, CppCompile actions enable coverage instrumentation if either the target or any of the target's direct dependencies are matched by --instrumentation_filter. The resulting coverage output needs to be filtered to just the specific targets that are matched.

However, C++ is not the only language which has that property. It's also possible for coverage implementations to generate overly-broad coverage data for other reasons, or as an oversight. In those cases, the semantics of --instrumentation_filter should still be enforced.

This adds "jars" and "runtime_jars" to dependency attributes in the InstrumentationSpec for Java rules in order to preserve behavior for some of the tests. That test case doesn't seem like a usual way of structuring that dependency, if you have the source files checked in (as opposed to just the jarfile), it makes more sense to use the java_library target instead of using the jarfile output. But "jars" and "resource_jars" are in fact runtime dependencies, so listing them is at least technically correct.

RELNOTES: Filter all (instead of just C++) source files for coverage output according to --instrumentation_filter and --instrument_test_targets.
PiperOrigin-RevId: 344797828

Author: Googler

src/main/java/com/google/devtools/build/lib/rules/java/JavaCommon.java

@@ -60,7 +60,8 @@ public class JavaCommon {
   public static final InstrumentationSpec JAVA_COLLECTION_SPEC =
       new InstrumentationSpec(FileTypeSet.of(JavaSemantics.JAVA_SOURCE))
           .withSourceAttributes("srcs")
-          .withDependencyAttributes("deps", "data", "resources", "exports", "runtime_deps");
+          .withDependencyAttributes(
+              "deps", "data", "resources", "resource_jars", "exports", "runtime_deps", "jars");
 
   private ClasspathConfiguredFragment classpathFragment = new ClasspathConfiguredFragment();
   private JavaCompilationArtifacts javaArtifacts = JavaCompilationArtifacts.EMPTY;

tools/test/CoverageOutputGenerator/java/com/google/devtools/coverageoutputgenerator/Constants.java

@@ -14,8 +14,6 @@
 
 package com.google.devtools.coverageoutputgenerator;
 
-import com.google.common.collect.ImmutableList;
-
 /**
  * Stores markers used by the lcov tracefile and gcov intermediate format file. See <a
  * href="http://ltp.sourceforge.net/coverage/lcov/geninfo.1.php">lcov documentation</a> and the flag
@@ -51,9 +49,4 @@ class Constants {
   static final String GCOV_BRANCH_NOTEXEC = "notexec";
   static final String GCOV_BRANCH_NOTTAKEN = "nottaken";
   static final String GCOV_BRANCH_TAKEN = "taken";
-  // Please keep in sync with the extensions in CppFileTypes.
-  static final ImmutableList<String> CC_EXTENSIONS =
-      ImmutableList.of(
-          ".cc", ".cpp", ".cxx", ".c++", ".C", ".c", ".cu", ".cl", ".h", ".hh", ".hpp", ".ipp",
-          ".hxx", ".inc", ".inl", ".tlh", ".tli", ".H");
 }

tools/test/CoverageOutputGenerator/java/com/google/devtools/coverageoutputgenerator/Coverage.java

@@ -14,7 +14,6 @@
 
 package com.google.devtools.coverageoutputgenerator;
 
-import static com.google.devtools.coverageoutputgenerator.Constants.CC_EXTENSIONS;
 import static java.util.Arrays.asList;
 
 import com.google.common.base.Preconditions;
@@ -75,7 +74,7 @@ static Coverage create(List<SourceFileCoverage> sourceFilesCoverage) {
    * @param coverage The initial coverage.
    * @param sourcesToKeep The filenames of the sources to keep from the initial coverage.
    */
-  static Coverage getOnlyTheseCcSources(Coverage coverage, Set<String> sourcesToKeep) {
+  static Coverage getOnlyTheseSources(Coverage coverage, Set<String> sourcesToKeep) {
     if (coverage == null || sourcesToKeep == null) {
       throw new IllegalArgumentException("Coverage and sourcesToKeep should not be null.");
     }
@@ -87,23 +86,13 @@ static Coverage getOnlyTheseCcSources(Coverage coverage, Set<String> sourcesToKe
     }
     Coverage finalCoverage = new Coverage();
     for (SourceFileCoverage source : coverage.getAllSourceFiles()) {
-      if (!isCcSourceFile(source.sourceFileName())
-          || sourcesToKeep.contains(source.sourceFileName())) {
+      if (sourcesToKeep.contains(source.sourceFileName())) {
         finalCoverage.add(source);
       }
     }
     return finalCoverage;
   }
 
-  private static boolean isCcSourceFile(String filename) {
-    for (String ccExtension : CC_EXTENSIONS) {
-      if (filename.endsWith(ccExtension)) {
-        return true;
-      }
-    }
-    return false;
-  }
-
   /**
    * Replaces the source file names in the current coverage with their mapping in the given map, if
    * it exists.

tools/test/CoverageOutputGenerator/java/com/google/devtools/coverageoutputgenerator/Main.java

@@ -14,7 +14,6 @@
 
 package com.google.devtools.coverageoutputgenerator;
 
-import static com.google.devtools.coverageoutputgenerator.Constants.CC_EXTENSIONS;
 import static com.google.devtools.coverageoutputgenerator.Constants.GCOV_EXTENSION;
 import static com.google.devtools.coverageoutputgenerator.Constants.GCOV_JSON_EXTENSION;
 import static com.google.devtools.coverageoutputgenerator.Constants.PROFDATA_EXTENSION;
@@ -24,6 +23,7 @@
 import static java.nio.file.StandardCopyOption.REPLACE_EXISTING;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Lists;
 import java.io.BufferedReader;
@@ -36,7 +36,6 @@
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.ArrayList;
-import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
@@ -75,7 +74,7 @@ static int runWithArgs(String... args) throws ExecutionException, InterruptedExc
     List<File> filesInCoverageDir =
         flags.coverageDir() != null
             ? getCoverageFilesInDir(flags.coverageDir())
-            : Collections.emptyList();
+            : ImmutableList.of();
     Coverage coverage =
         Coverage.merge(
             parseFiles(
@@ -150,12 +149,9 @@ static int runWithArgs(String... args) throws ExecutionException, InterruptedExc
     }
 
     if (flags.hasSourceFileManifest()) {
-      // The source file manifest is only required for C++ code coverage.
-      Set<String> ccSources = getCcSourcesFromSourceFileManifest(flags.sourceFileManifest());
-      if (!ccSources.isEmpty()) {
-        // Only filter out coverage if there were C++ sources found in the coverage manifest.
-        coverage = Coverage.getOnlyTheseCcSources(coverage, ccSources);
-      }
+      coverage =
+          Coverage.getOnlyTheseSources(
+              coverage, getSourcesFromSourceFileManifest(flags.sourceFileManifest()));
     }
 
     if (coverage.isEmpty()) {
@@ -198,13 +194,13 @@ static int runWithArgs(String... args) throws ExecutionException, InterruptedExc
    * <p>This method only returns the C++ source files, ignoring the other files as they are not
    * necessary when putting together the final coverage report.
    */
-  private static Set<String> getCcSourcesFromSourceFileManifest(String sourceFileManifest) {
+  private static Set<String> getSourcesFromSourceFileManifest(String sourceFileManifest) {
     Set<String> sourceFiles = new HashSet<>();
     try (FileInputStream inputStream = new FileInputStream(new File(sourceFileManifest));
         InputStreamReader inputStreamReader = new InputStreamReader(inputStream, UTF_8);
         BufferedReader reader = new BufferedReader(inputStreamReader)) {
       for (String line = reader.readLine(); line != null; line = reader.readLine()) {
-        if (!isMetadataFile(line) && isCcFile(line)) {
+        if (!isMetadataFile(line)) {
           sourceFiles.add(line);
         }
       }
@@ -218,15 +214,6 @@ private static boolean isMetadataFile(String filename) {
     return filename.endsWith(".gcno") || filename.endsWith(".em");
   }
 
-  private static boolean isCcFile(String filename) {
-    for (String ccExtension : CC_EXTENSIONS) {
-      if (filename.endsWith(ccExtension)) {
-        return true;
-      }
-    }
-    return false;
-  }
-
   private static List<File> getGcovInfoFiles(List<File> filesInCoverageDir) {
     List<File> gcovFiles = getFilesWithExtension(filesInCoverageDir, GCOV_EXTENSION);
     if (gcovFiles.isEmpty()) {

tools/test/CoverageOutputGenerator/javatests/com/google/devtools/coverageoutputgenerator/CoverageTest.java

@@ -191,18 +191,18 @@ public void testGetOnlyTheseSources() throws Exception {
 
     assertThat(
             getSourceFileNames(
-                Coverage.getOnlyTheseCcSources(coverage, sourcesToKeep).getAllSourceFiles()))
+                Coverage.getOnlyTheseSources(coverage, sourcesToKeep).getAllSourceFiles()))
         .containsExactly("source/common/protobuf/utility.cc", "source/common/grpc/common.cc");
   }
 
   @Test(expected = IllegalArgumentException.class)
   public void testGetOnlyTheseSourcesNullCoverage() {
-    Coverage.getOnlyTheseCcSources(null, new HashSet<>());
+    Coverage.getOnlyTheseSources(null, new HashSet<>());
   }
 
   @Test(expected = IllegalArgumentException.class)
   public void testGetOnlyTheseSourcesNullSources() {
-    Coverage.getOnlyTheseCcSources(new Coverage(), null);
+    Coverage.getOnlyTheseSources(new Coverage(), null);
   }
 
   @Test
@@ -213,7 +213,7 @@ public void testGetOnlyTheseSourcesEmptySources() throws Exception {
     coverage.add(new SourceFileCoverage("source/server/options.cc"));
     coverage.add(new SourceFileCoverage("source/server/manager.cc"));
 
-    assertThat(Coverage.getOnlyTheseCcSources(coverage, new HashSet<>()).getAllSourceFiles())
+    assertThat(Coverage.getOnlyTheseSources(coverage, new HashSet<>()).getAllSourceFiles())
         .isEmpty();
   }
 }

tools/test/collect_coverage.sh

@@ -200,9 +200,10 @@ fi
 #
 # --source_file_manifest  The absolute path of the coverage source file
 #                         manifest. CoverageOutputGenerator uses this file to
-#                         keep only the C++ sources found in the manifest.
-#                         For other languages the sources in the manifest are
-#                         ignored.
+#                         keep only the sources found in the manifest (that is,
+#                         only the sources of targets matched by
+#                         --instrumentation_filter, excluding test targets
+#                         unless --instrument_test_targets).
 
 if [[ "$IS_COVERAGE_SPAWN" == "1" ]]; then
   COVERAGE_DIR=$(resolve_links $COVERAGE_DIR)
@@ -231,4 +232,4 @@ fi
 # JAVA_RUNFILES is set to the runfiles of the test, which does not necessarily
 # contain a JVM (it does only if the test has a Java binary somewhere). So let
 # the LCOV merger discover where its own runfiles tree is.
-JAVA_RUNFILES= exec $LCOV_MERGER_CMD
+JAVA_RUNFILES= exec $LCOV_MERGER_CMD