Skip to content

Commit

Permalink
Change test.xml from BasicActionInput to Artifact so it's metadata ca…
Browse files Browse the repository at this point in the history 
…n be injected

This is achieved by adding test.xml to TestRunnerAction's outputs which means test.xml becomes a mandatory output of TestRunnerAction just like test.log.

test.xml should always be generated by either test actions or the separated spawn action enabled by --experimental_split_xml_generation.

Fixes bazelbuild#12554.
  • Loading branch information
@coeuvre
coeuvre committed Dec 1, 2020
1 parent 150be74 commit 5f198fc
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 33 deletions.
3 changes: 3 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,8 @@ private TestParams createTestAction(int shards) throws InterruptedException {

Artifact.DerivedArtifact testLog =
ruleContext.getPackageRelativeArtifact(dir.getRelative("test.log"), root);
Artifact.DerivedArtifact testXml =
ruleContext.getPackageRelativeArtifact(dir.getRelative("test.xml"), root);
Artifact.DerivedArtifact cacheStatus =
ruleContext.getPackageRelativeArtifact(dir.getRelative("test.cache_status"), root);

Expand Down Expand Up @@ -392,6 +394,7 @@ private TestParams createTestAction(int shards) throws InterruptedException {
testXmlGeneratorExecutable,
collectCoverageScript,
testLog,
testXml,
cacheStatus,
coverageArtifact,
coverageDirectory,
Expand Down
23 changes: 11 additions & 12 deletions src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ public class TestRunnerAction extends AbstractAction
private final BuildConfiguration configuration;
private final TestConfiguration testConfiguration;
private final Artifact testLog;
private final Artifact testXml;
private final Artifact cacheStatus;
private final PathFragment testWarningsPath;
private final PathFragment unusedRunfilesLogPath;
Expand All @@ -114,7 +115,6 @@ public class TestRunnerAction extends AbstractAction
private final PathFragment undeclaredOutputsAnnotationsDir;
private final PathFragment undeclaredOutputsManifestPath;
private final PathFragment undeclaredOutputsAnnotationsPath;
private final PathFragment xmlOutputPath;
@Nullable private final PathFragment testShard;
private final PathFragment testExitSafe;
private final PathFragment testStderr;
Expand Down Expand Up @@ -172,6 +172,7 @@ private static ImmutableSet<Artifact> nonNullAsSet(Artifact... artifacts) {
Artifact testXmlGeneratorScript, // Must be in inputs
@Nullable Artifact collectCoverageScript, // Must be in inputs, if not null
Artifact testLog,
Artifact testXml,
Artifact cacheStatus,
Artifact coverageArtifact,
@Nullable Artifact coverageDirectory,
Expand All @@ -193,7 +194,7 @@ private static ImmutableSet<Artifact> nonNullAsSet(Artifact... artifacts) {
NestedSetBuilder.wrap(Order.STABLE_ORDER, tools),
inputs,
runfilesSupplier,
nonNullAsSet(testLog, cacheStatus, coverageArtifact, coverageDirectory),
nonNullAsSet(testLog, testXml, cacheStatus, coverageArtifact, coverageDirectory),
configuration.getActionEnvironment());
Preconditions.checkState((collectCoverageScript == null) == (coverageArtifact == null));
this.testSetupScript = testSetupScript;
Expand All @@ -203,6 +204,7 @@ private static ImmutableSet<Artifact> nonNullAsSet(Artifact... artifacts) {
this.testConfiguration =
Preconditions.checkNotNull(configuration.getFragment(TestConfiguration.class));
this.testLog = testLog;
this.testXml = testXml;
this.cacheStatus = cacheStatus;
this.coverageData = coverageArtifact;
this.coverageDirectory = coverageDirectory;
Expand All @@ -220,7 +222,6 @@ private static ImmutableSet<Artifact> nonNullAsSet(Artifact... artifacts) {
this.testExitSafe = baseDir.getChild("test.exited_prematurely");
// testShard Path should be set only if sharding is enabled.
this.testShard = totalShards > 1 ? baseDir.getChild("test.shard") : null;
this.xmlOutputPath = baseDir.getChild("test.xml");
this.testWarningsPath = baseDir.getChild("test.warnings");
this.unusedRunfilesLogPath = baseDir.getChild("test.unused_runfiles_log");
this.testStderr = baseDir.getChild("test.err");
Expand Down Expand Up @@ -279,7 +280,7 @@ public boolean showsOutputUnconditionally() {

public List<ActionInput> getSpawnOutputs() {
final List<ActionInput> outputs = new ArrayList<>();
outputs.add(ActionInputHelper.fromPath(getXmlOutputPath()));
outputs.add(this.testXml);
outputs.add(ActionInputHelper.fromPath(getExitSafeFile()));
if (isSharded()) {
outputs.add(ActionInputHelper.fromPath(getTestShard()));
Expand Down Expand Up @@ -519,7 +520,6 @@ protected void deleteOutputs(
// shard count.

// We also need to remove *.(xml|data|shard|warnings|zip) files if they are present.
execRoot.getRelative(xmlOutputPath).delete();
execRoot.getRelative(testWarningsPath).delete();
execRoot.getRelative(unusedRunfilesLogPath).delete();
// Note that splitLogsPath points to a file inside the splitLogsDir so
Expand Down Expand Up @@ -615,7 +615,7 @@ public void setupEnvVariables(Map<String, String> env, Duration timeout) {
env.put("TEST_TOTAL_SHARDS", Integer.toString(getExecutionSettings().getTotalShards()));
env.put("TEST_SHARD_STATUS_FILE", getTestShard().getPathString());
}
env.put("XML_OUTPUT_FILE", getXmlOutputPath().getPathString());
env.put("XML_OUTPUT_FILE", testXml.getExecPathString());

if (!isEnableRunfiles()) {
// If runfiles are disabled, tell remote-runtest.sh/local-runtest.sh about that.
Expand Down Expand Up @@ -674,6 +674,10 @@ public Artifact getTestLog() {
return testLog;
}

public Artifact getTestXml() {
return testXml;
}

/** Returns all environment variables which must be set in order to run this test. */
public Map<String, String> getExtraTestEnv() {
return extraTestEnv;
Expand Down Expand Up @@ -739,11 +743,6 @@ public PathFragment getInfrastructureFailureFile() {
return testInfrastructureFailure;
}

/** Returns path to the optionally created XML output file created by the test. */
public PathFragment getXmlOutputPath() {
return xmlOutputPath;
}

/** Returns coverage data artifact or null if code coverage was not requested. */
@Nullable
public Artifact getCoverageData() {
Expand Down Expand Up @@ -1043,7 +1042,7 @@ public Path getInfrastructureFailureFile() {

/** Returns path to the optionally created XML output file created by the test. */
public Path getXmlOutputPath() {
return getPath(xmlOutputPath);
return getPath(testXml.getExecPath());
}

public Path getCoverageDirectory() {
Expand Down
50 changes: 29 additions & 21 deletions src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import com.google.devtools.build.lib.actions.SpawnContinuation;
import com.google.devtools.build.lib.actions.SpawnResult;
import com.google.devtools.build.lib.actions.TestExecException;
import com.google.devtools.build.lib.actions.cache.MetadataHandler;
import com.google.devtools.build.lib.analysis.actions.SpawnAction;
import com.google.devtools.build.lib.analysis.test.TestAttempt;
import com.google.devtools.build.lib.analysis.test.TestResult;
Expand Down Expand Up @@ -396,7 +397,7 @@ private static Spawn createXmlGeneratingSpawn(TestRunnerAction action, SpawnResu
ImmutableList.of(
action.getTestXmlGeneratorScript().getExecPath().getCallablePathString(),
action.getTestLog().getExecPathString(),
action.getXmlOutputPath().getPathString(),
action.getTestXml().getExecPathString(),
Long.toString(result.getWallTime().orElse(Duration.ZERO).getSeconds()),
Integer.toString(result.exitCode()));

Expand All @@ -419,7 +420,7 @@ private static Spawn createXmlGeneratingSpawn(TestRunnerAction action, SpawnResu
/*inputs=*/ NestedSetBuilder.create(
Order.STABLE_ORDER, action.getTestXmlGeneratorScript(), action.getTestLog()),
/*tools=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER),
/*outputs=*/ ImmutableSet.of(ActionInputHelper.fromPath(action.getXmlOutputPath())),
/*outputs=*/ ImmutableSet.of(action.getTestXml()),
SpawnAction.DEFAULT_RESOURCE_SET);
}

Expand Down Expand Up @@ -762,28 +763,35 @@ public TestAttemptContinuation execute()
// then we run a separate action to create a test.xml from test.log. We do this as a spawn
// rather than doing it locally in-process, as the test.log file may only exist remotely (when
// remote execution is enabled), and we do not want to have to download it.
Path xmlOutputPath = resolvedPaths.getXmlOutputPath();
if (executionOptions.splitXmlGeneration
&& fileOutErr.getOutputPath().exists()
&& !xmlOutputPath.exists()) {
Spawn xmlGeneratingSpawn = createXmlGeneratingSpawn(testAction, spawnResults.get(0));
SpawnStrategyResolver spawnStrategyResolver =
actionExecutionContext.getContext(SpawnStrategyResolver.class);
// We treat all failures to generate the test.xml here as catastrophic, and won't rerun
// the test if this fails. We redirect the output to a temporary file.
FileOutErr xmlSpawnOutErr = actionExecutionContext.getFileOutErr().childOutErr();
try {
SpawnContinuation xmlContinuation =
spawnStrategyResolver.beginExecution(
xmlGeneratingSpawn, actionExecutionContext.withFileOutErr(xmlSpawnOutErr));
return new BazelXmlCreationContinuation(
resolvedPaths, xmlSpawnOutErr, testResultDataBuilder, spawnResults, xmlContinuation);
} catch (InterruptedException e) {
closeSuppressed(e, xmlSpawnOutErr);
throw e;
MetadataHandler metadataHandler = actionExecutionContext.getMetadataHandler();
try {
// Check whether test.xml exists. If not, an IOException will be thrown.
metadataHandler.getMetadata(testAction.getTestXml());
} catch (IOException ignored) {
if (executionOptions.splitXmlGeneration) {
Spawn xmlGeneratingSpawn = createXmlGeneratingSpawn(testAction, spawnResults.get(0));
SpawnStrategyResolver spawnStrategyResolver =
actionExecutionContext.getContext(SpawnStrategyResolver.class);
// We treat all failures to generate the test.xml here as catastrophic, and won't rerun
// the test if this fails. We redirect the output to a temporary file.
FileOutErr xmlSpawnOutErr = actionExecutionContext.getFileOutErr().childOutErr();
try {
SpawnContinuation xmlContinuation =
spawnStrategyResolver.beginExecution(
xmlGeneratingSpawn, actionExecutionContext.withFileOutErr(xmlSpawnOutErr));
return new BazelXmlCreationContinuation(
resolvedPaths, xmlSpawnOutErr, testResultDataBuilder, spawnResults, xmlContinuation);
} catch (InterruptedException e) {
closeSuppressed(e, xmlSpawnOutErr);
throw e;
}
} else {
// If executionOptions.splitXmlGeneration is not enabled, test.xml should be generated as part of test action
throw new AssertionError("test.xml should be generated by test action");
}
}

Path xmlOutputPath = resolvedPaths.getXmlOutputPath();
TestCase details = parseTestResult(xmlOutputPath);
if (details != null) {
testResultDataBuilder.setTestCase(details);
Expand Down

0 comments on commit 5f198fc

Please sign in to comment.