From 00f11009191434cb97f2f519e5fd0cc42b74b05b Mon Sep 17 00:00:00 2001 From: Dmitry Grand Date: Tue, 21 Jul 2026 10:41:06 -0700 Subject: [PATCH 1/7] Do not unlock dashboard checks on emergency label --- .../lib/src/service/pull_request_manager.dart | 3 +- .../github/webhook_subscription_test.dart | 84 +------------------ 2 files changed, 3 insertions(+), 84 deletions(-) diff --git a/app_dart/lib/src/service/pull_request_manager.dart b/app_dart/lib/src/service/pull_request_manager.dart index e2a57589a..4c5ae2bc5 100644 --- a/app_dart/lib/src/service/pull_request_manager.dart +++ b/app_dart/lib/src/service/pull_request_manager.dart @@ -1067,8 +1067,9 @@ The "Merge" button is also unlocked. To bypass presubmits as well as the tree st } Future _unlockCheckrunsForEmergency() async { + // Unlock only the merge queue guard for emergency. Do not unlock + // dashboard checks. See: https://github.com/flutter/flutter/issues/189729 await _unlockCheckrun(Config.kMergeQueueLockName); - await _unlockCheckrun(Config.kDashboardCheckName); // Let the developer know what is happening with the MQ when this label is found the first time. try { 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 eaa5ad794..1ff306c65 100644 --- a/app_dart/test/request_handlers/github/webhook_subscription_test.dart +++ b/app_dart/test/request_handlers/github/webhook_subscription_test.dart @@ -2969,7 +2969,7 @@ void foo() { group('PullRequestLabelProcessor.processLabels', () { test( - 'applies emergency label on approved PRs (Merge Queue Guard only)', + 'applies emergency label on approved PRs', () async { final pullRequest = generatePullRequest( number: 123, @@ -3017,84 +3017,12 @@ void foo() { 'PullRequestLabelProcessor(flutter/flutter/pull/123): unlocked "Merge Queue Guard", allowing it to land as an emergency.', ), ), - logThat( - message: equals( - 'PullRequestLabelProcessor(flutter/flutter/pull/123): attempting to unlock the Dashboard Checks for emergency', - ), - ), - logThat( - message: equals( - 'PullRequestLabelProcessor(flutter/flutter/pull/123): failed to process the emergency label. "Dashboard Checks" check run is missing.', - ), - ), ]), ), ); }, ); - test('applies emergency label on approved PRs (Dashboard Checks only)', () async { - final pullRequest = generatePullRequest( - number: 123, - headSha: '6dcb09b5b57875f334f61aebed695e2e4193db5e', - labels: [IssueLabel(name: 'emergency')], - ); - - githubService.checkRunsMock = '''{ - "total_count": 1, - "check_runs": [ - { - "id": 3, - "head_sha": "6dcb09b5b57875f334f61aebed695e2e4193db5e", - "external_id": "", - "details_url": "https://example.com", - "status": "in_progress", - "started_at": "2018-05-04T01:14:52Z", - "name": "Dashboard Checks", - "check_suite": { - "id": 5 - } - } - ] -}'''; - - final pullRequestLabelProcessor = PullRequestLabelProcessor( - config: config, - githubService: githubService, - pullRequest: pullRequest, - ); - - await pullRequestLabelProcessor.processLabels(); - - expect( - log, - bufferedLoggerOf( - containsAll([ - logThat( - message: equals( - 'PullRequestLabelProcessor(flutter/flutter/pull/123): attempting to unlock the Merge Queue Guard for emergency', - ), - ), - logThat( - message: equals( - 'PullRequestLabelProcessor(flutter/flutter/pull/123): failed to process the emergency label. "Merge Queue Guard" check run is missing.', - ), - ), - logThat( - message: equals( - 'PullRequestLabelProcessor(flutter/flutter/pull/123): attempting to unlock the Dashboard Checks for emergency', - ), - ), - logThat( - message: equals( - 'PullRequestLabelProcessor(flutter/flutter/pull/123): unlocked "Dashboard Checks", allowing it to land as an emergency.', - ), - ), - ]), - ), - ); - }); - test( 'logs and gracefully skips emergency label on missing checkruns', () async { @@ -3144,16 +3072,6 @@ void foo() { 'failed to process the emergency label. "Merge Queue Guard" check run is missing.', ), ), - logThat( - message: contains( - 'attempting to unlock the Dashboard Checks for emergency', - ), - ), - logThat( - message: contains( - 'failed to process the emergency label. "Dashboard Checks" check run is missing.', - ), - ), ]), ), ); From a97b46da32cf60a4d6f2aea38e1f9e5699b14f92 Mon Sep 17 00:00:00 2001 From: Dmitry Grand Date: Tue, 21 Jul 2026 12:42:01 -0700 Subject: [PATCH 2/7] do not unlock `presubmit_guards` immediately for unified check run --- app_dart/lib/src/service/scheduler.dart | 30 +++++++++++----- app_dart/test/service/scheduler_test.dart | 43 +++++++++++++++++++++++ 2 files changed, 65 insertions(+), 8 deletions(-) diff --git a/app_dart/lib/src/service/scheduler.dart b/app_dart/lib/src/service/scheduler.dart index c7f793277..cb6a3a5dd 100644 --- a/app_dart/lib/src/service/scheduler.dart +++ b/app_dart/lib/src/service/scheduler.dart @@ -889,14 +889,6 @@ $s ); if (isUnifiedCheckRun) { - // Skip MQ Guard - await _githubChecksService.githubChecksUtil.updateCheckRun( - _config, - slug, - mqGuard, - status: CheckRunStatus.completed, - conclusion: CheckRunConclusion.success, - ); return flutterPresubmits; } else { // Skip Dashboard Checks @@ -983,6 +975,28 @@ $s status: CheckRunStatus.completed, conclusion: CheckRunConclusion.success, ); + if (lock.name == Config.kDashboardCheckName) { + final githubService = await _config.createGithubService(slug); + final mqGuard = (await githubService.getCheckRunsFiltered( + slug: slug, + ref: headSha, + checkName: Config.kMergeQueueLockName, + )).singleOrNull; + if (mqGuard != null) { + log.info( + 'Unlocking Merge Queue Guard for unified check run for $slug/$headSha', + ); + await _githubChecksService.githubChecksUtil.updateCheckRun( + _config, + slug, + mqGuard, + status: CheckRunStatus.completed, + conclusion: CheckRunConclusion.success, + ); + } else { + log.warn('Merge Queue Guard not found for $slug/$headSha'); + } + } } /// Fails the "Merge Queue Guard" check for a merge group. diff --git a/app_dart/test/service/scheduler_test.dart b/app_dart/test/service/scheduler_test.dart index 5e56d325b..991475ffe 100644 --- a/app_dart/test/service/scheduler_test.dart +++ b/app_dart/test/service/scheduler_test.dart @@ -3058,6 +3058,49 @@ targets: }, ); + test( + 'does not close Merge Queue Guard immediately for unified check run flow', + () 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 lock = await scheduler.lockMergeGroupChecks( + Config.flutterSlug, + 'sha123', + isUnifiedCheckRun: true, + ); + + expect(lock.name, Config.kDashboardCheckName); + + verifyNever( + mockGithubChecksUtil.updateCheckRun( + any, + any, + any, + status: CheckRunStatus.completed, + conclusion: CheckRunConclusion.success, + output: anyNamed('output'), + actions: anyNamed('actions'), + detailsUrl: anyNamed('detailsUrl'), + ), + ); + }, + ); + test( 'filters out presubmit targets that do not exist in main and do not filter targets not in main', () async { From c5f9be88a0cf7f1a763598802a827b47dd0d9fe1 Mon Sep 17 00:00:00 2001 From: Dmitry Grand Date: Tue, 21 Jul 2026 13:18:06 -0700 Subject: [PATCH 3/7] store mqGuard in `presubmit_guards` --- .../src/model/firestore/presubmit_guard.dart | 21 +++++++++ .../service/firestore/unified_check_run.dart | 2 + app_dart/lib/src/service/scheduler.dart | 45 ++++++++++++------- app_dart/test/service/scheduler_test.dart | 5 ++- .../lib/src/utilities/entity_generators.dart | 2 + .../lib/src/utilities/mocks.mocks.dart | 13 ++++-- 6 files changed, 66 insertions(+), 22 deletions(-) diff --git a/app_dart/lib/src/model/firestore/presubmit_guard.dart b/app_dart/lib/src/model/firestore/presubmit_guard.dart index fbb62ffa8..4b16c808b 100644 --- a/app_dart/lib/src/model/firestore/presubmit_guard.dart +++ b/app_dart/lib/src/model/firestore/presubmit_guard.dart @@ -49,6 +49,7 @@ final class PresubmitGuardId extends AppDocumentId { final class PresubmitGuard extends AppDocument { static const collectionId = 'presubmit_guards'; static const fieldCheckRun = 'check_run'; + static const fieldCheckRunGuard = 'check_run_guard'; static const fieldCheckRunId = 'check_run_id'; static const fieldPrNum = 'pr_num'; static const fieldSlug = 'slug'; @@ -117,6 +118,7 @@ final class PresubmitGuard extends AppDocument { required int creationTime, required String author, required int jobCount, + CheckRun? checkRunGuard, }) { return PresubmitGuard( checkRun: checkRun, @@ -128,6 +130,7 @@ final class PresubmitGuard extends AppDocument { creationTime: creationTime, remainingJobs: jobCount, failedJobs: 0, + checkRunGuard: checkRunGuard, ); } @@ -145,6 +148,7 @@ final class PresubmitGuard extends AppDocument { required String author, required int remainingJobs, required int failedJobs, + CheckRun? checkRunGuard, Map? jobs, }) { return PresubmitGuard._( @@ -159,6 +163,8 @@ final class PresubmitGuard extends AppDocument { fieldCheckRun: json.encode(checkRun.toJson()).toValue(), fieldRemainingJobs: remainingJobs.toValue(), fieldFailedJobs: failedJobs.toValue(), + if (checkRunGuard != null) + fieldCheckRunGuard: json.encode(checkRunGuard.toJson()).toValue(), if (jobs != null) fieldJobs: Value( mapValue: MapValue( @@ -202,6 +208,21 @@ final class PresubmitGuard extends AppDocument { String get checkRunJson => fields[fieldCheckRun]!.stringValue!; + CheckRun? get checkRunGuard { + if (fields[fieldCheckRunGuard]?.stringValue == null) { + return null; + } + final jsonData = + jsonDecode(fields[fieldCheckRunGuard]!.stringValue!) + as Map; + if (jsonData['conclusion'] == 'null') { + jsonData.remove('conclusion'); + } + return CheckRun.fromJson(jsonData); + } + + String? get checkRunGuardJson => fields[fieldCheckRunGuard]?.stringValue; + /// The repository that this stage is recorded for. RepositorySlug get slug { if (fields[fieldSlug] != null) { 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 290e74bf8..4ad203083 100644 --- a/app_dart/lib/src/service/firestore/unified_check_run.dart +++ b/app_dart/lib/src/service/firestore/unified_check_run.dart @@ -32,6 +32,7 @@ final class UnifiedCheckRun { required Config config, PullRequest? pullRequest, CheckRun? checkRun, + CheckRun? checkRunGuard, @visibleForTesting DateTime Function() utcNow = DateTime.timestamp, }) async { if (checkRun != null && @@ -49,6 +50,7 @@ final class UnifiedCheckRun { final creationTime = utcNow().millisecondsSinceEpoch; final guard = PresubmitGuard( checkRun: checkRun, + checkRunGuard: checkRunGuard, headSha: sha, slug: slug, prNum: pullRequest.number!, diff --git a/app_dart/lib/src/service/scheduler.dart b/app_dart/lib/src/service/scheduler.dart index cb6a3a5dd..c44b57282 100644 --- a/app_dart/lib/src/service/scheduler.dart +++ b/app_dart/lib/src/service/scheduler.dart @@ -364,7 +364,7 @@ class Scheduler { // The MQ only waits for "required status checks" before deciding whether to // merge the PR into the target branch. This required check added to both // the PR and to the merge group, and so it must be completed in both cases. - final lock = await lockMergeGroupChecks( + final lockResult = await lockMergeGroupChecks( slug, sha, // Override details url of merge queue guard check for users with unified @@ -374,6 +374,8 @@ class Scheduler { : null, isUnifiedCheckRun: isUnifiedCheckRun, ); + final lock = lockResult.lock; + final checkRunGuard = lockResult.checkRunGuard; // Track if we should unlock the merge group lock in case of non-fusion or // revert bots. @@ -410,11 +412,14 @@ class Scheduler { tasks: [], pullRequest: pullRequest, config: _config, + checkRun: lock, + checkRunGuard: checkRunGuard, ); await _runCiTestingStage( pullRequest: pullRequest, checkRunGuard: lock, + checkRunGuardCheck: checkRunGuard, logCrumb: logCrumb, // The if-branch already skips the engine build phase. @@ -449,6 +454,7 @@ class Scheduler { pullRequest: pullRequest, config: _config, checkRun: lock, + checkRunGuard: checkRunGuard, ); // Even though this appears to be an engine build, it could be a @@ -474,6 +480,7 @@ class Scheduler { pullRequest: pullRequest, config: _config, checkRun: lock, + checkRunGuard: checkRunGuard, ); } engineArtifacts = const EngineArtifacts.noFrameworkTests( @@ -640,11 +647,12 @@ class Scheduler { '${contentHash != null ? ', contentHash: $contentHash' : ''})'; log.info('$logCrumb: scheduling merge group checks'); - final lock = await lockMergeGroupChecks( + final lockResult = await lockMergeGroupChecks( slug, headSha, isUnifiedCheckRun: false, ); + final lock = lockResult.lock; // If the repo is not fusion, it doesn't run anything in the MQ, so just // close the merge group guard. @@ -668,17 +676,9 @@ class Scheduler { project: 'flutter', bucket: 'prod', ); - final availableTargets = { - ...mergeGroupTargets.where( - (target) => availableBuilders.contains(target.name), - ), - }; - if (availableTargets.length != mergeGroupTargets.length) { - log.warn( - '$logCrumb: missing builders for targets: ' - '${mergeGroupTargets.difference(availableTargets)}', - ); - } + final availableTargets = mergeGroupTargets.where( + (t) => availableBuilders.contains(t.name), + ); // Create the staging doc that will track our engine progress and allow us to unlock // the merge group lock later. @@ -690,6 +690,7 @@ class Scheduler { tasks: [...availableTargets.map((t) => t.name)], config: _config, checkRun: lock, + checkRunGuard: lockResult.checkRunGuard, ); // Create the minimal Commit needed to pass the next stage. @@ -857,7 +858,7 @@ $s /// While this check is still in progress, the merge queue will not merge the /// respective PR onto the target branch (e.g. main or master), because this /// check is "required". - Future lockMergeGroupChecks( + Future lockMergeGroupChecks( RepositorySlug slug, String headSha, { String? detailsUrl, @@ -889,7 +890,10 @@ $s ); if (isUnifiedCheckRun) { - return flutterPresubmits; + return CheckRunLockResult( + lock: flutterPresubmits, + checkRunGuard: mqGuard, + ); } else { // Skip Dashboard Checks await _githubChecksService.githubChecksUtil.updateCheckRun( @@ -899,7 +903,7 @@ $s status: CheckRunStatus.completed, conclusion: CheckRunConclusion.success, ); - return mqGuard; + return CheckRunLockResult(lock: mqGuard, checkRunGuard: null); } } @@ -1413,6 +1417,7 @@ detailsUrl: $detailsUrl Future _runCiTestingStage({ required PullRequest pullRequest, required CheckRun checkRunGuard, + CheckRun? checkRunGuardCheck, required String logCrumb, required _FlutterRepoTestsToRun testsToRun, }) async { @@ -1455,6 +1460,7 @@ detailsUrl: $detailsUrl config: _config, pullRequest: pullRequest, checkRun: checkRunGuard, + checkRunGuard: checkRunGuardCheck, ); // Here is where it gets fun: how do framework tests* know what engine @@ -2034,3 +2040,10 @@ enum _TaskCommitScheduling { return this == nonDefaultBranchSkipTestsByDefault; } } + +class CheckRunLockResult { + final CheckRun lock; + final CheckRun? checkRunGuard; + + const CheckRunLockResult({required this.lock, this.checkRunGuard}); +} diff --git a/app_dart/test/service/scheduler_test.dart b/app_dart/test/service/scheduler_test.dart index 991475ffe..6015bf299 100644 --- a/app_dart/test/service/scheduler_test.dart +++ b/app_dart/test/service/scheduler_test.dart @@ -3078,13 +3078,14 @@ targets: ); }); - final lock = await scheduler.lockMergeGroupChecks( + final lockResult = await scheduler.lockMergeGroupChecks( Config.flutterSlug, 'sha123', isUnifiedCheckRun: true, ); - expect(lock.name, Config.kDashboardCheckName); + expect(lockResult.lock.name, Config.kDashboardCheckName); + expect(lockResult.checkRunGuard?.name, Config.kMergeQueueLockName); verifyNever( mockGithubChecksUtil.updateCheckRun( diff --git a/packages/cocoon_integration_test/lib/src/utilities/entity_generators.dart b/packages/cocoon_integration_test/lib/src/utilities/entity_generators.dart index 5856df24a..76013fbab 100644 --- a/packages/cocoon_integration_test/lib/src/utilities/entity_generators.dart +++ b/packages/cocoon_integration_test/lib/src/utilities/entity_generators.dart @@ -315,6 +315,7 @@ PresubmitGuard generatePresubmitGuard({ github.RepositorySlug? slug, int prNum = 123, github.CheckRun? checkRun, + github.CheckRun? checkRunGuard, CiStage stage = CiStage.fusionTests, String headSha = 'abc', int creationTime = 1, @@ -327,6 +328,7 @@ PresubmitGuard generatePresubmitGuard({ slug: slug ?? github.RepositorySlug('flutter', 'flutter'), prNum: prNum, checkRun: checkRun ?? generateCheckRun(1), + checkRunGuard: checkRunGuard, stage: stage, headSha: headSha, creationTime: creationTime, 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 5d69d2556..1d3bbd1e4 100644 --- a/packages/cocoon_integration_test/lib/src/utilities/mocks.mocks.dart +++ b/packages/cocoon_integration_test/lib/src/utilities/mocks.mocks.dart @@ -177,6 +177,11 @@ class _FakeCheckRun_20 extends _i1.SmartFake implements _i7.CheckRun { : super(parent, parentInvocation); } +class _FakeCheckRunLockResult_20a extends _i1.SmartFake implements _i17.CheckRunLockResult { + _FakeCheckRunLockResult_20a(Object parent, Invocation parentInvocation) + : super(parent, parentInvocation); +} + class _FakePullRequest_21 extends _i1.SmartFake implements _i7.PullRequest { _FakePullRequest_21(Object parent, Invocation parentInvocation) : super(parent, parentInvocation); @@ -5712,7 +5717,7 @@ class MockScheduler extends _i1.Mock implements _i17.Scheduler { as _i13.Future); @override - _i13.Future<_i7.CheckRun> lockMergeGroupChecks( + _i13.Future<_i17.CheckRunLockResult> lockMergeGroupChecks( _i7.RepositorySlug? slug, String? headSha, { String? detailsUrl, @@ -5724,8 +5729,8 @@ class MockScheduler extends _i1.Mock implements _i17.Scheduler { [slug, headSha], {#detailsUrl: detailsUrl, #isUnifiedCheckRun: isUnifiedCheckRun}, ), - returnValue: _i13.Future<_i7.CheckRun>.value( - _FakeCheckRun_20( + returnValue: _i13.Future<_i17.CheckRunLockResult>.value( + _FakeCheckRunLockResult_20a( this, Invocation.method( #lockMergeGroupChecks, @@ -5738,7 +5743,7 @@ class MockScheduler extends _i1.Mock implements _i17.Scheduler { ), ), ) - as _i13.Future<_i7.CheckRun>); + as _i13.Future<_i17.CheckRunLockResult>); @override _i13.Future<_i7.CheckRun?> createAwaitingCicdLabelCheckRun( From df03ed8f4e01b2ce7216ea9d46c71ef700e32df6 Mon Sep 17 00:00:00 2001 From: Dmitry Grand Date: Tue, 21 Jul 2026 17:28:09 -0700 Subject: [PATCH 4/7] split checkRunGuard into dashboardChecka and mergeQueueGuard --- .../model/common/failed_presubmit_jobs.dart | 10 +-- .../rerun_all_failed_jobs.dart | 14 ++-- .../request_handlers/rerun_failed_job.dart | 2 +- .../service/firestore/unified_check_run.dart | 18 ++-- .../lib/src/service/luci_build_service.dart | 31 ++++--- app_dart/lib/src/service/scheduler.dart | 82 ++++++++++--------- .../rerun_all_failed_jobs_test.dart | 6 +- .../rerun_failed_job_test.dart | 6 +- .../firestore/unified_check_run_test.dart | 4 +- .../schedule_try_builds_test.dart | 4 +- .../service/scheduler/hash_workflow_test.dart | 2 +- app_dart/test/service/scheduler_test.dart | 64 ++++++++++----- .../lib/src/utilities/mocks.mocks.dart | 12 ++- 13 files changed, 145 insertions(+), 110 deletions(-) diff --git a/app_dart/lib/src/model/common/failed_presubmit_jobs.dart b/app_dart/lib/src/model/common/failed_presubmit_jobs.dart index bbe7e7ec6..9f71516cc 100644 --- a/app_dart/lib/src/model/common/failed_presubmit_jobs.dart +++ b/app_dart/lib/src/model/common/failed_presubmit_jobs.dart @@ -15,12 +15,12 @@ import '../firestore/base.dart'; /// /// See: [UnifiedCheckRun.reInitializeFailedJobs] class FailedJobsForRerun { - final CheckRun checkRunGuard; + final CheckRun dashboardChecks; final CiStage stage; final Map jobRetries; const FailedJobsForRerun({ - required this.checkRunGuard, + required this.dashboardChecks, required this.stage, required this.jobRetries, }); @@ -29,13 +29,13 @@ class FailedJobsForRerun { bool operator ==(Object other) => identical(this, other) || (other is FailedJobsForRerun && - other.checkRunGuard == checkRunGuard && + other.dashboardChecks == dashboardChecks && other.stage == stage && const DeepCollectionEquality().equals(other.jobRetries, jobRetries)); @override int get hashCode => Object.hashAll([ - checkRunGuard, + dashboardChecks, stage, ...jobRetries.keys, ...jobRetries.values, @@ -43,5 +43,5 @@ class FailedJobsForRerun { @override String toString() => - 'FailedChecksForRerun("$checkRunGuard", "$stage", "$jobRetries")'; + 'FailedChecksForRerun("$dashboardChecks", "$stage", "$jobRetries")'; } diff --git a/app_dart/lib/src/request_handlers/rerun_all_failed_jobs.dart b/app_dart/lib/src/request_handlers/rerun_all_failed_jobs.dart index 2b8da537d..1a2ad2256 100644 --- a/app_dart/lib/src/request_handlers/rerun_all_failed_jobs.dart +++ b/app_dart/lib/src/request_handlers/rerun_all_failed_jobs.dart @@ -76,7 +76,7 @@ final class RerunAllFailedJobs extends ApiRequestHandler { // We're doing a transactional update, which could fail if multiple tasks // are running at the same time so retry a sane amount of times before // giving up. - final failedChecks = await const RetryOptions().retry( + final failedJobs = await const RetryOptions().retry( () => UnifiedCheckRun.reInitializeFailedJobs( firestoreService: _firestore, slug: slug, @@ -85,7 +85,7 @@ final class RerunAllFailedJobs extends ApiRequestHandler { ), ); - if (failedChecks == null) { + if (failedJobs == null) { throw const BadRequestException('No failed jobs found to re-run'); } @@ -96,12 +96,12 @@ final class RerunAllFailedJobs extends ApiRequestHandler { final checkRetries = {}; for (final target in targets) { - if (failedChecks.jobRetries.containsKey(target.name)) { - checkRetries[target] = failedChecks.jobRetries[target.name]!; + if (failedJobs.jobRetries.containsKey(target.name)) { + checkRetries[target] = failedJobs.jobRetries[target.name]!; } } - if (checkRetries.length != failedChecks.jobRetries.length) { + if (checkRetries.length != failedJobs.jobRetries.length) { throw const NotFoundException( 'Failed to find all failed targets in presubmit targets', ); @@ -111,8 +111,8 @@ final class RerunAllFailedJobs extends ApiRequestHandler { targets: checkRetries, pullRequest: pullRequest, engineArtifacts: artifacts, - checkRunGuard: failedChecks.checkRunGuard, - stage: failedChecks.stage, + dashboardChecks: failedJobs.dashboardChecks, + stage: failedJobs.stage, ); return Response.json({ diff --git a/app_dart/lib/src/request_handlers/rerun_failed_job.dart b/app_dart/lib/src/request_handlers/rerun_failed_job.dart index e965ad8d5..484853229 100644 --- a/app_dart/lib/src/request_handlers/rerun_failed_job.dart +++ b/app_dart/lib/src/request_handlers/rerun_failed_job.dart @@ -111,7 +111,7 @@ final class RerunFailedJob extends ApiRequestHandler { targets: {target: retries}, pullRequest: pullRequest, engineArtifacts: artifacts, - checkRunGuard: rerunInfo.checkRunGuard, + dashboardChecks: rerunInfo.dashboardChecks, stage: rerunInfo.stage, ); 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 4ad203083..fca4cf3ec 100644 --- a/app_dart/lib/src/service/firestore/unified_check_run.dart +++ b/app_dart/lib/src/service/firestore/unified_check_run.dart @@ -31,11 +31,11 @@ final class UnifiedCheckRun { required List tasks, required Config config, PullRequest? pullRequest, - CheckRun? checkRun, - CheckRun? checkRunGuard, + CheckRun? dashboardChecks, + CheckRun? mergeQueueGuard, @visibleForTesting DateTime Function() utcNow = DateTime.timestamp, }) async { - if (checkRun != null && + if (dashboardChecks != null && pullRequest != null && config.flags.isUnifiedCheckRunFlowEnabledForUser( pullRequest.user!.login!, @@ -49,8 +49,8 @@ final class UnifiedCheckRun { // was succeeded so we are interested in a state of the latest one. final creationTime = utcNow().millisecondsSinceEpoch; final guard = PresubmitGuard( - checkRun: checkRun, - checkRunGuard: checkRunGuard, + checkRun: dashboardChecks, + checkRunGuard: mergeQueueGuard, headSha: sha, slug: slug, prNum: pullRequest.number!, @@ -66,7 +66,7 @@ final class UnifiedCheckRun { PresubmitJob.init( slug: slug, jobName: task, - checkRunId: checkRun.id!, + checkRunId: dashboardChecks.id!, creationTime: creationTime, ), ]; @@ -81,7 +81,7 @@ final class UnifiedCheckRun { sha: sha, stage: stage, tasks: tasks, - checkRunGuard: checkRun != null ? '$checkRun' : '', + checkRunGuard: mergeQueueGuard != null ? '$mergeQueueGuard' : '', ); } } @@ -165,7 +165,7 @@ final class UnifiedCheckRun { '$logCrumb: results = ${response.writeResults?.map((e) => e.toJson())}', ); return FailedJobsForRerun( - checkRunGuard: latestGuard.checkRun, + dashboardChecks: latestGuard.checkRun, jobRetries: checkRetries, stage: latestGuard.stage, ); @@ -249,7 +249,7 @@ final class UnifiedCheckRun { '$logCrumb: results = ${response.writeResults?.map((e) => e.toJson())}', ); return FailedJobsForRerun( - checkRunGuard: guard.checkRun, + dashboardChecks: guard.checkRun, jobRetries: {jobName: (latestCheck?.attemptNumber ?? 0) + 1}, stage: guard.stage, ); diff --git a/app_dart/lib/src/service/luci_build_service.dart b/app_dart/lib/src/service/luci_build_service.dart index b834078b9..1a60f7ea4 100644 --- a/app_dart/lib/src/service/luci_build_service.dart +++ b/app_dart/lib/src/service/luci_build_service.dart @@ -242,14 +242,16 @@ class LuciBuildService { required List targets, required PullRequest pullRequest, required EngineArtifacts engineArtifacts, - CheckRun? checkRunGuard, + CheckRun? dashboardChecks, + CheckRun? mergeQueueGuard, CiStage? stage, }) async { return _scheduleTryBuilds( targets: {for (final target in targets) target: 1}, pullRequest: pullRequest, engineArtifacts: engineArtifacts, - checkRunGuard: checkRunGuard, + dashboardChecks: dashboardChecks, + mergeQueueGuard: mergeQueueGuard, stage: stage, ); } @@ -259,14 +261,16 @@ class LuciBuildService { required Map targets, required PullRequest pullRequest, required EngineArtifacts engineArtifacts, - required CheckRun checkRunGuard, + CheckRun? dashboardChecks, + CheckRun? mergeQueueGuard, required CiStage stage, }) async { return _scheduleTryBuilds( targets: targets, pullRequest: pullRequest, engineArtifacts: engineArtifacts, - checkRunGuard: checkRunGuard, + dashboardChecks: dashboardChecks, + mergeQueueGuard: mergeQueueGuard, stage: stage, ); } @@ -276,7 +280,8 @@ class LuciBuildService { required Map targets, required PullRequest pullRequest, required EngineArtifacts engineArtifacts, - CheckRun? checkRunGuard, + CheckRun? dashboardChecks, + CheckRun? mergeQueueGuard, CiStage? stage, }) async { if (targets.isEmpty) { @@ -302,26 +307,26 @@ class LuciBuildService { 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. - if (isUnifiedCheckRunFlow && checkRunGuard != null) { + if (isUnifiedCheckRunFlow && dashboardChecks != null) { userData = PresubmitUserData( commit: CommitRef(slug: slug, sha: commitSha, branch: commitBranch), - guardCheckRunId: checkRunGuard.id!, - checkSuiteId: checkRunGuard.checkSuiteId, + guardCheckRunId: dashboardChecks.id!, + checkSuiteId: dashboardChecks.checkSuiteId, pullRequestNumber: pullRequest.number!, stage: stage, ); // We need to store PR to checkrun mapping in order to get PR later in // [Scheduler.proceedUnifiedCheckRunToTestingStage] method - checkRuns.add(checkRunGuard); + checkRuns.add(dashboardChecks); log.info( - 'Created unified check run ${checkRunGuard.id} for PR# ${pullRequest.number}', + 'Created unified check run ${dashboardChecks.id} for PR# ${pullRequest.number}', ); } 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. - if (!isUnifiedCheckRunFlow || checkRunGuard == null) { + if (!isUnifiedCheckRunFlow || dashboardChecks == null) { final checkRun = await _githubChecksUtil.createCheckRun( _config, target.slug, @@ -391,9 +396,9 @@ class LuciBuildService { userData: userData, properties: properties, // if unified check run flow is enabled, use guard check run othervise check run id. - tags: isUnifiedCheckRunFlow && checkRunGuard != null + tags: isUnifiedCheckRunFlow && dashboardChecks != null ? BuildTags([ - GuardCheckRunIdBuildTag(guardCheckRunId: checkRunGuard.id!), + GuardCheckRunIdBuildTag(guardCheckRunId: dashboardChecks.id!), if (attemptNumber > 1) CurrentAttemptBuildTag(attemptNumber: attemptNumber), if (isOrderedPresubmit) diff --git a/app_dart/lib/src/service/scheduler.dart b/app_dart/lib/src/service/scheduler.dart index c44b57282..ea1d52e87 100644 --- a/app_dart/lib/src/service/scheduler.dart +++ b/app_dart/lib/src/service/scheduler.dart @@ -374,8 +374,8 @@ class Scheduler { : null, isUnifiedCheckRun: isUnifiedCheckRun, ); - final lock = lockResult.lock; - final checkRunGuard = lockResult.checkRunGuard; + final dashboardChecks = lockResult.dashboardChecks; + final mergeQueueGuard = lockResult.mergeQueueGuard; // Track if we should unlock the merge group lock in case of non-fusion or // revert bots. @@ -412,14 +412,14 @@ class Scheduler { tasks: [], pullRequest: pullRequest, config: _config, - checkRun: lock, - checkRunGuard: checkRunGuard, + dashboardChecks: dashboardChecks, + mergeQueueGuard: mergeQueueGuard, ); await _runCiTestingStage( pullRequest: pullRequest, - checkRunGuard: lock, - checkRunGuardCheck: checkRunGuard, + dashboardChecks: dashboardChecks, + mergeQueueGuard: mergeQueueGuard, logCrumb: logCrumb, // The if-branch already skips the engine build phase. @@ -453,8 +453,8 @@ class Scheduler { tasks: [...presubmitTriggerTargets.map((t) => t.name)], pullRequest: pullRequest, config: _config, - checkRun: lock, - checkRunGuard: checkRunGuard, + dashboardChecks: dashboardChecks, + mergeQueueGuard: mergeQueueGuard, ); // Even though this appears to be an engine build, it could be a @@ -479,8 +479,8 @@ class Scheduler { tasks: [...presubmitTriggerTargets.map((t) => t.name)], pullRequest: pullRequest, config: _config, - checkRun: lock, - checkRunGuard: checkRunGuard, + dashboardChecks: dashboardChecks, + mergeQueueGuard: mergeQueueGuard, ); } engineArtifacts = const EngineArtifacts.noFrameworkTests( @@ -491,7 +491,8 @@ class Scheduler { targets: presubmitTriggerTargets, pullRequest: pullRequest, engineArtifacts: engineArtifacts, - checkRunGuard: lock, + dashboardChecks: dashboardChecks, + mergeQueueGuard: mergeQueueGuard, stage: isFusion ? CiStage.fusionEngineBuild : CiStage.genericTests, ); } on FormatException catch (e, s) { @@ -528,7 +529,7 @@ class Scheduler { // there are situations (see code above) when it needs to be unlocked // immediately. if (unlockMergeGroup) { - await unlockMergeQueueGuard(slug, sha, lock); + await unlockMergeQueueGuard(slug, sha, dashboardChecks); } log.info( 'Finished triggering builds for: pr ${pullRequest.number}, commit $sha, branch ${pullRequest.head!.ref} and slug $slug}', @@ -652,12 +653,13 @@ class Scheduler { headSha, isUnifiedCheckRun: false, ); - final lock = lockResult.lock; + final dashboardChecks = lockResult.dashboardChecks; + final mergeQueueGuard = lockResult.mergeQueueGuard!; // 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, lock); + await unlockMergeQueueGuard(slug, headSha, mergeQueueGuard); return; } @@ -689,8 +691,8 @@ class Scheduler { stage: CiStage.fusionEngineBuild, tasks: [...availableTargets.map((t) => t.name)], config: _config, - checkRun: lock, - checkRunGuard: lockResult.checkRunGuard, + dashboardChecks: dashboardChecks, + mergeQueueGuard: mergeQueueGuard, ); // Create the minimal Commit needed to pass the next stage. @@ -718,7 +720,7 @@ class Scheduler { // only required GitHub check. await failGuardForMergeGroup( slug: slug, - lock: lock, + lock: mergeQueueGuard, headSha: headSha, summary: 'Failed to schedule checks for merge group', details: @@ -864,7 +866,8 @@ $s String? detailsUrl, required bool isUnifiedCheckRun, }) async { - final mqGuard = await _githubChecksService.githubChecksUtil.createCheckRun( + final mergeQueueGuard = await _githubChecksService.githubChecksUtil + .createCheckRun( _config, slug, headSha, @@ -876,7 +879,7 @@ $s detailsUrl: isUnifiedCheckRun ? null : detailsUrl, ); - final flutterPresubmits = await _githubChecksService.githubChecksUtil + final dashboardChecks = await _githubChecksService.githubChecksUtil .createCheckRun( _config, slug, @@ -889,22 +892,20 @@ $s detailsUrl: isUnifiedCheckRun ? detailsUrl : null, ); - if (isUnifiedCheckRun) { - return CheckRunLockResult( - lock: flutterPresubmits, - checkRunGuard: mqGuard, - ); - } else { + if (!isUnifiedCheckRun) { // Skip Dashboard Checks await _githubChecksService.githubChecksUtil.updateCheckRun( _config, slug, - flutterPresubmits, + dashboardChecks, status: CheckRunStatus.completed, conclusion: CheckRunConclusion.success, ); - return CheckRunLockResult(lock: mqGuard, checkRunGuard: null); } + return CheckRunLockResult( + dashboardChecks: dashboardChecks, + mergeQueueGuard: mergeQueueGuard, + ); } /// Creates a pending check run for "Awaiting CICD label" if it doesn't exist. @@ -1416,8 +1417,8 @@ detailsUrl: $detailsUrl /// Schedules post-engine build tests (i.e. engine tests, and framework tests). Future _runCiTestingStage({ required PullRequest pullRequest, - required CheckRun checkRunGuard, - CheckRun? checkRunGuardCheck, + required CheckRun dashboardChecks, + CheckRun? mergeQueueGuard, required String logCrumb, required _FlutterRepoTestsToRun testsToRun, }) async { @@ -1459,8 +1460,8 @@ detailsUrl: $detailsUrl tasks: tasks, config: _config, pullRequest: pullRequest, - checkRun: checkRunGuard, - checkRunGuard: checkRunGuardCheck, + dashboardChecks: dashboardChecks, + mergeQueueGuard: mergeQueueGuard, ); // Here is where it gets fun: how do framework tests* know what engine @@ -1487,7 +1488,8 @@ detailsUrl: $detailsUrl targets: presubmitTargets, pullRequest: pullRequest, engineArtifacts: engineArtifacts, - checkRunGuard: checkRunGuard, + dashboardChecks: dashboardChecks, + mergeQueueGuard: mergeQueueGuard, stage: CiStage.fusionTests, ); } on FormatException catch (e, s) { @@ -1535,7 +1537,7 @@ detailsUrl: $detailsUrl try { await _runCiTestingStage( pullRequest: pullRequest, - checkRunGuard: checkRunGuard, + dashboardChecks: checkRunGuard, logCrumb: logCrumb, testsToRun: _FlutterRepoTestsToRun.engineTestsAndFrameworkTests, ); @@ -1820,7 +1822,8 @@ $stacktrace targets: targets, pullRequest: pullRequest, engineArtifacts: engineArtifacts, - checkRunGuard: null, + dashboardChecks: null, + mergeQueueGuard: null, stage: null, ); return const ProcessCheckRunResult.success(); @@ -1915,7 +1918,7 @@ $stacktrace targets: checkRetries, pullRequest: pullRequest, engineArtifacts: artifacts, - checkRunGuard: failedChecks.checkRunGuard, + dashboardChecks: failedChecks.dashboardChecks, stage: failedChecks.stage, ); @@ -2042,8 +2045,11 @@ enum _TaskCommitScheduling { } class CheckRunLockResult { - final CheckRun lock; - final CheckRun? checkRunGuard; + final CheckRun dashboardChecks; + final CheckRun? mergeQueueGuard; - const CheckRunLockResult({required this.lock, this.checkRunGuard}); + const CheckRunLockResult({ + required this.dashboardChecks, + this.mergeQueueGuard, + }); } diff --git a/app_dart/test/request_handlers/rerun_all_failed_jobs_test.dart b/app_dart/test/request_handlers/rerun_all_failed_jobs_test.dart index 9316256bf..4c5697cae 100644 --- a/app_dart/test/request_handlers/rerun_all_failed_jobs_test.dart +++ b/app_dart/test/request_handlers/rerun_all_failed_jobs_test.dart @@ -94,7 +94,7 @@ void main() { targets: anyNamed('targets'), pullRequest: anyNamed('pullRequest'), engineArtifacts: anyNamed('engineArtifacts'), - checkRunGuard: anyNamed('checkRunGuard'), + dashboardChecks: anyNamed('dashboardChecks'), stage: anyNamed('stage'), ), ).thenAnswer((_) async => []); @@ -113,7 +113,7 @@ void main() { targets: argThat(containsPair(targetA, 2), named: 'targets'), pullRequest: anyNamed('pullRequest'), engineArtifacts: anyNamed('engineArtifacts'), - checkRunGuard: anyNamed('checkRunGuard'), + dashboardChecks: anyNamed('dashboardChecks'), stage: anyNamed('stage'), ), ).called(1); @@ -165,7 +165,7 @@ void main() { targets: anyNamed('targets'), pullRequest: anyNamed('pullRequest'), engineArtifacts: anyNamed('engineArtifacts'), - checkRunGuard: anyNamed('checkRunGuard'), + dashboardChecks: anyNamed('dashboardChecks'), stage: anyNamed('stage'), ), ).thenAnswer((_) async => []); diff --git a/app_dart/test/request_handlers/rerun_failed_job_test.dart b/app_dart/test/request_handlers/rerun_failed_job_test.dart index 02da4dd22..b6c96b049 100644 --- a/app_dart/test/request_handlers/rerun_failed_job_test.dart +++ b/app_dart/test/request_handlers/rerun_failed_job_test.dart @@ -93,7 +93,7 @@ void main() { targets: anyNamed('targets'), pullRequest: anyNamed('pullRequest'), engineArtifacts: anyNamed('engineArtifacts'), - checkRunGuard: anyNamed('checkRunGuard'), + dashboardChecks: anyNamed('dashboardChecks'), stage: anyNamed('stage'), ), ).thenAnswer((_) async => []); @@ -113,7 +113,7 @@ void main() { targets: argThat(containsPair(targetA, 2), named: 'targets'), pullRequest: anyNamed('pullRequest'), engineArtifacts: anyNamed('engineArtifacts'), - checkRunGuard: anyNamed('checkRunGuard'), + dashboardChecks: anyNamed('dashboardChecks'), stage: anyNamed('stage'), ), ).called(1); @@ -166,7 +166,7 @@ void main() { targets: anyNamed('targets'), pullRequest: anyNamed('pullRequest'), engineArtifacts: anyNamed('engineArtifacts'), - checkRunGuard: anyNamed('checkRunGuard'), + dashboardChecks: anyNamed('dashboardChecks'), stage: anyNamed('stage'), ), ).thenAnswer((_) async => []); 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 aa47ae90f..c9353b5f5 100644 --- a/app_dart/test/service/firestore/unified_check_run_test.dart +++ b/app_dart/test/service/firestore/unified_check_run_test.dart @@ -61,7 +61,7 @@ void main() { tasks: ['linux', 'mac'], config: config, pullRequest: pullRequest, - checkRun: checkRun, + dashboardChecks: checkRun, ); final guardId = PresubmitGuard.documentIdFor( @@ -100,7 +100,7 @@ void main() { tasks: ['linux', 'mac'], config: config, pullRequest: pullRequest, - checkRun: checkRun, + mergeQueueGuard: checkRun, ); // Verify PresubmitGuard is NOT created 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 e31f0d190..bafc563e4 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 @@ -439,7 +439,7 @@ void main() { engineArtifacts: EngineArtifacts.builtFromSource( commitSha: pullRequest.head!.sha!, ), - checkRunGuard: checkRunGuard, + dashboardChecks: checkRunGuard, stage: CiStage.fusionTests, ), completion([isTarget.hasName('Linux foo')]), @@ -513,7 +513,7 @@ void main() { engineArtifacts: EngineArtifacts.builtFromSource( commitSha: pullRequest.head!.sha!, ), - checkRunGuard: null, // No guard provided + dashboardChecks: null, // No guard provided ), completion([isTarget.hasName('Linux foo')]), ); diff --git a/app_dart/test/service/scheduler/hash_workflow_test.dart b/app_dart/test/service/scheduler/hash_workflow_test.dart index e82cfbbfd..bce30a91e 100644 --- a/app_dart/test/service/scheduler/hash_workflow_test.dart +++ b/app_dart/test/service/scheduler/hash_workflow_test.dart @@ -54,7 +54,7 @@ void main() { targets: anyNamed('targets'), pullRequest: anyNamed('pullRequest'), engineArtifacts: anyNamed('engineArtifacts'), - checkRunGuard: anyNamed('checkRunGuard'), + dashboardChecks: anyNamed('dashboardChecks'), ), ).thenAnswer((inv) async { return []; diff --git a/app_dart/test/service/scheduler_test.dart b/app_dart/test/service/scheduler_test.dart index 6015bf299..364a9ab90 100644 --- a/app_dart/test/service/scheduler_test.dart +++ b/app_dart/test/service/scheduler_test.dart @@ -1152,7 +1152,8 @@ void main() { targets: anyNamed('targets'), pullRequest: anyNamed('pullRequest'), engineArtifacts: anyNamed('engineArtifacts'), - checkRunGuard: anyNamed('checkRunGuard'), + dashboardChecks: anyNamed('dashboardChecks'), + mergeQueueGuard: anyNamed('mergeQueueGuard'), stage: anyNamed('stage'), ), ).thenAnswer((inv) async { @@ -1456,7 +1457,8 @@ targets: targets: anyNamed('targets'), pullRequest: anyNamed('pullRequest'), engineArtifacts: anyNamed('engineArtifacts'), - checkRunGuard: anyNamed('checkRunGuard'), + dashboardChecks: anyNamed('dashboardChecks'), + mergeQueueGuard: anyNamed('mergeQueueGuard'), stage: anyNamed('stage'), ), ).thenAnswer((_) async => []); @@ -1476,7 +1478,8 @@ targets: ), pullRequest: pullRequest, engineArtifacts: anyNamed('engineArtifacts'), - checkRunGuard: anyNamed('checkRunGuard'), + dashboardChecks: anyNamed('dashboardChecks'), + mergeQueueGuard: anyNamed('mergeQueueGuard'), stage: CiStage.fusionEngineBuild, ), ).called(1); @@ -1601,7 +1604,8 @@ targets: targets: anyNamed('targets'), pullRequest: anyNamed('pullRequest'), engineArtifacts: anyNamed('engineArtifacts'), - checkRunGuard: anyNamed('checkRunGuard'), + dashboardChecks: anyNamed('dashboardChecks'), + mergeQueueGuard: anyNamed('mergeQueueGuard'), stage: anyNamed('stage'), ), ).thenAnswer((_) async => []); @@ -1621,7 +1625,8 @@ targets: ), pullRequest: pullRequest, engineArtifacts: anyNamed('engineArtifacts'), - checkRunGuard: anyNamed('checkRunGuard'), + dashboardChecks: anyNamed('dashboardChecks'), + mergeQueueGuard: anyNamed('mergeQueueGuard'), stage: CiStage.fusionTests, ), ).called(1); @@ -1883,7 +1888,8 @@ targets: targets: anyNamed('targets'), pullRequest: anyNamed('pullRequest'), engineArtifacts: anyNamed('engineArtifacts'), - checkRunGuard: anyNamed('checkRunGuard'), + dashboardChecks: anyNamed('dashboardChecks'), + mergeQueueGuard: anyNamed('mergeQueueGuard'), stage: anyNamed('stage'), ), ).thenAnswer((inv) async { @@ -1980,7 +1986,8 @@ targets: targets: captureAnyNamed('targets'), pullRequest: captureAnyNamed('pullRequest'), engineArtifacts: anyNamed('engineArtifacts'), - checkRunGuard: anyNamed('checkRunGuard'), + dashboardChecks: anyNamed('dashboardChecks'), + mergeQueueGuard: anyNamed('mergeQueueGuard'), stage: anyNamed('stage'), ), ); @@ -2034,7 +2041,8 @@ targets: targets: anyNamed('targets'), pullRequest: anyNamed('pullRequest'), engineArtifacts: anyNamed('engineArtifacts'), - checkRunGuard: anyNamed('checkRunGuard'), + dashboardChecks: anyNamed('dashboardChecks'), + mergeQueueGuard: anyNamed('mergeQueueGuard'), stage: anyNamed('stage'), ), ).thenAnswer((inv) async { @@ -2131,7 +2139,8 @@ targets: targets: captureAnyNamed('targets'), pullRequest: captureAnyNamed('pullRequest'), engineArtifacts: anyNamed('engineArtifacts'), - checkRunGuard: anyNamed('checkRunGuard'), + dashboardChecks: anyNamed('dashboardChecks'), + mergeQueueGuard: anyNamed('mergeQueueGuard'), stage: anyNamed('stage'), ), ); @@ -2380,7 +2389,8 @@ targets: targets: anyNamed('targets'), pullRequest: anyNamed('pullRequest'), engineArtifacts: anyNamed('engineArtifacts'), - checkRunGuard: anyNamed('checkRunGuard'), + dashboardChecks: anyNamed('dashboardChecks'), + mergeQueueGuard: anyNamed('mergeQueueGuard'), stage: anyNamed('stage'), ), ).thenAnswer((Invocation i) async { @@ -2722,7 +2732,8 @@ targets: targets: anyNamed('targets'), pullRequest: anyNamed('pullRequest'), engineArtifacts: anyNamed('engineArtifacts'), - checkRunGuard: anyNamed('checkRunGuard'), + dashboardChecks: anyNamed('dashboardChecks'), + mergeQueueGuard: anyNamed('mergeQueueGuard'), stage: anyNamed('stage'), ), ).thenAnswer((inv) async { @@ -2807,7 +2818,8 @@ targets: targets: captureAnyNamed('targets'), pullRequest: captureAnyNamed('pullRequest'), engineArtifacts: anyNamed('engineArtifacts'), - checkRunGuard: anyNamed('checkRunGuard'), + dashboardChecks: anyNamed('dashboardChecks'), + mergeQueueGuard: anyNamed('mergeQueueGuard'), stage: anyNamed('stage'), ), ); @@ -3084,8 +3096,8 @@ targets: isUnifiedCheckRun: true, ); - expect(lockResult.lock.name, Config.kDashboardCheckName); - expect(lockResult.checkRunGuard?.name, Config.kMergeQueueLockName); + expect(lockResult.dashboardChecks.name, Config.kDashboardCheckName); + expect(lockResult.mergeQueueGuard?.name, Config.kMergeQueueLockName); verifyNever( mockGithubChecksUtil.updateCheckRun( @@ -3438,7 +3450,8 @@ targets: targets: anyNamed('targets'), pullRequest: anyNamed('pullRequest'), engineArtifacts: anyNamed('engineArtifacts'), - checkRunGuard: anyNamed('checkRunGuard'), + dashboardChecks: anyNamed('dashboardChecks'), + mergeQueueGuard: anyNamed('mergeQueueGuard'), stage: anyNamed('stage'), ), ).thenAnswer((inv) async { @@ -3527,7 +3540,8 @@ targets: targets: captureAnyNamed('targets'), pullRequest: anyNamed('pullRequest'), engineArtifacts: anyNamed('engineArtifacts'), - checkRunGuard: anyNamed('checkRunGuard'), + dashboardChecks: anyNamed('dashboardChecks'), + mergeQueueGuard: anyNamed('mergeQueueGuard'), stage: anyNamed('stage'), ), ); @@ -3591,7 +3605,8 @@ targets: targets: anyNamed('targets'), pullRequest: anyNamed('pullRequest'), engineArtifacts: anyNamed('engineArtifacts'), - checkRunGuard: anyNamed('checkRunGuard'), + dashboardChecks: anyNamed('dashboardChecks'), + mergeQueueGuard: anyNamed('mergeQueueGuard'), stage: anyNamed('stage'), ), ).thenAnswer((inv) async { @@ -3733,7 +3748,8 @@ targets: targets: anyNamed('targets'), pullRequest: anyNamed('pullRequest'), engineArtifacts: anyNamed('engineArtifacts'), - checkRunGuard: anyNamed('checkRunGuard'), + dashboardChecks: anyNamed('dashboardChecks'), + mergeQueueGuard: anyNamed('mergeQueueGuard'), stage: anyNamed('stage'), ), ).thenAnswer((inv) async { @@ -3864,7 +3880,8 @@ targets: targets: anyNamed('targets'), pullRequest: anyNamed('pullRequest'), engineArtifacts: anyNamed('engineArtifacts'), - checkRunGuard: anyNamed('checkRunGuard'), + dashboardChecks: anyNamed('dashboardChecks'), + mergeQueueGuard: anyNamed('mergeQueueGuard'), stage: anyNamed('stage'), ), ).thenAnswer((inv) async { @@ -4780,7 +4797,8 @@ final class _CapturingFakeLuciBuildService extends Fake List scheduledTryBuilds = []; EngineArtifacts? engineArtifacts; PullRequest? pullRequest; - CheckRun? checkRunGuard; + CheckRun? dashboardChecks; + CheckRun? mergeQueueGuard; CiStage? stage; @override @@ -4788,13 +4806,15 @@ final class _CapturingFakeLuciBuildService extends Fake required List targets, required PullRequest pullRequest, required EngineArtifacts engineArtifacts, - CheckRun? checkRunGuard, + CheckRun? dashboardChecks, + CheckRun? mergeQueueGuard, CiStage? stage, }) async { scheduledTryBuilds = targets; this.engineArtifacts = engineArtifacts; this.pullRequest = pullRequest; - this.checkRunGuard = checkRunGuard; + this.dashboardChecks = dashboardChecks; + this.mergeQueueGuard = mergeQueueGuard; this.stage = stage; return targets; } 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 1d3bbd1e4..ae754ba5b 100644 --- a/packages/cocoon_integration_test/lib/src/utilities/mocks.mocks.dart +++ b/packages/cocoon_integration_test/lib/src/utilities/mocks.mocks.dart @@ -4147,7 +4147,8 @@ class MockLuciBuildService extends _i1.Mock implements _i17.LuciBuildService { required List<_i27.Target>? targets, required _i7.PullRequest? pullRequest, required _i28.EngineArtifacts? engineArtifacts, - _i7.CheckRun? checkRunGuard, + _i7.CheckRun? dashboardChecks, + _i7.CheckRun? mergeQueueGuard, _i17.CiStage? stage, }) => (super.noSuchMethod( @@ -4155,7 +4156,8 @@ class MockLuciBuildService extends _i1.Mock implements _i17.LuciBuildService { #targets: targets, #pullRequest: pullRequest, #engineArtifacts: engineArtifacts, - #checkRunGuard: checkRunGuard, + #dashboardChecks: dashboardChecks, + #mergeQueueGuard: mergeQueueGuard, #stage: stage, }), returnValue: _i13.Future>.value(<_i27.Target>[]), @@ -4167,7 +4169,8 @@ class MockLuciBuildService extends _i1.Mock implements _i17.LuciBuildService { required Map<_i27.Target, int>? targets, required _i7.PullRequest? pullRequest, required _i28.EngineArtifacts? engineArtifacts, - required _i7.CheckRun? checkRunGuard, + _i7.CheckRun? dashboardChecks, + _i7.CheckRun? mergeQueueGuard, required _i17.CiStage? stage, }) => (super.noSuchMethod( @@ -4175,7 +4178,8 @@ class MockLuciBuildService extends _i1.Mock implements _i17.LuciBuildService { #targets: targets, #pullRequest: pullRequest, #engineArtifacts: engineArtifacts, - #checkRunGuard: checkRunGuard, + #dashboardChecks: dashboardChecks, + #mergeQueueGuard: mergeQueueGuard, #stage: stage, }), returnValue: _i13.Future>.value(<_i27.Target>[]), From 346efbb1b3862fe680512bc39dac6dbb74452ab6 Mon Sep 17 00:00:00 2001 From: Dmitry Grand Date: Tue, 21 Jul 2026 18:51:17 -0700 Subject: [PATCH 5/7] unlock merge queue guard when jobs compleated and fail if in merge queue --- .../common/presubmit_guard_conclusion.dart | 14 ++- .../lib/src/model/firestore/ci_staging.dart | 6 +- .../service/firestore/unified_check_run.dart | 9 +- app_dart/lib/src/service/scheduler.dart | 117 +++++++++++------- .../test/model/firestore/ci_staging_test.dart | 14 +-- .../github/webhook_subscription_test.dart | 61 +++++---- app_dart/test/service/scheduler_test.dart | 44 +++++-- .../lib/src/utilities/mocks.mocks.dart | 97 ++++++++------- 8 files changed, 209 insertions(+), 153 deletions(-) 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 dc4d7c20e..dc6b3b15e 100644 --- a/app_dart/lib/src/model/common/presubmit_guard_conclusion.dart +++ b/app_dart/lib/src/model/common/presubmit_guard_conclusion.dart @@ -40,7 +40,8 @@ enum PresubmitGuardConclusionResult { class PresubmitGuardConclusion { final PresubmitGuardConclusionResult result; final int remaining; - final String? checkRunGuard; + final String? dashboardChecks; + final String? mergeQueueGuard; final int failed; final List failedJobNames; final String summary; @@ -49,7 +50,8 @@ class PresubmitGuardConclusion { const PresubmitGuardConclusion({ required this.result, required this.remaining, - required this.checkRunGuard, + this.dashboardChecks, + required this.mergeQueueGuard, required this.failed, this.failedJobNames = const [], required this.summary, @@ -70,7 +72,8 @@ class PresubmitGuardConclusion { (other is PresubmitGuardConclusion && other.result == result && other.remaining == remaining && - other.checkRunGuard == checkRunGuard && + other.dashboardChecks == dashboardChecks && + other.mergeQueueGuard == mergeQueueGuard && other.failed == failed && const ListEquality().equals( other.failedJobNames, @@ -83,7 +86,8 @@ class PresubmitGuardConclusion { int get hashCode => Object.hashAll([ result, remaining, - checkRunGuard, + dashboardChecks, + mergeQueueGuard, failed, Object.hashAll(failedJobNames), summary, @@ -92,5 +96,5 @@ class PresubmitGuardConclusion { @override String toString() => - 'BuildConclusion("$result", "$remaining", "$failed", "$failedJobNames", "$summary", "$details", "$checkRunGuard")'; + 'BuildConclusion("$result", "$remaining", "$failed", "$failedJobNames", "$summary", "$details", "$dashboardChecks", "$mergeQueueGuard")'; } diff --git a/app_dart/lib/src/model/firestore/ci_staging.dart b/app_dart/lib/src/model/firestore/ci_staging.dart index cc9340618..509a9551e 100644 --- a/app_dart/lib/src/model/firestore/ci_staging.dart +++ b/app_dart/lib/src/model/firestore/ci_staging.dart @@ -287,7 +287,7 @@ final class CiStaging extends AppDocument { return PresubmitGuardConclusion( result: PresubmitGuardConclusionResult.missing, remaining: remaining, - checkRunGuard: null, + mergeQueueGuard: null, failed: failed, summary: 'Check run "$checkRun" not present in $stage CI stage', details: 'Change $changeCrumb', @@ -353,7 +353,7 @@ final class CiStaging extends AppDocument { return PresubmitGuardConclusion( result: PresubmitGuardConclusionResult.internalError, remaining: -1, - checkRunGuard: null, + mergeQueueGuard: null, failed: failed, summary: 'Internal server error', details: @@ -391,7 +391,7 @@ $stack ? PresubmitGuardConclusionResult.ok : PresubmitGuardConclusionResult.internalError, remaining: remaining, - checkRunGuard: checkRunGuard ?? '', + mergeQueueGuard: checkRunGuard ?? '', failed: failed, summary: valid ? 'All tests passed' 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 fca4cf3ec..81bafcfc6 100644 --- a/app_dart/lib/src/service/firestore/unified_check_run.dart +++ b/app_dart/lib/src/service/firestore/unified_check_run.dart @@ -561,7 +561,8 @@ final class UnifiedCheckRun { return PresubmitGuardConclusion( result: PresubmitGuardConclusionResult.missing, remaining: presubmitGuard.remainingJobs, - checkRunGuard: presubmitGuard.checkRunJson, + dashboardChecks: presubmitGuard.checkRunJson, + mergeQueueGuard: presubmitGuard.checkRunGuardJson, failed: presubmitGuard.failedJobs, summary: 'Check run "${state.jobName}" not present in ${guardId.stage} CI stage', @@ -656,7 +657,8 @@ final class UnifiedCheckRun { return PresubmitGuardConclusion( result: PresubmitGuardConclusionResult.internalError, remaining: -1, - checkRunGuard: null, + dashboardChecks: null, + mergeQueueGuard: null, failed: failed, summary: 'Internal server error', details: @@ -691,7 +693,8 @@ $stack ? PresubmitGuardConclusionResult.ok : PresubmitGuardConclusionResult.internalError, remaining: remaining, - checkRunGuard: presubmitGuard.checkRunJson, + dashboardChecks: presubmitGuard.checkRunJson, + mergeQueueGuard: presubmitGuard.checkRunGuardJson, failed: failed, failedJobNames: valid ? presubmitGuard.failedJobNames : const [], summary: valid diff --git a/app_dart/lib/src/service/scheduler.dart b/app_dart/lib/src/service/scheduler.dart index ea1d52e87..3a391c89d 100644 --- a/app_dart/lib/src/service/scheduler.dart +++ b/app_dart/lib/src/service/scheduler.dart @@ -529,7 +529,14 @@ class Scheduler { // there are situations (see code above) when it needs to be unlocked // immediately. if (unlockMergeGroup) { - await unlockMergeQueueGuard(slug, sha, dashboardChecks); + if (isUnifiedCheckRun) { + await unlockMergeQueueGuard(slug, sha, dashboardChecks); + if (mergeQueueGuard != null) { + await unlockMergeQueueGuard(slug, sha, mergeQueueGuard); + } + } else if (mergeQueueGuard != null) { + await unlockMergeQueueGuard(slug, sha, mergeQueueGuard); + } } log.info( 'Finished triggering builds for: pr ${pullRequest.number}, commit $sha, branch ${pullRequest.head!.ref} and slug $slug}', @@ -868,16 +875,16 @@ $s }) async { final mergeQueueGuard = await _githubChecksService.githubChecksUtil .createCheckRun( - _config, - slug, - headSha, - Config.kMergeQueueLockName, - output: const CheckRunOutput( - title: Config.kMergeQueueLockName, - summary: kMergeQueueLockDescription, - ), - detailsUrl: isUnifiedCheckRun ? null : detailsUrl, - ); + _config, + slug, + headSha, + Config.kMergeQueueLockName, + output: const CheckRunOutput( + title: Config.kMergeQueueLockName, + summary: kMergeQueueLockDescription, + ), + detailsUrl: isUnifiedCheckRun ? null : detailsUrl, + ); final dashboardChecks = await _githubChecksService.githubChecksUtil .createCheckRun( @@ -980,28 +987,6 @@ $s status: CheckRunStatus.completed, conclusion: CheckRunConclusion.success, ); - if (lock.name == Config.kDashboardCheckName) { - final githubService = await _config.createGithubService(slug); - final mqGuard = (await githubService.getCheckRunsFiltered( - slug: slug, - ref: headSha, - checkName: Config.kMergeQueueLockName, - )).singleOrNull; - if (mqGuard != null) { - log.info( - 'Unlocking Merge Queue Guard for unified check run for $slug/$headSha', - ); - await _githubChecksService.githubChecksUtil.updateCheckRun( - _config, - slug, - mqGuard, - status: CheckRunStatus.completed, - conclusion: CheckRunConclusion.success, - ); - } else { - log.warn('Merge Queue Guard not found for $slug/$headSha'); - } - } } /// Fails the "Merge Queue Guard" check for a merge group. @@ -1211,7 +1196,7 @@ detailsUrl: $detailsUrl // will hold up all other PRs that are trying to land. if (check.isMergeGroup) { await _completeArtifacts(check.sha, false); - final guard = checkRunFromString(stagingConclusion.checkRunGuard!); + final guard = checkRunFromString(stagingConclusion.mergeQueueGuard!); await failGuardForMergeGroup( slug: check.slug, lock: guard, @@ -1241,7 +1226,7 @@ detailsUrl: $detailsUrl // If its a unified check run we need to require action on the guard. if (check.isMergeGroup) { await _completeArtifacts(check.sha, false); - final guard = checkRunFromString(stagingConclusion.checkRunGuard!); + final guard = checkRunFromString(stagingConclusion.mergeQueueGuard!); await failGuardForMergeGroup( slug: check.slug, lock: guard, @@ -1250,7 +1235,7 @@ detailsUrl: $detailsUrl details: stagingConclusion.details, ); } else if (check.isUnifiedCheckRun) { - final guard = checkRunFromString(stagingConclusion.checkRunGuard!); + final guard = checkRunFromString(stagingConclusion.dashboardChecks!); final detailsUrl = 'https://flutter-dashboard.appspot.com/#/presubmit?repo=${check.slug.name}&sha=${check.sha}'; await _requireActionForGuard( @@ -1282,7 +1267,7 @@ detailsUrl: $detailsUrl if (check.isMergeGroup) { await _completeArtifacts(check.sha, true); await _closeMergeQueue( - mergeQueueGuard: stagingConclusion.checkRunGuard!, + mergeQueueGuard: stagingConclusion.mergeQueueGuard!, slug: check.slug, sha: check.sha, stage: CiStage.fusionEngineBuild, @@ -1291,7 +1276,8 @@ detailsUrl: $detailsUrl } else { await _closeSuccessfulEngineBuildStage( checkRun: check.checkRun, - mergeQueueGuard: stagingConclusion.checkRunGuard!, + dashboardChecks: stagingConclusion.dashboardChecks, + mergeQueueGuard: stagingConclusion.mergeQueueGuard, slug: check.slug, sha: check.sha, logCrumb: logCrumb, @@ -1299,18 +1285,22 @@ detailsUrl: $detailsUrl } case CiStage.fusionTests: await _closeSuccessfulTestStage( - mergeQueueGuard: stagingConclusion.checkRunGuard!, + dashboardChecks: stagingConclusion.dashboardChecks, + mergeQueueGuard: stagingConclusion.mergeQueueGuard, slug: check.slug, sha: check.sha, logCrumb: logCrumb, + isUnifiedCheckRun: check.isUnifiedCheckRun, ); case CiStage.genericTests: if (check.isUnifiedCheckRun) { await _closeSuccessfulTestStage( - mergeQueueGuard: stagingConclusion.checkRunGuard!, + dashboardChecks: stagingConclusion.dashboardChecks, + mergeQueueGuard: stagingConclusion.mergeQueueGuard, slug: check.slug, sha: check.sha, logCrumb: logCrumb, + isUnifiedCheckRun: check.isUnifiedCheckRun, ); } else { // generic tests do not have a staging document nor are associated @@ -1348,7 +1338,8 @@ detailsUrl: $detailsUrl Future _closeSuccessfulEngineBuildStage({ required cocoon_checks.CheckRun checkRun, - required String mergeQueueGuard, + String? dashboardChecks, + String? mergeQueueGuard, required RepositorySlug slug, required String sha, required String logCrumb, @@ -1359,6 +1350,7 @@ detailsUrl: $detailsUrl await proceedToCiTestingStage( checkRun: checkRun, + dashboardChecks: dashboardChecks, mergeQueueGuard: mergeQueueGuard, slug: slug, sha: sha, @@ -1367,13 +1359,38 @@ detailsUrl: $detailsUrl } Future _closeSuccessfulTestStage({ - required String mergeQueueGuard, + String? dashboardChecks, + String? mergeQueueGuard, required RepositorySlug slug, required String sha, required String logCrumb, + required bool isUnifiedCheckRun, }) async { log.info('$logCrumb: Stage completed: ${CiStage.fusionTests}'); - await unlockMergeQueueGuard(slug, sha, checkRunFromString(mergeQueueGuard)); + if (isUnifiedCheckRun) { + if (dashboardChecks != null) { + await unlockMergeQueueGuard( + slug, + sha, + checkRunFromString(dashboardChecks), + ); + } + if (mergeQueueGuard != null) { + await unlockMergeQueueGuard( + slug, + sha, + checkRunFromString(mergeQueueGuard), + ); + } + } else { + if (mergeQueueGuard != null) { + await unlockMergeQueueGuard( + slug, + sha, + checkRunFromString(mergeQueueGuard), + ); + } + } } /// Returns the presubmit targets for the fusion repo [pullRequest] that should run for the given [stage]. @@ -1417,7 +1434,7 @@ detailsUrl: $detailsUrl /// Schedules post-engine build tests (i.e. engine tests, and framework tests). Future _runCiTestingStage({ required PullRequest pullRequest, - required CheckRun dashboardChecks, + CheckRun? dashboardChecks, CheckRun? mergeQueueGuard, required String logCrumb, required _FlutterRepoTestsToRun testsToRun, @@ -1516,11 +1533,10 @@ detailsUrl: $detailsUrl required cocoon_checks.CheckRun checkRun, required RepositorySlug slug, required String sha, - required String mergeQueueGuard, + String? dashboardChecks, + String? mergeQueueGuard, required String logCrumb, }) async { - final checkRunGuard = checkRunFromString(mergeQueueGuard); - final pullRequest = await findPullRequestCached( checkRun.id!, checkRun.name!, @@ -1537,7 +1553,12 @@ detailsUrl: $detailsUrl try { await _runCiTestingStage( pullRequest: pullRequest, - dashboardChecks: checkRunGuard, + dashboardChecks: dashboardChecks != null + ? checkRunFromString(dashboardChecks) + : null, + mergeQueueGuard: mergeQueueGuard != null + ? checkRunFromString(mergeQueueGuard) + : null, logCrumb: logCrumb, testsToRun: _FlutterRepoTestsToRun.engineTestsAndFrameworkTests, ); diff --git a/app_dart/test/model/firestore/ci_staging_test.dart b/app_dart/test/model/firestore/ci_staging_test.dart index fec100b63..0afdd6204 100644 --- a/app_dart/test/model/firestore/ci_staging_test.dart +++ b/app_dart/test/model/firestore/ci_staging_test.dart @@ -163,7 +163,7 @@ void main() { remaining: 1, result: PresubmitGuardConclusionResult.missing, failed: 0, - checkRunGuard: null, + mergeQueueGuard: null, summary: 'Check run "test" not present in engine CI stage', details: 'Change flutter_flutter_1234', ), @@ -231,7 +231,7 @@ void main() { remaining: 0, result: PresubmitGuardConclusionResult.ok, failed: 0, - checkRunGuard: '{}', + mergeQueueGuard: '{}', summary: 'All tests passed', details: ''' For CI stage engine: @@ -273,7 +273,7 @@ For CI stage engine: remaining: 1, result: PresubmitGuardConclusionResult.internalError, failed: 0, - checkRunGuard: '{}', + mergeQueueGuard: '{}', summary: 'Not a valid state transition for MacOS build_test', details: 'Attempted to transition the state of check run MacOS build_test from "success" to "unknown".', @@ -312,7 +312,7 @@ For CI stage engine: remaining: 1, result: PresubmitGuardConclusionResult.ok, failed: 0, - checkRunGuard: '{}', + mergeQueueGuard: '{}', summary: 'All tests passed', details: ''' For CI stage engine: @@ -355,7 +355,7 @@ For CI stage engine: remaining: 1, result: PresubmitGuardConclusionResult.ok, failed: 0, - checkRunGuard: '{}', + mergeQueueGuard: '{}', summary: 'All tests passed', details: ''' For CI stage engine: @@ -397,7 +397,7 @@ For CI stage engine: remaining: 1, result: PresubmitGuardConclusionResult.internalError, failed: 1, - checkRunGuard: '{}', + mergeQueueGuard: '{}', summary: 'Not a valid state transition for MacOS build_test', details: 'Attempted to transition the state of check run MacOS build_test from "failure" to "failure".', @@ -435,7 +435,7 @@ For CI stage engine: remaining: 1, result: PresubmitGuardConclusionResult.ok, failed: 1, - checkRunGuard: '{}', + mergeQueueGuard: '{}', summary: 'All tests passed', details: ''' For CI stage engine: 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 1ff306c65..43dad73be 100644 --- a/app_dart/test/request_handlers/github/webhook_subscription_test.dart +++ b/app_dart/test/request_handlers/github/webhook_subscription_test.dart @@ -2968,16 +2968,14 @@ void foo() { }); group('PullRequestLabelProcessor.processLabels', () { - test( - 'applies emergency label on approved PRs', - () async { - final pullRequest = generatePullRequest( - number: 123, - headSha: '6dcb09b5b57875f334f61aebed695e2e4193db5e', - labels: [IssueLabel(name: 'emergency')], - ); + test('applies emergency label on approved PRs', () async { + final pullRequest = generatePullRequest( + number: 123, + headSha: '6dcb09b5b57875f334f61aebed695e2e4193db5e', + labels: [IssueLabel(name: 'emergency')], + ); - githubService.checkRunsMock = '''{ + githubService.checkRunsMock = '''{ "total_count": 1, "check_runs": [ { @@ -2995,33 +2993,32 @@ void foo() { ] }'''; - final pullRequestLabelProcessor = PullRequestLabelProcessor( - config: config, - githubService: githubService, - pullRequest: pullRequest, - ); + final pullRequestLabelProcessor = PullRequestLabelProcessor( + config: config, + githubService: githubService, + pullRequest: pullRequest, + ); - await pullRequestLabelProcessor.processLabels(); + await pullRequestLabelProcessor.processLabels(); - expect( - log, - bufferedLoggerOf( - containsAll([ - logThat( - message: equals( - 'PullRequestLabelProcessor(flutter/flutter/pull/123): attempting to unlock the Merge Queue Guard for emergency', - ), + expect( + log, + bufferedLoggerOf( + containsAll([ + logThat( + message: equals( + 'PullRequestLabelProcessor(flutter/flutter/pull/123): attempting to unlock the Merge Queue Guard for emergency', ), - logThat( - message: equals( - 'PullRequestLabelProcessor(flutter/flutter/pull/123): unlocked "Merge Queue Guard", allowing it to land as an emergency.', - ), + ), + logThat( + message: equals( + 'PullRequestLabelProcessor(flutter/flutter/pull/123): unlocked "Merge Queue Guard", allowing it to land as an emergency.', ), - ]), - ), - ); - }, - ); + ), + ]), + ), + ); + }); test( 'logs and gracefully skips emergency label on missing checkruns', diff --git a/app_dart/test/service/scheduler_test.dart b/app_dart/test/service/scheduler_test.dart index 364a9ab90..a8f01fdf6 100644 --- a/app_dart/test/service/scheduler_test.dart +++ b/app_dart/test/service/scheduler_test.dart @@ -4400,15 +4400,20 @@ targets: 'fails the merge queue guard when a test check run fails (merge group)', () async { final pullRequest = generatePullRequest(); - final checkRunGuard = generateCheckRun( + final dashboardChecks = generateCheckRun( 1234, + name: Config.kDashboardCheckName, + startedAt: DateTime.now(), + ); + final mergeQueueGuard = generateCheckRun( + 5678, name: Config.kMergeQueueLockName, startedAt: DateTime.now(), ); await PrCheckRuns.initializeDocument( firestoreService: firestore, - checks: [checkRunGuard], + checks: [dashboardChecks, mergeQueueGuard], pullRequest: pullRequest, ); @@ -4418,7 +4423,8 @@ targets: // Initialize presubmit guard for tests stage firestore.putDocument( PresubmitGuard( - checkRun: checkRunGuard, + checkRun: dashboardChecks, + checkRunGuard: mergeQueueGuard, headSha: pullRequest.head!.sha!, slug: pullRequest.base!.repo!.slug(), prNum: pullRequest.number!, @@ -4436,7 +4442,7 @@ targets: PresubmitJob.init( slug: pullRequest.base!.repo!.slug(), jobName: 'Linux test', - checkRunId: checkRunGuard.id!, + checkRunId: dashboardChecks.id!, creationTime: DateTime.now().millisecondsSinceEpoch, ), ); @@ -4447,7 +4453,7 @@ targets: sha: pullRequest.head!.sha!, branch: 'gh-readonly-queue/master/pr-123-abc', ), - guardCheckRunId: checkRunGuard.id, + guardCheckRunId: dashboardChecks.id, stage: CiStage.fusionTests, checkSuiteId: 2, pullRequestNumber: pullRequest.number, @@ -4468,14 +4474,36 @@ targets: mockGithubChecksUtil.updateCheckRun( any, any, - any, - status: anyNamed('status'), - conclusion: CheckRunConclusion.failure, // Merge queue failure + argThat( + isA().having( + (c) => c.name, + 'name', + Config.kMergeQueueLockName, + ), + ), + status: CheckRunStatus.completed, + conclusion: CheckRunConclusion.failure, detailsUrl: anyNamed('detailsUrl'), output: anyNamed('output'), ), ).called(1); + verifyNever( + mockGithubChecksUtil.updateCheckRun( + any, + any, + argThat( + isA().having( + (c) => c.name, + 'name', + Config.kDashboardCheckName, + ), + ), + status: anyNamed('status'), + conclusion: anyNamed('conclusion'), + ), + ); + final guards = await firestore.query(PresubmitGuard.collectionId, {}); final guard = PresubmitGuard.fromDocument(guards.single); expect(guard.failedJobs, 1); 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 ae754ba5b..db12283be 100644 --- a/packages/cocoon_integration_test/lib/src/utilities/mocks.mocks.dart +++ b/packages/cocoon_integration_test/lib/src/utilities/mocks.mocks.dart @@ -11,7 +11,7 @@ import 'dart:typed_data' as _i26; import 'package:buildbucket/buildbucket_pb.dart' as _i6; import 'package:cocoon_common/rpc_model.dart' as _i19; import 'package:cocoon_integration_test/src/fakes/fake_entry.dart' as _i25; -import 'package:cocoon_service/cocoon_service.dart' as _i17; +import 'package:cocoon_service/cocoon_service.dart' as _i16; import 'package:cocoon_service/src/foundation/github_checks_util.dart' as _i10; import 'package:cocoon_service/src/model/ci_yaml/ci_yaml.dart' as _i37; import 'package:cocoon_service/src/model/ci_yaml/target.dart' as _i27; @@ -48,7 +48,7 @@ import 'package:graphql/client.dart' as _i8; import 'package:http/http.dart' as _i5; import 'package:mockito/mockito.dart' as _i1; import 'package:mockito/src/dummies.dart' as _i20; -import 'package:neat_cache/neat_cache.dart' as _i16; +import 'package:neat_cache/neat_cache.dart' as _i17; import 'package:process/src/interface/process_manager.dart' as _i35; // ignore_for_file: type=lint @@ -177,11 +177,6 @@ class _FakeCheckRun_20 extends _i1.SmartFake implements _i7.CheckRun { : super(parent, parentInvocation); } -class _FakeCheckRunLockResult_20a extends _i1.SmartFake implements _i17.CheckRunLockResult { - _FakeCheckRunLockResult_20a(Object parent, Invocation parentInvocation) - : super(parent, parentInvocation); -} - class _FakePullRequest_21 extends _i1.SmartFake implements _i7.PullRequest { _FakePullRequest_21(Object parent, Invocation parentInvocation) : super(parent, parentInvocation); @@ -382,13 +377,19 @@ class _FakeRepositorySlug_57 extends _i1.SmartFake : super(parent, parentInvocation); } -class _FakeEntry_58 extends _i1.SmartFake implements _i16.Entry { - _FakeEntry_58(Object parent, Invocation parentInvocation) +class _FakeCheckRunLockResult_58 extends _i1.SmartFake + implements _i16.CheckRunLockResult { + _FakeCheckRunLockResult_58(Object parent, Invocation parentInvocation) : super(parent, parentInvocation); } -class _FakeCache_59 extends _i1.SmartFake implements _i16.Cache { - _FakeCache_59(Object parent, Invocation parentInvocation) +class _FakeEntry_59 extends _i1.SmartFake implements _i17.Entry { + _FakeEntry_59(Object parent, Invocation parentInvocation) + : super(parent, parentInvocation); +} + +class _FakeCache_60 extends _i1.SmartFake implements _i17.Cache { + _FakeCache_60(Object parent, Invocation parentInvocation) : super(parent, parentInvocation); } @@ -396,7 +397,7 @@ class _FakeCache_59 extends _i1.SmartFake implements _i16.Cache { /// /// See the documentation for Mockito's code generation for more information. class MockAccessTokenService extends _i1.Mock - implements _i17.AccessTokenService { + implements _i16.AccessTokenService { MockAccessTokenService() { _i1.throwOnMissingStub(this); } @@ -482,7 +483,7 @@ class MockBigQueryService extends _i1.Mock implements _i18.BigQueryService { /// A class which mocks [BranchService]. /// /// See the documentation for Mockito's code generation for more information. -class MockBranchService extends _i1.Mock implements _i17.BranchService { +class MockBranchService extends _i1.Mock implements _i16.BranchService { MockBranchService() { _i1.throwOnMissingStub(this); } @@ -511,7 +512,7 @@ class MockBranchService extends _i1.Mock implements _i17.BranchService { /// /// See the documentation for Mockito's code generation for more information. // ignore: must_be_immutable -class MockBuildBucketClient extends _i1.Mock implements _i17.BuildBucketClient { +class MockBuildBucketClient extends _i1.Mock implements _i16.BuildBucketClient { MockBuildBucketClient() { _i1.throwOnMissingStub(this); } @@ -1111,15 +1112,15 @@ class MockConfig extends _i1.Mock implements _i2.Config { as Set); @override - _i17.DynamicConfig get flags => + _i16.DynamicConfig get flags => (super.noSuchMethod( Invocation.getter(#flags), - returnValue: _i20.dummyValue<_i17.DynamicConfig>( + returnValue: _i20.dummyValue<_i16.DynamicConfig>( this, Invocation.getter(#flags), ), ) - as _i17.DynamicConfig); + as _i16.DynamicConfig); @override String wrongHeadBranchPullRequestMessage(String? branch) => @@ -1851,7 +1852,7 @@ class MockIssuesService extends _i1.Mock implements _i7.IssuesService { /// /// See the documentation for Mockito's code generation for more information. class MockGithubChecksService extends _i1.Mock - implements _i17.GithubChecksService { + implements _i16.GithubChecksService { MockGithubChecksService() { _i1.throwOnMissingStub(this); } @@ -1890,7 +1891,7 @@ class MockGithubChecksService extends _i1.Mock @override _i13.Future updateCheckStatus({ required _i6.Build? build, - required _i17.LuciBuildService? luciBuildService, + required _i16.LuciBuildService? luciBuildService, required _i7.RepositorySlug? slug, required int? checkRunId, bool? rescheduled = false, @@ -4097,7 +4098,7 @@ class MockHttpClientResponse extends _i1.Mock /// A class which mocks [LuciBuildService]. /// /// See the documentation for Mockito's code generation for more information. -class MockLuciBuildService extends _i1.Mock implements _i17.LuciBuildService { +class MockLuciBuildService extends _i1.Mock implements _i16.LuciBuildService { MockLuciBuildService() { _i1.throwOnMissingStub(this); } @@ -4149,7 +4150,7 @@ class MockLuciBuildService extends _i1.Mock implements _i17.LuciBuildService { required _i28.EngineArtifacts? engineArtifacts, _i7.CheckRun? dashboardChecks, _i7.CheckRun? mergeQueueGuard, - _i17.CiStage? stage, + _i16.CiStage? stage, }) => (super.noSuchMethod( Invocation.method(#scheduleTryBuilds, [], { @@ -4171,7 +4172,7 @@ class MockLuciBuildService extends _i1.Mock implements _i17.LuciBuildService { required _i28.EngineArtifacts? engineArtifacts, _i7.CheckRun? dashboardChecks, _i7.CheckRun? mergeQueueGuard, - required _i17.CiStage? stage, + required _i16.CiStage? stage, }) => (super.noSuchMethod( Invocation.method(#reScheduleTryBuilds, [], { @@ -4249,7 +4250,7 @@ class MockLuciBuildService extends _i1.Mock implements _i17.LuciBuildService { _i30.CheckRunEvent? checkRunEvent, { required _i31.CommitRef? commit, required _i27.Target? target, - required _i17.Task? task, + required _i16.Task? task, }) => (super.noSuchMethod( Invocation.method( @@ -4369,7 +4370,7 @@ class MockLuciBuildService extends _i1.Mock implements _i17.LuciBuildService { _i13.Future rerunBuilder({ required _i31.CommitRef? commit, required _i27.Target? target, - required _i17.Task? task, + required _i16.Task? task, Iterable<_i34.BuildTag>? tags = const [], }) => (super.noSuchMethod( @@ -4386,7 +4387,7 @@ class MockLuciBuildService extends _i1.Mock implements _i17.LuciBuildService { @override _i13.Future rerunDartInternalReleaseBuilder({ required _i31.CommitRef? commit, - required _i17.Task? task, + required _i16.Task? task, }) => (super.noSuchMethod( Invocation.method(#rerunDartInternalReleaseBuilder, [], { @@ -5467,7 +5468,7 @@ class MockBeginTransactionResponse extends _i1.Mock /// /// See the documentation for Mockito's code generation for more information. class MockPullRequestLabelProcessor extends _i1.Mock - implements _i17.PullRequestLabelProcessor { + implements _i16.PullRequestLabelProcessor { MockPullRequestLabelProcessor() { _i1.throwOnMissingStub(this); } @@ -5555,13 +5556,13 @@ class MockPullRequestLabelProcessor extends _i1.Mock /// A class which mocks [Scheduler]. /// /// See the documentation for Mockito's code generation for more information. -class MockScheduler extends _i1.Mock implements _i17.Scheduler { +class MockScheduler extends _i1.Mock implements _i16.Scheduler { MockScheduler() { _i1.throwOnMissingStub(this); } @override - _i13.Future addCommits(List<_i17.Commit>? commits) => + _i13.Future addCommits(List<_i16.Commit>? commits) => (super.noSuchMethod( Invocation.method(#addCommits, [commits]), returnValue: _i13.Future.value(), @@ -5677,7 +5678,7 @@ class MockScheduler extends _i1.Mock implements _i17.Scheduler { String? baseRef, _i7.RepositorySlug? slug, String? headSha, - _i17.CiStage? stage, + _i16.CiStage? stage, ) => (super.noSuchMethod( Invocation.method(#getMergeGroupTargetsForStage, [ @@ -5721,7 +5722,7 @@ class MockScheduler extends _i1.Mock implements _i17.Scheduler { as _i13.Future); @override - _i13.Future<_i17.CheckRunLockResult> lockMergeGroupChecks( + _i13.Future<_i16.CheckRunLockResult> lockMergeGroupChecks( _i7.RepositorySlug? slug, String? headSha, { String? detailsUrl, @@ -5733,8 +5734,8 @@ class MockScheduler extends _i1.Mock implements _i17.Scheduler { [slug, headSha], {#detailsUrl: detailsUrl, #isUnifiedCheckRun: isUnifiedCheckRun}, ), - returnValue: _i13.Future<_i17.CheckRunLockResult>.value( - _FakeCheckRunLockResult_20a( + returnValue: _i13.Future<_i16.CheckRunLockResult>.value( + _FakeCheckRunLockResult_58( this, Invocation.method( #lockMergeGroupChecks, @@ -5747,7 +5748,7 @@ class MockScheduler extends _i1.Mock implements _i17.Scheduler { ), ), ) - as _i13.Future<_i17.CheckRunLockResult>); + as _i13.Future<_i16.CheckRunLockResult>); @override _i13.Future<_i7.CheckRun?> createAwaitingCicdLabelCheckRun( @@ -5866,7 +5867,8 @@ class MockScheduler extends _i1.Mock implements _i17.Scheduler { required _i30.CheckRun? checkRun, required _i7.RepositorySlug? slug, required String? sha, - required String? mergeQueueGuard, + String? dashboardChecks, + String? mergeQueueGuard, required String? logCrumb, }) => (super.noSuchMethod( @@ -5874,6 +5876,7 @@ class MockScheduler extends _i1.Mock implements _i17.Scheduler { #checkRun: checkRun, #slug: slug, #sha: sha, + #dashboardChecks: dashboardChecks, #mergeQueueGuard: mergeQueueGuard, #logCrumb: logCrumb, }), @@ -5974,52 +5977,52 @@ class MockScheduler extends _i1.Mock implements _i17.Scheduler { /// A class which mocks [Cache]. /// /// See the documentation for Mockito's code generation for more information. -class MockCache extends _i1.Mock implements _i16.Cache<_i26.Uint8List> { +class MockCache extends _i1.Mock implements _i17.Cache<_i26.Uint8List> { MockCache() { _i1.throwOnMissingStub(this); } @override - _i16.Entry<_i26.Uint8List> operator [](String? key) => + _i17.Entry<_i26.Uint8List> operator [](String? key) => (super.noSuchMethod( Invocation.method(#[], [key]), - returnValue: _FakeEntry_58<_i26.Uint8List>( + returnValue: _FakeEntry_59<_i26.Uint8List>( this, Invocation.method(#[], [key]), ), ) - as _i16.Entry<_i26.Uint8List>); + as _i17.Entry<_i26.Uint8List>); @override - _i16.Cache<_i26.Uint8List> withPrefix(String? prefix) => + _i17.Cache<_i26.Uint8List> withPrefix(String? prefix) => (super.noSuchMethod( Invocation.method(#withPrefix, [prefix]), - returnValue: _FakeCache_59<_i26.Uint8List>( + returnValue: _FakeCache_60<_i26.Uint8List>( this, Invocation.method(#withPrefix, [prefix]), ), ) - as _i16.Cache<_i26.Uint8List>); + as _i17.Cache<_i26.Uint8List>); @override - _i16.Cache withCodec(_i12.Codec? codec) => + _i17.Cache withCodec(_i12.Codec? codec) => (super.noSuchMethod( Invocation.method(#withCodec, [codec]), - returnValue: _FakeCache_59( + returnValue: _FakeCache_60( this, Invocation.method(#withCodec, [codec]), ), ) - as _i16.Cache); + as _i17.Cache); @override - _i16.Cache<_i26.Uint8List> withTTL(Duration? ttl) => + _i17.Cache<_i26.Uint8List> withTTL(Duration? ttl) => (super.noSuchMethod( Invocation.method(#withTTL, [ttl]), - returnValue: _FakeCache_59<_i26.Uint8List>( + returnValue: _FakeCache_60<_i26.Uint8List>( this, Invocation.method(#withTTL, [ttl]), ), ) - as _i16.Cache<_i26.Uint8List>); + as _i17.Cache<_i26.Uint8List>); } From 6130e4974845398412a0493ad886b9047e6c17b7 Mon Sep 17 00:00:00 2001 From: Dmitry Grand Date: Tue, 21 Jul 2026 18:51:49 -0700 Subject: [PATCH 6/7] format --- app_dart/lib/src/service/luci_build_service.dart | 4 +++- app_dart/lib/src/service/pull_request_manager.dart | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/app_dart/lib/src/service/luci_build_service.dart b/app_dart/lib/src/service/luci_build_service.dart index 1a60f7ea4..68614a762 100644 --- a/app_dart/lib/src/service/luci_build_service.dart +++ b/app_dart/lib/src/service/luci_build_service.dart @@ -398,7 +398,9 @@ class LuciBuildService { // if unified check run flow is enabled, use guard check run othervise check run id. tags: isUnifiedCheckRunFlow && dashboardChecks != null ? BuildTags([ - GuardCheckRunIdBuildTag(guardCheckRunId: dashboardChecks.id!), + GuardCheckRunIdBuildTag( + guardCheckRunId: dashboardChecks.id!, + ), if (attemptNumber > 1) CurrentAttemptBuildTag(attemptNumber: attemptNumber), if (isOrderedPresubmit) diff --git a/app_dart/lib/src/service/pull_request_manager.dart b/app_dart/lib/src/service/pull_request_manager.dart index 4c5ae2bc5..81177b3f7 100644 --- a/app_dart/lib/src/service/pull_request_manager.dart +++ b/app_dart/lib/src/service/pull_request_manager.dart @@ -1067,7 +1067,7 @@ The "Merge" button is also unlocked. To bypass presubmits as well as the tree st } Future _unlockCheckrunsForEmergency() async { - // Unlock only the merge queue guard for emergency. Do not unlock + // Unlock only the merge queue guard for emergency. Do not unlock // dashboard checks. See: https://github.com/flutter/flutter/issues/189729 await _unlockCheckrun(Config.kMergeQueueLockName); From 18666ae6bb0adf49112cb6ab135229a45ee04578 Mon Sep 17 00:00:00 2001 From: Dmitry Grand Date: Tue, 21 Jul 2026 19:00:47 -0700 Subject: [PATCH 7/7] ai review --- 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 3a391c89d..dcaace996 100644 --- a/app_dart/lib/src/service/scheduler.dart +++ b/app_dart/lib/src/service/scheduler.dart @@ -1366,7 +1366,7 @@ detailsUrl: $detailsUrl required String logCrumb, required bool isUnifiedCheckRun, }) async { - log.info('$logCrumb: Stage completed: ${CiStage.fusionTests}'); + log.info('$logCrumb: Test stage completed'); if (isUnifiedCheckRun) { if (dashboardChecks != null) { await unlockMergeQueueGuard(