Flip default for experimental_forward_instrumented_files_info_by_default

Googler · copybara-github · commit 5b216b27435a · 2021-06-18T11:14:24.000-07:00
This changes the default behavior for coverage from "forward nothing" to "forward InstrumentedFilesInfo from non-tool dependencies". This is still not quite ideal behavior, since typically attributes which provide source files (like srcs) are non-tool dependencies. But it's an improvement because it avoids the need for explicit coverage configuration for the entire chain of dependencies from test to source file.

In cases where an aspect returned InstrumentedFilesInfo previously, the InstrumentedFilesInfo provider for the base rule is ignored, so the behavior will be unchanged.

The bit about coverage_common.instrumented_files_info is merged into the related section under "Advanced Topics" in the rules documentation, since it's no longer required for the whole dependency chain to configure coverage explicitly in order for that to work at all.

RELNOTES: Forward coverage-instrumented files from non-tool dependencies by default.
PiperOrigin-RevId: 380224680
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java
@@ -380,7 +380,7 @@ public String getTypeDescription() {
 
   @Option(
       name = "experimental_forward_instrumented_files_info_by_default",
-      defaultValue = "false",
+      defaultValue = "true",
       documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS,
       effectTags = {OptionEffectTag.AFFECTS_OUTPUTS},
       help =
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java b/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java
@@ -1117,18 +1117,16 @@ public void instrumentedFilesInfoFromBaseRuleAndAspectUsesAspect() throws Except
         .containsExactly("a");
     assertThat(getInstrumentedFiles("//aspect:instrumented_file_info_from_base_target"))
         .containsExactly("b");
-    assertThat(getInstrumentedFilesInfo("//aspect:rule_target_no_coverage")).isNull();
+    assertThat(getInstrumentedFiles("//aspect:rule_target_no_coverage")).isEmpty();
     assertThat(getInstrumentedFiles("//aspect:instrumented_files_info_only_from_aspect"))
         .containsExactly("a");
     assertThat(getInstrumentedFiles("//aspect:no_instrumented_files_info")).isEmpty();
   }
 
   private List<String> getInstrumentedFiles(String label) throws InterruptedException {
     return ActionsTestUtil.baseArtifactNames(
-        getInstrumentedFilesInfo(label).getInstrumentedFiles());
-  }
-
-  private InstrumentedFilesInfo getInstrumentedFilesInfo(String label) throws InterruptedException {
-    return getConfiguredTarget(label).get(InstrumentedFilesInfo.STARLARK_CONSTRUCTOR);
+        getConfiguredTarget(label)
+            .get(InstrumentedFilesInfo.STARLARK_CONSTRUCTOR)
+            .getInstrumentedFiles());
   }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java
@@ -941,8 +941,10 @@ public void testInstrumentedFilesForwardedFromDepsByDefaultExperimentFlag() thro
     // Current behavior is that nothing gets forwarded if IntstrumentedFilesInfo is not configured.
     // That means that source files are not collected for the coverage manifest unless the entire
     // dependency chain between the test and the source file explicitly configures coverage.
-    // New behavior is protected by --experimental_forward_instrumented_files_info_by_default.
-    useConfiguration("--collect_code_coverage");
+    // New behavior is controlled by --experimental_forward_instrumented_files_info_by_default,
+    // which is now on by default.
+    useConfiguration(
+        "--collect_code_coverage", "--noexperimental_forward_instrumented_files_info_by_default");
     ConfiguredTarget target = getConfiguredTarget("//test/starlark:outer");
     InstrumentedFilesInfo provider = target.get(InstrumentedFilesInfo.STARLARK_CONSTRUCTOR);
     assertWithMessage("InstrumentedFilesInfo should be set.").that(provider).isNotNull();
@@ -952,8 +954,7 @@ public void testInstrumentedFilesForwardedFromDepsByDefaultExperimentFlag() thro
     // dependencies. Coverage still needs to be configured for rules that handle source files for
     // languages which support coverage instrumentation, but not every wrapper rule in the
     // dependency chain needs to configure that for instrumentation to be correct.
-    useConfiguration(
-        "--collect_code_coverage", "--experimental_forward_instrumented_files_info_by_default");
+    useConfiguration("--collect_code_coverage");
     target = getConfiguredTarget("//test/starlark:outer");
     provider = target.get(InstrumentedFilesInfo.STARLARK_CONSTRUCTOR);
     assertWithMessage("InstrumentedFilesInfo should be set.").that(provider).isNotNull();
