Skip to content

Commit 74285e2

Browse files
fmeumcopybara-github
fmeum
authored and
copybara-github
committed
Add basic support for coverage on Windows
The core idea of this change is to make `collect_coverage` an `sh_binary` instead of a single `.sh` file as the former can be executed on Windows via the native launcher it creates. However, this requires a bunch of work in the test framework: * Ensure that the `collect_coverage` script has its runfiles included in `TestActionBuilder`. Along the way and to ensure consistency, clean up unnecessary cases for the runfiles of the LCOV merger tool. * Add support for launching the coverage wrapper to `tw`, which didn't have any logic for this case. * Hide the runfiles env variables meant for the test from the coverage wrapper. The wrapper has its own runfiles tree and would otherwise be mislead to reuse the test's runfiles tree, which doesn't work. Instead, wrap the environment before invoking the wrapper and then unwrap them in the wrapper. * Modify the coverage output generator to emit `\n` line endings on all platforms for consistency (and to get existing tests to pass). With these changes, `bazel_coverage_starlark_test` now passes on Windows after adding batch test variants of shell tests. Support for Java and C++ will be added in future PRs. Work towards #6374 Work towards #18839 Closes #26603. PiperOrigin-RevId: 799018475 Change-Id: Icdb1e6e4698b2a749fbb17f7e1a9aaa34e895d70
1 parent 2056b86 commit 74285e2

File tree

12 files changed

+254
-130
lines changed

12 files changed

+254
-130
lines changed

‎src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -307,7 +307,6 @@ public static RuleClass getTestBaseRule(RuleDefinitionEnvironment env) {
307307
.cfg(
308308
ExecutionTransitionFactory.createFactory(
309309
DEFAULT_TEST_RUNNER_EXEC_GROUP_NAME))
310-
.singleArtifact()
311310
.value(labelCache.get(toolsRepository + "//tools/test:collect_coverage")))
312311
// Input files for test actions collecting code coverage
313312
.add(

‎src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java‎

Lines changed: 15 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@
2727
import com.google.devtools.build.lib.actions.ArtifactRoot;
2828
import com.google.devtools.build.lib.analysis.Allowlist;
2929
import com.google.devtools.build.lib.analysis.AnalysisEnvironment;
30-
import com.google.devtools.build.lib.analysis.FileProvider;
3130
import com.google.devtools.build.lib.analysis.FilesToRunProvider;
3231
import com.google.devtools.build.lib.analysis.PackageSpecificationProvider;
3332
import com.google.devtools.build.lib.analysis.PrerequisiteArtifacts;
@@ -221,19 +220,21 @@ private TestParams createTestAction()
221220
: ruleContext.getPrerequisiteArtifact("$xml_generator_script");
222221
inputsBuilder.add(testXmlGeneratorExecutable);
223222

224-
Artifact collectCoverageScript = null;
223+
FilesToRunProvider collectCoverageScript = null;
225224
TreeMap<String, String> extraTestEnv = new TreeMap<>();
226225

227226
int runsPerTest = getRunsPerTest(ruleContext);
228227
int shardCount = getShardCount(ruleContext);
229228

230-
NestedSetBuilder<Artifact> lcovMergerFilesToRun = NestedSetBuilder.compileOrder();
231-
Artifact lcovMergerRunfilesTree = null;
229+
NestedSet<Artifact> lcovMergerFilesToRun = NestedSetBuilder.emptySet(Order.STABLE_ORDER);
232230

233231
TestTargetExecutionSettings executionSettings;
234232
if (collectCodeCoverage) {
235-
collectCoverageScript = ruleContext.getPrerequisiteArtifact("$collect_coverage_script");
236-
inputsBuilder.add(collectCoverageScript);
233+
collectCoverageScript =
234+
ruleContext
235+
.getPrerequisite("$collect_coverage_script")
236+
.getProvider(FilesToRunProvider.class);
237+
inputsBuilder.addTransitive(collectCoverageScript.getFilesToRun());
237238
inputsBuilder.addTransitive(instrumentedFiles.getCoverageSupportFiles());
238239
// Add instrumented file manifest artifact to the list of inputs. This file will contain
239240
// exec paths of all source files that should be included into the code coverage output.
@@ -283,28 +284,15 @@ private TestParams createTestAction()
283284
if (lcovMergerAttr != null) {
284285
TransitiveInfoCollection lcovMerger = ruleContext.getPrerequisite(lcovMergerAttr);
285286
FilesToRunProvider lcovFilesToRun = lcovMerger.getProvider(FilesToRunProvider.class);
286-
if (lcovFilesToRun != null) {
287-
extraTestEnv.put(LCOV_MERGER, lcovFilesToRun.getExecutable().getExecPathString());
288-
inputsBuilder.addTransitive(lcovFilesToRun.getFilesToRun());
289-
lcovMergerFilesToRun.addTransitive(lcovFilesToRun.getFilesToRun());
290-
if (lcovFilesToRun.getRunfilesSupport() != null) {
291-
lcovMergerRunfilesTree = lcovFilesToRun.getRunfilesSupport().getRunfilesTreeArtifact();
292-
}
293-
} else {
294-
NestedSet<Artifact> filesToBuild =
295-
lcovMerger.getProvider(FileProvider.class).getFilesToBuild();
296-
297-
if (filesToBuild.isSingleton()) {
298-
Artifact lcovMergerArtifact = filesToBuild.getSingleton();
299-
extraTestEnv.put(LCOV_MERGER, lcovMergerArtifact.getExecPathString());
300-
inputsBuilder.add(lcovMergerArtifact);
301-
lcovMergerFilesToRun.add(lcovMergerArtifact);
302-
} else {
303-
ruleContext.attributeError(
304-
lcovMergerAttr,
305-
"the LCOV merger should be either an executable or a single artifact");
306-
}
287+
// Both executable targets and single artifacts have a FilesToRunProvider.
288+
if (lcovFilesToRun == null) {
289+
ruleContext.attributeError(
290+
lcovMergerAttr,
291+
"the LCOV merger should be either an executable or a single artifact");
307292
}
293+
extraTestEnv.put(LCOV_MERGER, lcovFilesToRun.getExecutable().getExecPathString());
294+
inputsBuilder.addTransitive(lcovFilesToRun.getFilesToRun());
295+
lcovMergerFilesToRun = lcovFilesToRun.getFilesToRun();
308296
}
309297

310298
Artifact instrumentedFileManifest =
@@ -417,7 +405,6 @@ private TestParams createTestAction()
417405
cancelConcurrentTests,
418406
splitCoveragePostProcessing,
419407
lcovMergerFilesToRun,
420-
lcovMergerRunfilesTree,
421408
// Network allowlist only makes sense in workspaces which explicitly add it, use an
422409
// empty one as a fallback.
423410
MoreObjects.firstNonNull(

‎src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java‎

Lines changed: 8 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@
4747
import com.google.devtools.build.lib.actions.SpawnExecutedEvent;
4848
import com.google.devtools.build.lib.actions.SpawnResult;
4949
import com.google.devtools.build.lib.actions.TestExecException;
50+
import com.google.devtools.build.lib.analysis.FilesToRunProvider;
5051
import com.google.devtools.build.lib.analysis.PackageSpecificationProvider;
5152
import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
5253
import com.google.devtools.build.lib.analysis.config.RunUnder;
@@ -59,7 +60,6 @@
5960
import com.google.devtools.build.lib.buildeventstream.TestFileNameConstants;
6061
import com.google.devtools.build.lib.cmdline.Label;
6162
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
62-
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
6363
import com.google.devtools.build.lib.events.Event;
6464
import com.google.devtools.build.lib.server.FailureDetails.Execution.Code;
6565
import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
@@ -115,7 +115,7 @@ public class TestRunnerAction extends AbstractAction
115115
private final Artifact runfilesTree;
116116
private final Artifact testSetupScript;
117117
private final Artifact testXmlGeneratorScript;
118-
private final Artifact collectCoverageScript;
118+
private final FilesToRunProvider collectCoverageScript;
119119
private final BuildConfigurationValue configuration;
120120
private final TestConfiguration testConfiguration;
121121
private final Artifact testLog;
@@ -167,8 +167,7 @@ public class TestRunnerAction extends AbstractAction
167167
private final CancelConcurrentTests cancelConcurrentTests;
168168

169169
private final boolean splitCoveragePostProcessing;
170-
private final NestedSetBuilder<Artifact> lcovMergerFilesToRun;
171-
@Nullable private final Artifact lcovMergerRunfilesTree;
170+
private final NestedSet<Artifact> lcovMergerFilesToRun;
172171

173172
// TODO(b/192694287): Remove once we migrate all tests from the allowlist.
174173
private final PackageSpecificationProvider networkAllowlist;
@@ -197,7 +196,8 @@ private static ImmutableSet<Artifact> nonNullAsSet(Artifact... artifacts) {
197196
Artifact runfilesTree,
198197
Artifact testSetupScript, // Must be in inputs
199198
Artifact testXmlGeneratorScript, // Must be in inputs
200-
@Nullable Artifact collectCoverageScript, // Must be in inputs, if not null
199+
@Nullable
200+
FilesToRunProvider collectCoverageScript, // filesToRun must be in input, if not null
201201
Artifact testLog,
202202
Artifact cacheStatus,
203203
Artifact coverageArtifact,
@@ -213,8 +213,7 @@ private static ImmutableSet<Artifact> nonNullAsSet(Artifact... artifacts) {
213213
@Nullable PathFragment shExecutable,
214214
CancelConcurrentTests cancelConcurrentTests,
215215
boolean splitCoveragePostProcessing,
216-
NestedSetBuilder<Artifact> lcovMergerFilesToRun,
217-
@Nullable Artifact lcovMergerRunfilesTree,
216+
NestedSet<Artifact> lcovMergerFilesToRun,
218217
PackageSpecificationProvider networkAllowlist) {
219218
super(
220219
owner,
@@ -272,7 +271,6 @@ private static ImmutableSet<Artifact> nonNullAsSet(Artifact... artifacts) {
272271
this.cancelConcurrentTests = cancelConcurrentTests;
273272
this.splitCoveragePostProcessing = splitCoveragePostProcessing;
274273
this.lcovMergerFilesToRun = lcovMergerFilesToRun;
275-
this.lcovMergerRunfilesTree = lcovMergerRunfilesTree;
276274
this.networkAllowlist = networkAllowlist;
277275

278276
// Mark all possible test outputs for deletion before test execution.
@@ -331,11 +329,6 @@ public final ActionEnvironment getEnvironment() {
331329
return configuration.getActionEnvironment();
332330
}
333331

334-
@Nullable
335-
public Artifact getLcovMergerRunfilesTree() {
336-
return lcovMergerRunfilesTree;
337-
}
338-
339332
public BuildConfigurationValue getConfiguration() {
340333
return configuration;
341334
}
@@ -348,7 +341,7 @@ public boolean getSplitCoveragePostProcessing() {
348341
return splitCoveragePostProcessing;
349342
}
350343

351-
public NestedSetBuilder<Artifact> getLcovMergerFilesToRun() {
344+
public NestedSet<Artifact> getLcovMergerFilesToRun() {
352345
return lcovMergerFilesToRun;
353346
}
354347

@@ -1080,7 +1073,7 @@ public Artifact getTestXmlGeneratorScript() {
10801073
}
10811074

10821075
@Nullable
1083-
public Artifact getCollectCoverageScript() {
1076+
public FilesToRunProvider getCollectCoverageScript() {
10841077
return collectCoverageScript;
10851078
}
10861079

‎src/main/java/com/google/devtools/build/lib/analysis/test/TestStrategy.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -218,6 +218,7 @@ public static ImmutableList<String> expandedArgsFromAction(TestRunnerAction test
218218
args.add(
219219
testAction
220220
.getCollectCoverageScript()
221+
.getExecutable()
221222
.getExecPath()
222223
.getCallablePathStringForOs(executionOs));
223224
}

‎src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -494,7 +494,7 @@ private static Spawn createCoveragePostProcessingSpawn(
494494
List<ActionInput> expandedCoverageDir,
495495
Path tmpDirRoot) {
496496
ImmutableList<String> args =
497-
ImmutableList.of(action.getCollectCoverageScript().getExecPathString());
497+
ImmutableList.of(action.getCollectCoverageScript().getExecutable().getExecPathString());
498498

499499
Map<String, String> testEnvironment =
500500
createEnvironment(actionExecutionContext, action, tmpDirRoot);
@@ -504,6 +504,11 @@ private static Spawn createCoveragePostProcessingSpawn(
504504
"TEST_TOTAL_SHARDS", Integer.toString(action.getExecutionSettings().getTotalShards()));
505505
testEnvironment.put(TEST_NAME_ENV, action.getTestName());
506506
testEnvironment.put("IS_COVERAGE_SPAWN", "1");
507+
// Let the coverage script locate its own runfiles tree, which is separate from the test
508+
// runfiles.
509+
testEnvironment.remove("RUNFILES_DIR");
510+
testEnvironment.remove("JAVA_RUNFILES");
511+
testEnvironment.remove("PYTHON_RUNFILES");
507512

508513
return new SimpleSpawn(
509514
action,
@@ -513,13 +518,10 @@ private static Spawn createCoveragePostProcessingSpawn(
513518
/* inputs= */ NestedSetBuilder.<ActionInput>compileOrder()
514519
.addTransitive(action.getInputs())
515520
.addAll(expandedCoverageDir)
516-
.add(action.getCollectCoverageScript())
517521
.add(action.getCoverageManifest())
518-
.addTransitive(action.getLcovMergerFilesToRun().build())
519522
.build(),
520523
/* tools= */ NestedSetBuilder.emptySet(Order.STABLE_ORDER),
521-
/* outputs= */ ImmutableSet.of(
522-
ActionInputHelper.fromPath(action.getCoverageData().getExecPath())),
524+
/* outputs= */ ImmutableSet.of(action.getCoverageData()),
523525
/* mandatoryOutputs= */ null,
524526
SpawnAction.DEFAULT_RESOURCE_SET);
525527
}

‎src/test/shell/bazel/BUILD‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -603,6 +603,7 @@ sh_test(
603603
args = ["released"],
604604
data = [":test-deps"],
605605
tags = [
606+
# TODO: Drop after the next coverage tools release.
606607
"no_windows",
607608
],
608609
)
@@ -619,9 +620,6 @@ sh_test(
619620
"//tools/test/CoverageOutputGenerator/java/com/google/devtools/coverageoutputgenerator:coverage_empty_workspace",
620621
"//tools/test/CoverageOutputGenerator/java/com/google/devtools/coverageoutputgenerator:coverage_output_generator_repo",
621622
],
622-
tags = [
623-
"no_windows",
624-
],
625623
)
626624

627625
sh_test(

0 commit comments

Comments
 (0)