From 5aa538e4bc4c6ba8a30dc72afb1bc4d37e0c28f1 Mon Sep 17 00:00:00 2001 From: Florian Weikert Date: Fri, 11 Mar 2016 16:27:13 +0000 Subject: Fixed a problem with wrong test statuses when using notest_keep_going (especially with sharding). -- MOS_MIGRATED_REVID=116975152 --- .../devtools/build/lib/buildtool/BuildResult.java | 4 +-- .../build/lib/runtime/AggregatingTestListener.java | 5 +--- .../build/lib/runtime/TestResultAnalyzer.java | 32 ++++++++++++++-------- 3 files changed, 24 insertions(+), 17 deletions(-) (limited to 'src/main/java') diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildResult.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildResult.java index 7ae2da9f69..42bf33d22f 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildResult.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildResult.java @@ -118,8 +118,8 @@ public final class BuildResult { /** * Whether some targets were skipped because of {@code setStopOnFirstFailure}. */ - public boolean skippedTargetsBecauseOfEarlierFailure() { - return stopOnFirstFailure && !getSuccess(); + public boolean getStopOnFirstFailure() { + return stopOnFirstFailure; } /** diff --git a/src/main/java/com/google/devtools/build/lib/runtime/AggregatingTestListener.java b/src/main/java/com/google/devtools/build/lib/runtime/AggregatingTestListener.java index 424ce41b3a..94e0a7947c 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/AggregatingTestListener.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/AggregatingTestListener.java @@ -59,7 +59,6 @@ public class AggregatingTestListener { private final EventBus eventBus; private final EventHandlerPreconditions preconditionHelper; private volatile boolean blazeHalted = false; - private volatile boolean skippedTestsBecauseOfEarlierFailure; // summaryLock guards concurrent access to these two collections, which should be kept // synchronized with each other. @@ -165,7 +164,7 @@ public class AggregatingTestListener { } finalSummary = analyzer - .markUnbuilt(summary, blazeHalted, skippedTestsBecauseOfEarlierFailure) + .markUnbuilt(summary, blazeHalted) .build(); // These are never going to run; removing them marks the target complete. @@ -192,8 +191,6 @@ public class AggregatingTestListener { BuildResult result = event.getResult(); if (result.wasCatastrophe()) { blazeHalted = true; - } else if (result.skippedTargetsBecauseOfEarlierFailure()) { - skippedTestsBecauseOfEarlierFailure = true; } buildComplete(result.getActualTargets(), result.getSuccessfulTargets()); } diff --git a/src/main/java/com/google/devtools/build/lib/runtime/TestResultAnalyzer.java b/src/main/java/com/google/devtools/build/lib/runtime/TestResultAnalyzer.java index a5b038a68f..8206527d4f 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/TestResultAnalyzer.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/TestResultAnalyzer.java @@ -15,9 +15,11 @@ package com.google.devtools.build.lib.runtime; import com.google.common.collect.Sets; import com.google.common.eventbus.EventBus; +import com.google.common.eventbus.Subscribe; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.TransitiveInfoCollection; +import com.google.devtools.build.lib.buildtool.buildevent.BuildCompleteEvent; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadCompatible; import com.google.devtools.build.lib.exec.ExecutionOptions; @@ -48,6 +50,10 @@ public class TestResultAnalyzer { private final ExecutionOptions executionOptions; private final EventBus eventBus; + // Store information about potential failures in the presence of --nokeep_going or + // --notest_keep_going. + private boolean skipTargetsOnFailure; + /** * @param summaryOptions Parsed test summarization options. * @param executionOptions Parsed build/test execution options. @@ -61,6 +67,12 @@ public class TestResultAnalyzer { this.summaryOptions = summaryOptions; this.executionOptions = executionOptions; this.eventBus = eventBus; + eventBus.register(this); + } + + @Subscribe + public void doneBuild(BuildCompleteEvent event) { + skipTargetsOnFailure = event.getResult().getStopOnFirstFailure(); } /** @@ -272,24 +284,22 @@ public class TestResultAnalyzer { // tests with no status and post it here. TestSummary summary = summaryBuilder.peek(); BlazeTestStatus status = summary.getStatus(); - if (status != BlazeTestStatus.NO_STATUS) { + if (skipTargetsOnFailure) { + status = BlazeTestStatus.NO_STATUS; + } else if (status != BlazeTestStatus.NO_STATUS) { status = aggregateStatus(status, BlazeTestStatus.INCOMPLETE); } return summaryBuilder.setStatus(status); } - TestSummary.Builder markUnbuilt( - TestSummary.Builder summary, boolean blazeHalted, boolean stopOnFirstFailure) { - // stopOnFirstFailure = true means that at least on of the options keep_going and - // test_keep_going is set to false. - // Consequently, we mark all unbuilt targets with NO_STATUS instead of FAILED_TO_BUILD in - // order to indicate that Blaze has skipped these targets. + TestSummary.Builder markUnbuilt(TestSummary.Builder summary, boolean blazeHalted) { BlazeTestStatus runStatus = - blazeHalted - ? BlazeTestStatus.BLAZE_HALTED_BEFORE_TESTING : ( - executionOptions.testCheckUpToDate || stopOnFirstFailure - ? BlazeTestStatus.NO_STATUS : BlazeTestStatus.FAILED_TO_BUILD); + blazeHalted + ? BlazeTestStatus.BLAZE_HALTED_BEFORE_TESTING + : (executionOptions.testCheckUpToDate || skipTargetsOnFailure + ? BlazeTestStatus.NO_STATUS + : BlazeTestStatus.FAILED_TO_BUILD); return summary.setStatus(runStatus); } -- cgit v1.2.3