Skip to content

Commit

Permalink
Split coverage post-processing into its own spawn
Browse files Browse the repository at this point in the history
This change fixes b/79147227 by introducing a flag that makes coverage post-processing happen in its own spawn. The runfiles in the coverage spawn are those of the LCOV merger instead of the test runfiles.

The flag introduced is --experimental_split_coverage_postprocessing which depends on the flag --experimental_fetch_all_coverage_outputs being enabled too.

PiperOrigin-RevId: 339699090
  • Loading branch information
oquenchil authored and copybara-github committed Oct 29, 2020
1 parent ad18145 commit a445cda
Show file tree
Hide file tree
Showing 9 changed files with 536 additions and 115 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,9 @@ private TestParams createTestAction(int shards) {

int runsPerTest = testConfiguration.getRunsPerTestForLabel(ruleContext.getLabel());

NestedSetBuilder<Artifact> lcovMergerFilesToRun = NestedSetBuilder.compileOrder();
RunfilesSupplier lcovMergerRunfilesSupplier = null;

TestTargetExecutionSettings executionSettings;
if (collectCodeCoverage) {
collectCoverageScript = ruleContext.getHostPrerequisiteArtifact("$collect_coverage_script");
Expand Down Expand Up @@ -251,6 +254,12 @@ private TestParams createTestAction(int shards) {
if (lcovFilesToRun != null) {
extraTestEnv.put(LCOV_MERGER, lcovFilesToRun.getExecutable().getExecPathString());
inputsBuilder.addTransitive(lcovFilesToRun.getFilesToRun());

lcovMergerFilesToRun.addTransitive(lcovFilesToRun.getFilesToRun());
if (lcovFilesToRun.getRunfilesSupport() != null) {
lcovMergerFilesToRun.add(lcovFilesToRun.getRunfilesSupport().getRunfilesMiddleman());
}
lcovMergerRunfilesSupplier = lcovFilesToRun.getRunfilesSupplier();
} else {
NestedSet<Artifact> filesToBuild =
lcovMerger.getProvider(FileProvider.class).getFilesToBuild();
Expand All @@ -259,6 +268,7 @@ private TestParams createTestAction(int shards) {
Artifact lcovMergerArtifact = filesToBuild.getSingleton();
extraTestEnv.put(LCOV_MERGER, lcovMergerArtifact.getExecPathString());
inputsBuilder.add(lcovMergerArtifact);
lcovMergerFilesToRun.add(lcovMergerArtifact);
} else {
ruleContext.attributeError(
lcovMergerAttr,
Expand Down Expand Up @@ -372,6 +382,7 @@ private TestParams createTestAction(int shards) {
tools.add(executionSettings.getExecutable());
tools.addAll(additionalTools.build());
}
boolean splitCoveragePostProcessing = testConfiguration.splitCoveragePostProcessing();
TestRunnerAction testRunnerAction =
new TestRunnerAction(
ruleContext.getActionOwner(),
Expand All @@ -396,7 +407,10 @@ private TestParams createTestAction(int shards) {
? ShToolchain.getPathOrError(ruleContext)
: null,
cancelConcurrentTests,
tools.build());
tools.build(),
splitCoveragePostProcessing,
lcovMergerFilesToRun,
lcovMergerRunfilesSupplier);

testOutputs.addAll(testRunnerAction.getSpawnOutputs());
testOutputs.addAll(testRunnerAction.getOutputs());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,14 @@ public static class TestOptions extends FragmentOptions {
+ "an exclusive test run locally")
public boolean incompatibleExclusiveTestSandboxed;

@Option(
name = "experimental_split_coverage_postprocessing",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
effectTags = {OptionEffectTag.EXECUTION},
help = "If true, then Bazel will run coverage postprocessing for test in a new spawn.")
public boolean splitCoveragePostProcessing;

@Override
public FragmentOptions getHost() {
TestOptions hostOptions = (TestOptions) getDefault();
Expand Down Expand Up @@ -387,6 +395,10 @@ public boolean fetchAllCoverageOutputs() {

public boolean incompatibleExclusiveTestSandboxed() { return options.incompatibleExclusiveTestSandboxed; }

public boolean splitCoveragePostProcessing() {
return options.splitCoveragePostProcessing;
}

/**
* Option converter that han handle two styles of value for "--runs_per_test":
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,10 @@ public class TestRunnerAction extends AbstractAction

private final boolean cancelConcurrentTestsOnSuccess;

private final boolean splitCoveragePostProcessing;
private final NestedSetBuilder<Artifact> lcovMergerFilesToRun;
private final RunfilesSupplier lcovMergerRunfilesSupplier;

private static ImmutableSet<Artifact> nonNullAsSet(Artifact... artifacts) {
ImmutableSet.Builder<Artifact> builder = ImmutableSet.builder();
for (Artifact artifact : artifacts) {
Expand Down Expand Up @@ -180,7 +184,10 @@ private static ImmutableSet<Artifact> nonNullAsSet(Artifact... artifacts) {
String workspaceName,
@Nullable PathFragment shExecutable,
boolean cancelConcurrentTestsOnSuccess,
Iterable<Artifact> tools) {
Iterable<Artifact> tools,
boolean splitCoveragePostProcessing,
NestedSetBuilder<Artifact> lcovMergerFilesToRun,
RunfilesSupplier lcovMergerRunfilesSupplier) {
super(
owner,
NestedSetBuilder.wrap(Order.STABLE_ORDER, tools),
Expand Down Expand Up @@ -236,6 +243,13 @@ private static ImmutableSet<Artifact> nonNullAsSet(Artifact... artifacts) {
configuration.getActionEnvironment().getInheritedEnv(),
configuration.getTestActionEnvironment().getInheritedEnv()));
this.cancelConcurrentTestsOnSuccess = cancelConcurrentTestsOnSuccess;
this.splitCoveragePostProcessing = splitCoveragePostProcessing;
this.lcovMergerFilesToRun = lcovMergerFilesToRun;
this.lcovMergerRunfilesSupplier = lcovMergerRunfilesSupplier;
}

public RunfilesSupplier getLcovMergerRunfilesSupplier() {
return lcovMergerRunfilesSupplier;
}

public BuildConfiguration getConfiguration() {
Expand All @@ -246,6 +260,18 @@ public final PathFragment getBaseDir() {
return baseDir;
}

public boolean getSplitCoveragePostProcessing() {
return splitCoveragePostProcessing;
}

public NestedSetBuilder<Artifact> getLcovMergerFilesToRun() {
return lcovMergerFilesToRun;
}

public Artifact getCoverageDirectoryTreeArtifact() {
return coverageDirectory;
}

@Override
public boolean showsOutputUnconditionally() {
return true;
Expand All @@ -266,7 +292,9 @@ public List<ActionInput> getSpawnOutputs() {
outputs.add(ActionInputHelper.fromPath(getUndeclaredOutputsManifestPath()));
outputs.add(ActionInputHelper.fromPath(getUndeclaredOutputsAnnotationsPath()));
if (isCoverageMode()) {
outputs.add(coverageData);
if (!splitCoveragePostProcessing) {
outputs.add(coverageData);
}
if (coverageDirectory != null) {
outputs.add(coverageDirectory);
}
Expand Down Expand Up @@ -607,6 +635,8 @@ public void setupEnvVariables(Map<String, String> env, Duration timeout) {
env.put("COVERAGE_MANIFEST", getCoverageManifest().getExecPathString());
env.put("COVERAGE_DIR", getCoverageDirectory().getPathString());
env.put("COVERAGE_OUTPUT_FILE", getCoverageData().getExecPathString());
env.put("SPLIT_COVERAGE_POST_PROCESSING", splitCoveragePostProcessing ? "1" : "0");
env.put("IS_COVERAGE_SPAWN", "0");
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ private static int getTestAttemptsPerLabel(
* the "categorical timeouts" which are based on the --test_timeout flag. A rule picks its timeout
* but ends up with the same effective value as all other rules in that bucket.
*/
protected final Duration getTimeout(TestRunnerAction testAction) {
protected static final Duration getTimeout(TestRunnerAction testAction) {
BuildConfiguration configuration = testAction.getConfiguration();
return configuration
.getFragment(TestConfiguration.class)
Expand Down
Loading

0 comments on commit a445cda

Please sign in to comment.