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 0cce67b917cb5d..0430d2e7180107 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 @@ -201,6 +201,9 @@ private TestParams createTestAction(int shards) { int runsPerTest = testConfiguration.getRunsPerTestForLabel(ruleContext.getLabel()); + NestedSetBuilder lcovMergerFilesToRun = NestedSetBuilder.compileOrder(); + RunfilesSupplier lcovMergerRunfilesSupplier = null; + TestTargetExecutionSettings executionSettings; if (collectCodeCoverage) { collectCoverageScript = ruleContext.getHostPrerequisiteArtifact("$collect_coverage_script"); @@ -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 filesToBuild = lcovMerger.getProvider(FileProvider.class).getFilesToBuild(); @@ -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, @@ -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(), @@ -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()); diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java index 3f8577f0c0c387..15e90d40e88238 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java @@ -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(); @@ -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": * 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 7c2fb099a55889..2f64c41c8e1003 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 @@ -142,6 +142,10 @@ public class TestRunnerAction extends AbstractAction private final boolean cancelConcurrentTestsOnSuccess; + private final boolean splitCoveragePostProcessing; + private final NestedSetBuilder lcovMergerFilesToRun; + private final RunfilesSupplier lcovMergerRunfilesSupplier; + private static ImmutableSet nonNullAsSet(Artifact... artifacts) { ImmutableSet.Builder builder = ImmutableSet.builder(); for (Artifact artifact : artifacts) { @@ -180,7 +184,10 @@ private static ImmutableSet nonNullAsSet(Artifact... artifacts) { String workspaceName, @Nullable PathFragment shExecutable, boolean cancelConcurrentTestsOnSuccess, - Iterable tools) { + Iterable tools, + boolean splitCoveragePostProcessing, + NestedSetBuilder lcovMergerFilesToRun, + RunfilesSupplier lcovMergerRunfilesSupplier) { super( owner, NestedSetBuilder.wrap(Order.STABLE_ORDER, tools), @@ -236,6 +243,13 @@ private static ImmutableSet 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() { @@ -246,6 +260,18 @@ public final PathFragment getBaseDir() { return baseDir; } + public boolean getSplitCoveragePostProcessing() { + return splitCoveragePostProcessing; + } + + public NestedSetBuilder getLcovMergerFilesToRun() { + return lcovMergerFilesToRun; + } + + public Artifact getCoverageDirectoryTreeArtifact() { + return coverageDirectory; + } + @Override public boolean showsOutputUnconditionally() { return true; @@ -266,7 +292,9 @@ public List 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); } @@ -607,6 +635,8 @@ public void setupEnvVariables(Map 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"); } } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestStrategy.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestStrategy.java index f98b7d5e0c08b4..6f7e1718ffc490 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestStrategy.java @@ -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) 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 f54d963dd186ef..ccccfbc6710d35 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 @@ -16,13 +16,16 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; +import com.google.common.base.Verify; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.io.ByteStreams; import com.google.common.util.concurrent.ListenableFuture; import com.google.devtools.build.lib.actions.ActionExecutionContext; +import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.ActionInputHelper; +import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.ArtifactPathResolver; import com.google.devtools.build.lib.actions.EnvironmentalExecException; import com.google.devtools.build.lib.actions.ExecException; @@ -99,16 +102,9 @@ public TestRunnerSpawn createTestRunnerSpawn( "cannot run local tests with --nobuild_runfile_manifests", TestAction.Code.LOCAL_TEST_PREREQ_UNMET); } - Path execRoot = actionExecutionContext.getExecRoot(); - ArtifactPathResolver pathResolver = actionExecutionContext.getPathResolver(); - Path runfilesDir = pathResolver.convertPath(action.getExecutionSettings().getRunfilesDir()); - Path tmpDir = pathResolver.convertPath(tmpDirRoot.getChild(TestStrategy.getTmpDirName(action))); - Map env = setupEnvironment( - action, actionExecutionContext.getClientEnv(), execRoot, runfilesDir, tmpDir); - if (executionOptions.splitXmlGeneration) { - env.put("EXPERIMENTAL_SPLIT_XML_GENERATION", "1"); - } - Path workingDirectory = runfilesDir.getRelative(action.getRunfilesPrefix()); + Map testEnvironment = + createEnvironment( + actionExecutionContext, action, tmpDirRoot, executionOptions.splitXmlGeneration); Map executionInfo = new TreeMap<>(action.getTestProperties().getExecutionInfo()); @@ -130,7 +126,7 @@ public TestRunnerSpawn createTestRunnerSpawn( new SimpleSpawn( action, getArgs(action), - ImmutableMap.copyOf(env), + ImmutableMap.copyOf(testEnvironment), ImmutableMap.copyOf(executionInfo), action.getRunfilesSupplier(), ImmutableMap.of(), @@ -140,6 +136,11 @@ public TestRunnerSpawn createTestRunnerSpawn( : NestedSetBuilder.emptySet(Order.STABLE_ORDER), ImmutableSet.copyOf(action.getSpawnOutputs()), localResourceUsage); + Path execRoot = actionExecutionContext.getExecRoot(); + ArtifactPathResolver pathResolver = actionExecutionContext.getPathResolver(); + Path runfilesDir = pathResolver.convertPath(action.getExecutionSettings().getRunfilesDir()); + Path tmpDir = pathResolver.convertPath(tmpDirRoot.getChild(TestStrategy.getTmpDirName(action))); + Path workingDirectory = runfilesDir.getRelative(action.getRunfilesPrefix()); return new StandaloneTestRunnerSpawn( action, actionExecutionContext, spawn, tmpDir, workingDirectory, execRoot); } @@ -279,8 +280,11 @@ private StandaloneFailedAttemptResult processTestAttempt( return new StandaloneFailedAttemptResult(data); } - private Map setupEnvironment( - TestRunnerAction action, Map clientEnv, Path execRoot, Path runfilesDir, + private static Map setupEnvironment( + TestRunnerAction action, + Map clientEnv, + Path execRoot, + Path runfilesDir, Path tmpDir) { PathFragment relativeTmpDir; if (tmpDir.startsWith(execRoot)) { @@ -333,27 +337,32 @@ private TestAttemptContinuation beginTestAttempt( testOutErr, streamed, startTimeMillis, - spawnContinuation); + spawnContinuation, + /* testResultDataBuilder= */ null, + /* spawnResults= */ null); + } + + private static void appendCoverageLog(FileOutErr coverageOutErr, FileOutErr outErr) + throws IOException { + writeOutFile(coverageOutErr.getErrorPath(), outErr.getOutputPath()); + writeOutFile(coverageOutErr.getOutputPath(), outErr.getOutputPath()); } - /** In rare cases, we might write something to stderr. Append it to the real test.log. */ - private static void appendStderr(FileOutErr outErr) throws IOException { - Path stdErr = outErr.getErrorPath(); - FileStatus stat = stdErr.statNullable(); + private static void writeOutFile(Path inFilePath, Path outFilePath) throws IOException { + FileStatus stat = inFilePath.statNullable(); if (stat != null) { try { if (stat.getSize() > 0) { - Path stdOut = outErr.getOutputPath(); - if (stdOut.exists()) { - stdOut.setWritable(true); + if (outFilePath.exists()) { + outFilePath.setWritable(true); } - try (OutputStream out = stdOut.getOutputStream(true); - InputStream in = stdErr.getInputStream()) { + try (OutputStream out = outFilePath.getOutputStream(true); + InputStream in = inFilePath.getInputStream()) { ByteStreams.copy(in, out); } } } finally { - stdErr.delete(); + inFilePath.delete(); } } } @@ -413,6 +422,65 @@ private static Spawn createXmlGeneratingSpawn(TestRunnerAction action, SpawnResu SpawnAction.DEFAULT_RESOURCE_SET); } + /** + * A spawn to generate a test.xml file from the test log. This is only used if the test does not + * generate a test.xml file itself. + */ + private static Spawn createCoveragePostProcessingSpawn( + ActionExecutionContext actionExecutionContext, + TestRunnerAction action, + List expandedCoverageDir, + Path tmpDirRoot, + boolean splitXmlGeneration) { + ImmutableList args = + ImmutableList.of(action.getCollectCoverageScript().getExecPathString()); + + Map testEnvironment = + createEnvironment(actionExecutionContext, action, tmpDirRoot, splitXmlGeneration); + + testEnvironment.put("TEST_SHARD_INDEX", Integer.toString(action.getShardNum())); + testEnvironment.put( + "TEST_TOTAL_SHARDS", Integer.toString(action.getExecutionSettings().getTotalShards())); + testEnvironment.put("TEST_NAME", action.getTestName()); + testEnvironment.put("IS_COVERAGE_SPAWN", "1"); + return new SimpleSpawn( + action, + args, + ImmutableMap.copyOf(testEnvironment), + ImmutableMap.copyOf(action.getExecutionInfo()), + action.getLcovMergerRunfilesSupplier(), + /* filesetMappings= */ ImmutableMap.of(), + /* inputs= */ NestedSetBuilder.compileOrder() + .addAll(expandedCoverageDir) + .add(action.getCollectCoverageScript()) + .add(action.getCoverageDirectoryTreeArtifact()) + .add(action.getCoverageManifest()) + .addTransitive(action.getLcovMergerFilesToRun().build()) + .build(), + /* tools= */ NestedSetBuilder.emptySet(Order.STABLE_ORDER), + /* outputs= */ ImmutableSet.of( + ActionInputHelper.fromPath(action.getCoverageData().getExecPathString())), + SpawnAction.DEFAULT_RESOURCE_SET); + } + + private static Map createEnvironment( + ActionExecutionContext actionExecutionContext, + TestRunnerAction action, + Path tmpDirRoot, + boolean splitXmlGeneration) { + Path execRoot = actionExecutionContext.getExecRoot(); + ArtifactPathResolver pathResolver = actionExecutionContext.getPathResolver(); + Path runfilesDir = pathResolver.convertPath(action.getExecutionSettings().getRunfilesDir()); + Path tmpDir = pathResolver.convertPath(tmpDirRoot.getChild(TestStrategy.getTmpDirName(action))); + Map testEnvironment = + setupEnvironment( + action, actionExecutionContext.getClientEnv(), execRoot, runfilesDir, tmpDir); + if (splitXmlGeneration) { + testEnvironment.put("EXPERIMENTAL_SPLIT_XML_GENERATION", "1"); + } + return testEnvironment; + } + @Override public TestResult newCachedTestResult( Path execRoot, TestRunnerAction action, TestResultData data) { @@ -509,6 +577,8 @@ private final class BazelTestAttemptContinuation extends TestAttemptContinuation private final Closeable streamed; private final long startTimeMillis; private final SpawnContinuation spawnContinuation; + private TestResultData.Builder testResultDataBuilder; + private ImmutableList spawnResults; BazelTestAttemptContinuation( TestRunnerAction testAction, @@ -518,7 +588,9 @@ private final class BazelTestAttemptContinuation extends TestAttemptContinuation FileOutErr fileOutErr, Closeable streamed, long startTimeMillis, - SpawnContinuation spawnContinuation) { + SpawnContinuation spawnContinuation, + TestResultData.Builder testResultDataBuilder, + ImmutableList spawnResults) { this.testAction = testAction; this.actionExecutionContext = actionExecutionContext; this.spawn = spawn; @@ -527,6 +599,8 @@ private final class BazelTestAttemptContinuation extends TestAttemptContinuation this.streamed = streamed; this.startTimeMillis = startTimeMillis; this.spawnContinuation = spawnContinuation; + this.testResultDataBuilder = testResultDataBuilder; + this.spawnResults = spawnResults; } @Nullable @@ -536,58 +610,136 @@ public ListenableFuture getFuture() { } @Override - public TestAttemptContinuation execute() throws InterruptedException, ExecException { - // We have two protos to represent test attempts: - // 1. com.google.devtools.build.lib.view.test.TestStatus.TestResultData represents both failed - // attempts and finished tests. Bazel stores this to disk to persist cached test result - // information across server restarts. - // 2. com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.TestResult - // represents only individual attempts (failed or not). Bazel reports this as an event to - // the Build Event Protocol, but never saves it to disk. - // - // The TestResult proto is always constructed from a TestResultData instance, either one that - // is created right here, or one that is read back from disk. - TestResultData.Builder builder; - ImmutableList spawnResults; - try { - SpawnContinuation nextContinuation = spawnContinuation.execute(); - if (!nextContinuation.isDone()) { - return new BazelTestAttemptContinuation( + public TestAttemptContinuation execute() + throws InterruptedException, ExecException, IOException { + + if (testResultDataBuilder == null) { + // We have two protos to represent test attempts: + // 1. com.google.devtools.build.lib.view.test.TestStatus.TestResultData represents both + // failed attempts and finished tests. Bazel stores this to disk to persist cached test + // result information across server restarts. + // 2. com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.TestResult + // represents only individual attempts (failed or not). Bazel reports this as an event to + // the Build Event Protocol, but never saves it to disk. + // + // The TestResult proto is always constructed from a TestResultData instance, either one + // that + // is created right here, or one that is read back from disk. + TestResultData.Builder builder = null; + ImmutableList spawnResults; + try { + SpawnContinuation nextContinuation = spawnContinuation.execute(); + if (!nextContinuation.isDone()) { + return new BazelTestAttemptContinuation( + testAction, + actionExecutionContext, + spawn, + resolvedPaths, + fileOutErr, + streamed, + startTimeMillis, + nextContinuation, + builder, + /* spawnResults= */ null); + } + spawnResults = nextContinuation.get(); + builder = TestResultData.newBuilder(); + builder.setTestPassed(true).setStatus(BlazeTestStatus.PASSED); + } catch (SpawnExecException e) { + if (e.isCatastrophic()) { + closeSuppressed(e, streamed); + closeSuppressed(e, fileOutErr); + throw e; + } + if (!e.getSpawnResult().setupSuccess()) { + closeSuppressed(e, streamed); + closeSuppressed(e, fileOutErr); + // Rethrow as the test could not be run and thus there's no point in retrying. + throw e; + } + spawnResults = ImmutableList.of(e.getSpawnResult()); + builder = TestResultData.newBuilder(); + builder + .setTestPassed(false) + .setStatus(e.hasTimedOut() ? BlazeTestStatus.TIMEOUT : BlazeTestStatus.FAILED); + } catch (InterruptedException e) { + closeSuppressed(e, streamed); + closeSuppressed(e, fileOutErr); + throw e; + } + long endTimeMillis = actionExecutionContext.getClock().currentTimeMillis(); + + // SpawnActionContext guarantees the first entry to correspond to the spawn passed in (there + // may be additional entries due to tree artifact handling). + SpawnResult primaryResult = spawnResults.get(0); + + // The SpawnResult of a remotely cached or remotely executed action may not have walltime + // set. We fall back to the time measured here for backwards compatibility. + long durationMillis = endTimeMillis - startTimeMillis; + durationMillis = + primaryResult.getWallTime().orElse(Duration.ofMillis(durationMillis)).toMillis(); + + builder + .setStartTimeMillisEpoch(startTimeMillis) + .addTestTimes(durationMillis) + .addTestProcessTimes(durationMillis) + .setRunDurationMillis(durationMillis) + .setHasCoverage(testAction.isCoverageMode()); + + if (testAction.isCoverageMode() && testAction.getSplitCoveragePostProcessing()) { + actionExecutionContext + .getMetadataHandler() + .getMetadata(testAction.getCoverageDirectoryTreeArtifact()); + ImmutableSet expandedCoverageDir = + actionExecutionContext + .getMetadataHandler() + .getTreeArtifactChildren( + (SpecialArtifact) testAction.getCoverageDirectoryTreeArtifact()); + Spawn coveragePostProcessingSpawn = + createCoveragePostProcessingSpawn( + actionExecutionContext, + testAction, + ImmutableList.copyOf(expandedCoverageDir), + tmpDirRoot, + executionOptions.splitXmlGeneration); + SpawnStrategyResolver spawnStrategyResolver = + actionExecutionContext.getContext(SpawnStrategyResolver.class); + + Path testRoot = + actionExecutionContext.getInputPath(testAction.getTestLog()).getParentDirectory(); + + Path out = testRoot.getChild("coverage.log"); + Path err = testRoot.getChild("coverage.err"); + FileOutErr coverageOutErr = new FileOutErr(out, err); + ActionExecutionContext actionExecutionContextWithCoverageFileOutErr = + actionExecutionContext.withFileOutErr(coverageOutErr); + + SpawnContinuation coveragePostProcessingContinuation = + spawnStrategyResolver.beginExecution( + coveragePostProcessingSpawn, actionExecutionContextWithCoverageFileOutErr); + writeOutFile(coverageOutErr.getErrorPath(), coverageOutErr.getOutputPath()); + appendCoverageLog(coverageOutErr, fileOutErr); + return new BazelCoveragePostProcessingContinuation( testAction, actionExecutionContext, spawn, resolvedPaths, fileOutErr, streamed, - startTimeMillis, - nextContinuation); - } - spawnResults = nextContinuation.get(); - builder = TestResultData.newBuilder(); - builder.setTestPassed(true).setStatus(BlazeTestStatus.PASSED); - } catch (SpawnExecException e) { - if (e.isCatastrophic()) { - closeSuppressed(e, streamed); - closeSuppressed(e, fileOutErr); - throw e; + builder, + spawnResults, + coveragePostProcessingContinuation); + } else { + this.spawnResults = spawnResults; + this.testResultDataBuilder = builder; } - if (!e.getSpawnResult().setupSuccess()) { - closeSuppressed(e, streamed); - closeSuppressed(e, fileOutErr); - // Rethrow as the test could not be run and thus there's no point in retrying. - throw e; - } - spawnResults = ImmutableList.of(e.getSpawnResult()); - builder = TestResultData.newBuilder(); - builder - .setTestPassed(false) - .setStatus(e.hasTimedOut() ? BlazeTestStatus.TIMEOUT : BlazeTestStatus.FAILED); - } catch (InterruptedException e) { - closeSuppressed(e, streamed); - closeSuppressed(e, fileOutErr); - throw e; } - long endTimeMillis = actionExecutionContext.getClock().currentTimeMillis(); + + Verify.verify( + !(testAction.isCoverageMode() && testAction.getSplitCoveragePostProcessing()) + || testAction.getCoverageData().getPath().exists()); + Verify.verifyNotNull(spawnResults); + Verify.verifyNotNull(testResultDataBuilder); try { if (!fileOutErr.hasRecordedOutput()) { @@ -595,7 +747,7 @@ public TestAttemptContinuation execute() throws InterruptedException, ExecExcept FileSystemUtils.touchFile(fileOutErr.getOutputPath()); } // Append any error output to the test.log. This is very rare. - appendStderr(fileOutErr); + writeOutFile(fileOutErr.getErrorPath(), fileOutErr.getOutputPath()); fileOutErr.close(); if (streamed != null) { streamed.close(); @@ -604,23 +756,6 @@ public TestAttemptContinuation execute() throws InterruptedException, ExecExcept throw new EnvironmentalExecException(e, Code.TEST_OUT_ERR_IO_EXCEPTION); } - // SpawnActionContext guarantees the first entry to correspond to the spawn passed in (there - // may be additional entries due to tree artifact handling). - SpawnResult primaryResult = spawnResults.get(0); - - // The SpawnResult of a remotely cached or remotely executed action may not have walltime - // set. We fall back to the time measured here for backwards compatibility. - long durationMillis = endTimeMillis - startTimeMillis; - durationMillis = - primaryResult.getWallTime().orElse(Duration.ofMillis(durationMillis)).toMillis(); - - builder.setStartTimeMillisEpoch(startTimeMillis); - builder.addTestTimes(durationMillis); - builder.addTestProcessTimes(durationMillis); - builder.setRunDurationMillis(durationMillis); - if (testAction.isCoverageMode()) { - builder.setHasCoverage(true); - } // 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 @@ -630,7 +765,7 @@ public TestAttemptContinuation execute() throws InterruptedException, ExecExcept if (executionOptions.splitXmlGeneration && fileOutErr.getOutputPath().exists() && !xmlOutputPath.exists()) { - Spawn xmlGeneratingSpawn = createXmlGeneratingSpawn(testAction, primaryResult); + 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 @@ -641,7 +776,7 @@ public TestAttemptContinuation execute() throws InterruptedException, ExecExcept spawnStrategyResolver.beginExecution( xmlGeneratingSpawn, actionExecutionContext.withFileOutErr(xmlSpawnOutErr)); return new BazelXmlCreationContinuation( - resolvedPaths, xmlSpawnOutErr, builder, spawnResults, xmlContinuation); + resolvedPaths, xmlSpawnOutErr, testResultDataBuilder, spawnResults, xmlContinuation); } catch (InterruptedException e) { closeSuppressed(e, xmlSpawnOutErr); throw e; @@ -650,11 +785,11 @@ public TestAttemptContinuation execute() throws InterruptedException, ExecExcept TestCase details = parseTestResult(xmlOutputPath); if (details != null) { - builder.setTestCase(details); + testResultDataBuilder.setTestCase(details); } BuildEventStreamProtos.TestResult.ExecutionInfo executionInfo = - extractExecutionInfo(primaryResult, builder); + extractExecutionInfo(spawnResults.get(0), testResultDataBuilder); StandaloneTestResult standaloneTestResult = StandaloneTestResult.builder() .setSpawnResults(spawnResults) @@ -662,7 +797,7 @@ public TestAttemptContinuation execute() throws InterruptedException, ExecExcept // instance, as we may have to rename the output files in case the test needs to be // rerun (if it failed here _and_ is marked flaky _and_ the number of flaky attempts // is larger than 1). - .setTestResultDataBuilder(builder) + .setTestResultDataBuilder(testResultDataBuilder) .setExecutionInfo(executionInfo) .build(); return TestAttemptContinuation.of(standaloneTestResult); @@ -734,4 +869,97 @@ public TestAttemptContinuation execute() throws InterruptedException, ExecExcept return TestAttemptContinuation.of(standaloneTestResult); } } + + private final class BazelCoveragePostProcessingContinuation extends TestAttemptContinuation { + private final ResolvedPaths resolvedPaths; + private final FileOutErr fileOutErr; + private final Closeable streamed; + private final TestResultData.Builder testResultDataBuilder; + private final ImmutableList primarySpawnResults; + private final SpawnContinuation spawnContinuation; + private final TestRunnerAction testAction; + private final ActionExecutionContext actionExecutionContext; + private final Spawn spawn; + + BazelCoveragePostProcessingContinuation( + TestRunnerAction testAction, + ActionExecutionContext actionExecutionContext, + Spawn spawn, + ResolvedPaths resolvedPaths, + FileOutErr fileOutErr, + Closeable streamed, + TestResultData.Builder testResultDataBuilder, + ImmutableList primarySpawnResults, + SpawnContinuation spawnContinuation) { + this.testAction = testAction; + this.actionExecutionContext = actionExecutionContext; + this.spawn = spawn; + this.resolvedPaths = resolvedPaths; + this.fileOutErr = fileOutErr; + this.streamed = streamed; + this.testResultDataBuilder = testResultDataBuilder; + this.primarySpawnResults = primarySpawnResults; + this.spawnContinuation = spawnContinuation; + } + + @Nullable + @Override + public ListenableFuture getFuture() { + return spawnContinuation.getFuture(); + } + + @Override + public TestAttemptContinuation execute() throws InterruptedException, ExecException { + SpawnContinuation nextContinuation = null; + try { + nextContinuation = spawnContinuation.execute(); + if (!nextContinuation.isDone()) { + return new BazelCoveragePostProcessingContinuation( + testAction, + actionExecutionContext, + spawn, + resolvedPaths, + fileOutErr, + streamed, + testResultDataBuilder, + ImmutableList.builder() + .addAll(primarySpawnResults) + .addAll(nextContinuation.get()) + .build(), + nextContinuation); + } + } catch (SpawnExecException e) { + if (e.isCatastrophic()) { + closeSuppressed(e, streamed); + closeSuppressed(e, fileOutErr); + throw e; + } + if (!e.getSpawnResult().setupSuccess()) { + closeSuppressed(e, streamed); + closeSuppressed(e, fileOutErr); + // Rethrow as the test could not be run and thus there's no point in retrying. + throw e; + } + testResultDataBuilder + .setTestPassed(false) + .setStatus(e.hasTimedOut() ? BlazeTestStatus.TIMEOUT : BlazeTestStatus.FAILED); + } catch (ExecException | InterruptedException e) { + closeSuppressed(e, fileOutErr); + closeSuppressed(e, streamed); + throw e; + } + + return new BazelTestAttemptContinuation( + testAction, + actionExecutionContext, + spawn, + resolvedPaths, + fileOutErr, + streamed, + /* startTimeMillis= */ 0, + nextContinuation, + testResultDataBuilder, + primarySpawnResults); + } + } } diff --git a/src/test/shell/bazel/remote/BUILD b/src/test/shell/bazel/remote/BUILD index b4d07d39cdf79d..2d400be1f510c9 100644 --- a/src/test/shell/bazel/remote/BUILD +++ b/src/test/shell/bazel/remote/BUILD @@ -25,6 +25,7 @@ sh_test( ":remote_utils", ":uds_proxy.py", "//src/test/shell/bazel:test-deps", + "//src/test/shell/bazel/testdata:jdk_http_archives_filegroup", "//src/tools/remote:worker", "@bazel_tools//tools/bash/runfiles", ], diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index 26444e671f806f..440a23939da82c 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -2259,4 +2259,103 @@ EOF expect_log "INFO: Build completed successfully" } +# This test uses the flag experimental_split_coverage_postprocessing. Without +# the flag coverage won't work remotely. Without the flag, tests and coverage +# post-processing happen in the same spawn, but only the runfiles tree of the +# tests is made available to the spawn. The solution was not to merge the +# runfiles tree which could cause its own problems but to split both into +# different spawns. The reason why this only failed remotely and not locally was +# because the coverage post-processing tool escaped the sandbox to find its own +# runfiles. The error we would see here without the flag would be "Cannot find +# runfiles". See #4685. +function test_rbe_coverage_produces_report() { + mkdir -p java/factorial + + JAVA_TOOLCHAIN="@bazel_tools//tools/jdk:toolchain" + add_to_bazelrc "build --java_toolchain=${JAVA_TOOLCHAIN}" + add_to_bazelrc "build --host_java_toolchain=${JAVA_TOOLCHAIN}" + if is_darwin; then + add_to_bazelrc "build --javabase=@openjdk14_darwin_archive//:runtime" + add_to_bazelrc "build --host_javabase=@openjdk14_darwin_archive//:runtime" + fi + JAVA_TOOLS_ZIP="released" + COVERAGE_GENERATOR_DIR="released" + + cd java/factorial + + cat > BUILD <<'EOF' +java_library( + name = "fact", + srcs = ["Factorial.java"], +) + +java_test( + name = "fact-test", + size = "small", + srcs = ["FactorialTest.java"], + test_class = "factorial.FactorialTest", + deps = [ + ":fact", + ], +) + +EOF + + cat > Factorial.java <<'EOF' +package factorial; + +public class Factorial { + public static int factorial(int x) { + return x <= 0 ? 1 : x * factorial(x-1); + } +} +EOF + + cat > FactorialTest.java <<'EOF' +package factorial; + +import static org.junit.Assert.*; + +import org.junit.Test; + +public class FactorialTest { + @Test + public void testFactorialOfZeroIsOne() throws Exception { + assertEquals(Factorial.factorial(3),6); + } +} +EOF + cd ../.. + + cat $(rlocation io_bazel/src/test/shell/bazel/testdata/jdk_http_archives) >> WORKSPACE + + bazel coverage \ + --test_output=all \ + --experimental_fetch_all_coverage_outputs \ + --experimental_split_coverage_postprocessing \ + --spawn_strategy=remote \ + --remote_executor=grpc://localhost:${worker_port} \ + --instrumentation_filter=//java/factorial \ + //java/factorial:fact-test >& $TEST_log || fail "Shouldn't fail" + + local expected_result="SF:java/factorial/Factorial.java +FN:3,factorial/Factorial:: ()V +FN:5,factorial/Factorial::factorial (I)I +FNDA:0,factorial/Factorial:: ()V +FNDA:1,factorial/Factorial::factorial (I)I +FNF:2 +FNH:1 +BRDA:5,0,0,1 +BRDA:5,0,1,1 +BRF:2 +BRH:2 +DA:3,0 +DA:5,1 +LH:1 +LF:2 +end_of_record" + + assert_equals "$expected_result" "$(cat bazel-testlogs/java/factorial/fact-test/coverage.dat)" +} + run_suite "Remote execution and remote cache tests" diff --git a/src/test/shell/bazel/testdata/BUILD b/src/test/shell/bazel/testdata/BUILD index a331aac21e20dd..0b1caf646e7610 100644 --- a/src/test/shell/bazel/testdata/BUILD +++ b/src/test/shell/bazel/testdata/BUILD @@ -30,5 +30,5 @@ filegroup( srcs = [ "//src/test/shell/bazel/testdata:jdk_http_archives", ], - visibility = ["//src/test/shell/bazel:__pkg__"], + visibility = ["//src/test/shell/bazel:__subpackages__"], ) diff --git a/tools/test/collect_coverage.sh b/tools/test/collect_coverage.sh index 3c2bb9cc56c0b1..a5f69a0531a352 100755 --- a/tools/test/collect_coverage.sh +++ b/tools/test/collect_coverage.sh @@ -25,6 +25,25 @@ # Script expects that it will be started in the execution root directory and # not in the test's runfiles directory. +function resolve_links() { + local name="$1" + + if [ -e "$name" ]; then + # resolve all links, keep path absolute + while [ -L "$name" ]; do + local target=$(readlink "$name") + if [ "$(echo "$target" | head -c1)" = "/" ]; then + name="$target" + else + name="$(dirname "$name")/$target" + fi + done + echo "$name" + else + false # fail the function + fi +} + if [[ -z "$COVERAGE_MANIFEST" ]]; then echo -- echo Coverage runner: \$COVERAGE_MANIFEST is not set @@ -32,7 +51,6 @@ if [[ -z "$COVERAGE_MANIFEST" ]]; then env | sort exit 1 fi - # When collect_coverage.sh is used, test runner must be instructed not to cd # to the test's runfiles directory. export ROOT="$PWD" @@ -56,7 +74,6 @@ if ! [[ $COVERAGE_OUTPUT_FILE == $ROOT* ]]; then COVERAGE_OUTPUT_FILE=$ROOT/$COVERAGE_OUTPUT_FILE fi - # Java # -------------------------------------- export JAVA_COVERAGE_FILE=$COVERAGE_DIR/jvcov.dat @@ -125,32 +142,46 @@ if [[ ! -z "${JAVA_RUNTIME_CLASSPATH_FOR_COVERAGE}" ]]; then "${SINGLE_JAR_TOOL}" "@$single_jar_params_file" fi -# TODO(bazel-team): cd should be avoided. -cd "$TEST_SRCDIR/$TEST_WORKSPACE" -# Execute the test. -"$@" -TEST_STATUS=$? +if [[ "$IS_COVERAGE_SPAWN" == "0" ]]; then + # TODO(bazel-team): cd should be avoided. + cd "$TEST_SRCDIR/$TEST_WORKSPACE" -# Always create the coverage report. -touch $COVERAGE_OUTPUT_FILE + # Execute the test. + "$@" + TEST_STATUS=$? -if [[ $TEST_STATUS -ne 0 ]]; then - echo -- - echo Coverage runner: Not collecting coverage for failed test. - echo The following commands failed with status $TEST_STATUS - echo "$@" - exit $TEST_STATUS + # Always create the coverage report. + if [[ "$SPLIT_COVERAGE_POST_PROCESSING" == "0" ]]; then + touch $COVERAGE_OUTPUT_FILE + fi + + if [[ $TEST_STATUS -ne 0 ]]; then + echo -- + echo Coverage runner: Not collecting coverage for failed test. + echo The following commands failed with status $TEST_STATUS + echo "$@" + exit $TEST_STATUS + fi fi + +# ------------------EXPERIMENTAL--------------------- +# After this point we can run the code necessary for the coverage spawn + +if [[ "$SPLIT_COVERAGE_POST_PROCESSING" == "1" && "$IS_COVERAGE_SPAWN" == "0" ]]; then + exit 0 +fi + +if [[ "$SPLIT_COVERAGE_POST_PROCESSING" == "1" && "$IS_COVERAGE_SPAWN" == "1" ]]; then + touch $COVERAGE_OUTPUT_FILE +fi # TODO(bazel-team): cd should be avoided. cd $ROOT - # Call the C++ code coverage collection script. if [[ "$CC_CODE_COVERAGE_SCRIPT" ]]; then eval "${CC_CODE_COVERAGE_SCRIPT}" fi - # Export the command line that invokes LcovMerger with the flags: # --coverage_dir The absolute path of the directory where the # intermediate coverage reports are located. @@ -172,6 +203,12 @@ fi # keep only the C++ sources found in the manifest. # For other languages the sources in the manifest are # ignored. + +if [[ "$IS_COVERAGE_SPAWN" == "1" ]]; then + COVERAGE_DIR=$(resolve_links $COVERAGE_DIR) + COVERAGE_MANIFEST=$(resolve_links $COVERAGE_MANIFEST) +fi + LCOV_MERGER_CMD="${LCOV_MERGER} --coverage_dir=${COVERAGE_DIR} \ --output_file=${COVERAGE_OUTPUT_FILE} \ --filter_sources=/usr/bin/.+ \ @@ -194,4 +231,4 @@ fi # JAVA_RUNFILES is set to the runfiles of the test, which does not necessarily # contain a JVM (it does only if the test has a Java binary somewhere). So let # the LCOV merger discover where its own runfiles tree is. -JAVA_RUNFILES= exec $LCOV_MERGER_CMD +JAVA_RUNFILES= exec $LCOV_MERGER_CMD \ No newline at end of file