Fixed a problem with wrong test statuses when using notest_keep_going (especially with sharding).

--
MOS_MIGRATED_REVID=116975152

3 files changed, 24 insertions, 17 deletions

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);
   }