From 87333ea2fb044196910ff3bb15c25e3e66d19b7b Mon Sep 17 00:00:00 2001 From: "Dmitry Grand (dmgr)" Date: Wed, 23 Sep 2026 15:46:58 -0700 Subject: [PATCH 1/7] draft --- .../src/foundation/github_checks_util.dart | 29 ++++---- .../common/presubmit_guard_conclusion.dart | 4 +- app_dart/lib/src/service/config.dart | 4 ++ .../lib/src/service/luci_build_service.dart | 48 +++++++++++++ app_dart/lib/src/service/scheduler.dart | 67 +++++++++++++++---- .../firestore/unified_check_run_test.dart | 4 +- .../service/github_checks_service_test.dart | 4 +- .../lib/src/utilities/mocks.mocks.dart | 9 +-- 8 files changed, 134 insertions(+), 35 deletions(-) diff --git a/app_dart/lib/src/foundation/github_checks_util.dart b/app_dart/lib/src/foundation/github_checks_util.dart index 7a1728c81f..fc62801b68 100644 --- a/app_dart/lib/src/foundation/github_checks_util.dart +++ b/app_dart/lib/src/foundation/github_checks_util.dart @@ -6,7 +6,6 @@ import 'dart:core'; import 'package:cocoon_server/logging.dart'; import 'package:github/github.dart' as github; -import 'package:github/hooks.dart'; import 'package:retry/retry.dart'; import '../request_handling/http_utils.dart'; @@ -17,18 +16,24 @@ import '../service/config.dart'; class GithubChecksUtil { const GithubChecksUtil(); Future> allCheckRuns( - github.GitHub gitHubClient, - CheckSuiteEvent checkSuiteEvent, + Config config, + github.RepositorySlug slug, + int checkSuiteId, ) async { - final allCheckRuns = await gitHubClient.checks.checkRuns - .listCheckRunsInSuite( - checkSuiteEvent.repository!.slug(), - checkSuiteId: checkSuiteEvent.checkSuite!.id!, - ) - .toList(); - return { - for (github.CheckRun check in allCheckRuns) check.name as String: check, - }; + final gitHubClient = await config.createGitHubClient(slug: slug); + const r = RetryOptions(maxAttempts: 3, delayFactor: Duration(seconds: 2)); + return r.retry( + () async { + final allCheckRuns = await gitHubClient.checks.checkRuns + .listCheckRunsInSuite(slug, checkSuiteId: checkSuiteId) + .toList(); + return { + for (github.CheckRun check in allCheckRuns) + check.name as String: check, + }; + }, + retryIf: (Exception e) => e is github.GitHubError || e is SocketException, + ); } Future getCheckSuite( 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 dc6b3b15ee..4ef5eaed67 100644 --- a/app_dart/lib/src/model/common/presubmit_guard_conclusion.dart +++ b/app_dart/lib/src/model/common/presubmit_guard_conclusion.dart @@ -62,9 +62,9 @@ class PresubmitGuardConclusion { bool get isPending => isOk && remaining > 0; - bool get isFailed => isOk && !isPending && failed > 0; + bool get isFailed => isOk && failed > 0; - bool get isComplete => isOk && !isPending && !isFailed; + bool get isSuccessed => isOk && !isPending && !isFailed; @override bool operator ==(Object other) => diff --git a/app_dart/lib/src/service/config.dart b/app_dart/lib/src/service/config.dart index 26a1d137ca..e6344b26bc 100644 --- a/app_dart/lib/src/service/config.dart +++ b/app_dart/lib/src/service/config.dart @@ -85,6 +85,10 @@ interface class Config extends DynamicallyUpdatedConfig { /// for users opted into the unified checkrun flow. static const String kDashboardCheckName = 'Dashboard Checks'; + /// A required check that fails if at least one job is failed and reset to + /// in-progress when all the failed jobs are retried. + static const String kPresubmitCheckName = 'Presubmit'; + final CacheService _cache; final SecretManager _secrets; final http.Client _httpClient; diff --git a/app_dart/lib/src/service/luci_build_service.dart b/app_dart/lib/src/service/luci_build_service.dart index 2d81c1804a..3c15d812a1 100644 --- a/app_dart/lib/src/service/luci_build_service.dart +++ b/app_dart/lib/src/service/luci_build_service.dart @@ -443,6 +443,54 @@ class LuciBuildService { ); } + // Set the presubmit check run status to `CheckRunStatus.inProgress` if + // Re-run all Failed Jobs. + final isRerun = targets.values.first > 1; + if (isRerun && stage != null && dashboardChecks != null) { + try { + final presubmitGuardDoc = await _firestore.getDocument( + PresubmitGuard.documentNameFor( + slug: slug, + prNum: pullRequest.number!, + checkRunId: dashboardChecks.id!, + stage: stage, + ), + ); + final guard = PresubmitGuard.fromDocument(presubmitGuardDoc); + final checkRun = guard.checkRun; + + if (guard.failedJobs == 0) { + log.info('Re-requesting presubmit check run for Guard $guard'); + // final checks = await _githubChecksUtil.allCheckRuns( + // _config, + // slug, + // checkRun.checkSuiteId!, + // ); + // log.info('Found check runs: ${checks.keys.join(', ')}'); + // final presubmitChecks = checks[Config.kPresubmitCheckName]!; + + await _githubChecksUtil.createCheckRun( + _config, + slug, + checkRun.headSha!, + Config.kPresubmitCheckName, + output: const CheckRunOutput( + title: Config.kPresubmitCheckName, + summary: Scheduler.kPresubmitCheckDescription, + ), + detailsUrl: checkRun.detailsUrl, + ); + } + } catch (e, s) { + // We are not going to block on this error. + log.warn( + 'Failed to re-request dashboard checks for PR# ${pullRequest.number}', + e, + s, + ); + } + } + return targets.keys.toList(); } diff --git a/app_dart/lib/src/service/scheduler.dart b/app_dart/lib/src/service/scheduler.dart index 4f24c2cd0d..34b957862b 100644 --- a/app_dart/lib/src/service/scheduler.dart +++ b/app_dart/lib/src/service/scheduler.dart @@ -129,6 +129,23 @@ class Scheduler { 'to merge your PR without presubmit checks (a rare situation, typically ' 'an emergency), then you can use the `emergency` label.'; + /// Briefly describes what the "Presubmit" check is for. + /// + /// Find more details about this check at [kPresubmitCheckName]. + /// + /// This description appears next to the Github check run in the pull request + /// and merge queue UI. + static const String kPresubmitCheckDescription = + 'The presubmit check is a GitHub check that prevents a PR from being ' + 'merged or enqueued before it is ready. It becomes green automatically ' + 'when all tests pass. It will fail if at least one job is failed and ' + 'reset to in-progress when all the failed jobs are retried. If it fails, ' + 'you can view failure details, get execution logs, and re-run failed ' + 'jobs on the presubmit dashboard page. If you suspect that this check is ' + 'not working correctly, contact #hackers-infra on Discord. If you need ' + 'to merge your PR without presubmit checks (a rare situation, typically ' + 'an emergency), then you can use the `emergency` label.'; + /// Ensure [commits] exist in Cocoon. /// /// If the commit already exists, it is ignored. @@ -874,8 +891,19 @@ $s ), detailsUrl: isPresubmit ? detailsUrl : null, ); - - if (!isPresubmit) { + if (isPresubmit) { + await _githubChecksService.githubChecksUtil.createCheckRun( + _config, + slug, + headSha, + Config.kPresubmitCheckName, + output: const CheckRunOutput( + title: Config.kPresubmitCheckName, + summary: kPresubmitCheckDescription, + ), + detailsUrl: detailsUrl, + ); + } else { // Skip Dashboard Checks await _githubChecksService.githubChecksUtil.updateCheckRun( _config, @@ -993,30 +1021,41 @@ $s ); } - Future _requireActionForGuard({ + Future _requireActionForPresubmit({ required RepositorySlug slug, - required CheckRun lock, + required int checkSuiteId, required String headSha, required String summary, required String details, String? detailsUrl, }) async { log.info(''' -Require action for merge group guard ${lock.id} for: -head sha: $headSha -slug: $slug -summary: $summary -details: $details -detailsUrl: $detailsUrl +Require action for ${Config.kPresubmitCheckName} +with: + summary: $summary + details: $details + detailsUrl: $detailsUrl +defined in: + repository: $slug + check suite: $checkSuiteId + head sha: $headSha '''); + final checks = await _githubChecksService.githubChecksUtil.allCheckRuns( + _config, + slug, + checkSuiteId, + ); + log.info('Found check runs: ${checks.keys.join(', ')}'); + final presubmitChecks = checks[Config.kPresubmitCheckName]!; + await _githubChecksService.githubChecksUtil.updateCheckRun( _config, slug, - lock, + presubmitChecks, status: CheckRunStatus.completed, conclusion: CheckRunConclusion.actionRequired, output: CheckRunOutput( - title: Config.kDashboardCheckName, + title: Config.kPresubmitCheckName, summary: summary, text: details, ), @@ -1214,9 +1253,9 @@ detailsUrl: $detailsUrl final guard = checkRunFromString(stagingConclusion.dashboardChecks!); final detailsUrl = 'https://flutter-dashboard.appspot.com/#/presubmit?repo=${check.slug.name}&sha=${check.sha}'; - await _requireActionForGuard( + await _requireActionForPresubmit( slug: check.slug, - lock: guard, + checkSuiteId: guard.checkSuiteId!, headSha: check.sha, summary: _githubChecksService.getGithubSummaryWithHeader(''' **[Failed Presubmit Jobs Details]($detailsUrl)** 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 2a8ad462de..9c1f3fbe30 100644 --- a/app_dart/test/service/firestore/unified_check_run_test.dart +++ b/app_dart/test/service/firestore/unified_check_run_test.dart @@ -189,7 +189,7 @@ void main() { expect(result1.remaining, 1); expect(result1.failed, 0); expect(result1.isOk, true); - expect(result1.isComplete, false); + expect(result1.isSuccessed, false); expect(result1.isPending, true); final result2 = await UnifiedCheckRun.markConclusion( @@ -207,7 +207,7 @@ void main() { expect(result2.remaining, 0); expect(result2.failed, 0); expect(result2.isOk, true); - expect(result2.isComplete, true); + expect(result2.isSuccessed, true); expect(result2.isPending, false); final checkDoc = await PresubmitJob.fromFirestore( diff --git a/app_dart/test/service/github_checks_service_test.dart b/app_dart/test/service/github_checks_service_test.dart index f0511621c2..61d9d88319 100644 --- a/app_dart/test/service/github_checks_service_test.dart +++ b/app_dart/test/service/github_checks_service_test.dart @@ -46,7 +46,9 @@ void main() { ); final checkRuns = {'Cocoon': checkRun}; // ignore: discarded_futures - when(mockGithubChecksUtil.allCheckRuns(any, any)).thenAnswer((_) async { + when(mockGithubChecksUtil.allCheckRuns(any, any, any)).thenAnswer(( + _, + ) async { return checkRuns; }); }); diff --git a/packages/cocoon_integration_test/lib/src/utilities/mocks.mocks.dart b/packages/cocoon_integration_test/lib/src/utilities/mocks.mocks.dart index 844434e46f..394d9ee538 100644 --- a/packages/cocoon_integration_test/lib/src/utilities/mocks.mocks.dart +++ b/packages/cocoon_integration_test/lib/src/utilities/mocks.mocks.dart @@ -1984,11 +1984,12 @@ class MockGithubChecksUtil extends _i1.Mock implements _i10.GithubChecksUtil { @override _i13.Future> allCheckRuns( - _i7.GitHub? gitHubClient, - _i21.CheckSuiteEvent? checkSuiteEvent, + _i2.Config? config, + _i7.RepositorySlug? slug, + int? checkSuiteId, ) => (super.noSuchMethod( - Invocation.method(#allCheckRuns, [gitHubClient, checkSuiteEvent]), + Invocation.method(#allCheckRuns, [config, slug, checkSuiteId]), returnValue: _i13.Future>.value( {}, ), @@ -5789,7 +5790,7 @@ class MockScheduler extends _i1.Mock implements _i2.Scheduler { _i7.CheckRun? lock, ) => (super.noSuchMethod( - Invocation.method(#unlockCheckRun, [slug, headSha, lock]), + Invocation.method(#unlockMergeQueueGuard, [slug, headSha, lock]), returnValue: _i13.Future.value(), returnValueForMissingStub: _i13.Future.value(), ) From 3d722665d9a06edc68417cd9e2bde7540f747db4 Mon Sep 17 00:00:00 2001 From: "Dmitry Grand (dmgr)" Date: Thu, 24 Sep 2026 15:02:37 -0700 Subject: [PATCH 2/7] initial implementation --- .../common/presubmit_completed_check.dart | 3 + .../presubmit_subscription.dart | 6 +- .../src/service/github_checks_service.dart | 10 +- .../lib/src/service/luci_build_service.dart | 112 ++++++++-------- .../luci_build_service/build_tags.dart | 19 +++ app_dart/lib/src/service/scheduler.dart | 123 +++++++++++++----- .../presubmit_luci_subscription_test.dart | 36 ++--- .../presubmit_ordered_subscription_test.dart | 4 +- .../service/github_checks_service_test.dart | 14 +- app_dart/test/service/scheduler_test.dart | 78 ++++++++--- .../lib/src/utilities/mocks.mocks.dart | 23 ++-- 11 files changed, 274 insertions(+), 154 deletions(-) diff --git a/app_dart/lib/src/model/common/presubmit_completed_check.dart b/app_dart/lib/src/model/common/presubmit_completed_check.dart index 8bafb12740..fbe5a9a46d 100644 --- a/app_dart/lib/src/model/common/presubmit_completed_check.dart +++ b/app_dart/lib/src/model/common/presubmit_completed_check.dart @@ -42,6 +42,7 @@ class PresubmitCompletedJob { final String? summary; final int? buildNumber; final Int64? buildId; + final String? author; const PresubmitCompletedJob({ required this.name, @@ -60,6 +61,7 @@ class PresubmitCompletedJob { this.summary, this.buildNumber, this.buildId, + this.author, }); /// Creates a [PresubmitCompletedJob] from a BuildBucket [Build]. @@ -89,6 +91,7 @@ class PresubmitCompletedJob { ].join('\n---\n'), buildNumber: build.number, buildId: build.id, + author: BuildTags.fromStringPairs(build.tags).author, ); } diff --git a/app_dart/lib/src/request_handlers/presubmit_subscription.dart b/app_dart/lib/src/request_handlers/presubmit_subscription.dart index 7bb90a7a9d..6aac26bfe0 100644 --- a/app_dart/lib/src/request_handlers/presubmit_subscription.dart +++ b/app_dart/lib/src/request_handlers/presubmit_subscription.dart @@ -33,7 +33,7 @@ import '../service/scheduler/ci_yaml_fetcher.dart'; /// * Checking remaining build attempts and rescheduling failed builds. /// * Suppressing failing conclusions if a test is marked as suppressed. /// * Updating GitHub Check Run statuses for individual presubmit builds. -/// * Calling [Scheduler.processCheckRunCompleted] to progress CI stages or merge queues. +/// * Calling [Scheduler.processBuildCompleted] to progress CI stages or merge queues. base class PresubmitSubscription extends SubscriptionHandler { /// Creates an endpoint for listening to LUCI status updates. const PresubmitSubscription({ @@ -134,7 +134,7 @@ base class PresubmitSubscription extends SubscriptionHandler { /// /// Evaluates whether a failing task should be automatically retried up to /// [_getMaxAttempt]. If the build is not rescheduled, updates GitHub check - /// run status and notifies [Scheduler.processCheckRunCompleted]. + /// run status and notifies [Scheduler.processBuildCompleted]. Future _processBuild({ required bbv2.Build build, required PresubmitUserData userData, @@ -225,7 +225,7 @@ base class PresubmitSubscription extends SubscriptionHandler { : null, summaryPrepend: suppressedMessage, ); - await _scheduler.processCheckRunCompleted(check); + await _scheduler.processBuildCompleted(check); } } diff --git a/app_dart/lib/src/service/github_checks_service.dart b/app_dart/lib/src/service/github_checks_service.dart index 4496164a01..269f281a20 100644 --- a/app_dart/lib/src/service/github_checks_service.dart +++ b/app_dart/lib/src/service/github_checks_service.dart @@ -11,7 +11,7 @@ import '../model/bbv2_extension.dart'; import 'config.dart'; import 'luci_build_service.dart'; -const String kGithubSummary = ''' +const String kCheckRunHeader = ''' **[Understanding a LUCI build failure](https://github.com/flutter/flutter/blob/master/docs/infra/Understanding-a-LUCI-build-failure.md)** '''; @@ -98,7 +98,7 @@ class GithubChecksService { allFields: true, ), ); - var summary = getGithubSummary(buildbucketBuild.summaryMarkdown); + var summary = getSummary(buildbucketBuild.summaryMarkdown); if (summaryPrepend != null && summaryPrepend.isNotEmpty) { summary = '$summaryPrepend\n\n$summary'; } @@ -122,11 +122,11 @@ class GithubChecksService { /// Appends triage wiki page to `summaryMarkdown` from LUCI build so that people can easily /// reference from github check run page. - String getGithubSummary(String? summary) { - return getGithubSummaryWithHeader(kGithubSummary, summary); + String getSummary(String? summary) { + return getSummaryWithHeader(kCheckRunHeader, summary); } - String getGithubSummaryWithHeader(String header, String? summary) { + String getSummaryWithHeader(String header, String? summary) { if (summary == null) { return '${header}Empty summaryMarkdown'; } diff --git a/app_dart/lib/src/service/luci_build_service.dart b/app_dart/lib/src/service/luci_build_service.dart index 3c15d812a1..03e05d1441 100644 --- a/app_dart/lib/src/service/luci_build_service.dart +++ b/app_dart/lib/src/service/luci_build_service.dart @@ -303,8 +303,8 @@ class LuciBuildService { final checkRuns = []; late PresubmitUserData userData; - // If the unified check run flow is enabled, do not create individual - // check runs for each target but use the guard check run instead. + // In presubmit do not create individual check runs for each target but use + // the guard check run instead. if (dashboardChecks != null) { userData = PresubmitUserData( commit: CommitRef(slug: slug, sha: commitSha, branch: commitBranch), @@ -322,8 +322,7 @@ class LuciBuildService { } for (final MapEntry(key: target, value: attemptNumber) in targets.entries) { - // If the unified check run flow is disabled create individual check runs - // for each target. + // In merge queue create individual check runs for each target. if (dashboardChecks == null) { final checkRun = await _githubChecksUtil.createCheckRun( _config, @@ -393,22 +392,17 @@ class LuciBuildService { cipdVersion: cipdVersion, userData: userData, properties: properties, - // if unified check run flow is enabled, use guard check run othervise check run id. - tags: dashboardChecks != null - ? BuildTags([ - GuardCheckRunIdBuildTag( - guardCheckRunId: dashboardChecks.id!, - ), - if (attemptNumber > 1) - CurrentAttemptBuildTag(attemptNumber: attemptNumber), - if (isOrderedPresubmit) - OrderingKeyTag(orderingKey: pullRequest.head!.sha!), - ]) - : BuildTags([ - GitHubCheckRunIdBuildTag(checkRunId: userData.checkRunId!), - if (isOrderedPresubmit) - OrderingKeyTag(orderingKey: pullRequest.head!.sha!), - ]), + // In merge queue use check run id othervise guard check run. + tags: BuildTags([ + if (pullRequest.user?.login != null) + AuthorBuildTag(value: pullRequest.user!.login!), + if (dashboardChecks != null) + GuardCheckRunIdBuildTag(guardCheckRunId: dashboardChecks.id!), + if (attemptNumber > 1) + CurrentAttemptBuildTag(attemptNumber: attemptNumber), + if (isOrderedPresubmit) + OrderingKeyTag(orderingKey: pullRequest.head!.sha!), + ]), dimensions: requestedDimensions, ), ), @@ -445,52 +439,48 @@ class LuciBuildService { // Set the presubmit check run status to `CheckRunStatus.inProgress` if // Re-run all Failed Jobs. - final isRerun = targets.values.first > 1; - if (isRerun && stage != null && dashboardChecks != null) { - try { - final presubmitGuardDoc = await _firestore.getDocument( - PresubmitGuard.documentNameFor( - slug: slug, - prNum: pullRequest.number!, - checkRunId: dashboardChecks.id!, - stage: stage, - ), - ); - final guard = PresubmitGuard.fromDocument(presubmitGuardDoc); - final checkRun = guard.checkRun; - - if (guard.failedJobs == 0) { - log.info('Re-requesting presubmit check run for Guard $guard'); - // final checks = await _githubChecksUtil.allCheckRuns( - // _config, - // slug, - // checkRun.checkSuiteId!, - // ); - // log.info('Found check runs: ${checks.keys.join(', ')}'); - // final presubmitChecks = checks[Config.kPresubmitCheckName]!; - - await _githubChecksUtil.createCheckRun( - _config, - slug, - checkRun.headSha!, - Config.kPresubmitCheckName, - output: const CheckRunOutput( - title: Config.kPresubmitCheckName, - summary: Scheduler.kPresubmitCheckDescription, + if (pullRequest.user?.login != null && + _config.flags.isResetFailedCheckRunEnabledForUser( + pullRequest.user!.login!, + )) { + final isRerun = targets.values.first > 1; + if (isRerun && stage != null && dashboardChecks != null) { + try { + final presubmitGuardDoc = await _firestore.getDocument( + PresubmitGuard.documentNameFor( + slug: slug, + prNum: pullRequest.number!, + checkRunId: dashboardChecks.id!, + stage: stage, ), - detailsUrl: checkRun.detailsUrl, + ); + final guard = PresubmitGuard.fromDocument(presubmitGuardDoc); + final checkRun = guard.checkRun; + + if (guard.failedJobs == 0) { + log.info('Re-creating Presubmit check run for Guard $guard'); + await _githubChecksUtil.createCheckRun( + _config, + slug, + checkRun.headSha!, + Config.kPresubmitCheckName, + output: const CheckRunOutput( + title: Config.kPresubmitCheckName, + summary: Scheduler.kPresubmitCheckDescription, + ), + detailsUrl: checkRun.detailsUrl, + ); + } + } catch (e, s) { + // We are not going to block on this error. + log.warn( + 'Failed to re-create Presubmit check run for PR# ${pullRequest.number}', + e, + s, ); } - } catch (e, s) { - // We are not going to block on this error. - log.warn( - 'Failed to re-request dashboard checks for PR# ${pullRequest.number}', - e, - s, - ); } } - return targets.keys.toList(); } diff --git a/app_dart/lib/src/service/luci_build_service/build_tags.dart b/app_dart/lib/src/service/luci_build_service/build_tags.dart index 2e5c374702..cf210e3cd9 100644 --- a/app_dart/lib/src/service/luci_build_service/build_tags.dart +++ b/app_dart/lib/src/service/luci_build_service/build_tags.dart @@ -89,6 +89,12 @@ final class BuildTags { final prTag = getTagOfType(); return prTag!.pullRequestNumber; } + + /// GitHub Pull Request Author + String? get author { + final tag = getTagOfType(); + return tag?.value; + } } /// Valid tags for [bbv2.ScheduleBuildRequest.tags]. @@ -170,6 +176,8 @@ sealed class BuildTag { return TriggerdByBuildTag(email: pair.value); case OrderingKeyTag._keyName: return OrderingKeyTag(orderingKey: pair.value); + case AuthorBuildTag._keyName: + return AuthorBuildTag(value: pair.value); } return UnknownBuildTag(key: pair.key, value: pair.value); } @@ -243,6 +251,17 @@ final class UserAgentBuildTag extends BuildTag { final String value; } +/// The author of the commit that triggered the build. +final class AuthorBuildTag extends BuildTag { + static const _keyName = 'author'; + + AuthorBuildTag({required this.value}) : super(_keyName, value); + + /// Name of the author. + final String value; +} + + /// Groups builds together, i.e. by a (Gerrit) CL, (GitHub) PR or (Git) commit. sealed class BuildSetBuildTag extends BuildTag { static const _keyName = 'buildset'; diff --git a/app_dart/lib/src/service/scheduler.dart b/app_dart/lib/src/service/scheduler.dart index 34b957862b..f35263be26 100644 --- a/app_dart/lib/src/service/scheduler.dart +++ b/app_dart/lib/src/service/scheduler.dart @@ -382,7 +382,12 @@ class Scheduler { sha, detailsUrl: 'https://flutter-dashboard.appspot.com/#/presubmit?repo=${slug.name}&sha=$sha', - isPresubmit: true, + isMergeQueue: false, + isResetFailedCheckRunEnabled: + pullRequest.user?.login != null && + _config.flags.isResetFailedCheckRunEnabledForUser( + pullRequest.user!.login!, + ), ); final dashboardChecks = lockResult.dashboardChecks; final mergeQueueGuard = lockResult.mergeQueueGuard; @@ -651,7 +656,8 @@ class Scheduler { final lockResult = await lockMergeGroupChecks( slug, headSha, - isPresubmit: false, + isMergeQueue: true, + isResetFailedCheckRunEnabled: false, ); final dashboardChecks = lockResult.dashboardChecks; final mergeQueueGuard = lockResult.mergeQueueGuard!; @@ -864,7 +870,8 @@ $s RepositorySlug slug, String headSha, { String? detailsUrl, - required bool isPresubmit, + required bool isMergeQueue, + required bool isResetFailedCheckRunEnabled, }) async { final mergeQueueGuard = await _githubChecksService.githubChecksUtil .createCheckRun( @@ -876,9 +883,8 @@ $s title: Config.kMergeQueueLockName, summary: kMergeQueueLockDescription, ), - detailsUrl: isPresubmit ? null : detailsUrl, ); - + // TODO(ievdokdm): Remove dashboard check for merge groups. final dashboardChecks = await _githubChecksService.githubChecksUtil .createCheckRun( _config, @@ -889,9 +895,19 @@ $s title: Config.kDashboardCheckName, summary: kDashboardChecksDescription, ), - detailsUrl: isPresubmit ? detailsUrl : null, + detailsUrl: isMergeQueue ? null : detailsUrl, ); - if (isPresubmit) { + + if (isMergeQueue) { + // Skip Dashboard Checks + await _githubChecksService.githubChecksUtil.updateCheckRun( + _config, + slug, + dashboardChecks, + status: CheckRunStatus.completed, + conclusion: CheckRunConclusion.success, + ); + } else if (isResetFailedCheckRunEnabled) { await _githubChecksService.githubChecksUtil.createCheckRun( _config, slug, @@ -903,15 +919,6 @@ $s ), detailsUrl: detailsUrl, ); - } else { - // Skip Dashboard Checks - await _githubChecksService.githubChecksUtil.updateCheckRun( - _config, - slug, - dashboardChecks, - status: CheckRunStatus.completed, - conclusion: CheckRunConclusion.success, - ); } return CheckRunLockResult( dashboardChecks: dashboardChecks, @@ -1021,6 +1028,44 @@ $s ); } + Future _requireActionForDashboardChecks({ + required RepositorySlug slug, + required CheckRun lock, + required String headSha, + required String summary, + required String details, + String? detailsUrl, + }) async { + log.info(''' +Require action for merge group guard ${lock.id} for: +head sha: $headSha +slug: $slug +summary: $summary +details: $details +detailsUrl: $detailsUrl +'''); + await _githubChecksService.githubChecksUtil.updateCheckRun( + _config, + slug, + lock, + status: CheckRunStatus.completed, + conclusion: CheckRunConclusion.actionRequired, + output: CheckRunOutput( + title: Config.kDashboardCheckName, + summary: summary, + text: details, + ), + detailsUrl: detailsUrl, + actions: [ + const CheckRunAction( + label: 'Re-run Failed', + description: 'Re-run failed tests', + identifier: 're_run_failed', + ), + ], + ); + } + Future _requireActionForPresubmit({ required RepositorySlug slug, required int checkSuiteId, @@ -1046,12 +1091,12 @@ defined in: checkSuiteId, ); log.info('Found check runs: ${checks.keys.join(', ')}'); - final presubmitChecks = checks[Config.kPresubmitCheckName]!; + final presubmitCheck = checks[Config.kPresubmitCheckName]!; await _githubChecksService.githubChecksUtil.updateCheckRun( _config, slug, - presubmitChecks, + presubmitCheck, status: CheckRunStatus.completed, conclusion: CheckRunConclusion.actionRequired, output: CheckRunOutput( @@ -1136,11 +1181,11 @@ defined in: return getTargetsToRun(presubmitTargets, filesChanged); } - /// Process a completed GitHub `check_run`. + /// Process a completed LUCI Build. /// /// Handles both fusion engine build and test stages, and both pull requests /// and merge groups. - Future processCheckRunCompleted(PresubmitCompletedJob check) async { + Future processBuildCompleted(PresubmitCompletedJob check) async { if (kCheckRunsToIgnore.contains(check.name)) { return true; } @@ -1253,18 +1298,33 @@ defined in: final guard = checkRunFromString(stagingConclusion.dashboardChecks!); final detailsUrl = 'https://flutter-dashboard.appspot.com/#/presubmit?repo=${check.slug.name}&sha=${check.sha}'; - await _requireActionForPresubmit( - slug: check.slug, - checkSuiteId: guard.checkSuiteId!, - headSha: check.sha, - summary: _githubChecksService.getGithubSummaryWithHeader(''' + final summary = _githubChecksService.getSummaryWithHeader(''' **[Failed Presubmit Jobs Details]($detailsUrl)** - -''', kDashboardChecksDescription), - details: - 'Failed presubmit jobs:\n${stagingConclusion.failedJobNames.map((name) => "- `$name`").join("\n")}', - detailsUrl: detailsUrl, - ); +''', kDashboardChecksDescription); + final details = + 'Failed presubmit jobs:\n${stagingConclusion.failedJobNames.map((name) => "- `$name`").join("\n")}'; + // Require action for Dashboard Checks check run or Presubmit check run + // 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, + ); + } else { + await _requireActionForDashboardChecks( + slug: check.slug, + lock: guard, + headSha: check.sha, + summary: summary, + details: details, + detailsUrl: detailsUrl, + ); + } } return true; } @@ -1819,6 +1879,7 @@ $stacktrace /// filters them by [names], and schedules them. /// /// It handles both fusion and non-fusion repositories. + @Deprecated('after unified check run flow is enabled') Future reRunTargets( RepositorySlug slug, PullRequest pullRequest, diff --git a/app_dart/test/request_handlers/presubmit_luci_subscription_test.dart b/app_dart/test/request_handlers/presubmit_luci_subscription_test.dart index dd8d6f6a17..ec0ac75570 100644 --- a/app_dart/test/request_handlers/presubmit_luci_subscription_test.dart +++ b/app_dart/test/request_handlers/presubmit_luci_subscription_test.dart @@ -91,7 +91,7 @@ void main() { mockGithubChecksService.conclusionForResult(any), ).thenAnswer((_) => github.CheckRunConclusion.empty); when( - mockScheduler.processCheckRunCompleted(any), + mockScheduler.processBuildCompleted(any), ).thenAnswer((_) async => true); tester.message = createPushMessage( @@ -120,7 +120,7 @@ void main() { ), ).called(1); - verify(mockScheduler.processCheckRunCompleted(any)).called(1); + verify(mockScheduler.processBuildCompleted(any)).called(1); }); test( @@ -162,7 +162,7 @@ void main() { rescheduled: true, ), ).called(1); - verifyNever(mockScheduler.processCheckRunCompleted(any)); + verifyNever(mockScheduler.processBuildCompleted(any)); }, ); @@ -245,7 +245,7 @@ void main() { rescheduled: true, ), ).called(1); - verifyNever(mockScheduler.processCheckRunCompleted(any)); + verifyNever(mockScheduler.processBuildCompleted(any)); }); test( @@ -265,7 +265,7 @@ void main() { mockGithubChecksService.conclusionForResult(any), ).thenAnswer((_) => github.CheckRunConclusion.empty); when( - mockScheduler.processCheckRunCompleted(any), + mockScheduler.processBuildCompleted(any), ).thenAnswer((_) async => true); final userData = PresubmitUserData( @@ -311,7 +311,7 @@ void main() { ), ).called(1); - verify(mockScheduler.processCheckRunCompleted(any)).called(1); + verify(mockScheduler.processBuildCompleted(any)).called(1); }, ); @@ -332,7 +332,7 @@ void main() { mockGithubChecksService.conclusionForResult(any), ).thenAnswer((_) => github.CheckRunConclusion.empty); when( - mockScheduler.processCheckRunCompleted(any), + mockScheduler.processBuildCompleted(any), ).thenAnswer((_) async => true); final userData = PresubmitUserData( @@ -376,7 +376,7 @@ void main() { rescheduled: false, ), ).called(1); - verify(mockScheduler.processCheckRunCompleted(any)).called(1); + verify(mockScheduler.processBuildCompleted(any)).called(1); }, ); @@ -485,7 +485,7 @@ void main() { // Check that the build.input.properties extracted from build_large_fields // contains the git_ref property encoded in the test data. expect(build.input.properties.fields, contains('git_ref')); - verifyNever(mockScheduler.processCheckRunCompleted(any)); + verifyNever(mockScheduler.processBuildCompleted(any)); }); test('Close the MQ guard once presubmit compleated', () async { @@ -518,13 +518,13 @@ void main() { mockGithubChecksService.conclusionForResult(bbv2.Status.SUCCESS), ).thenAnswer((_) => github.CheckRunConclusion.success); when( - mockScheduler.processCheckRunCompleted(any), + mockScheduler.processBuildCompleted(any), ).thenAnswer((_) async => true); await tester.post(handler); final captured = verify( - mockScheduler.processCheckRunCompleted(captureAny), + mockScheduler.processBuildCompleted(captureAny), ).captured; expect(captured, hasLength(1)); expect( @@ -580,7 +580,7 @@ void main() { ).thenAnswer((_) async => true); when( - mockScheduler.processCheckRunCompleted(any), + mockScheduler.processBuildCompleted(any), ).thenAnswer((_) async => true); tester.message = createPushMessage( @@ -611,7 +611,7 @@ void main() { ).called(1); final captured = verify( - mockScheduler.processCheckRunCompleted(captureAny), + mockScheduler.processBuildCompleted(captureAny), ).captured; expect(captured, hasLength(1)); expect( @@ -663,7 +663,7 @@ void main() { ); when( - mockScheduler.processCheckRunCompleted(any), + mockScheduler.processBuildCompleted(any), ).thenAnswer((_) async => true); tester.message = createPushMessage( @@ -687,7 +687,7 @@ void main() { ); final captured = verify( - mockScheduler.processCheckRunCompleted(captureAny), + mockScheduler.processBuildCompleted(captureAny), ).captured; expect(captured, hasLength(1)); expect( @@ -726,7 +726,7 @@ void main() { ); when( - mockScheduler.processCheckRunCompleted(any), + mockScheduler.processBuildCompleted(any), ).thenAnswer((_) async => true); tester.message = createPushMessage( @@ -750,7 +750,7 @@ void main() { ); final captured = verify( - mockScheduler.processCheckRunCompleted(captureAny), + mockScheduler.processBuildCompleted(captureAny), ).captured; expect(captured, hasLength(1)); expect( @@ -858,7 +858,7 @@ void main() { ), ); - verifyNever(mockScheduler.processCheckRunCompleted(any)); + verifyNever(mockScheduler.processBuildCompleted(any)); final updatedCurrentJob = await PresubmitJob.fromFirestore( firestore, diff --git a/app_dart/test/request_handlers/presubmit_ordered_subscription_test.dart b/app_dart/test/request_handlers/presubmit_ordered_subscription_test.dart index 13efb12e0d..8e3f4ba1d6 100644 --- a/app_dart/test/request_handlers/presubmit_ordered_subscription_test.dart +++ b/app_dart/test/request_handlers/presubmit_ordered_subscription_test.dart @@ -81,7 +81,7 @@ void main() { mockGithubChecksService.conclusionForResult(any), ).thenAnswer((_) => github.CheckRunConclusion.empty); when( - mockScheduler.processCheckRunCompleted(any), + mockScheduler.processBuildCompleted(any), ).thenAnswer((_) async => true); tester.message = createPushMessage( @@ -105,7 +105,7 @@ void main() { final response = await tester.post(handler); expect(response, Response.emptyOk); - verify(mockScheduler.processCheckRunCompleted(any)).called(1); + verify(mockScheduler.processBuildCompleted(any)).called(1); }, ); } diff --git a/app_dart/test/service/github_checks_service_test.dart b/app_dart/test/service/github_checks_service_test.dart index 61d9d88319..b1feeaddbd 100644 --- a/app_dart/test/service/github_checks_service_test.dart +++ b/app_dart/test/service/github_checks_service_test.dart @@ -197,16 +197,16 @@ void main() { group('getGithubSummary', () { test('nonempty summaryMarkdown', () async { const summaryMarkdown = 'test'; - const expectedSummary = '$kGithubSummary$summaryMarkdown'; + const expectedSummary = '$kCheckRunHeader$summaryMarkdown'; expect( - githubChecksService.getGithubSummary(summaryMarkdown), + githubChecksService.getSummary(summaryMarkdown), expectedSummary, ); }); test('empty summaryMarkdown', () async { - const expectedSummary = '${kGithubSummary}Empty summaryMarkdown'; - expect(githubChecksService.getGithubSummary(null), expectedSummary); + const expectedSummary = '${kCheckRunHeader}Empty summaryMarkdown'; + expect(githubChecksService.getSummary(null), expectedSummary); }); test('really large summaryMarkdown', () async { @@ -215,11 +215,11 @@ void main() { summaryMarkdown += 'test '; } expect( - githubChecksService.getGithubSummary(summaryMarkdown), - startsWith('$kGithubSummary[TRUNCATED...]'), + githubChecksService.getSummary(summaryMarkdown), + startsWith('$kCheckRunHeader[TRUNCATED...]'), ); expect( - githubChecksService.getGithubSummary(summaryMarkdown).length, + githubChecksService.getSummary(summaryMarkdown).length, lessThan(65535), ); }); diff --git a/app_dart/test/service/scheduler_test.dart b/app_dart/test/service/scheduler_test.dart index 7a71fc4237..f844da2044 100644 --- a/app_dart/test/service/scheduler_test.dart +++ b/app_dart/test/service/scheduler_test.dart @@ -1614,7 +1614,7 @@ targets: () async { for (final ignored in Scheduler.kCheckRunsToIgnore) { expect( - await scheduler.processCheckRunCompleted( + await scheduler.processBuildCompleted( PresubmitCompletedJob( name: ignored, sha: 'abc123', @@ -2029,7 +2029,7 @@ targets: ); test( - 'does not close Merge Queue Guard immediately for unified check run flow', + 'create Presubmit check-run when not in merge queue and resetFailedCheckRun is enabled', () async { when( mockGithubChecksUtil.createCheckRun( @@ -2051,21 +2051,20 @@ targets: final lockResult = await scheduler.lockMergeGroupChecks( Config.flutterSlug, 'sha123', - isPresubmit: true, + isMergeQueue: false, + isResetFailedCheckRunEnabled: true, ); expect(lockResult.dashboardChecks.name, Config.kDashboardCheckName); - expect(lockResult.mergeQueueGuard?.name, Config.kMergeQueueLockName); - verifyNever( - mockGithubChecksUtil.updateCheckRun( + verify( + mockGithubChecksUtil.createCheckRun( any, any, any, - status: CheckRunStatus.completed, - conclusion: CheckRunConclusion.success, + Config.kPresubmitCheckName, output: anyNamed('output'), - actions: anyNamed('actions'), + conclusion: anyNamed('conclusion'), detailsUrl: anyNamed('detailsUrl'), ), ); @@ -2539,6 +2538,47 @@ targets: }); group('merge groups', () { + test('close Dashboard Checks immediately for merge groups', () async { + when( + mockGithubChecksUtil.createCheckRun( + any, + any, + any, + any, + output: anyNamed('output'), + conclusion: anyNamed('conclusion'), + detailsUrl: anyNamed('detailsUrl'), + ), + ).thenAnswer((Invocation invocation) async { + return generateCheckRun( + invocation.positionalArguments[2].hashCode, + name: invocation.positionalArguments[3] as String, + ); + }); + + final lockResult = await scheduler.lockMergeGroupChecks( + Config.flutterSlug, + 'sha123', + isMergeQueue: true, + isResetFailedCheckRunEnabled: true, + ); + + expect(lockResult.dashboardChecks.name, Config.kDashboardCheckName); + expect(lockResult.mergeQueueGuard?.name, Config.kMergeQueueLockName); + + verify( + mockGithubChecksUtil.updateCheckRun( + any, + any, + any, + status: CheckRunStatus.completed, + conclusion: CheckRunConclusion.success, + output: anyNamed('output'), + actions: anyNamed('actions'), + detailsUrl: anyNamed('detailsUrl'), + ), + ); + }); test('schedule some work on prod', () async { ciYamlFetcher.setCiYamlFrom(singleCiYaml, engine: fusionDualCiYaml); final luci = MockLuciBuildService(); @@ -3467,7 +3507,7 @@ targets: ); expect( - await scheduler.processCheckRunCompleted(linuxCompleted), + await scheduler.processBuildCompleted(linuxCompleted), isFalse, ); verifyNever( @@ -3501,7 +3541,7 @@ targets: ); expect( - await scheduler.processCheckRunCompleted(macCompleted), + await scheduler.processBuildCompleted(macCompleted), isTrue, ); verify( @@ -3675,7 +3715,7 @@ targets: ), ); expect( - await scheduler.processCheckRunCompleted(linuxFailed), + await scheduler.processBuildCompleted(linuxFailed), isFalse, ); @@ -3698,7 +3738,7 @@ targets: ), ); expect( - await scheduler.processCheckRunCompleted(macSucceeded), + await scheduler.processBuildCompleted(macSucceeded), isTrue, ); @@ -4112,7 +4152,7 @@ targets: final check = PresubmitCompletedJob.fromBuild(build, userData); - expect(await scheduler.processCheckRunCompleted(check), isTrue); + expect(await scheduler.processBuildCompleted(check), isTrue); // Should schedule tests for the next stage (fusionTests) expect(fakeLuciBuildService.scheduledTryBuilds, isNotEmpty); @@ -4186,7 +4226,7 @@ targets: final check = PresubmitCompletedJob.fromBuild(build, userData); - expect(await scheduler.processCheckRunCompleted(check), isTrue); + expect(await scheduler.processBuildCompleted(check), isTrue); verify( mockGithubChecksUtil.updateCheckRun( @@ -4296,7 +4336,7 @@ targets: final check = PresubmitCompletedJob.fromBuild(build, userData); - expect(await scheduler.processCheckRunCompleted(check), isTrue); + expect(await scheduler.processBuildCompleted(check), isTrue); final verification = verify( mockGithubChecksUtil.updateCheckRun( @@ -4375,7 +4415,7 @@ targets: // First test succeeds: merge queue guard remains locked while 'Mac test' is still pending. expect( - await scheduler.processCheckRunCompleted( + await scheduler.processBuildCompleted( PresubmitCompletedJob.fromBuild(linuxBuild, userData), ), isFalse, @@ -4406,7 +4446,7 @@ targets: // Second (final) test succeeds: all tests succeeded, so merge queue guard is unlocked. expect( - await scheduler.processCheckRunCompleted( + await scheduler.processBuildCompleted( PresubmitCompletedJob.fromBuild(macBuild, userData), ), isTrue, @@ -4514,7 +4554,7 @@ targets: final check = PresubmitCompletedJob.fromBuild(build, userData); - expect(await scheduler.processCheckRunCompleted(check), isTrue); + expect(await scheduler.processBuildCompleted(check), isTrue); verify( mockGithubChecksUtil.updateCheckRun( diff --git a/packages/cocoon_integration_test/lib/src/utilities/mocks.mocks.dart b/packages/cocoon_integration_test/lib/src/utilities/mocks.mocks.dart index 394d9ee538..c264f2e56a 100644 --- a/packages/cocoon_integration_test/lib/src/utilities/mocks.mocks.dart +++ b/packages/cocoon_integration_test/lib/src/utilities/mocks.mocks.dart @@ -1914,7 +1914,7 @@ class MockGithubChecksService extends _i1.Mock as _i13.Future); @override - String getGithubSummary(String? summary) => + String getSummary(String? summary) => (super.noSuchMethod( Invocation.method(#getGithubSummary, [summary]), returnValue: _i19.dummyValue( @@ -1925,7 +1925,7 @@ class MockGithubChecksService extends _i1.Mock as String); @override - String getGithubSummaryWithHeader(String? header, String? summary) => + String getSummaryWithHeader(String? header, String? summary) => (super.noSuchMethod( Invocation.method(#getGithubSummaryWithHeader, [header, summary]), returnValue: _i19.dummyValue( @@ -5733,13 +5733,18 @@ class MockScheduler extends _i1.Mock implements _i2.Scheduler { _i7.RepositorySlug? slug, String? headSha, { String? detailsUrl, - required bool? isPresubmit, + required bool? isMergeQueue, + required bool? isResetFailedCheckRunEnabled, }) => (super.noSuchMethod( Invocation.method( #lockMergeGroupChecks, [slug, headSha], - {#detailsUrl: detailsUrl, #isPresubmit: isPresubmit}, + { + #detailsUrl: detailsUrl, + #isMergeQueue: isMergeQueue, + #isResetFailedCheckRunEnabled: isResetFailedCheckRunEnabled, + }, ), returnValue: _i13.Future<_i2.CheckRunLockResult>.value( _FakeCheckRunLockResult_58( @@ -5747,7 +5752,11 @@ class MockScheduler extends _i1.Mock implements _i2.Scheduler { Invocation.method( #lockMergeGroupChecks, [slug, headSha], - {#detailsUrl: detailsUrl, #isPresubmit: isPresubmit}, + { + #detailsUrl: detailsUrl, + #isMergeQueue: isMergeQueue, + #isResetFailedCheckRunEnabled: isResetFailedCheckRunEnabled, + }, ), ), ), @@ -5849,9 +5858,7 @@ class MockScheduler extends _i1.Mock implements _i2.Scheduler { as _i13.Future>); @override - _i13.Future processCheckRunCompleted( - _i38.PresubmitCompletedJob? check, - ) => + _i13.Future processBuildCompleted(_i38.PresubmitCompletedJob? check) => (super.noSuchMethod( Invocation.method(#processCheckRunCompleted, [check]), returnValue: _i13.Future.value(false), From f1c5b8cfb40194589235436bec38230623953df4 Mon Sep 17 00:00:00 2001 From: "Dmitry Grand (dmgr)" Date: Thu, 24 Sep 2026 16:31:50 -0700 Subject: [PATCH 3/7] unit tests --- .../lib/src/service/luci_build_service.dart | 6 +- .../luci_build_service/build_tags.dart | 1 - app_dart/lib/src/service/scheduler.dart | 81 +++- .../service/github_checks_service_test.dart | 5 +- .../schedule_try_builds_test.dart | 173 ++++++++ app_dart/test/service/scheduler_test.dart | 399 +++++++++++++++++- 6 files changed, 625 insertions(+), 40 deletions(-) diff --git a/app_dart/lib/src/service/luci_build_service.dart b/app_dart/lib/src/service/luci_build_service.dart index 03e05d1441..eb27325592 100644 --- a/app_dart/lib/src/service/luci_build_service.dart +++ b/app_dart/lib/src/service/luci_build_service.dart @@ -303,7 +303,7 @@ class LuciBuildService { final checkRuns = []; late PresubmitUserData userData; - // In presubmit do not create individual check runs for each target but use + // In presubmit do not create individual check runs for each target but use // the guard check run instead. if (dashboardChecks != null) { userData = PresubmitUserData( @@ -437,7 +437,7 @@ class LuciBuildService { ); } - // Set the presubmit check run status to `CheckRunStatus.inProgress` if + // Set the presubmit check run status to `CheckRunStatus.inProgress` if // Re-run all Failed Jobs. if (pullRequest.user?.login != null && _config.flags.isResetFailedCheckRunEnabledForUser( @@ -462,7 +462,7 @@ class LuciBuildService { await _githubChecksUtil.createCheckRun( _config, slug, - checkRun.headSha!, + checkRun.headSha ?? commitSha, Config.kPresubmitCheckName, output: const CheckRunOutput( title: Config.kPresubmitCheckName, diff --git a/app_dart/lib/src/service/luci_build_service/build_tags.dart b/app_dart/lib/src/service/luci_build_service/build_tags.dart index cf210e3cd9..c023c30749 100644 --- a/app_dart/lib/src/service/luci_build_service/build_tags.dart +++ b/app_dart/lib/src/service/luci_build_service/build_tags.dart @@ -261,7 +261,6 @@ final class AuthorBuildTag extends BuildTag { final String value; } - /// Groups builds together, i.e. by a (Gerrit) CL, (GitHub) PR or (Git) commit. sealed class BuildSetBuildTag extends BuildTag { static const _keyName = 'buildset'; diff --git a/app_dart/lib/src/service/scheduler.dart b/app_dart/lib/src/service/scheduler.dart index f35263be26..9195d5e153 100644 --- a/app_dart/lib/src/service/scheduler.dart +++ b/app_dart/lib/src/service/scheduler.dart @@ -391,6 +391,7 @@ class Scheduler { ); final dashboardChecks = lockResult.dashboardChecks; final mergeQueueGuard = lockResult.mergeQueueGuard; + final presubmit = lockResult.presubmit; // Track if we should unlock the merge group lock in case of non-fusion or // revert bots. @@ -535,6 +536,9 @@ class Scheduler { if (mergeQueueGuard != null) { await unlockMergeQueueGuard(slug, sha, mergeQueueGuard); } + if (presubmit != null) { + await unlockMergeQueueGuard(slug, sha, presubmit); + } } log.info( 'Finished triggering builds for: pr ${pullRequest.number}, commit $sha, branch ${pullRequest.head!.ref} and slug $slug}', @@ -898,6 +902,7 @@ $s detailsUrl: isMergeQueue ? null : detailsUrl, ); + CheckRun? presubmit; if (isMergeQueue) { // Skip Dashboard Checks await _githubChecksService.githubChecksUtil.updateCheckRun( @@ -908,7 +913,7 @@ $s conclusion: CheckRunConclusion.success, ); } else if (isResetFailedCheckRunEnabled) { - await _githubChecksService.githubChecksUtil.createCheckRun( + presubmit = await _githubChecksService.githubChecksUtil.createCheckRun( _config, slug, headSha, @@ -923,6 +928,7 @@ $s return CheckRunLockResult( dashboardChecks: dashboardChecks, mergeQueueGuard: mergeQueueGuard, + presubmit: presubmit, ); } @@ -1091,7 +1097,13 @@ defined in: checkSuiteId, ); log.info('Found check runs: ${checks.keys.join(', ')}'); - final presubmitCheck = checks[Config.kPresubmitCheckName]!; + final presubmitCheck = checks[Config.kPresubmitCheckName]; + if (presubmitCheck == null) { + log.warn( + '${Config.kPresubmitCheckName} check run not found in check suite $checkSuiteId for $slug/$headSha', + ); + return; + } await _githubChecksService.githubChecksUtil.updateCheckRun( _config, @@ -1268,22 +1280,17 @@ defined in: return false; } - // Are there tests remaining? Keep waiting. - if (stagingConclusion.isPending) { - log.info( - '$logCrumb: not progressing, remaining work count: ${stagingConclusion.remaining}', - ); - return false; - } - if (stagingConclusion.isFailed) { // Something failed in the current CI stage: // - // * If this is a pull request: keep the merge guard open and do not proceed - // to the next stage. Let the author sort out what's up. // * If this is a merge group: kick the pull request out of the queue, and // let the author sort it out. - // If its a unified check run we need to require action on the guard. + // * If this is a pull request and resetFailedCheckRun flag is enabled: + // keep the `Merge Queue Guard` and `Dashboard Checks` open and require + // action on the `Presubmit` check run. + // * If this is a pull request and resetFailedCheckRun flag is disabled: + // keep the `Merge Queue Guard` open and require action on the + // `Dashboard Checks` check run. if (check.isMergeGroup) { await _completeArtifacts(check.sha, false); final guard = checkRunFromString(stagingConclusion.mergeQueueGuard!); @@ -1329,6 +1336,13 @@ defined in: return true; } + // Are there tests remaining? Keep waiting. + if (stagingConclusion.isPending) { + log.info( + '$logCrumb: not progressing, remaining work count: ${stagingConclusion.remaining}', + ); + return false; + } // The logic for finishing a stage is different between build and test stages: // // * If this is a build stage, then: @@ -1439,11 +1453,25 @@ defined in: } } else { if (dashboardChecks != null) { - await unlockMergeQueueGuard( - check.slug, - check.sha, - checkRunFromString(dashboardChecks), - ); + final dashboardCheckRun = checkRunFromString(dashboardChecks); + await unlockMergeQueueGuard(check.slug, check.sha, dashboardCheckRun); + if (check.author != null && + _config.flags.isResetFailedCheckRunEnabledForUser(check.author!)) { + final checkSuiteId = + check.checkSuiteId ?? dashboardCheckRun.checkSuiteId; + if (checkSuiteId != null) { + final checks = await _githubChecksService.githubChecksUtil + .allCheckRuns(_config, check.slug, checkSuiteId); + final presubmitCheck = checks[Config.kPresubmitCheckName]; + if (presubmitCheck != null) { + await unlockMergeQueueGuard( + check.slug, + check.sha, + presubmitCheck, + ); + } + } + } } if (mergeQueueGuard != null) { await unlockMergeQueueGuard( @@ -1770,6 +1798,7 @@ $stacktrace switch (name) { case Config.kMergeQueueLockName: case Config.kDashboardCheckName: + case Config.kPresubmitCheckName: final checkSuiteId = checkRunEvent.checkRun!.checkSuite!.id!; log.debug( '$logCrumb: Requested re-run of "$name" for ' @@ -1964,11 +1993,23 @@ $stacktrace checkRunEvent.checkRun!.checkSuite!.id!, ); + var guardCheckRunId = checkRunEvent.checkRun!.id!; + if (checkRunEvent.checkRun!.name == Config.kPresubmitCheckName) { + final guard = await UnifiedCheckRun.getLatestPresubmitGuardForPrNum( + firestoreService: _firestore, + slug: slug, + prNum: pullRequest!.number!, + ); + if (guard != null) { + guardCheckRunId = guard.checkRunId; + } + } + final failedChecks = await UnifiedCheckRun.reInitializeFailedJobs( firestoreService: _firestore, slug: slug, prNum: pullRequest!.number!, - guardCheckRunId: checkRunEvent.checkRun!.id!, + guardCheckRunId: guardCheckRunId, ); if (failedChecks == null || failedChecks.jobRetries.isEmpty) { @@ -2132,9 +2173,11 @@ enum _TaskCommitScheduling { class CheckRunLockResult { final CheckRun dashboardChecks; final CheckRun? mergeQueueGuard; + final CheckRun? presubmit; const CheckRunLockResult({ required this.dashboardChecks, this.mergeQueueGuard, + this.presubmit, }); } diff --git a/app_dart/test/service/github_checks_service_test.dart b/app_dart/test/service/github_checks_service_test.dart index b1feeaddbd..9aafa82452 100644 --- a/app_dart/test/service/github_checks_service_test.dart +++ b/app_dart/test/service/github_checks_service_test.dart @@ -198,10 +198,7 @@ void main() { test('nonempty summaryMarkdown', () async { const summaryMarkdown = 'test'; const expectedSummary = '$kCheckRunHeader$summaryMarkdown'; - expect( - githubChecksService.getSummary(summaryMarkdown), - expectedSummary, - ); + expect(githubChecksService.getSummary(summaryMarkdown), expectedSummary); }); test('empty summaryMarkdown', () async { diff --git a/app_dart/test/service/luci_build_service/schedule_try_builds_test.dart b/app_dart/test/service/luci_build_service/schedule_try_builds_test.dart index 7762693c18..7e829fd610 100644 --- a/app_dart/test/service/luci_build_service/schedule_try_builds_test.dart +++ b/app_dart/test/service/luci_build_service/schedule_try_builds_test.dart @@ -13,13 +13,16 @@ import 'package:cocoon_service/src/model/firestore/base.dart'; import 'package:cocoon_service/src/model/firestore/pr_check_runs.dart'; import 'package:cocoon_service/src/model/firestore/presubmit_guard.dart'; import 'package:cocoon_service/src/service/cache_service.dart'; +import 'package:cocoon_service/src/service/config.dart'; import 'package:cocoon_service/src/service/firestore.dart'; import 'package:cocoon_service/src/service/flags/dynamic_config.dart'; import 'package:cocoon_service/src/service/flags/ordered_presubmit_flags.dart'; +import 'package:cocoon_service/src/service/flags/reset_failed_check_run.dart'; import 'package:cocoon_service/src/service/luci_build_service.dart'; import 'package:cocoon_service/src/service/luci_build_service/build_tags.dart'; import 'package:cocoon_service/src/service/luci_build_service/engine_artifacts.dart'; import 'package:cocoon_service/src/service/luci_build_service/user_data.dart'; +import 'package:cocoon_service/src/service/scheduler.dart'; import 'package:fixnum/fixnum.dart'; import 'package:github/github.dart'; import 'package:mockito/mockito.dart'; @@ -629,6 +632,176 @@ void main() { ); }, ); + + test( + 're-creates Presubmit check run on reScheduleTryBuilds when resetFailedCheckRun is enabled and failedJobs == 0', + () async { + final pullRequest = generatePullRequest( + id: 1, + repo: 'flutter', + headSha: 'headsha123', + ); + + final buildTarget = generateTarget( + 1, + properties: {'os': 'abc'}, + slug: RepositorySlug.full('flutter/flutter'), + name: 'Linux foo', + ); + + luci = LuciBuildService( + config: FakeConfig( + dynamicConfig: DynamicConfig( + resetFailedCheckRun: ResetFailedCheckRun(useForAll: true), + ), + ), + cache: CacheService.inMemory(), + buildBucketClient: mockBuildBucketClient, + githubChecksUtil: mockGithubChecksUtil, + pubsub: pubSub, + gerritService: gerritService, + firestore: firestore, + ); + + final checkRunGuard = generateCheckRun( + 1234, + name: Config.kDashboardCheckName, + ); + + final guard = PresubmitGuard( + checkRun: checkRunGuard, + headSha: 'headsha123', + slug: RepositorySlug.full('flutter/flutter'), + prNum: pullRequest.number!, + stage: CiStage.fusionTests, + creationTime: 123456789, + author: pullRequest.user!.login!, + remainingJobs: 1, + failedJobs: 0, + ); + await firestore.writeViaTransaction( + documentsToWrites([guard], exists: false), + ); + + when( + mockGithubChecksUtil.createCheckRun( + any, + any, + any, + any, + output: anyNamed('output'), + conclusion: anyNamed('conclusion'), + detailsUrl: anyNamed('detailsUrl'), + ), + ).thenAnswer( + (_) async => generateCheckRun(999, name: Config.kPresubmitCheckName), + ); + + await expectLater( + luci.reScheduleTryBuilds( + pullRequest: pullRequest, + targets: {buildTarget: 2}, + engineArtifacts: EngineArtifacts.builtFromSource( + commitSha: pullRequest.head!.sha!, + ), + dashboardChecks: checkRunGuard, + stage: CiStage.fusionTests, + ), + completion([isTarget.hasName('Linux foo')]), + ); + + verify( + mockGithubChecksUtil.createCheckRun( + any, + RepositorySlug.full('flutter/flutter'), + 'headsha123', + Config.kPresubmitCheckName, + output: const CheckRunOutput( + title: Config.kPresubmitCheckName, + summary: Scheduler.kPresubmitCheckDescription, + ), + detailsUrl: checkRunGuard.detailsUrl, + ), + ).called(1); + }, + ); + + test( + 'does not re-create Presubmit check run on reScheduleTryBuilds when resetFailedCheckRun is enabled and failedJobs > 0', + () async { + final pullRequest = generatePullRequest( + id: 1, + repo: 'flutter', + headSha: 'headsha123', + ); + + final buildTarget = generateTarget( + 1, + properties: {'os': 'abc'}, + slug: RepositorySlug.full('flutter/flutter'), + name: 'Linux foo', + ); + + luci = LuciBuildService( + config: FakeConfig( + dynamicConfig: DynamicConfig( + resetFailedCheckRun: ResetFailedCheckRun(useForAll: true), + ), + ), + cache: CacheService.inMemory(), + buildBucketClient: mockBuildBucketClient, + githubChecksUtil: mockGithubChecksUtil, + pubsub: pubSub, + gerritService: gerritService, + firestore: firestore, + ); + + final checkRunGuard = generateCheckRun( + 1234, + name: Config.kDashboardCheckName, + ); + + final guard = PresubmitGuard( + checkRun: checkRunGuard, + headSha: 'headsha123', + slug: RepositorySlug.full('flutter/flutter'), + prNum: pullRequest.number!, + stage: CiStage.fusionTests, + creationTime: 123456789, + author: pullRequest.user!.login!, + remainingJobs: 1, + failedJobs: 1, + ); + await firestore.writeViaTransaction( + documentsToWrites([guard], exists: false), + ); + + await expectLater( + luci.reScheduleTryBuilds( + pullRequest: pullRequest, + targets: {buildTarget: 2}, + engineArtifacts: EngineArtifacts.builtFromSource( + commitSha: pullRequest.head!.sha!, + ), + dashboardChecks: checkRunGuard, + stage: CiStage.fusionTests, + ), + completion([isTarget.hasName('Linux foo')]), + ); + + verifyNever( + mockGithubChecksUtil.createCheckRun( + any, + any, + any, + Config.kPresubmitCheckName, + output: anyNamed('output'), + conclusion: anyNamed('conclusion'), + detailsUrl: anyNamed('detailsUrl'), + ), + ); + }, + ); }); group('Ordered Presubmit', () { diff --git a/app_dart/test/service/scheduler_test.dart b/app_dart/test/service/scheduler_test.dart index f844da2044..ac510e1e73 100644 --- a/app_dart/test/service/scheduler_test.dart +++ b/app_dart/test/service/scheduler_test.dart @@ -20,6 +20,7 @@ import 'package:cocoon_service/src/model/firestore/commit.dart' as fs; import 'package:cocoon_service/src/model/firestore/task.dart' as fs; import 'package:cocoon_service/src/model/github/checks.dart' as cocoon_checks; import 'package:cocoon_service/src/service/big_query.dart'; +import 'package:cocoon_service/src/service/flags/reset_failed_check_run.dart'; import 'package:cocoon_service/src/service/luci_build_service/engine_artifacts.dart'; import 'package:cocoon_service/src/service/luci_build_service/pending_task.dart'; import 'package:cocoon_service/src/service/luci_build_service/user_data.dart'; @@ -3540,10 +3541,7 @@ targets: ), ); - expect( - await scheduler.processBuildCompleted(macCompleted), - isTrue, - ); + expect(await scheduler.processBuildCompleted(macCompleted), isTrue); verify( mockGithubChecksUtil.updateCheckRun( any, @@ -3714,12 +3712,26 @@ targets: checkSuiteId: linuxCheckRun.checkSuiteId, ), ); - expect( - await scheduler.processBuildCompleted(linuxFailed), - isFalse, - ); + expect(await scheduler.processBuildCompleted(linuxFailed), isTrue); + + verify( + mockGithubChecksUtil.updateCheckRun( + any, + Config.flutterSlug, + argThat( + isA().having( + (c) => c.name, + 'name', + Config.kMergeQueueLockName, + ), + ), + status: CheckRunStatus.completed, + conclusion: CheckRunConclusion.failure, + output: anyNamed('output'), + ), + ).called(1); - // 2. Mac engine_build succeeds -> stage finishes with 1 failed check, failing Merge Queue Guard + // 2. Mac engine_build succeeds -> stage finishes with 1 failed check final macSucceeded = PresubmitCompletedJob.fromBuild( generateBbv2Build( Int64(102), @@ -3737,10 +3749,7 @@ targets: checkSuiteId: macCheckRun.checkSuiteId, ), ); - expect( - await scheduler.processBuildCompleted(macSucceeded), - isTrue, - ); + expect(await scheduler.processBuildCompleted(macSucceeded), isTrue); verify( mockGithubChecksUtil.updateCheckRun( @@ -4572,6 +4581,370 @@ targets: expect(guard.remainingJobs, 0); }, ); + + test( + 'when resetFailedCheckRun is enabled, fails Presubmit immediately on first build failure and keeps Dashboard Checks in progress', + () async { + config = FakeConfig( + dynamicConfig: DynamicConfig( + resetFailedCheckRun: ResetFailedCheckRun(useForAll: true), + ), + ); + scheduler = Scheduler( + githubService: config.githubService ?? FakeGithubService(), + cache: cache, + config: config, + githubChecksService: GithubChecksService( + config, + githubChecksUtil: mockGithubChecksUtil, + ), + getFilesChanged: getFilesChanged, + ciYamlFetcher: ciYamlFetcher, + luciBuildService: MockLuciBuildService(), + contentAwareHash: fakeContentAwareHash, + firestore: firestore, + bigQuery: bigQuery, + ); + + final pullRequest = generatePullRequest(repo: 'packages'); + final dashboardCheckRun = generateCheckRun( + 1234, + name: Config.kDashboardCheckName, + checkSuite: 2, + startedAt: DateTime.now(), + ); + final presubmitCheckRun = generateCheckRun( + 5678, + name: Config.kPresubmitCheckName, + checkSuite: 2, + startedAt: DateTime.now(), + ); + + when(mockGithubChecksUtil.allCheckRuns(any, any, 2)).thenAnswer( + (_) async => { + Config.kDashboardCheckName: dashboardCheckRun, + Config.kPresubmitCheckName: presubmitCheckRun, + }, + ); + + await PrCheckRuns.initializeDocument( + firestoreService: firestore, + checks: [dashboardCheckRun], + pullRequest: pullRequest, + ); + + firestore.putDocument( + PresubmitGuard( + checkRun: dashboardCheckRun, + headSha: pullRequest.head!.sha!, + slug: pullRequest.base!.repo!.slug(), + prNum: pullRequest.number!, + stage: CiStage.genericTests, + author: pullRequest.user!.login!, + creationTime: DateTime.now().millisecondsSinceEpoch, + jobs: { + 'Linux test': TaskStatus.waitingForBackfill, + 'Mac test': TaskStatus.waitingForBackfill, + }, + remainingJobs: 2, + failedJobs: 0, + ), + ); + + firestore.putDocument( + PresubmitJob.init( + slug: pullRequest.base!.repo!.slug(), + jobName: 'Linux test', + checkRunId: dashboardCheckRun.id!, + creationTime: DateTime.now().millisecondsSinceEpoch, + ), + ); + firestore.putDocument( + PresubmitJob.init( + slug: pullRequest.base!.repo!.slug(), + jobName: 'Mac test', + checkRunId: dashboardCheckRun.id!, + creationTime: DateTime.now().millisecondsSinceEpoch, + ), + ); + + final userData = PresubmitUserData( + commit: CommitRef( + slug: pullRequest.base!.repo!.slug(), + sha: pullRequest.head!.sha!, + branch: 'main', + ), + guardCheckRunId: dashboardCheckRun.id, + stage: CiStage.genericTests, + checkSuiteId: 2, + pullRequestNumber: pullRequest.number, + ); + + // 1. First build ('Linux test') fails while 'Mac test' is still pending. + final linuxFailedBuild = generateBbv2Build( + Int64(1), + name: 'Linux 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(linuxFailedBuild, userData), + ), + isTrue, + ); + + // Presubmit check run must be failed immediately on first failure. + final verification = verify( + mockGithubChecksUtil.updateCheckRun( + any, + pullRequest.base!.repo!.slug(), + argThat( + isA().having( + (c) => c.name, + 'name', + Config.kPresubmitCheckName, + ), + ), + status: CheckRunStatus.completed, + conclusion: CheckRunConclusion.actionRequired, + detailsUrl: anyNamed('detailsUrl'), + output: captureAnyNamed('output'), + actions: anyNamed('actions'), + ), + ); + verification.called(1); + final output = verification.captured.single as CheckRunOutput; + expect(output.text, 'Failed presubmit jobs:\n- `Linux test`'); + + // Dashboard Checks must remain in progress (not updated). + verifyNever( + mockGithubChecksUtil.updateCheckRun( + any, + any, + argThat( + isA().having( + (c) => c.name, + 'name', + Config.kDashboardCheckName, + ), + ), + status: anyNamed('status'), + conclusion: anyNamed('conclusion'), + detailsUrl: anyNamed('detailsUrl'), + output: anyNamed('output'), + actions: anyNamed('actions'), + ), + ); + + // 2. Second build ('Mac test') succeeds -> stage finishes with 1 failed check, Dashboard Checks still in progress. + final macSucceededBuild = generateBbv2Build( + Int64(2), + name: 'Mac test', + status: bbv2.Status.SUCCESS, + tags: [ + bbv2.StringPair(key: 'current_attempt', value: '1'), + bbv2.StringPair(key: 'author', value: pullRequest.user!.login!), + ], + ); + + expect( + await scheduler.processBuildCompleted( + PresubmitCompletedJob.fromBuild(macSucceededBuild, userData), + ), + isTrue, + ); + + verifyNever( + mockGithubChecksUtil.updateCheckRun( + any, + any, + argThat( + isA().having( + (c) => c.name, + 'name', + Config.kDashboardCheckName, + ), + ), + status: anyNamed('status'), + conclusion: anyNamed('conclusion'), + detailsUrl: anyNamed('detailsUrl'), + output: anyNamed('output'), + actions: anyNamed('actions'), + ), + ); + }, + ); + + test( + 'when resetFailedCheckRun is enabled, completes both Presubmit and Dashboard Checks when all tests pass', + () async { + config = FakeConfig( + dynamicConfig: DynamicConfig( + resetFailedCheckRun: ResetFailedCheckRun(useForAll: true), + ), + ); + scheduler = Scheduler( + githubService: config.githubService ?? FakeGithubService(), + cache: cache, + config: config, + githubChecksService: GithubChecksService( + config, + githubChecksUtil: mockGithubChecksUtil, + ), + getFilesChanged: getFilesChanged, + ciYamlFetcher: ciYamlFetcher, + luciBuildService: MockLuciBuildService(), + contentAwareHash: fakeContentAwareHash, + firestore: firestore, + bigQuery: bigQuery, + ); + + final pullRequest = generatePullRequest(repo: 'packages'); + final dashboardCheckRun = generateCheckRun( + 1234, + name: Config.kDashboardCheckName, + checkSuite: 2, + startedAt: DateTime.now(), + ); + final presubmitCheckRun = generateCheckRun( + 5678, + name: Config.kPresubmitCheckName, + checkSuite: 2, + startedAt: DateTime.now(), + ); + final mergeQueueGuard = generateCheckRun( + 9012, + name: Config.kMergeQueueLockName, + checkSuite: 2, + startedAt: DateTime.now(), + ); + + when(mockGithubChecksUtil.allCheckRuns(any, any, 2)).thenAnswer( + (_) async => { + Config.kDashboardCheckName: dashboardCheckRun, + Config.kPresubmitCheckName: presubmitCheckRun, + Config.kMergeQueueLockName: mergeQueueGuard, + }, + ); + + await PrCheckRuns.initializeDocument( + firestoreService: firestore, + checks: [dashboardCheckRun, mergeQueueGuard], + pullRequest: pullRequest, + ); + + firestore.putDocument( + PresubmitGuard( + checkRun: dashboardCheckRun, + checkRunGuard: mergeQueueGuard, + headSha: pullRequest.head!.sha!, + slug: pullRequest.base!.repo!.slug(), + prNum: pullRequest.number!, + stage: CiStage.genericTests, + author: pullRequest.user!.login!, + creationTime: DateTime.now().millisecondsSinceEpoch, + jobs: {'Linux test': TaskStatus.waitingForBackfill}, + remainingJobs: 1, + failedJobs: 0, + ), + ); + + firestore.putDocument( + PresubmitJob.init( + slug: pullRequest.base!.repo!.slug(), + jobName: 'Linux test', + checkRunId: dashboardCheckRun.id!, + creationTime: DateTime.now().millisecondsSinceEpoch, + ), + ); + + final userData = PresubmitUserData( + commit: CommitRef( + slug: pullRequest.base!.repo!.slug(), + sha: pullRequest.head!.sha!, + branch: 'main', + ), + guardCheckRunId: dashboardCheckRun.id, + stage: CiStage.genericTests, + checkSuiteId: 2, + pullRequestNumber: pullRequest.number, + ); + + final build = generateBbv2Build( + Int64(1), + name: 'Linux test', + status: bbv2.Status.SUCCESS, + tags: [ + bbv2.StringPair(key: 'current_attempt', value: '1'), + bbv2.StringPair(key: 'author', value: pullRequest.user!.login!), + bbv2.StringPair( + key: 'buildset', + value: 'sha/git/${pullRequest.head!.sha!}', + ), + ], + ); + + expect( + await scheduler.processBuildCompleted( + PresubmitCompletedJob.fromBuild(build, userData), + ), + isTrue, + ); + + verify( + mockGithubChecksUtil.updateCheckRun( + any, + pullRequest.base!.repo!.slug(), + argThat( + isA().having( + (c) => c.name, + 'name', + Config.kDashboardCheckName, + ), + ), + status: CheckRunStatus.completed, + conclusion: CheckRunConclusion.success, + ), + ).called(1); + + verify( + mockGithubChecksUtil.updateCheckRun( + any, + pullRequest.base!.repo!.slug(), + argThat( + isA().having( + (c) => c.name, + 'name', + Config.kPresubmitCheckName, + ), + ), + status: CheckRunStatus.completed, + conclusion: CheckRunConclusion.success, + ), + ).called(1); + + verify( + mockGithubChecksUtil.updateCheckRun( + any, + pullRequest.base!.repo!.slug(), + argThat( + isA().having( + (c) => c.name, + 'name', + Config.kMergeQueueLockName, + ), + ), + status: CheckRunStatus.completed, + conclusion: CheckRunConclusion.success, + ), + ).called(1); + }, + ); }); }); From ef5054f195d3e0df66421bd0e6e1e453dfd80aeb Mon Sep 17 00:00:00 2001 From: "Dmitry Grand (dmgr)" Date: Thu, 24 Sep 2026 16:36:19 -0700 Subject: [PATCH 4/7] renamings --- app_dart/lib/src/service/scheduler.dart | 35 +++++++------------ .../lib/src/utilities/mocks.mocks.dart | 14 ++++---- 2 files changed, 19 insertions(+), 30 deletions(-) diff --git a/app_dart/lib/src/service/scheduler.dart b/app_dart/lib/src/service/scheduler.dart index 9195d5e153..fa05db62e3 100644 --- a/app_dart/lib/src/service/scheduler.dart +++ b/app_dart/lib/src/service/scheduler.dart @@ -532,12 +532,12 @@ class Scheduler { // there are situations (see code above) when it needs to be unlocked // immediately. if (unlockMergeGroup) { - await unlockMergeQueueGuard(slug, sha, dashboardChecks); + await unlockCheckRun(slug, sha, dashboardChecks); if (mergeQueueGuard != null) { - await unlockMergeQueueGuard(slug, sha, mergeQueueGuard); + await unlockCheckRun(slug, sha, mergeQueueGuard); } if (presubmit != null) { - await unlockMergeQueueGuard(slug, sha, presubmit); + await unlockCheckRun(slug, sha, presubmit); } } log.info( @@ -669,7 +669,7 @@ class Scheduler { // If the repo is not fusion, it doesn't run anything in the MQ, so just // close the merge group guard. if (!isFusion) { - await unlockMergeQueueGuard(slug, headSha, mergeQueueGuard); + await unlockCheckRun(slug, headSha, mergeQueueGuard); return; } @@ -983,20 +983,13 @@ $s } } - /// Completes the "Merge Queue Guard" check run. - /// - /// If the guard is guarding a merge group, this immediately makes the merge - /// group eligible for landing onto the target branch (e.g. master), depending - /// on the success of the merge groups queued in front of this one. - /// - /// If the guard is guarding a pull request, this immediately makes the pull - /// request eligible for enqueuing into the merge queue. - Future unlockMergeQueueGuard( + /// Completes the checkruns such as: + Future unlockCheckRun( RepositorySlug slug, String headSha, CheckRun lock, ) async { - log.info('Unlocking Merge Queue Guard for $slug/$headSha'); + log.info('Unlocking check-run: ${lock.name} for $slug/$headSha'); await _githubChecksService.githubChecksUtil.updateCheckRun( _config, slug, @@ -1445,7 +1438,7 @@ defined in: // with a merge group - they are only used to collect commit stats. log.warn('$logCrumb: generic tests have no merge queue guard.'); } else if (mergeQueueGuard != null) { - await unlockMergeQueueGuard( + await unlockCheckRun( check.slug, check.sha, checkRunFromString(mergeQueueGuard), @@ -1454,7 +1447,7 @@ defined in: } else { if (dashboardChecks != null) { final dashboardCheckRun = checkRunFromString(dashboardChecks); - await unlockMergeQueueGuard(check.slug, check.sha, dashboardCheckRun); + await unlockCheckRun(check.slug, check.sha, dashboardCheckRun); if (check.author != null && _config.flags.isResetFailedCheckRunEnabledForUser(check.author!)) { final checkSuiteId = @@ -1464,17 +1457,13 @@ defined in: .allCheckRuns(_config, check.slug, checkSuiteId); final presubmitCheck = checks[Config.kPresubmitCheckName]; if (presubmitCheck != null) { - await unlockMergeQueueGuard( - check.slug, - check.sha, - presubmitCheck, - ); + await unlockCheckRun(check.slug, check.sha, presubmitCheck); } } } } if (mergeQueueGuard != null) { - await unlockMergeQueueGuard( + await unlockCheckRun( check.slug, check.sha, checkRunFromString(mergeQueueGuard), @@ -1518,7 +1507,7 @@ defined in: // Unlock the guarding check_run. final checkRunGuard = checkRunFromString(mergeQueueGuard); - await unlockMergeQueueGuard(slug, sha, checkRunGuard); + await unlockCheckRun(slug, sha, checkRunGuard); } /// Schedules post-engine build tests (i.e. engine tests, and framework tests). diff --git a/packages/cocoon_integration_test/lib/src/utilities/mocks.mocks.dart b/packages/cocoon_integration_test/lib/src/utilities/mocks.mocks.dart index c264f2e56a..ab1200d060 100644 --- a/packages/cocoon_integration_test/lib/src/utilities/mocks.mocks.dart +++ b/packages/cocoon_integration_test/lib/src/utilities/mocks.mocks.dart @@ -1916,10 +1916,10 @@ class MockGithubChecksService extends _i1.Mock @override String getSummary(String? summary) => (super.noSuchMethod( - Invocation.method(#getGithubSummary, [summary]), + Invocation.method(#getSummary, [summary]), returnValue: _i19.dummyValue( this, - Invocation.method(#getGithubSummary, [summary]), + Invocation.method(#getSummary, [summary]), ), ) as String); @@ -1927,10 +1927,10 @@ class MockGithubChecksService extends _i1.Mock @override String getSummaryWithHeader(String? header, String? summary) => (super.noSuchMethod( - Invocation.method(#getGithubSummaryWithHeader, [header, summary]), + Invocation.method(#getSummaryWithHeader, [header, summary]), returnValue: _i19.dummyValue( this, - Invocation.method(#getGithubSummaryWithHeader, [header, summary]), + Invocation.method(#getSummaryWithHeader, [header, summary]), ), ) as String); @@ -5793,13 +5793,13 @@ class MockScheduler extends _i1.Mock implements _i2.Scheduler { as _i13.Future); @override - _i13.Future unlockMergeQueueGuard( + _i13.Future unlockCheckRun( _i7.RepositorySlug? slug, String? headSha, _i7.CheckRun? lock, ) => (super.noSuchMethod( - Invocation.method(#unlockMergeQueueGuard, [slug, headSha, lock]), + Invocation.method(#unlockCheckRun, [slug, headSha, lock]), returnValue: _i13.Future.value(), returnValueForMissingStub: _i13.Future.value(), ) @@ -5860,7 +5860,7 @@ class MockScheduler extends _i1.Mock implements _i2.Scheduler { @override _i13.Future processBuildCompleted(_i38.PresubmitCompletedJob? check) => (super.noSuchMethod( - Invocation.method(#processCheckRunCompleted, [check]), + Invocation.method(#processBuildCompleted, [check]), returnValue: _i13.Future.value(false), ) as _i13.Future); From c12123bf61faba66a456a36f00318a6b45927b6f Mon Sep 17 00:00:00 2001 From: "Dmitry Grand (dmgr)" Date: Fri, 25 Sep 2026 14:43:25 -0700 Subject: [PATCH 5/7] fix AI review --- app_dart/lib/src/foundation/github_checks_util.dart | 2 +- .../lib/src/model/common/presubmit_guard_conclusion.dart | 2 +- .../request_handlers/github/webhook_subscription_test.dart | 7 ++++--- .../test/service/firestore/unified_check_run_test.dart | 4 ++-- 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/app_dart/lib/src/foundation/github_checks_util.dart b/app_dart/lib/src/foundation/github_checks_util.dart index fc62801b68..f5026ad9ef 100644 --- a/app_dart/lib/src/foundation/github_checks_util.dart +++ b/app_dart/lib/src/foundation/github_checks_util.dart @@ -3,12 +3,12 @@ // found in the LICENSE file. import 'dart:core'; +import 'dart:io'; import 'package:cocoon_server/logging.dart'; import 'package:github/github.dart' as github; import 'package:retry/retry.dart'; -import '../request_handling/http_utils.dart'; import '../service/config.dart'; /// Wrapper class for github checkrun service. This is used to simplify 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 4ef5eaed67..c088e7d2ef 100644 --- a/app_dart/lib/src/model/common/presubmit_guard_conclusion.dart +++ b/app_dart/lib/src/model/common/presubmit_guard_conclusion.dart @@ -64,7 +64,7 @@ class PresubmitGuardConclusion { bool get isFailed => isOk && failed > 0; - bool get isSuccessed => isOk && !isPending && !isFailed; + bool get isSucceeded => isOk && !isPending && !isFailed; @override bool operator ==(Object other) => diff --git a/app_dart/test/request_handlers/github/webhook_subscription_test.dart b/app_dart/test/request_handlers/github/webhook_subscription_test.dart index ef462c1f51..c64e5f20f4 100644 --- a/app_dart/test/request_handlers/github/webhook_subscription_test.dart +++ b/app_dart/test/request_handlers/github/webhook_subscription_test.dart @@ -150,9 +150,10 @@ void main() { conclusion: anyNamed('conclusion'), detailsUrl: anyNamed('detailsUrl'), ), - ).thenAnswer((_) async { - return CheckRun.fromJson(const { + ).thenAnswer((Invocation invocation) async { + return CheckRun.fromJson({ 'id': 1, + 'name': invocation.positionalArguments[3], 'started_at': '2020-05-10T02:49:31Z', 'check_suite': {'id': 2}, }); @@ -3421,7 +3422,7 @@ void foo() { ), logThat( message: equals( - 'Unlocking Merge Queue Guard for flutter/packages/c9affbbb12aa40cb3afbe94b9ea6b119a256bebf', + 'Unlocking check-run: Merge Queue Guard for flutter/packages/c9affbbb12aa40cb3afbe94b9ea6b119a256bebf', ), ), ]), 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 9c1f3fbe30..40deb86cb6 100644 --- a/app_dart/test/service/firestore/unified_check_run_test.dart +++ b/app_dart/test/service/firestore/unified_check_run_test.dart @@ -189,7 +189,7 @@ void main() { expect(result1.remaining, 1); expect(result1.failed, 0); expect(result1.isOk, true); - expect(result1.isSuccessed, false); + expect(result1.isSucceeded, false); expect(result1.isPending, true); final result2 = await UnifiedCheckRun.markConclusion( @@ -207,7 +207,7 @@ void main() { expect(result2.remaining, 0); expect(result2.failed, 0); expect(result2.isOk, true); - expect(result2.isSuccessed, true); + expect(result2.isSucceeded, true); expect(result2.isPending, false); final checkDoc = await PresubmitJob.fromFirestore( From 038728c24075eaf5090a70460e133f521cbbabca Mon Sep 17 00:00:00 2001 From: "Dmitry Grand (dmgr)" Date: Fri, 25 Sep 2026 14:49:39 -0700 Subject: [PATCH 6/7] fix ai review --- app_dart/lib/src/service/luci_build_service.dart | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app_dart/lib/src/service/luci_build_service.dart b/app_dart/lib/src/service/luci_build_service.dart index eb27325592..70d42d0430 100644 --- a/app_dart/lib/src/service/luci_build_service.dart +++ b/app_dart/lib/src/service/luci_build_service.dart @@ -392,12 +392,14 @@ class LuciBuildService { cipdVersion: cipdVersion, userData: userData, properties: properties, - // In merge queue use check run id othervise guard check run. + // In merge queue use check run id otherwise guard check run id. tags: BuildTags([ if (pullRequest.user?.login != null) AuthorBuildTag(value: pullRequest.user!.login!), if (dashboardChecks != null) GuardCheckRunIdBuildTag(guardCheckRunId: dashboardChecks.id!), + if (dashboardChecks == null && userData.checkRunId != null) + GitHubCheckRunIdBuildTag(checkRunId: userData.checkRunId!), if (attemptNumber > 1) CurrentAttemptBuildTag(attemptNumber: attemptNumber), if (isOrderedPresubmit) From 111a929503502f9d9ab2fa750772520b100b2f5a Mon Sep 17 00:00:00 2001 From: "Dmitry Grand (dmgr)" Date: Fri, 25 Sep 2026 15:03:12 -0700 Subject: [PATCH 7/7] trim trailing white space --- app_dart/lib/src/service/scheduler.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app_dart/lib/src/service/scheduler.dart b/app_dart/lib/src/service/scheduler.dart index fa05db62e3..6f7c5ac466 100644 --- a/app_dart/lib/src/service/scheduler.dart +++ b/app_dart/lib/src/service/scheduler.dart @@ -1074,7 +1074,7 @@ detailsUrl: $detailsUrl String? detailsUrl, }) async { log.info(''' -Require action for ${Config.kPresubmitCheckName} +Require action for ${Config.kPresubmitCheckName} with: summary: $summary details: $details