[6.2.0]Add test coverage support to android_local_test (#17467)

ted-xie · tjgq · Bencodes · web-flow · commit b874e5fd7ed6 · 2023-03-13T15:19:28.000-07:00
* Propagate runfiles for singlejar. Since the singlejar toolchain attribute is currently defined as a single file and carried around as an Artifact instead of a FilesToRunProvider, it's not possible to implement it as a wrapper script that dispatches to an actual implementation somewhere in its runfiles. This would be useful to experiment with cross-platform action sharing. PiperOrigin-RevId: 492487124 Change-Id: Ib0f80314eae09bd865b3f31a4180bf068833cdf4 * Add test coverage support to android_local_test Adding test coverage support to `android_local_test`. #15827 Closes #15840. RELNOTES: Adds coverage metric support to android_local_test PiperOrigin-RevId: 508549884 Change-Id: I6977efa51ca1c7a6df1f776fe1a326d07989a185 --------- Co-authored-by: Googler <tjgq@google.com> Co-authored-by: Benjamin Lee <ben@ben.cm> Co-authored-by: kshyanashree <109167932+kshyanashree@users.noreply.github.com>
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/BUILD b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/BUILD
@@ -27,6 +27,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:actions/custom_command_line",
         "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
         "//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories",
+        "//src/main/java/com/google/devtools/build/lib/analysis:config/execution_transition_factory",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/toolchain_type_requirement",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/composing_transition_factory",
         "//src/main/java/com/google/devtools/build/lib/analysis:rule_definition_environment",
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/BazelAndroidLocalTest.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/BazelAndroidLocalTest.java
@@ -13,6 +13,7 @@
 // limitations under the License.
 package com.google.devtools.build.lib.bazel.rules.android;
 
+import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.Artifact;
@@ -34,6 +35,9 @@
 /** An implementation for the "android_local_test" rule. */
 public class BazelAndroidLocalTest extends AndroidLocalTestBase {
 
+  private static final String JACOCO_COVERAGE_RUNNER_MAIN_CLASS =
+      "com.google.testing.coverage.JacocoCoverageRunner";
+
   public BazelAndroidLocalTest() {
     super(BazelAndroidSemantics.INSTANCE);
   }
@@ -76,9 +80,14 @@ protected String addCoverageSupport(
       JavaTargetAttributes.Builder attributesBuilder,
       String mainClass)
       throws RuleErrorException {
-    // coverage does not yet work with android_local_test
-    ruleContext.throwWithRuleError("android_local_test does not yet support coverage");
-    return "";
+    // This method can be called only for *_binary/*_test targets.
+    Preconditions.checkNotNull(executable);
+
+    helper.addCoverageSupport();
+
+    // We do not add the instrumented jar to the runtime classpath, but provide it in the shell
+    // script via an environment variable.
+    return JACOCO_COVERAGE_RUNNER_MAIN_CLASS;
   }
 
   @Override
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/BazelAndroidLocalTestRule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/BazelAndroidLocalTestRule.java
@@ -25,6 +25,7 @@
 import com.google.devtools.build.lib.analysis.BaseRuleClasses;
 import com.google.devtools.build.lib.analysis.RuleDefinition;
 import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment;
+import com.google.devtools.build.lib.analysis.config.ExecutionTransitionFactory;
 import com.google.devtools.build.lib.bazel.rules.java.BazelJavaRuleClasses.BaseJavaBinaryRule;
 import com.google.devtools.build.lib.packages.ImplicitOutputsFunction;
 import com.google.devtools.build.lib.packages.RuleClass;
@@ -82,7 +83,11 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment envi
         .removeAttribute("main_class")
         .removeAttribute("resources")
         .removeAttribute("use_testrunner")
-        .removeAttribute(":java_launcher")
+        .removeAttribute(":java_launcher") // Input files for test actions collecting code coverage
+        .add(
+            attr(":lcov_merger", LABEL)
+                .cfg(ExecutionTransitionFactory.create())
+                .value(BaseRuleClasses.getCoverageOutputGeneratorLabel()))
         .cfg(
             new ConfigFeatureFlagTransitionFactory(AndroidFeatureFlagSetProvider.FEATURE_FLAG_ATTR))
         .build();
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AarImport.java b/src/main/java/com/google/devtools/build/lib/rules/android/AarImport.java
@@ -20,6 +20,7 @@
 import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.FileProvider;
+import com.google.devtools.build.lib.analysis.FilesToRunProvider;
 import com.google.devtools.build.lib.analysis.OutputGroupInfo;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTargetFactory;
@@ -403,7 +404,7 @@ private static SpawnAction createAarEmbeddedJarsExtractorActions(
   private static SpawnAction createAarJarsMergingActions(
       RuleContext ruleContext, Artifact jarsTreeArtifact, Artifact mergedJar, Artifact paramFile) {
     SpawnAction.Builder builder = new SpawnAction.Builder().useDefaultShellEnvironment();
-    Artifact singleJar = JavaToolchainProvider.from(ruleContext).getSingleJar();
+    FilesToRunProvider singleJar = JavaToolchainProvider.from(ruleContext).getSingleJar();
     return builder
         .setExecutable(singleJar)
         .setMnemonic("AarJarsMerger")
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java
@@ -1957,7 +1957,7 @@ private static SpawnAction.Builder createSpawnActionBuilder(RuleContext ruleCont
   }
 
   private static SpawnAction.Builder singleJarSpawnActionBuilder(RuleContext ruleContext) {
-    Artifact singleJar = JavaToolchainProvider.from(ruleContext).getSingleJar();
+    FilesToRunProvider singleJar = JavaToolchainProvider.from(ruleContext).getSingleJar();
     SpawnAction.Builder builder =
         createSpawnActionBuilder(ruleContext).useDefaultShellEnvironment();
     builder.setExecutable(singleJar);
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidLocalTestBase.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidLocalTestBase.java
@@ -20,6 +20,7 @@
 import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
 import com.google.devtools.build.lib.analysis.Allowlist;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
+import com.google.devtools.build.lib.analysis.FilesToRunProvider;
 import com.google.devtools.build.lib.analysis.OutputGroupInfo;
 import com.google.devtools.build.lib.analysis.RequiredConfigFragmentsProvider;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder;
@@ -29,6 +30,7 @@
 import com.google.devtools.build.lib.analysis.Runfiles;
 import com.google.devtools.build.lib.analysis.RunfilesProvider;
 import com.google.devtools.build.lib.analysis.RunfilesSupport;
+import com.google.devtools.build.lib.analysis.SourceManifestAction;
 import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
 import com.google.devtools.build.lib.analysis.actions.Substitution;
 import com.google.devtools.build.lib.analysis.actions.Template;
@@ -66,6 +68,7 @@
 import com.google.devtools.build.lib.rules.java.OneVersionCheckActionBuilder;
 import com.google.devtools.build.lib.rules.java.proto.GeneratedExtensionRegistryProvider;
 import com.google.devtools.build.lib.util.OS;
+import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.ArrayList;
 import java.util.List;
@@ -292,7 +295,7 @@ public ConfiguredTarget create(RuleContext ruleContext)
         originalMainClass,
         filesToBuildBuilder,
         javaExecutable,
-        /* createCoverageMetadataJar= */ true);
+        /* createCoverageMetadataJar= */ false);
 
     Artifact oneVersionOutputArtifact = null;
     JavaConfiguration javaConfig = ruleContext.getFragment(JavaConfiguration.class);
@@ -364,6 +367,60 @@ public ConfiguredTarget create(RuleContext ruleContext)
 
     JavaInfo.Builder javaInfoBuilder = JavaInfo.Builder.create();
 
+    NestedSetBuilder<Pair<String, String>> coverageEnvironment = NestedSetBuilder.stableOrder();
+    NestedSetBuilder<Artifact> coverageSupportFiles = NestedSetBuilder.stableOrder();
+    if (ruleContext.getConfiguration().isCodeCoverageEnabled()) {
+
+      // Create an artifact that contains the runfiles relative paths of the jars on the runtime
+      // classpath. Using SourceManifestAction is the only reliable way to match the runfiles
+      // creation code.
+      Artifact runtimeClasspathArtifact =
+          ruleContext.getUniqueDirectoryArtifact(
+              "runtime_classpath_for_coverage",
+              "runtime_classpath.txt",
+              ruleContext.getBinOrGenfilesDirectory());
+      ruleContext.registerAction(
+          new SourceManifestAction(
+              SourceManifestAction.ManifestType.SOURCES_ONLY,
+              ruleContext.getActionOwner(),
+              runtimeClasspathArtifact,
+              new Runfiles.Builder(
+                      ruleContext.getWorkspaceName(),
+                      ruleContext.getConfiguration().legacyExternalRunfiles())
+                  // This matches the code below in collectDefaultRunfiles.
+                  .addTransitiveArtifactsWrappedInStableOrder(javaCommon.getRuntimeClasspath())
+                  .build(),
+              null,
+              true));
+      filesToBuildBuilder.add(runtimeClasspathArtifact);
+
+      // Pass the artifact through an environment variable in the coverage environment so it
+      // can be read by the coverage collection script.
+      coverageEnvironment.add(
+          new Pair<>(
+              "JAVA_RUNTIME_CLASSPATH_FOR_COVERAGE", runtimeClasspathArtifact.getExecPathString()));
+      // Add the file to coverageSupportFiles so it ends up as an input for the test action
+      // when coverage is enabled.
+      coverageSupportFiles.add(runtimeClasspathArtifact);
+
+      // Make single jar reachable from the coverage environment because it needs to be executed
+      // by the coverage collection script.
+      FilesToRunProvider singleJar = JavaToolchainProvider.from(ruleContext).getSingleJar();
+      coverageEnvironment.add(
+          new Pair<>("SINGLE_JAR_TOOL", singleJar.getExecutable().getExecPathString()));
+      coverageSupportFiles.addTransitive(singleJar.getFilesToRun());
+    }
+
+    javaCommon.addTransitiveInfoProviders(
+        builder,
+        javaInfoBuilder,
+        filesToBuild,
+        classJar,
+        coverageEnvironment.build(),
+        coverageSupportFiles.build());
+    javaCommon.addGenJarsProvider(
+        builder, javaInfoBuilder, outputs.genClass(), outputs.genSource());
+
     javaCommon.addTransitiveInfoProviders(builder, javaInfoBuilder, filesToBuild, classJar);
     javaCommon.addGenJarsProvider(
         builder, javaInfoBuilder, outputs.genClass(), outputs.genSource());
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/ApkActionsBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/android/ApkActionsBuilder.java
@@ -418,7 +418,7 @@ private void signApk(
 
   private static void setSingleJarAsExecutable(
       RuleContext ruleContext, SpawnAction.Builder builder) {
-    Artifact singleJar = JavaToolchainProvider.from(ruleContext).getSingleJar();
+    FilesToRunProvider singleJar = JavaToolchainProvider.from(ruleContext).getSingleJar();
     builder.setExecutable(singleJar);
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/DeployArchiveBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/java/DeployArchiveBuilder.java
@@ -26,6 +26,7 @@
 import com.google.devtools.build.lib.actions.ParamFileInfo;
 import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType;
 import com.google.devtools.build.lib.actions.ResourceSet;
+import com.google.devtools.build.lib.analysis.FilesToRunProvider;
 import com.google.devtools.build.lib.analysis.RuleContext;
 import com.google.devtools.build.lib.analysis.actions.CustomCommandLine;
 import com.google.devtools.build.lib.analysis.actions.SpawnAction;
@@ -420,7 +421,7 @@ public void build() throws InterruptedException {
       inputs.add(libModules);
     }
 
-    Artifact singlejar = JavaToolchainProvider.from(ruleContext).getSingleJar();
+    FilesToRunProvider singlejar = JavaToolchainProvider.from(ruleContext).getSingleJar();
 
     String toolchainIdentifier = null;
     try {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaBinary.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaBinary.java
@@ -31,6 +31,7 @@
 import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
 import com.google.devtools.build.lib.analysis.Allowlist;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
+import com.google.devtools.build.lib.analysis.FilesToRunProvider;
 import com.google.devtools.build.lib.analysis.OutputGroupInfo;
 import com.google.devtools.build.lib.analysis.PrerequisiteArtifacts;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder;
@@ -510,9 +511,10 @@ public ConfiguredTarget create(RuleContext ruleContext)
 
       // Make single jar reachable from the coverage environment because it needs to be executed
       // by the coverage collection script.
-      Artifact singleJar = JavaToolchainProvider.from(ruleContext).getSingleJar();
-      coverageEnvironment.add(new Pair<>("SINGLE_JAR_TOOL", singleJar.getExecPathString()));
-      coverageSupportFiles.add(singleJar);
+      FilesToRunProvider singleJar = JavaToolchainProvider.from(ruleContext).getSingleJar();
+      coverageEnvironment.add(
+          new Pair<>("SINGLE_JAR_TOOL", singleJar.getExecutable().getExecPathString()));
+      coverageSupportFiles.addTransitive(singleJar.getFilesToRun());
     }
 
     common.addTransitiveInfoProviders(
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaToolchain.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaToolchain.java
@@ -78,7 +78,7 @@ public ConfiguredTarget create(RuleContext ruleContext)
                 .get("reduced_classpath_incompatible_processors", Type.STRING_LIST));
     boolean forciblyDisableHeaderCompilation =
         ruleContext.attributes().get("forcibly_disable_header_compilation", Type.BOOLEAN);
-    Artifact singleJar = ruleContext.getPrerequisiteArtifact("singlejar");
+    FilesToRunProvider singleJar = ruleContext.getExecutablePrerequisite("singlejar");
     FilesToRunProvider oneVersion = ruleContext.getExecutablePrerequisite("oneversion");
     Artifact oneVersionAllowlist = ruleContext.getPrerequisiteArtifact("oneversion_whitelist");
     Artifact genClass = ruleContext.getPrerequisiteArtifact("genclass");
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaToolchainProvider.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaToolchainProvider.java
@@ -112,7 +112,7 @@ public static JavaToolchainProvider create(
       ImmutableSet<String> headerCompilerBuiltinProcessors,
       ImmutableSet<String> reducedClasspathIncompatibleProcessors,
       boolean forciblyDisableHeaderCompilation,
-      Artifact singleJar,
+      FilesToRunProvider singleJar,
       @Nullable FilesToRunProvider oneVersion,
       @Nullable Artifact oneVersionAllowlist,
       Artifact genClass,
@@ -174,7 +174,7 @@ public static JavaToolchainProvider create(
   private final ImmutableSet<String> headerCompilerBuiltinProcessors;
   private final ImmutableSet<String> reducedClasspathIncompatibleProcessors;
   private final boolean forciblyDisableHeaderCompilation;
-  private final Artifact singleJar;
+  private final FilesToRunProvider singleJar;
   @Nullable private final FilesToRunProvider oneVersion;
   @Nullable private final Artifact oneVersionAllowlist;
   private final Artifact genClass;
@@ -208,7 +208,7 @@ private JavaToolchainProvider(
       ImmutableSet<String> headerCompilerBuiltinProcessors,
       ImmutableSet<String> reducedClasspathIncompatibleProcessors,
       boolean forciblyDisableHeaderCompilation,
-      Artifact singleJar,
+      FilesToRunProvider singleJar,
       @Nullable FilesToRunProvider oneVersion,
       @Nullable Artifact oneVersionAllowlist,
       Artifact genClass,
@@ -334,14 +334,14 @@ public boolean getForciblyDisableHeaderCompilation() {
     return forciblyDisableHeaderCompilation;
   }
 
-  /** Returns the {@link Artifact} of the SingleJar deploy jar */
+  /** Returns the {@link FilesToRunProvider} of the SingleJar tool. */
   @Override
-  public Artifact getSingleJar() {
+  public FilesToRunProvider getSingleJar() {
     return singleJar;
   }
 
   /**
-   * Return the {@link Artifact} of the binary that enforces one-version compliance of java
+   * Return the {@link FilesToRunProvider} of the tool that enforces one-version compliance of Java
    * binaries.
    */
   @Override
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaToolchainRule.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaToolchainRule.java
@@ -153,7 +153,6 @@ The Java target version (e.g., '6' or '7'). It specifies for which Java runtime
         .add(
             attr("singlejar", LABEL_LIST)
                 .mandatory()
-                .singleArtifact()
                 // This needs to be in the execution configuration.
                 .cfg(ExecutionTransitionFactory.create())
                 .allowedFileTypes(FileTypeSet.ANY_FILE)
diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/java/JavaToolchainStarlarkApiProviderApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/java/JavaToolchainStarlarkApiProviderApi.java
@@ -49,13 +49,13 @@ public interface JavaToolchainStarlarkApiProviderApi extends StructApi {
   @StarlarkMethod(name = "target_version", doc = "The java target version.", structField = true)
   String getTargetVersion();
 
-  @StarlarkMethod(name = "single_jar", doc = "The SingleJar deploy jar.", structField = true)
-  FileApi getSingleJar();
+  @StarlarkMethod(name = "single_jar", doc = "The SingleJar tool.", structField = true)
+  FilesToRunProviderApi<? extends FileApi> getSingleJar();
 
   @Nullable
   @StarlarkMethod(
       name = "one_version_tool",
-      doc = "The artifact that enforces One-Version compliance of java binaries.",
+      doc = "The tool that enforces One-Version compliance of java binaries.",
       structField = true,
       allowReturnNones = true)
   FilesToRunProviderApi<? extends FileApi> getOneVersionBinary();
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/rules/android/BazelAndroidLocalTestTest.java b/src/test/java/com/google/devtools/build/lib/bazel/rules/android/BazelAndroidLocalTestTest.java
@@ -69,16 +69,6 @@ public void testDisallowPrecompiledJars() throws Exception {
         "    srcs = ['lib.jar'])");
   }
 
-  @Test
-  public void testCoverageThrowsError() throws Exception {
-    useConfiguration("--collect_code_coverage");
-    checkError("java/test",
-        "test",
-        "android_local_test does not yet support coverage",
-        "android_local_test(name = 'test',",
-        "    srcs = ['test.java'])");
-  }
-
   @Test
   public void testNoAndroidAllJarsPropertiesFileThrowsError() throws Exception {
     checkError("java/test",
diff --git a/src/test/shell/bazel/android/BUILD b/src/test/shell/bazel/android/BUILD
@@ -49,6 +49,25 @@ android_sh_test(
     ],
 )
 
+android_sh_test(
+    name = "android_local_test_integration_test",
+    size = "large",
+    srcs = ["android_local_test_integration_test.sh"],
+    data = [
+        ":android_helper",
+        # TODO(b/226204219): Remove this dep once Bazel properly loads d8
+        # in create_android_sdk_rules()
+        "//external:android/d8_jar_import",
+        "//external:android_sdk_for_testing",
+        "//src/test/shell/bazel:test-deps",
+    ],
+    # See https://github.com/bazelbuild/bazel/issues/8235
+    tags = [
+        "no-remote",
+        "no_windows",
+    ],
+)
+
 android_sh_test(
     name = "aapt_integration_test",
     size = "large",
diff --git a/src/test/shell/bazel/android/android_local_test_integration_test.sh b/src/test/shell/bazel/android/android_local_test_integration_test.sh

Original file line number	Diff line number	Diff line change
`@@ -1957,7 +1957,7 @@ private static SpawnAction.Builder createSpawnActionBuilder(RuleContext ruleCont`
`1957`	`1957`	`}`
`1958`	`1958`
`1959`	`1959`	`private static SpawnAction.Builder singleJarSpawnActionBuilder(RuleContext ruleContext) {`
`1960`		`- Artifact singleJar = JavaToolchainProvider.from(ruleContext).getSingleJar();`
	`1960`	`+ FilesToRunProvider singleJar = JavaToolchainProvider.from(ruleContext).getSingleJar();`
`1961`	`1961`	`SpawnAction.Builder builder =`
`1962`	`1962`	`createSpawnActionBuilder(ruleContext).useDefaultShellEnvironment();`
`1963`	`1963`	`builder.setExecutable(singleJar);`
Original file line number	Diff line number	Diff line change
`@@ -418,7 +418,7 @@ private void signApk(`
`418`	`418`
`419`	`419`	`private static void setSingleJarAsExecutable(`
`420`	`420`	`RuleContext ruleContext, SpawnAction.Builder builder) {`
`421`		`- Artifact singleJar = JavaToolchainProvider.from(ruleContext).getSingleJar();`
	`421`	`+ FilesToRunProvider singleJar = JavaToolchainProvider.from(ruleContext).getSingleJar();`
`422`	`422`	`builder.setExecutable(singleJar);`
`423`	`423`	`}`
`424`	`424`