Simplify Test summary creation

- return a TestSummary rather than a Builder
- only call build() once
- remove some dead code
- avoid creating some unnecessary intermediate lists

This code is performance critical in that the thread executing it is
holding a lock on AggregatingTestListener. On builds which have a lot of
executing tests (e.g., high values of --runs_per_test) this can lead to
significant lock contention.

It might be better to refactor this code to just store the received test
attempt results and only aggregate when all attempts for a specific test
are done, rather than trying to do incremental work. That should make
the code simpler, although it might be a bit slower, but allows multiple
test reports to process in parallel.

PiperOrigin-RevId: 241903073

src/main/java/com/google/devtools/build/lib/runtime/TestResultAnalyzer.java

@@ -100,7 +100,7 @@ public boolean differentialAnalyzeAndReport(
     int passCount = 0;
 
     for (ConfiguredTarget testTarget : testTargets) {
-      TestSummary summary = aggregateAndReportSummary(testTarget, listener).build();
+      TestSummary summary = aggregateAndReportSummary(testTarget, listener);
       summaries.add(summary);
 
       // Finished aggregating; build the final console output.
@@ -135,20 +135,19 @@ private static BlazeTestStatus aggregateStatus(BlazeTestStatus status, BlazeTest
   }
 
   /**
-   * Helper for differential analysis which aggregates the TestSummary
-   * for an individual target, reporting runs on the EventBus if necessary.
+   * Helper for differential analysis which aggregates the TestSummary for an individual target,
+   * reporting runs on the EventBus if necessary.
    */
-  private TestSummary.Builder aggregateAndReportSummary(
-      ConfiguredTarget testTarget,
-      AggregatingTestListener listener) {
+  private TestSummary aggregateAndReportSummary(
+      ConfiguredTarget testTarget, AggregatingTestListener listener) {
 
     // If already reported by the listener, no work remains for this target.
     TestSummary.Builder summary = listener.getCurrentSummary(testTarget);
     Label testLabel = AliasProvider.getDependencyLabel(testTarget);
     Preconditions.checkNotNull(summary,
         "%s did not complete test filtering, but has a test result", testLabel);
     if (listener.targetReported(testTarget)) {
-      return summary;
+      return summary.build();
     }
 
     Collection<Artifact> incompleteRuns = listener.getIncompleteRuns(testTarget);
@@ -182,8 +181,9 @@ private TestSummary.Builder aggregateAndReportSummary(
     }
 
     // The target was not posted by the listener and must be posted now.
-    eventBus.post(summary.build());
-    return summary;
+    TestSummary result = summary.build();
+    eventBus.post(result);
+    return result;
   }
 
   /**
@@ -250,19 +250,16 @@ public TestSummary.Builder incrementalAnalyze(TestSummary.Builder summaryBuilder
       }
     }
 
-    List<Path> passed = new ArrayList<>();
     if (result.getData().hasPassedLog()) {
-      passed.add(result.getTestLogPath().getRelative(result.getData().getPassedLog()));
+      summaryBuilder.addPassedLog(
+          result.getTestLogPath().getRelative(result.getData().getPassedLog()));
     }
-    List<Path> failed = new ArrayList<>();
     for (String path : result.getData().getFailedLogsList()) {
-      failed.add(result.getTestLogPath().getRelative(path));
+      summaryBuilder.addFailedLog(result.getTestLogPath().getRelative(path));
     }
 
     summaryBuilder
         .addTestTimes(result.getData().getTestTimesList())
-        .addPassedLogs(passed)
-        .addFailedLogs(failed)
         .addWarnings(result.getData().getWarningList())
         .collectFailedTests(result.getData().getTestCase())
         .countTotalTestCases(result.getData().getTestCase())

src/main/java/com/google/devtools/build/lib/runtime/TestSummary.java

@@ -139,12 +139,24 @@ public Builder addPassedLogs(List<Path> passedLogs) {
       return this;
     }
 
+    public Builder addPassedLog(Path passedLog) {
+      checkMutation(passedLog);
+      summary.passedLogs.add(passedLog);
+      return this;
+    }
+
     public Builder addFailedLogs(List<Path> failedLogs) {
       checkMutation(failedLogs);
       summary.failedLogs.addAll(failedLogs);
       return this;
     }
 
+    public Builder addFailedLog(Path failedLog) {
+      checkMutation(failedLog);
+      summary.failedLogs.add(failedLog);
+      return this;
+    }
+
     public Builder addTotalTestCases(int totalTestCases) {
       checkMutation(totalTestCases);
       summary.totalTestCases += totalTestCases;
@@ -356,15 +368,6 @@ public static Builder newBuilder() {
     return new Builder();
   }
 
-  /**
-   * Creates a new Builder initialized with a copy of the existing object's values.
-   */
-  public static Builder newBuilderFromExisting(TestSummary existing) {
-    Builder builder = new Builder();
-    builder.mergeFrom(existing);
-    return builder;
-  }
-
   public Label getLabel() {
     return AliasProvider.getDependencyLabel(target);
   }

src/test/java/com/google/devtools/build/lib/runtime/TestSummaryTest.java

@@ -222,13 +222,6 @@ public void testBuilder() throws Exception {
     TestSummary sixtyCached = basicBuilder.setNumCached(60).build();
     assertThat(sixtyCached.numCached()).isEqualTo(60);
     assertThat(fiftyCached.numCached()).isEqualTo(50);
-
-    TestSummary failedCacheTemplate = TestSummary.newBuilderFromExisting(fiftyCached)
-        .setStatus(BlazeTestStatus.FAILED)
-        .build();
-    assertThat(failedCacheTemplate.numCached()).isEqualTo(50);
-    assertThat(failedCacheTemplate.getStatus()).isEqualTo(BlazeTestStatus.FAILED);
-    assertThat(failedCacheTemplate.getTotalTestCases()).isEqualTo(fiftyCached.getTotalTestCases());
   }
 
   @Test