From b752e2b69f179327c5fa6fb23c0748d0326adb01 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 | 22 ++++++++++++++---- 3 files changed, 32 insertions(+), 16 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 b9cf03173f05a3..81513e0ad8ac19 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 @@ -349,6 +349,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); @@ -397,6 +399,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 c444175fa5115a..bd987bf7054398 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 @@ -107,6 +107,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; @@ -118,7 +119,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; @@ -176,6 +176,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, @@ -197,7 +198,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; @@ -206,6 +207,7 @@ private static ImmutableSet nonNullAsSet(Artifact... artifacts) { this.configuration = checkNotNull(configuration); this.testConfiguration = checkNotNull(configuration.getFragment(TestConfiguration.class)); this.testLog = testLog; + this.testXml = testXml; this.cacheStatus = cacheStatus; this.coverageData = coverageArtifact; this.coverageDirectory = coverageDirectory; @@ -223,7 +225,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"); @@ -283,7 +284,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())); @@ -521,7 +522,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 @@ -617,7 +617,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. @@ -676,6 +676,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 ActionEnvironment getExtraTestEnv() { return extraTestEnv; @@ -741,11 +745,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() { @@ -1045,7 +1044,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 96b4e88fa0d1c0..914ca7b35d8787 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; @@ -404,7 +405,7 @@ private static Spawn createXmlGeneratingSpawn( 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())); ImmutableMap.Builder envBuilder = ImmutableMap.builder(); @@ -430,7 +431,7 @@ private static Spawn createXmlGeneratingSpawn( /*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); } @@ -772,14 +773,27 @@ public TestAttemptContinuation execute() } + Path xmlOutputPath = resolvedPaths.getXmlOutputPath(); + boolean testXmlExists = xmlOutputPath.exists(); + if (!testXmlExists) { + MetadataHandler metadataHandler = actionExecutionContext.getMetadataHandler(); + if (metadataHandler != null) { + try { + // Check whether test.xml exists. If not, an IOException will be thrown. + metadataHandler.getMetadata(testAction.getTestXml()); + testXmlExists = true; + } catch (IOException ignored) { + } + } + } + // If the test did not create a test.xml, and --experimental_split_xml_generation is enabled, // 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()) { + && !testXmlExists) { Spawn xmlGeneratingSpawn = createXmlGeneratingSpawn(testAction, spawn.getEnvironment(), spawnResults.get(0)); SpawnStrategyResolver spawnStrategyResolver =