Print Passed and Failed methods in detailed test summary

Previously, Bazel only printed Failed methods in the detailed test summary when using the `--test_summary=detailed` option. With this change, both Passed and Failed methods are now printed, providing a more comprehensive view of the test results. Fix bazelbuild#18685 Closes bazelbuild#19099. RELNOTES: The `--test_summary=detailed` option now also prints passed test cases. PiperOrigin-RevId: 553737487 Change-Id: I332b70d916394de7caed7a07667b46087724a6c1
thirtyseven · Sep 13, 2023 · 6d8dbb6 · 6d8dbb6
1 parent 842282e
commit 6d8dbb6
Show file tree

Hide file tree

Showing 5 changed files with 153 additions and 56 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/TerminalTestResultNotifier.java b/src/main/java/com/google/devtools/build/lib/runtime/TerminalTestResultNotifier.java
@@ -142,13 +142,13 @@ private boolean duplicateLabels(Set<TestSummary> summaries) {
    * @param summaries summaries of tests {@link TestSummary}
    * @param showAllTests if true, print information about each test regardless of its status
    * @param showNoStatusTests if true, print information about not executed tests (no status tests)
-   * @param printFailedTestCases if true, print details about which test cases in a test failed
+   * @param showAllTestCases if true, print all test cases status and detailed information
    */
   private void printSummary(
       Set<TestSummary> summaries,
       boolean showAllTests,
       boolean showNoStatusTests,
-      boolean printFailedTestCases) {
+      boolean showAllTestCases) {
     boolean withConfig = duplicateLabels(summaries);
     int numFailedToBuildReported = 0;
     for (TestSummary summary : summaries) {
@@ -171,7 +171,7 @@ private void printSummary(
           printer,
           testLogPathFormatter,
           summaryOptions.verboseSummary,
-          printFailedTestCases,
+          showAllTestCases,
           withConfig);
     }
   }
@@ -243,25 +243,25 @@ public void notify(Set<TestSummary> summaries, int numberOfExecutedTargets) {
       case DETAILED:
         printSummary(
             summaries,
-            /* showAllTests= */ false,
+            /* showAllTests= */ true,
             /* showNoStatusTests= */ true,
-            /* printFailedTestCases= */ true);
+            /* showAllTestCases= */ true);
         break;
 
       case SHORT:
         printSummary(
             summaries,
             /* showAllTests= */ true,
             /* showNoStatusTests= */ false,
-            /* printFailedTestCases= */ false);
+            /* showAllTestCases= */ false);
         break;
 
       case TERSE:
         printSummary(
             summaries,
             /* showAllTests= */ false,
             /* showNoStatusTests= */ false,
-            /* printFailedTestCases= */ false);
+            /* showAllTestCases= */ false);
         break;
 
       case TESTCASE:

diff --git a/src/main/java/com/google/devtools/build/lib/runtime/TestSummary.java b/src/main/java/com/google/devtools/build/lib/runtime/TestSummary.java
@@ -41,6 +41,7 @@
 import com.google.devtools.build.lib.view.test.TestStatus.BlazeTestStatus;
 import com.google.devtools.build.lib.view.test.TestStatus.FailedTestCasesStatus;
 import com.google.devtools.build.lib.view.test.TestStatus.TestCase;
+import com.google.devtools.build.lib.view.test.TestStatus.TestCase.Status;
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import com.google.protobuf.util.Durations;
 import com.google.protobuf.util.Timestamps;
@@ -218,9 +219,20 @@ private int traverseTestCases(TestCase testCase) {
       if (testCase.getStatus() != TestCase.Status.PASSED) {
         this.summary.failedTestCases.add(testCase);
       }
+
+      if (testCase.getStatus() == Status.PASSED) {
+        this.summary.passedTestCases.add(testCase);
+      }
+
       return 1;
     }
 
+    public Builder addPassedTestCases(List<TestCase> testCases) {
+      checkMutation(testCases);
+      summary.passedTestCases.addAll(testCases);
+      return this;
+    }
+
     @CanIgnoreReturnValue
     public Builder addFailedTestCases(List<TestCase> testCases, FailedTestCasesStatus status) {
       checkMutation(status);
@@ -396,6 +408,7 @@ private void makeSummaryImmutable() {
   private boolean ranRemotely;
   private boolean wasUnreportedWrongSize;
   private List<TestCase> failedTestCases = new ArrayList<>();
+  private final List<TestCase> passedTestCases = new ArrayList<>();
   private List<Path> passedLogs = new ArrayList<>();
   private List<Path> failedLogs = new ArrayList<>();
   private List<String> warnings = new ArrayList<>();
@@ -507,6 +520,10 @@ public List<TestCase> getFailedTestCases() {
     return failedTestCases;
   }
 
+  public List<TestCase> getPassedTestCases() {
+    return passedTestCases;
+  }
+
   public List<Path> getCoverageFiles() {
     return coverageFiles;
   }

diff --git a/src/main/java/com/google/devtools/build/lib/runtime/TestSummaryPrinter.java b/src/main/java/com/google/devtools/build/lib/runtime/TestSummaryPrinter.java
@@ -24,6 +24,7 @@
 import com.google.devtools.build.lib.view.test.TestStatus.BlazeTestStatus;
 import com.google.devtools.build.lib.view.test.TestStatus.FailedTestCasesStatus;
 import com.google.devtools.build.lib.view.test.TestStatus.TestCase;
+import com.google.devtools.build.lib.view.test.TestStatus.TestCase.Status;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.List;
@@ -117,26 +118,21 @@ public static void print(
       AnsiTerminalPrinter terminalPrinter,
       TestLogPathFormatter testLogPathFormatter,
       boolean verboseSummary,
-      boolean printFailedTestCases) {
-    print(
-        summary,
-        terminalPrinter,
-        testLogPathFormatter,
-        verboseSummary,
-        printFailedTestCases,
-        false);
+      boolean showAllTestCases) {
+    print(summary, terminalPrinter, testLogPathFormatter, verboseSummary, showAllTestCases, false);
   }
 
   /**
    * Prints summary status for a single test.
+   *
    * @param terminalPrinter The printer to print to
    */
   public static void print(
       TestSummary summary,
       AnsiTerminalPrinter terminalPrinter,
       TestLogPathFormatter testLogPathFormatter,
       boolean verboseSummary,
-      boolean printFailedTestCases,
+      boolean showAllTestCases,
       boolean withConfigurationName) {
     BlazeTestStatus status = summary.getStatus();
     // Skip output for tests that failed to build.
@@ -158,29 +154,35 @@ public static void print(
             + (verboseSummary ? getAttemptSummary(summary) + getTimeSummary(summary) : "")
             + "\n");
 
-    if (printFailedTestCases && summary.getStatus() == BlazeTestStatus.FAILED) {
-      if (summary.getFailedTestCasesStatus() == FailedTestCasesStatus.NOT_AVAILABLE) {
-        terminalPrinter.print(
-            Mode.WARNING + "    (individual test case information not available) "
-            + Mode.DEFAULT + "\n");
-      } else {
-        for (TestCase testCase : summary.getFailedTestCases()) {
-          if (testCase.getStatus() != TestCase.Status.PASSED) {
-            TestSummaryPrinter.printTestCase(terminalPrinter, testCase);
-          }
-        }
+    if (showAllTestCases) {
+      for (TestCase testCase : summary.getPassedTestCases()) {
+        TestSummaryPrinter.printTestCase(terminalPrinter, testCase);
+      }
 
-        if (summary.getFailedTestCasesStatus() != FailedTestCasesStatus.FULL) {
+      if (summary.getStatus() == BlazeTestStatus.FAILED) {
+        if (summary.getFailedTestCasesStatus() == FailedTestCasesStatus.NOT_AVAILABLE) {
           terminalPrinter.print(
               Mode.WARNING
-              + "    (some shards did not report details, list of failed test"
-              + " cases incomplete)\n"
-              + Mode.DEFAULT);
+                  + "    (individual test case information not available) "
+                  + Mode.DEFAULT
+                  + "\n");
+        } else {
+          for (TestCase testCase : summary.getFailedTestCases()) {
+            if (testCase.getStatus() != TestCase.Status.PASSED) {
+              TestSummaryPrinter.printTestCase(terminalPrinter, testCase);
+            }
+          }
+
+          if (summary.getFailedTestCasesStatus() != FailedTestCasesStatus.FULL) {
+            terminalPrinter.print(
+                Mode.WARNING
+                    + "    (some shards did not report details, list of failed test"
+                    + " cases incomplete)\n"
+                    + Mode.DEFAULT);
+          }
         }
       }
-    }
-
-    if (!printFailedTestCases) {
+    } else {
       for (String warning : summary.getWarnings()) {
         terminalPrinter.print("  " + AnsiTerminalPrinter.Mode.WARNING + "WARNING: "
             + AnsiTerminalPrinter.Mode.DEFAULT + warning + "\n");
@@ -205,12 +207,8 @@ public static void print(
     }
   }
 
-  /**
-   * Prints the result of an individual test case. It is assumed not to have
-   * passed, since passed test cases are not reported.
-   */
-  static void printTestCase(
-      AnsiTerminalPrinter terminalPrinter, TestCase testCase) {
+  /** Prints the result of an individual test case. */
+  static void printTestCase(AnsiTerminalPrinter terminalPrinter, TestCase testCase) {
     String timeSummary;
     if (testCase.hasRunDurationMillis()) {
       timeSummary = " ("
@@ -220,16 +218,17 @@ static void printTestCase(
       timeSummary = "";
     }
 
+    Mode mode = (testCase.getStatus() == Status.PASSED) ? Mode.INFO : Mode.ERROR;
     terminalPrinter.print(
         "    "
-        + Mode.ERROR
-        + Strings.padEnd(testCase.getStatus().toString(), 8, ' ')
-        + Mode.DEFAULT
-        + testCase.getClassName()
-        + "."
-        + testCase.getName()
-        + timeSummary
-        + "\n");
+            + mode
+            + Strings.padEnd(testCase.getStatus().toString(), 8, ' ')
+            + Mode.DEFAULT
+            + testCase.getClassName()
+            + "."
+            + testCase.getName()
+            + timeSummary
+            + "\n");
   }
 
   /**

diff --git a/src/test/java/com/google/devtools/build/lib/runtime/TestSummaryTest.java b/src/test/java/com/google/devtools/build/lib/runtime/TestSummaryTest.java
@@ -445,6 +445,30 @@ public void testTestCaseNamesShownWhenNeeded() throws Exception {
     verify(printerFailed).print(find("FAILED.*orange *\\(1\\.5"));
   }
 
+  @Test
+  public void testShowTestCaseNames() throws Exception {
+    TestCase detailPassed = newDetail("strawberry", TestCase.Status.PASSED, 1000L);
+    TestCase detailFailed = newDetail("orange", TestCase.Status.FAILED, 1500L);
+
+    TestSummary summaryPassed =
+        createPassedTestSummary(BlazeTestStatus.PASSED, Arrays.asList(detailPassed));
+
+    TestSummary summaryFailed =
+        createTestSummaryWithDetails(
+            BlazeTestStatus.FAILED, Arrays.asList(detailPassed, detailFailed));
+    assertThat(summaryFailed.getStatus()).isEqualTo(BlazeTestStatus.FAILED);
+
+    AnsiTerminalPrinter printerPassed = Mockito.mock(AnsiTerminalPrinter.class);
+    TestSummaryPrinter.print(summaryPassed, printerPassed, Path::getPathString, true, true);
+    verify(printerPassed).print(contains("//package:name"));
+    verify(printerPassed).print(find("PASSED.*strawberry *\\(1\\.0"));
+
+    AnsiTerminalPrinter printerFailed = Mockito.mock(AnsiTerminalPrinter.class);
+    TestSummaryPrinter.print(summaryFailed, printerFailed, Path::getPathString, true, true);
+    verify(printerFailed).print(contains("//package:name"));
+    verify(printerFailed).print(find("FAILED.*orange *\\(1\\.5"));
+  }
+
   @Test
   public void testTestCaseNamesOrdered() throws Exception {
     TestCase[] details = {
@@ -500,13 +524,13 @@ public void testCachedResultsFirstInSort() throws Exception {
 
   @Test
   public void testCollectingFailedDetails() throws Exception {
-    TestCase rootCase = TestCase.newBuilder()
-        .setName("tests")
-        .setRunDurationMillis(5000L)
-        .addChild(newDetail("apple", TestCase.Status.FAILED, 1000L))
-        .addChild(newDetail("banana", TestCase.Status.PASSED, 1000L))
-        .addChild(newDetail("cherry", TestCase.Status.ERROR, 1000L))
-        .build();
+    TestCase rootCase =
+        TestCase.newBuilder()
+            .setName("tests")
+            .setRunDurationMillis(5000L)
+            .addChild(newDetail("apple", TestCase.Status.FAILED, 1000L))
+            .addChild(newDetail("cherry", TestCase.Status.ERROR, 1000L))
+            .build();
 
     TestSummary summary =
         getTemplateBuilder().collectTestCases(rootCase).setStatus(BlazeTestStatus.FAILED).build();
@@ -518,6 +542,46 @@ public void testCollectingFailedDetails() throws Exception {
     verify(printer).print(find("ERROR.*cherry"));
   }
 
+  @Test
+  public void testCollectingAllDetails() throws Exception {
+    TestCase rootCase =
+        TestCase.newBuilder()
+            .setName("tests")
+            .setRunDurationMillis(5000L)
+            .addChild(newDetail("apple", TestCase.Status.FAILED, 1000L))
+            .addChild(newDetail("banana", TestCase.Status.PASSED, 1000L))
+            .addChild(newDetail("cherry", TestCase.Status.ERROR, 1000L))
+            .build();
+
+    TestSummary summary =
+        getTemplateBuilder().collectTestCases(rootCase).setStatus(BlazeTestStatus.FAILED).build();
+
+    AnsiTerminalPrinter printer = Mockito.mock(AnsiTerminalPrinter.class);
+    TestSummaryPrinter.print(summary, printer, Path::getPathString, true, true);
+    verify(printer).print(contains("//package:name"));
+    verify(printer).print(find("FAILED.*apple"));
+    verify(printer).print(find("PASSED.*banana"));
+    verify(printer).print(find("ERROR.*cherry"));
+  }
+
+  @Test
+  public void testCollectingPassedDetails() throws Exception {
+    TestCase rootCase =
+        TestCase.newBuilder()
+            .setName("tests")
+            .setRunDurationMillis(5000L)
+            .addChild(newDetail("apple", TestCase.Status.PASSED, 1000L))
+            .build();
+
+    TestSummary summary =
+        getTemplateBuilder().collectTestCases(rootCase).setStatus(BlazeTestStatus.PASSED).build();
+
+    AnsiTerminalPrinter printer = Mockito.mock(AnsiTerminalPrinter.class);
+    TestSummaryPrinter.print(summary, printer, Path::getPathString, true, true);
+    verify(printer).print(contains("//package:name"));
+    verify(printer).print(find("PASSED.*apple"));
+  }
+
   @Test
   public void countTotalTestCases() throws Exception {
     TestCase rootCase =
@@ -605,6 +669,10 @@ private ConfiguredTarget stubTarget() throws Exception {
     return target(PATH, TARGET_NAME);
   }
 
+  private TestSummary createPassedTestSummary(BlazeTestStatus status, List<TestCase> details) {
+    return getTemplateBuilder().setStatus(status).addPassedTestCases(details).build();
+  }
+
   private TestSummary createTestSummaryWithDetails(BlazeTestStatus status,
       List<TestCase> details) {
     TestSummary summary = getTemplateBuilder()

diff --git a/src/test/shell/bazel/bazel_test_test.sh b/src/test/shell/bazel/bazel_test_test.sh
@@ -645,7 +645,7 @@ EOF
   expect_log "name=\"dir/fail\""
 }
 
-function test_detailed_test_summary() {
+function test_detailed_test_summary_for_failed_test() {
   copy_examples
   create_workspace_with_default_repos WORKSPACE
   setup_javatest_support
@@ -658,6 +658,19 @@ function test_detailed_test_summary() {
   expect_log 'FAILED.*com\.example\.myproject\.Fail\.testFail'
 }
 
+function test_detailed_test_summary_for_passed_test() {
+  copy_examples
+  create_workspace_with_default_repos WORKSPACE
+  setup_javatest_support
+
+  local java_native_tests=//examples/java-native/src/test/java/com/example/myproject
+
+  bazel test --test_summary=detailed "${java_native_tests}:hello" >& $TEST_log \
+    || fail "expected success"
+  expect_log 'PASSED.*com\.example\.myproject\.TestHello\.testNoArgument'
+  expect_log 'PASSED.*com\.example\.myproject\.TestHello\.testWithArgument'
+}
+
 # This test uses "--ignore_all_rc_files" since outside .bazelrc files can pollute
 # this environment. Just "--bazelrc=/dev/null" is not sufficient to fix.
 function test_flaky_test() {