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
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 871f2dd..88c40b8 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
@@ -100,7 +100,7 @@
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,12 +135,11 @@
}
/**
- * 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);
@@ -148,7 +147,7 @@
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 @@
}
// 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 @@
}
}
- 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())
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
index 486c833..a5cd20d 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/TestSummary.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/TestSummary.java
@@ -139,12 +139,24 @@
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 @@
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);
}
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
index 3a885d2..8bf021d 100644
--- a/src/test/java/com/google/devtools/build/lib/runtime/TestSummaryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/runtime/TestSummaryTest.java
@@ -222,13 +222,6 @@
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
