Update `Presubmit` check-run only if number of failed jobs changed. (#5169) fix: https://github.com/flutter/flutter/issues/194455 Previously, once a job failed, every subsequent build event triggered a redundant GitHub API call to update the `Presubmit` check run even when the job succeeded. Since we display a list of failed jobs in a `Presubmit` check run description we still have to update it every time new job failed or failed job re-run. Added `failedCountChanged` to `PresubmitGuardConclusion` (set in `UnifiedCheckRun.markConclusion`) and updated `Scheduler.processBuildCompleted` to only update the `Presubmit` check run when the number of failed jobs changes.
diff --git a/app_dart/lib/src/model/common/presubmit_guard_conclusion.dart b/app_dart/lib/src/model/common/presubmit_guard_conclusion.dart index c088e7d..f05484c 100644 --- a/app_dart/lib/src/model/common/presubmit_guard_conclusion.dart +++ b/app_dart/lib/src/model/common/presubmit_guard_conclusion.dart
@@ -43,6 +43,7 @@ final String? dashboardChecks; final String? mergeQueueGuard; final int failed; + final bool failedCountChanged; final List<String> failedJobNames; final String summary; final String details; @@ -53,6 +54,7 @@ this.dashboardChecks, required this.mergeQueueGuard, required this.failed, + this.failedCountChanged = false, this.failedJobNames = const [], required this.summary, required this.details, @@ -75,6 +77,7 @@ other.dashboardChecks == dashboardChecks && other.mergeQueueGuard == mergeQueueGuard && other.failed == failed && + other.failedCountChanged == failedCountChanged && const ListEquality<String>().equals( other.failedJobNames, failedJobNames, @@ -89,6 +92,7 @@ dashboardChecks, mergeQueueGuard, failed, + failedCountChanged, Object.hashAll(failedJobNames), summary, details, @@ -96,5 +100,5 @@ @override String toString() => - 'BuildConclusion("$result", "$remaining", "$failed", "$failedJobNames", "$summary", "$details", "$dashboardChecks", "$mergeQueueGuard")'; + 'BuildConclusion("$result", "$remaining", "$failed", "$failedCountChanged", "$failedJobNames", "$summary", "$details", "$dashboardChecks", "$mergeQueueGuard")'; }
diff --git a/app_dart/lib/src/service/firestore/unified_check_run.dart b/app_dart/lib/src/service/firestore/unified_check_run.dart index bf90895..30f4c11 100644 --- a/app_dart/lib/src/service/firestore/unified_check_run.dart +++ b/app_dart/lib/src/service/firestore/unified_check_run.dart
@@ -566,6 +566,7 @@ var remaining = -1; var failed = -1; + var failedCountChanged = false; var valid = false; late final PresubmitGuard presubmitGuard; @@ -586,6 +587,7 @@ presubmitGuard = PresubmitGuard.fromDocument(presubmitGuardDocument); final jobs = presubmitGuard.jobs; + final previousFailedCount = jobs.values.where((s) => s.isFailure).length; // Check if the build is present in the guard before trying to load it. if (jobs[state.jobName] == null) { @@ -598,7 +600,7 @@ remaining: jobs.values.where((s) => !s.isComplete).length, dashboardChecks: presubmitGuard.checkRunJson, mergeQueueGuard: presubmitGuard.checkRunGuardJson, - failed: jobs.values.where((s) => s.isFailure).length, + failed: previousFailedCount, summary: 'Check run "${state.jobName}" not present in ${guardId.stage} CI stage', details: 'Change $changeCrumb', @@ -652,6 +654,7 @@ presubmitJob.status = status; remaining = jobs.values.where((s) => !s.isComplete).length; failed = jobs.values.where((s) => s.isFailure).length; + failedCountChanged = failed != previousFailedCount; } on DetailedApiRequestError catch (e, stack) { if (e.status == 404) { // An attempt to read a document not in firestore should not be retried. @@ -701,6 +704,7 @@ dashboardChecks: presubmitGuard.checkRunJson, mergeQueueGuard: presubmitGuard.checkRunGuardJson, failed: failed, + failedCountChanged: valid && failedCountChanged, failedJobNames: valid ? presubmitGuard.failedJobNames : const [], summary: valid ? 'Successfully updated presubmit guard status'
diff --git a/app_dart/lib/src/service/scheduler.dart b/app_dart/lib/src/service/scheduler.dart index 262425a..c2721c1 100644 --- a/app_dart/lib/src/service/scheduler.dart +++ b/app_dart/lib/src/service/scheduler.dart
@@ -1308,14 +1308,16 @@ // based on if the resetFailedCheckRun flag is enabled. if (check.author != null && _config.flags.isResetFailedCheckRunEnabledForUser(check.author!)) { - await _requireActionForPresubmit( - slug: check.slug, - checkSuiteId: guard.checkSuiteId!, - headSha: check.sha, - summary: summary, - details: details, - detailsUrl: detailsUrl, - ); + if (stagingConclusion.failedCountChanged) { + await _requireActionForPresubmit( + slug: check.slug, + checkSuiteId: guard.checkSuiteId!, + headSha: check.sha, + summary: summary, + details: details, + detailsUrl: detailsUrl, + ); + } } else { await _requireActionForDashboardChecks( slug: check.slug,
diff --git a/app_dart/test/service/firestore/unified_check_run_test.dart b/app_dart/test/service/firestore/unified_check_run_test.dart index dee101d..ff5ab41 100644 --- a/app_dart/test/service/firestore/unified_check_run_test.dart +++ b/app_dart/test/service/firestore/unified_check_run_test.dart
@@ -240,7 +240,27 @@ expect(result.result, PresubmitGuardConclusionResult.ok); expect(result.remaining, 1); expect(result.failed, 1); + expect(result.failedCountChanged, isTrue); expect(result.failedJobNames, ['linux']); + + // Subsequent non-failing update does not change failed count. + final result2 = await UnifiedCheckRun.markConclusion( + firestoreService: firestoreService, + guardId: guardId, + state: const PresubmitJobState( + jobName: 'mac', + status: TaskStatus.succeeded, + attemptNumber: 1, + startTime: 2000, + endTime: 3000, + ), + ); + + expect(result2.result, PresubmitGuardConclusionResult.ok); + expect(result2.remaining, 0); + expect(result2.failed, 1); + expect(result2.failedCountChanged, isFalse); + expect(result2.failedJobNames, ['linux']); }); test('handles missing check gracefully', () async {
diff --git a/app_dart/test/service/scheduler_test.dart b/app_dart/test/service/scheduler_test.dart index bfde357..8bc5065 100644 --- a/app_dart/test/service/scheduler_test.dart +++ b/app_dart/test/service/scheduler_test.dart
@@ -4726,7 +4726,8 @@ ), ); - // 2. Second build ('Mac test') succeeds -> stage finishes with 1 failed check, Dashboard Checks still in progress. + // 2. Second build ('Mac test') succeeds -> stage finishes with 1 failed check, + // neither Presubmit nor Dashboard Checks is updated since failed count did not change. final macSucceededBuild = generateBbv2Build( Int64(2), name: 'Mac test', @@ -4752,6 +4753,25 @@ isA<CheckRun>().having( (c) => c.name, 'name', + Config.kPresubmitCheckName, + ), + ), + status: anyNamed('status'), + conclusion: anyNamed('conclusion'), + detailsUrl: anyNamed('detailsUrl'), + output: anyNamed('output'), + actions: anyNamed('actions'), + ), + ); + + verifyNever( + mockGithubChecksUtil.updateCheckRun( + any, + any, + argThat( + isA<CheckRun>().having( + (c) => c.name, + 'name', Config.kDashboardCheckName, ), ), @@ -4762,6 +4782,50 @@ actions: anyNamed('actions'), ), ); + + // 3. If 'Mac test' fails, failed count changes from 1 to 2 -> Presubmit check is updated. + final macFailedBuild = generateBbv2Build( + Int64(3), + name: 'Mac test', + status: bbv2.Status.FAILURE, + tags: [ + bbv2.StringPair(key: 'current_attempt', value: '1'), + bbv2.StringPair(key: 'author', value: pullRequest.user!.login!), + ], + ); + + expect( + await scheduler.processBuildCompleted( + PresubmitCompletedJob.fromBuild(macFailedBuild, userData), + ), + isTrue, + ); + + final secondVerification = verify( + mockGithubChecksUtil.updateCheckRun( + any, + pullRequest.base!.repo!.slug(), + argThat( + isA<CheckRun>().having( + (c) => c.name, + 'name', + Config.kPresubmitCheckName, + ), + ), + status: CheckRunStatus.completed, + conclusion: CheckRunConclusion.actionRequired, + detailsUrl: anyNamed('detailsUrl'), + output: captureAnyNamed('output'), + actions: anyNamed('actions'), + ), + ); + secondVerification.called(1); + final secondOutput = + secondVerification.captured.single as CheckRunOutput; + expect( + secondOutput.text, + 'Failed presubmit jobs:\n- `Linux test`\n- `Mac test`', + ); }, );