Skip to content

Commit

Permalink
Flip default for experimental_forward_instrumented_files_info_by_default
Browse files Browse the repository at this point in the history
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
  • Loading branch information
Googler authored and copybara-github committed Jun 18, 2021
1 parent d09077e commit 5b216b2
Show file tree
Hide file tree
Showing 3 changed files with 10 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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();
Expand Down

0 comments on commit 5b216b2

Please sign in to comment.