From 5f198fc92c3c412924f9605d9c7a63583166568f Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Tue, 1 Dec 2020 14:38:38 +0800 Subject: [PATCH] Change test.xml from BasicActionInput to Artifact so it's metadata can 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 #12554. --- .../lib/analysis/test/TestActionBuilder.java | 3 ++ .../lib/analysis/test/TestRunnerAction.java | 23 ++++----- .../lib/exec/StandaloneTestStrategy.java | 50 +++++++++++-------- 3 files changed, 43 insertions(+), 33 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java index c1a92c7b72c406..010af0d60115fd 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java @@ -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); @@ -392,6 +394,7 @@ private TestParams createTestAction(int shards) throws InterruptedException { testXmlGeneratorExecutable, collectCoverageScript, testLog, + testXml, cacheStatus, coverageArtifact, coverageDirectory, diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java index 312c43d5e58bb9..5f2f917b3de08c 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java @@ -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; @@ -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; @@ -172,6 +172,7 @@ private static ImmutableSet 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, @@ -193,7 +194,7 @@ private static ImmutableSet 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; @@ -203,6 +204,7 @@ private static ImmutableSet 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; @@ -220,7 +222,6 @@ private static ImmutableSet 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"); @@ -279,7 +280,7 @@ public boolean showsOutputUnconditionally() { public List getSpawnOutputs() { final List outputs = new ArrayList<>(); - outputs.add(ActionInputHelper.fromPath(getXmlOutputPath())); + outputs.add(this.testXml); outputs.add(ActionInputHelper.fromPath(getExitSafeFile())); if (isSharded()) { outputs.add(ActionInputHelper.fromPath(getTestShard())); @@ -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 @@ -615,7 +615,7 @@ public void setupEnvVariables(Map 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. @@ -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 getExtraTestEnv() { return extraTestEnv; @@ -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() { @@ -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() { diff --git a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java index fd1214daf9cedb..60658475c1a810 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java @@ -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; @@ -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())); @@ -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); } @@ -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);