From 25ab148f330ecc402a1f390ff286997caece2136 Mon Sep 17 00:00:00 2001
From: Googler <leba@google.com>
Date: Fri, 11 Nov 2022 06:47:45 -0800
Subject: [PATCH] [Skymeld] Fix an issue where non-test targets were reported
 as tests when there's no target.

Assume a situation where we have a build without any target.
In regular blaze, we expect the output to be "Found 0 targets", while in Skymeld it was "Found 0 test targets".

This happened because regular blaze checks for the nullness of `analysisResult.getTargetsToTest()` to confirm that we shouldn't print "Found 0 test targets". Skymeld, on the other hand, uses the BuildResultListener which initializes targetsToTest as an empty set instead of null. This resulted in it printing "Found 0 test targets".

This CL fixes that by detaching the logic of whether or not we should report the tests from both BuildResultListener and AnalysisResult, and instead getting this info from the BuildRequest.

PiperOrigin-RevId: 487810344
Change-Id: I59e900699ab226219488ca22ae93fedbc3763d21
---
 .../AnalysisAndExecutionPhaseRunner.java      |  9 ++-
 .../lib/buildtool/AnalysisPhaseRunner.java    | 55 +++++++++++--------
 2 files changed, 38 insertions(+), 26 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisAndExecutionPhaseRunner.java b/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisAndExecutionPhaseRunner.java
index 648660c3e67696..2b31142b4586a5 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisAndExecutionPhaseRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisAndExecutionPhaseRunner.java
@@ -135,9 +135,14 @@ static AnalysisAndExecutionResult execute(
                 executionSetupCallback,
                 buildConfigurationCreatedCallback);
       }
+
       BuildResultListener buildResultListener = env.getBuildResultListener();
-      AnalysisPhaseRunner.reportTargets(
-          env, buildResultListener.getAnalyzedTargets(), buildResultListener.getAnalyzedTests());
+      if (request.shouldRunTests()) {
+        AnalysisPhaseRunner.reportTargetsWithTests(
+            env, buildResultListener.getAnalyzedTargets(), buildResultListener.getAnalyzedTests());
+      } else {
+        AnalysisPhaseRunner.reportTargets(env, buildResultListener.getAnalyzedTargets());
+      }
 
     } else {
       env.getReporter().handle(Event.progress("Loading complete."));
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java b/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java
index 0f6600de39fe4a..ee5f93c166baa0 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java
@@ -13,6 +13,7 @@
 // limitations under the License.
 package com.google.devtools.build.lib.buildtool;
 
+import com.google.common.base.Preconditions;
 import com.google.common.base.Stopwatch;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
@@ -143,7 +144,14 @@ public static AnalysisResult execute(
         module.afterAnalysis(env, request, buildOptions, analysisResult);
       }
 
-      reportTargets(env, analysisResult.getTargetsToBuild(), analysisResult.getTargetsToTest());
+      if (request.shouldRunTests()) {
+        reportTargetsWithTests(
+            env,
+            analysisResult.getTargetsToBuild(),
+            Preconditions.checkNotNull(analysisResult.getTargetsToTest()));
+      } else {
+        reportTargets(env, analysisResult.getTargetsToBuild());
+      }
 
       for (ConfiguredTarget target : analysisResult.getTargetsToSkip()) {
         BuildConfigurationValue config =
@@ -323,39 +331,38 @@ private static void postTopLevelStatusEvents(
     }
   }
 
-  static void reportTargets(
+  static void reportTargetsWithTests(
       CommandEnvironment env,
       Collection<ConfiguredTarget> targetsToBuild,
       Collection<ConfiguredTarget> targetsToTest) {
-    if (targetsToTest != null) {
-      int testCount = targetsToTest.size();
-      int targetCount = targetsToBuild.size() - testCount;
-      if (targetCount == 0) {
-        env.getReporter()
-            .handle(
-                Event.info(
-                    "Found "
-                        + testCount
-                        + (testCount == 1 ? " test target..." : " test targets...")));
-      } else {
-        env.getReporter()
-            .handle(
-                Event.info(
-                    "Found "
-                        + targetCount
-                        + (targetCount == 1 ? " target and " : " targets and ")
-                        + testCount
-                        + (testCount == 1 ? " test target..." : " test targets...")));
-      }
+    int testCount = targetsToTest.size();
+    int targetCount = targetsToBuild.size() - testCount;
+    if (targetCount == 0) {
+      env.getReporter()
+          .handle(
+              Event.info(
+                  "Found "
+                      + testCount
+                      + (testCount == 1 ? " test target..." : " test targets...")));
     } else {
-      int targetCount = targetsToBuild.size();
       env.getReporter()
           .handle(
               Event.info(
-                  "Found " + targetCount + (targetCount == 1 ? " target..." : " targets...")));
+                  "Found "
+                      + targetCount
+                      + (targetCount == 1 ? " target and " : " targets and ")
+                      + testCount
+                      + (testCount == 1 ? " test target..." : " test targets...")));
     }
   }
 
+  static void reportTargets(CommandEnvironment env, Collection<ConfiguredTarget> targetsToBuild) {
+    int targetCount = targetsToBuild.size();
+    env.getReporter()
+        .handle(
+            Event.info("Found " + targetCount + (targetCount == 1 ? " target..." : " targets...")));
+  }
+
   /**
    * Turns target patterns from the command line into parsed equivalents for single targets.
    *