From 766cd0ed2906546b3d2c75771d65b15aeb07bb4e Mon Sep 17 00:00:00 2001 From: Googler Date: Fri, 25 Jun 2021 19:54:03 -0700 Subject: [PATCH] Add new attempt_count field to BES. PiperOrigin-RevId: 381590740 --- .../proto/build_event_stream.proto | 5 ++ .../lib/runtime/TestResultAggregator.java | 4 +- .../build/lib/runtime/TestSummary.java | 20 ++++- .../lib/runtime/TestResultAggregatorTest.java | 81 +++++++++++++++++++ 4 files changed, 106 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto b/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto index 4b52edbbc471a2..bd944eb9769fbe 100644 --- a/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto +++ b/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto @@ -713,6 +713,11 @@ message TestSummary { // Value of runs_per_test for the test. int32 run_count = 10; + // Number of attempts. + // If there are a different number of attempts per shard, the highest attempt + // count across all shards for each run is used. + int32 attempt_count = 15; + // Number of shards. int32 shard_count = 11; diff --git a/src/main/java/com/google/devtools/build/lib/runtime/TestResultAggregator.java b/src/main/java/com/google/devtools/build/lib/runtime/TestResultAggregator.java index 1a120a3c13b468..d8c420878ff439 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/TestResultAggregator.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/TestResultAggregator.java @@ -190,10 +190,12 @@ private void incrementalAnalyze(TestResult result) { Preconditions.checkNotNull(target, "The existing TestSummary must be associated with a target"); TestParams testParams = target.getProvider(TestProvider.class).getTestParams(); + int shardNumber = result.getShardNum(); + summary.addShardAttempts(shardNumber, result.getData().getTestTimesCount()); + if (!testParams.runsDetectsFlakes()) { status = aggregateStatus(status, result.getData().getStatus()); } else { - int shardNumber = result.getShardNum(); int runsPerTestForLabel = testParams.getRuns(); List singleShardStatuses = summary.addShardStatus(shardNumber, result.getData().getStatus()); 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 12c393ab47b960..000ffdff8e34c2 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 @@ -44,6 +44,7 @@ import com.google.protobuf.util.Durations; import com.google.protobuf.util.Timestamps; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.List; @@ -315,6 +316,13 @@ public ImmutableList addShardStatus(int shardNumber, BlazeTestS return ImmutableList.copyOf(statuses); } + /** Records new attempts for the given shard of the target. */ + public Builder addShardAttempts(int shardNumber, int newAtttempts) { + checkMutation(); + summary.shardAttempts[shardNumber] += newAtttempts; + return this; + } + /** * Returns the created TestSummary object. * Any actions following a build() will create another copy of the same values. @@ -360,6 +368,7 @@ private void makeSummaryImmutable() { private BuildConfiguration configuration; private BlazeTestStatus status; private boolean skipped; + private final int[] shardAttempts; private int numCached; private int numLocalActionCached; private boolean actionRan; @@ -383,9 +392,9 @@ private void makeSummaryImmutable() { private TestSummary(ConfiguredTarget target) { this.target = target; TestParams testParams = getTestParams(); - shardRunStatuses = - createAndInitialize( - testParams.runsDetectsFlakes() ? Math.max(testParams.getShards(), 1) : 0); + int sz = Math.max(testParams.getShards(), 1); + shardAttempts = new int[sz]; + shardRunStatuses = createAndInitialize(testParams.runsDetectsFlakes() ? sz : 0); } private static ImmutableList> createAndInitialize(int sz) { @@ -540,6 +549,10 @@ public List getTestTimes() { return testTimes; } + public int getNumAttempts() { + return Arrays.stream(this.shardAttempts).max().getAsInt(); + } + public int getNumCached() { return numCached; } @@ -610,6 +623,7 @@ public BuildEventStreamProtos.BuildEvent asStreamProto(BuildEventContext convert .setOverallStatus(BuildEventStreamerUtils.bepStatus(status)) .setTotalNumCached(getNumCached()) .setTotalRunCount(totalRuns()) + .setAttemptCount(getNumAttempts()) .setRunCount(testParams.getRuns()) .setShardCount(testParams.getShards()) .setFirstStartTime(Timestamps.fromMillis(firstStartTimeMillis)) diff --git a/src/test/java/com/google/devtools/build/lib/runtime/TestResultAggregatorTest.java b/src/test/java/com/google/devtools/build/lib/runtime/TestResultAggregatorTest.java index 7211f799e77784..a816bae3aef907 100644 --- a/src/test/java/com/google/devtools/build/lib/runtime/TestResultAggregatorTest.java +++ b/src/test/java/com/google/devtools/build/lib/runtime/TestResultAggregatorTest.java @@ -112,6 +112,80 @@ public void timingAggregation() { assertThat(summary.getTotalRunDurationMillis()).isEqualTo(11); } + @Test + public void attemptCount_agggregatesSingleShardSingleAttempt() { + when(mockParams.runsDetectsFlakes()).thenReturn(true); + TestResultAggregator underTest = createAggregatorWithTestRuns(1); + + underTest.testEvent( + shardedTestResult( + TestResultData.newBuilder().addAllTestTimes(ImmutableList.of(1L, 2L)), + /*shardNum=*/ 0)); + + assertThat(underTest.aggregateAndReportSummary(false).getNumAttempts()).isEqualTo(2); + } + + @Test + public void attemptCount_agggregatesSingleShardMultipleAttempts() { + when(mockParams.runsDetectsFlakes()).thenReturn(true); + TestResultAggregator underTest = createAggregatorWithTestRuns(2); + + underTest.testEvent( + shardedTestResult( + TestResultData.newBuilder().addAllTestTimes(ImmutableList.of(1L, 2L)), + /*shardNum=*/ 0)); + underTest.testEvent( + shardedTestResult( + TestResultData.newBuilder().addAllTestTimes(ImmutableList.of(3L, 4L)), + /*shardNum=*/ 0)); + + assertThat(underTest.aggregateAndReportSummary(false).getNumAttempts()).isEqualTo(4); + } + + @Test + public void attemptCount_agggregatesMultipleShardsMultipleAttempts() { + when(mockParams.runsDetectsFlakes()).thenReturn(true); + when(mockParams.getShards()).thenReturn(2); + TestResultAggregator underTest = createAggregatorWithTestRuns(3); + + underTest.testEvent( + shardedTestResult( + TestResultData.newBuilder().addAllTestTimes(ImmutableList.of(1L, 2L, 3L)), + /*shardNum=*/ 0)); + underTest.testEvent( + shardedTestResult( + TestResultData.newBuilder().addAllTestTimes(ImmutableList.of(3L, 4L)), + /*shardNum=*/ 1)); + underTest.testEvent( + shardedTestResult( + TestResultData.newBuilder().addAllTestTimes(ImmutableList.of(3L, 4L)), + /*shardNum=*/ 1)); + + assertThat(underTest.aggregateAndReportSummary(false).getNumAttempts()).isEqualTo(4); + } + + @Test + public void attemptCount_agggregatesMultipleShardsSingleShardHasMostAttempts() { + when(mockParams.runsDetectsFlakes()).thenReturn(true); + when(mockParams.getShards()).thenReturn(2); + TestResultAggregator underTest = createAggregatorWithTestRuns(3); + + underTest.testEvent( + shardedTestResult( + TestResultData.newBuilder().addAllTestTimes(ImmutableList.of(1L, 2L, 3L, 4L, 5L)), + /*shardNum=*/ 0)); + underTest.testEvent( + shardedTestResult( + TestResultData.newBuilder().addAllTestTimes(ImmutableList.of(3L, 4L)), + /*shardNum=*/ 1)); + underTest.testEvent( + shardedTestResult( + TestResultData.newBuilder().addAllTestTimes(ImmutableList.of(3L, 4L)), + /*shardNum=*/ 1)); + + assertThat(underTest.aggregateAndReportSummary(false).getNumAttempts()).isEqualTo(5); + } + @Test public void cancelConcurrentTests_cancellationAfterPassIgnored() { when(mockParams.runsDetectsFlakes()).thenReturn(true); @@ -180,4 +254,11 @@ private static TestResult testResult(TestResultData.Builder data, boolean locall when(mockTestAction.getTestOutputsMapping(any(), any())).thenReturn(ImmutableList.of()); return new TestResult(mockTestAction, data.build(), locallyCached, /*systemFailure=*/ null); } + + private static TestResult shardedTestResult(TestResultData.Builder data, int shardNum) { + TestRunnerAction mockTestAction = mock(TestRunnerAction.class); + when(mockTestAction.getTestOutputsMapping(any(), any())).thenReturn(ImmutableList.of()); + when(mockTestAction.getShardNum()).thenReturn(shardNum); + return new TestResult(mockTestAction, data.build(), /*cached=*/ false, /*systemFailure=*/ null); + } }