From e32ef662067ae343563bd9d2a72bf63cebb6bffe Mon Sep 17 00:00:00 2001 From: Dmitry Grand Date: Tue, 30 Jun 2026 13:07:12 -0700 Subject: [PATCH 01/13] initial implementation of imedeate failing the checkRunGuard once job failed --- .../common/presubmit_guard_conclusion.dart | 2 +- app_dart/lib/src/service/scheduler.dart | 16 ++-- .../firestore/unified_check_run_test.dart | 2 + app_dart/test/service/scheduler_test.dart | 92 +++++++++++++++++++ packages/cocoon_common/lib/guard_status.dart | 8 +- .../cocoon_common/test/guard_status_test.dart | 11 +++ 6 files changed, 118 insertions(+), 13 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 10e09508c4..9b7d4e2543 100644 --- a/app_dart/lib/src/model/common/presubmit_guard_conclusion.dart +++ b/app_dart/lib/src/model/common/presubmit_guard_conclusion.dart @@ -56,7 +56,7 @@ 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; diff --git a/app_dart/lib/src/service/scheduler.dart b/app_dart/lib/src/service/scheduler.dart index ced9831a60..b20307850b 100644 --- a/app_dart/lib/src/service/scheduler.dart +++ b/app_dart/lib/src/service/scheduler.dart @@ -1204,14 +1204,6 @@ detailsUrl: $detailsUrl 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: // @@ -1250,6 +1242,14 @@ detailsUrl: $detailsUrl 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: 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 1f8e0f2fc1..7d16aa09f5 100644 --- a/app_dart/test/service/firestore/unified_check_run_test.dart +++ b/app_dart/test/service/firestore/unified_check_run_test.dart @@ -276,6 +276,8 @@ void main() { expect(result.result, PresubmitGuardConclusionResult.ok); expect(result.remaining, 1); expect(result.failed, 1); + expect(result.isFailed, true); + expect(result.isPending, true); }); test('handles missing check gracefully', () async { diff --git a/app_dart/test/service/scheduler_test.dart b/app_dart/test/service/scheduler_test.dart index 7f849316c5..4c7937a12e 100644 --- a/app_dart/test/service/scheduler_test.dart +++ b/app_dart/test/service/scheduler_test.dart @@ -4344,6 +4344,98 @@ targets: }, ); + test( + 'fails the guard check immediately when a test check run fails even if jobs remain', + () async { + final pullRequest = generatePullRequest(); + final checkRunGuard = generateCheckRun( + 1234, + name: Config.kDashboardCheckName, + startedAt: DateTime.now(), + ); + + await PrCheckRuns.initializeDocument( + firestoreService: firestore, + checks: [checkRunGuard], + pullRequest: pullRequest, + ); + + firestore.putDocument( + PresubmitGuard( + checkRun: checkRunGuard, + headSha: pullRequest.head!.sha!, + slug: pullRequest.base!.repo!.slug(), + prNum: pullRequest.number!, + stage: CiStage.fusionTests, + author: pullRequest.user!.login!, + creationTime: DateTime.now().millisecondsSinceEpoch, + jobs: { + 'Linux test 1': TaskStatus.waitingForBackfill, + 'Linux test 2': TaskStatus.waitingForBackfill, + }, + remainingJobs: 2, + failedJobs: 0, + ), + ); + + firestore.putDocument( + PresubmitJob.init( + slug: pullRequest.base!.repo!.slug(), + jobName: 'Linux test 1', + checkRunId: checkRunGuard.id!, + creationTime: DateTime.now().millisecondsSinceEpoch, + ), + ); + + final userData = PresubmitUserData( + commit: CommitRef( + slug: pullRequest.base!.repo!.slug(), + sha: pullRequest.head!.sha!, + branch: 'main', + ), + guardCheckRunId: checkRunGuard.id, + stage: CiStage.fusionTests, + checkSuiteId: 2, + pullRequestNumber: pullRequest.number, + ); + + final build = generateBbv2Build( + Int64(1), + name: 'Linux test 1', + status: bbv2.Status.FAILURE, + tags: [ + bbv2.StringPair(key: 'current_attempt', value: '1'), + bbv2.StringPair( + key: 'buildset', + value: 'sha/git/${pullRequest.head!.sha!}', + ), + ], + ); + + final check = PresubmitCompletedJob.fromBuild(build, userData); + + expect(await scheduler.processCheckRunCompleted(check), isTrue); + + verify( + mockGithubChecksUtil.updateCheckRun( + any, + any, + any, + status: anyNamed('status'), + conclusion: CheckRunConclusion.actionRequired, + detailsUrl: anyNamed('detailsUrl'), + output: anyNamed('output'), + actions: anyNamed('actions'), + ), + ).called(1); + + final guards = await firestore.query(PresubmitGuard.collectionId, {}); + final guard = PresubmitGuard.fromDocument(guards.single); + expect(guard.failedJobs, 1); + expect(guard.remainingJobs, 1); + }, + ); + test('closes merge queue guard in merge group success', () async { final pullRequest = generatePullRequest(); final checkRunGuard = generateCheckRun( diff --git a/packages/cocoon_common/lib/guard_status.dart b/packages/cocoon_common/lib/guard_status.dart index bef6aa1b18..e9f4d43d11 100644 --- a/packages/cocoon_common/lib/guard_status.dart +++ b/packages/cocoon_common/lib/guard_status.dart @@ -33,14 +33,14 @@ enum GuardStatus { required int remainingBuilds, required int totalBuilds, }) { - if (failedBuilds == 0 && remainingBuilds == 0) { + if (failedBuilds > 0) { + return GuardStatus.failed; + } else if (remainingBuilds == 0) { return GuardStatus.succeeded; } else if (remainingBuilds == totalBuilds) { return GuardStatus.waitingForBackfill; - } else if (remainingBuilds > 0) { - return GuardStatus.inProgress; } else { - return GuardStatus.failed; + return GuardStatus.inProgress; } } } diff --git a/packages/cocoon_common/test/guard_status_test.dart b/packages/cocoon_common/test/guard_status_test.dart index b7d10e7a9b..086f0211c4 100644 --- a/packages/cocoon_common/test/guard_status_test.dart +++ b/packages/cocoon_common/test/guard_status_test.dart @@ -50,5 +50,16 @@ void main() { GuardStatus.inProgress, ); }); + + test('returns failed even if there are remaining builds', () { + expect( + GuardStatus.calculate( + failedBuilds: 1, + remainingBuilds: 5, + totalBuilds: 10, + ), + GuardStatus.failed, + ); + }); }); } From c3c5bda581f5e7fba2b4832a8b9d4a436275ea59 Mon Sep 17 00:00:00 2001 From: Dmitry Grand Date: Tue, 30 Jun 2026 13:13:07 -0700 Subject: [PATCH 02/13] moved failed and remaining counters into current state class --- .../common/presubmit_guard_conclusion.dart | 42 ++++++++++++++----- .../lib/src/model/firestore/ci_staging.dart | 18 +++++--- .../service/firestore/unified_check_run.dart | 18 +++++--- app_dart/lib/src/service/scheduler.dart | 4 +- .../test/model/firestore/ci_staging_test.dart | 21 ++++------ .../firestore/unified_check_run_test.dart | 16 +++---- 6 files changed, 72 insertions(+), 47 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 9b7d4e2543..2ca0bc7201 100644 --- a/app_dart/lib/src/model/common/presubmit_guard_conclusion.dart +++ b/app_dart/lib/src/model/common/presubmit_guard_conclusion.dart @@ -32,31 +32,53 @@ enum PresubmitGuardConclusionResult { internalError, } +/// Represents the current state of jobs in a CI stage. +class PresubmitGuardState { + final int remaining; + final int failed; + + const PresubmitGuardState({ + required this.remaining, + required this.failed, + }); + + @override + bool operator ==(Object other) => + identical(this, other) || + (other is PresubmitGuardState && + other.remaining == remaining && + other.failed == failed); + + @override + int get hashCode => Object.hash(remaining, failed); + + @override + String toString() => 'PresubmitGuardState("$remaining", "$failed")'; +} + /// Results from attempting to mark a staging task as completed. /// /// See: [PresubmitGuard.markConclusion] class PresubmitGuardConclusion { final PresubmitGuardConclusionResult result; - final int remaining; + final PresubmitGuardState currentState; final String? checkRunGuard; - final int failed; final String summary; final String details; const PresubmitGuardConclusion({ required this.result, - required this.remaining, + required this.currentState, required this.checkRunGuard, - required this.failed, required this.summary, required this.details, }); bool get isOk => result == PresubmitGuardConclusionResult.ok; - bool get isPending => isOk && remaining > 0; + bool get isPending => isOk && currentState.remaining > 0; - bool get isFailed => isOk && failed > 0; + bool get isFailed => isOk && currentState.failed > 0; bool get isComplete => isOk && !isPending && !isFailed; @@ -65,23 +87,21 @@ class PresubmitGuardConclusion { identical(this, other) || (other is PresubmitGuardConclusion && other.result == result && - other.remaining == remaining && + other.currentState == currentState && other.checkRunGuard == checkRunGuard && - other.failed == failed && other.summary == summary && other.details == details); @override int get hashCode => Object.hashAll([ result, - remaining, + currentState, checkRunGuard, - failed, summary, details, ]); @override String toString() => - 'BuildConclusion("$result", "$remaining", "$failed", "$summary", "$details", "$checkRunGuard")'; + 'BuildConclusion("$result", "$currentState", "$summary", "$details", "$checkRunGuard")'; } diff --git a/app_dart/lib/src/model/firestore/ci_staging.dart b/app_dart/lib/src/model/firestore/ci_staging.dart index cc93406187..286d1d0da6 100644 --- a/app_dart/lib/src/model/firestore/ci_staging.dart +++ b/app_dart/lib/src/model/firestore/ci_staging.dart @@ -286,9 +286,11 @@ final class CiStaging extends AppDocument { await firestoreService.rollback(transaction); return PresubmitGuardConclusion( result: PresubmitGuardConclusionResult.missing, - remaining: remaining, + currentState: PresubmitGuardState( + remaining: remaining, + failed: failed, + ), checkRunGuard: null, - failed: failed, summary: 'Check run "$checkRun" not present in $stage CI stage', details: 'Change $changeCrumb', ); @@ -352,9 +354,11 @@ final class CiStaging extends AppDocument { await firestoreService.rollback(transaction); return PresubmitGuardConclusion( result: PresubmitGuardConclusionResult.internalError, - remaining: -1, + currentState: PresubmitGuardState( + remaining: -1, + failed: failed, + ), checkRunGuard: null, - failed: failed, summary: 'Internal server error', details: ''' @@ -390,9 +394,11 @@ $stack result: valid ? PresubmitGuardConclusionResult.ok : PresubmitGuardConclusionResult.internalError, - remaining: remaining, + currentState: PresubmitGuardState( + remaining: remaining, + failed: failed, + ), checkRunGuard: checkRunGuard ?? '', - failed: failed, summary: valid ? 'All tests passed' : 'Not a valid state transition for $checkRun', 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 7d621829a3..3e0d7f4d5c 100644 --- a/app_dart/lib/src/service/firestore/unified_check_run.dart +++ b/app_dart/lib/src/service/firestore/unified_check_run.dart @@ -558,9 +558,11 @@ final class UnifiedCheckRun { await firestoreService.rollback(transaction); return PresubmitGuardConclusion( result: PresubmitGuardConclusionResult.missing, - remaining: presubmitGuard.remainingJobs, + currentState: PresubmitGuardState( + remaining: presubmitGuard.remainingJobs, + failed: presubmitGuard.failedJobs, + ), checkRunGuard: presubmitGuard.checkRunJson, - failed: presubmitGuard.failedJobs, summary: 'Check run "${state.jobName}" not present in ${guardId.stage} CI stage', details: 'Change $changeCrumb', @@ -653,9 +655,11 @@ final class UnifiedCheckRun { await firestoreService.rollback(transaction); return PresubmitGuardConclusion( result: PresubmitGuardConclusionResult.internalError, - remaining: -1, + currentState: PresubmitGuardState( + remaining: -1, + failed: failed, + ), checkRunGuard: null, - failed: failed, summary: 'Internal server error', details: ''' @@ -688,9 +692,11 @@ $stack result: valid ? PresubmitGuardConclusionResult.ok : PresubmitGuardConclusionResult.internalError, - remaining: remaining, + currentState: PresubmitGuardState( + remaining: remaining, + failed: failed, + ), checkRunGuard: presubmitGuard.checkRunJson, - failed: failed, summary: valid ? 'Successfully updated presubmit guard status' : 'Not a valid state transition for ${state.jobName}', diff --git a/app_dart/lib/src/service/scheduler.dart b/app_dart/lib/src/service/scheduler.dart index b20307850b..f7fc4e2df8 100644 --- a/app_dart/lib/src/service/scheduler.dart +++ b/app_dart/lib/src/service/scheduler.dart @@ -1235,7 +1235,7 @@ detailsUrl: $detailsUrl ''', kDashboardChecksDescription), details: - 'For ${check.stage} CI stage ${stagingConclusion.failed} checks failed', + 'For ${check.stage} CI stage ${stagingConclusion.currentState.failed} checks failed', detailsUrl: detailsUrl, ); } @@ -1245,7 +1245,7 @@ detailsUrl: $detailsUrl // Are there tests remaining? Keep waiting. if (stagingConclusion.isPending) { log.info( - '$logCrumb: not progressing, remaining work count: ${stagingConclusion.remaining}', + '$logCrumb: not progressing, remaining work count: ${stagingConclusion.currentState.remaining}', ); return false; } diff --git a/app_dart/test/model/firestore/ci_staging_test.dart b/app_dart/test/model/firestore/ci_staging_test.dart index fec100b633..8ef99be62f 100644 --- a/app_dart/test/model/firestore/ci_staging_test.dart +++ b/app_dart/test/model/firestore/ci_staging_test.dart @@ -160,9 +160,8 @@ void main() { expect( result, const PresubmitGuardConclusion( - remaining: 1, + currentState: PresubmitGuardState(remaining: 1, failed: 0), result: PresubmitGuardConclusionResult.missing, - failed: 0, checkRunGuard: null, summary: 'Check run "test" not present in engine CI stage', details: 'Change flutter_flutter_1234', @@ -228,9 +227,8 @@ void main() { expect( result, const PresubmitGuardConclusion( - remaining: 0, + currentState: PresubmitGuardState(remaining: 0, failed: 0), result: PresubmitGuardConclusionResult.ok, - failed: 0, checkRunGuard: '{}', summary: 'All tests passed', details: ''' @@ -270,9 +268,8 @@ For CI stage engine: expect( result, const PresubmitGuardConclusion( - remaining: 1, + currentState: PresubmitGuardState(remaining: 1, failed: 0), result: PresubmitGuardConclusionResult.internalError, - failed: 0, checkRunGuard: '{}', summary: 'Not a valid state transition for MacOS build_test', details: @@ -309,9 +306,8 @@ For CI stage engine: expect( result, const PresubmitGuardConclusion( - remaining: 1, + currentState: PresubmitGuardState(remaining: 1, failed: 0), result: PresubmitGuardConclusionResult.ok, - failed: 0, checkRunGuard: '{}', summary: 'All tests passed', details: ''' @@ -352,9 +348,8 @@ For CI stage engine: expect( result, const PresubmitGuardConclusion( - remaining: 1, + currentState: PresubmitGuardState(remaining: 1, failed: 0), result: PresubmitGuardConclusionResult.ok, - failed: 0, checkRunGuard: '{}', summary: 'All tests passed', details: ''' @@ -394,9 +389,8 @@ For CI stage engine: expect( result, const PresubmitGuardConclusion( - remaining: 1, + currentState: PresubmitGuardState(remaining: 1, failed: 1), result: PresubmitGuardConclusionResult.internalError, - failed: 1, checkRunGuard: '{}', summary: 'Not a valid state transition for MacOS build_test', details: @@ -432,9 +426,8 @@ For CI stage engine: expect( result, const PresubmitGuardConclusion( - remaining: 1, + currentState: PresubmitGuardState(remaining: 1, failed: 1), result: PresubmitGuardConclusionResult.ok, - failed: 1, checkRunGuard: '{}', summary: 'All tests passed', details: ''' 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 7d16aa09f5..93b5036a8f 100644 --- a/app_dart/test/service/firestore/unified_check_run_test.dart +++ b/app_dart/test/service/firestore/unified_check_run_test.dart @@ -187,8 +187,8 @@ void main() { ); expect(result.result, PresubmitGuardConclusionResult.ok); - expect(result.remaining, 1); - expect(result.failed, 0); + expect(result.currentState.remaining, 1); + expect(result.currentState.failed, 0); final checkDoc = await PresubmitJob.fromFirestore( firestoreService, @@ -220,8 +220,8 @@ void main() { ), ); - expect(result1.remaining, 1); - expect(result1.failed, 0); + expect(result1.currentState.remaining, 1); + expect(result1.currentState.failed, 0); expect(result1.isOk, true); expect(result1.isComplete, false); expect(result1.isPending, true); @@ -238,8 +238,8 @@ void main() { ), ); - expect(result2.remaining, 0); - expect(result2.failed, 0); + expect(result2.currentState.remaining, 0); + expect(result2.currentState.failed, 0); expect(result2.isOk, true); expect(result2.isComplete, true); expect(result2.isPending, false); @@ -274,8 +274,8 @@ void main() { ); expect(result.result, PresubmitGuardConclusionResult.ok); - expect(result.remaining, 1); - expect(result.failed, 1); + expect(result.currentState.remaining, 1); + expect(result.currentState.failed, 1); expect(result.isFailed, true); expect(result.isPending, true); }); From ec8cc41e69f82885de5a8e88846bb8c41e1240b9 Mon Sep 17 00:00:00 2001 From: Dmitry Grand Date: Tue, 30 Jun 2026 13:45:17 -0700 Subject: [PATCH 03/13] added previous state --- .../common/presubmit_guard_conclusion.dart | 6 +++++- .../lib/src/model/firestore/ci_staging.dart | 5 +++++ .../service/firestore/unified_check_run.dart | 18 ++++++++++-------- .../test/model/firestore/ci_staging_test.dart | 7 +++++++ .../firestore/unified_check_run_test.dart | 8 ++++++++ 5 files changed, 35 insertions(+), 9 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 2ca0bc7201..960240a0ec 100644 --- a/app_dart/lib/src/model/common/presubmit_guard_conclusion.dart +++ b/app_dart/lib/src/model/common/presubmit_guard_conclusion.dart @@ -61,6 +61,7 @@ class PresubmitGuardState { /// See: [PresubmitGuard.markConclusion] class PresubmitGuardConclusion { final PresubmitGuardConclusionResult result; + final PresubmitGuardState previousState; final PresubmitGuardState currentState; final String? checkRunGuard; final String summary; @@ -68,6 +69,7 @@ class PresubmitGuardConclusion { const PresubmitGuardConclusion({ required this.result, + required this.previousState, required this.currentState, required this.checkRunGuard, required this.summary, @@ -87,6 +89,7 @@ class PresubmitGuardConclusion { identical(this, other) || (other is PresubmitGuardConclusion && other.result == result && + other.previousState == previousState && other.currentState == currentState && other.checkRunGuard == checkRunGuard && other.summary == summary && @@ -95,6 +98,7 @@ class PresubmitGuardConclusion { @override int get hashCode => Object.hashAll([ result, + previousState, currentState, checkRunGuard, summary, @@ -103,5 +107,5 @@ class PresubmitGuardConclusion { @override String toString() => - 'BuildConclusion("$result", "$currentState", "$summary", "$details", "$checkRunGuard")'; + 'BuildConclusion("$result", "$previousState", "$currentState", "$summary", "$details", "$checkRunGuard")'; } diff --git a/app_dart/lib/src/model/firestore/ci_staging.dart b/app_dart/lib/src/model/firestore/ci_staging.dart index 286d1d0da6..b2f15f354c 100644 --- a/app_dart/lib/src/model/firestore/ci_staging.dart +++ b/app_dart/lib/src/model/firestore/ci_staging.dart @@ -232,6 +232,7 @@ final class CiStaging extends AppDocument { var failed = -1; var total = -1; var valid = false; + var previousState = const PresubmitGuardState(remaining: -1, failed: -1); String? checkRunGuard; TaskConclusion? recordedConclusion; @@ -273,6 +274,7 @@ final class CiStaging extends AppDocument { throw '$logCrumb: missing field "$kTotalField" for $transaction / ${doc.fields}'; } total = maybeTotal; + previousState = PresubmitGuardState(remaining: remaining, failed: failed); // We will have check_runs scheduled after the engine was built successfully, so missing the checkRun field // is an OK response to have. All fields should have been written at creation time. @@ -286,6 +288,7 @@ final class CiStaging extends AppDocument { await firestoreService.rollback(transaction); return PresubmitGuardConclusion( result: PresubmitGuardConclusionResult.missing, + previousState: previousState, currentState: PresubmitGuardState( remaining: remaining, failed: failed, @@ -354,6 +357,7 @@ final class CiStaging extends AppDocument { await firestoreService.rollback(transaction); return PresubmitGuardConclusion( result: PresubmitGuardConclusionResult.internalError, + previousState: previousState, currentState: PresubmitGuardState( remaining: -1, failed: failed, @@ -394,6 +398,7 @@ $stack result: valid ? PresubmitGuardConclusionResult.ok : PresubmitGuardConclusionResult.internalError, + previousState: previousState, currentState: PresubmitGuardState( remaining: remaining, failed: failed, 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 3e0d7f4d5c..ae9928f54d 100644 --- a/app_dart/lib/src/service/firestore/unified_check_run.dart +++ b/app_dart/lib/src/service/firestore/unified_check_run.dart @@ -532,6 +532,7 @@ final class UnifiedCheckRun { var remaining = -1; var failed = -1; var valid = false; + var previousState = const PresubmitGuardState(remaining: -1, failed: -1); late final PresubmitGuard presubmitGuard; late final PresubmitJob presubmitJob; @@ -549,6 +550,10 @@ final class UnifiedCheckRun { transaction: transaction, ); presubmitGuard = PresubmitGuard.fromDocument(presubmitGuardDocument); + previousState = PresubmitGuardState( + remaining: presubmitGuard.remainingJobs, + failed: presubmitGuard.failedJobs, + ); // Check if the build is present in the guard before trying to load it. if (presubmitGuard.jobs[state.jobName] == null) { @@ -558,6 +563,7 @@ final class UnifiedCheckRun { await firestoreService.rollback(transaction); return PresubmitGuardConclusion( result: PresubmitGuardConclusionResult.missing, + previousState: previousState, currentState: PresubmitGuardState( remaining: presubmitGuard.remainingJobs, failed: presubmitGuard.failedJobs, @@ -655,10 +661,8 @@ final class UnifiedCheckRun { await firestoreService.rollback(transaction); return PresubmitGuardConclusion( result: PresubmitGuardConclusionResult.internalError, - currentState: PresubmitGuardState( - remaining: -1, - failed: failed, - ), + previousState: previousState, + currentState: PresubmitGuardState(remaining: -1, failed: failed), checkRunGuard: null, summary: 'Internal server error', details: @@ -692,10 +696,8 @@ $stack result: valid ? PresubmitGuardConclusionResult.ok : PresubmitGuardConclusionResult.internalError, - currentState: PresubmitGuardState( - remaining: remaining, - failed: failed, - ), + previousState: previousState, + currentState: PresubmitGuardState(remaining: remaining, failed: failed), checkRunGuard: presubmitGuard.checkRunJson, summary: valid ? 'Successfully updated presubmit guard status' diff --git a/app_dart/test/model/firestore/ci_staging_test.dart b/app_dart/test/model/firestore/ci_staging_test.dart index 8ef99be62f..ec2886c69d 100644 --- a/app_dart/test/model/firestore/ci_staging_test.dart +++ b/app_dart/test/model/firestore/ci_staging_test.dart @@ -160,6 +160,7 @@ void main() { expect( result, const PresubmitGuardConclusion( + previousState: PresubmitGuardState(remaining: 1, failed: 0), currentState: PresubmitGuardState(remaining: 1, failed: 0), result: PresubmitGuardConclusionResult.missing, checkRunGuard: null, @@ -227,6 +228,7 @@ void main() { expect( result, const PresubmitGuardConclusion( + previousState: PresubmitGuardState(remaining: 1, failed: 0), currentState: PresubmitGuardState(remaining: 0, failed: 0), result: PresubmitGuardConclusionResult.ok, checkRunGuard: '{}', @@ -268,6 +270,7 @@ For CI stage engine: expect( result, const PresubmitGuardConclusion( + previousState: PresubmitGuardState(remaining: 1, failed: 0), currentState: PresubmitGuardState(remaining: 1, failed: 0), result: PresubmitGuardConclusionResult.internalError, checkRunGuard: '{}', @@ -306,6 +309,7 @@ For CI stage engine: expect( result, const PresubmitGuardConclusion( + previousState: PresubmitGuardState(remaining: 1, failed: 1), currentState: PresubmitGuardState(remaining: 1, failed: 0), result: PresubmitGuardConclusionResult.ok, checkRunGuard: '{}', @@ -348,6 +352,7 @@ For CI stage engine: expect( result, const PresubmitGuardConclusion( + previousState: PresubmitGuardState(remaining: 1, failed: 1), currentState: PresubmitGuardState(remaining: 1, failed: 0), result: PresubmitGuardConclusionResult.ok, checkRunGuard: '{}', @@ -389,6 +394,7 @@ For CI stage engine: expect( result, const PresubmitGuardConclusion( + previousState: PresubmitGuardState(remaining: 1, failed: 1), currentState: PresubmitGuardState(remaining: 1, failed: 1), result: PresubmitGuardConclusionResult.internalError, checkRunGuard: '{}', @@ -426,6 +432,7 @@ For CI stage engine: expect( result, const PresubmitGuardConclusion( + previousState: PresubmitGuardState(remaining: 1, failed: 0), currentState: PresubmitGuardState(remaining: 1, failed: 1), result: PresubmitGuardConclusionResult.ok, checkRunGuard: '{}', 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 93b5036a8f..3da464d386 100644 --- a/app_dart/test/service/firestore/unified_check_run_test.dart +++ b/app_dart/test/service/firestore/unified_check_run_test.dart @@ -187,6 +187,8 @@ void main() { ); expect(result.result, PresubmitGuardConclusionResult.ok); + expect(result.previousState.remaining, 2); + expect(result.previousState.failed, 0); expect(result.currentState.remaining, 1); expect(result.currentState.failed, 0); @@ -220,6 +222,8 @@ void main() { ), ); + expect(result1.previousState.remaining, 2); + expect(result1.previousState.failed, 0); expect(result1.currentState.remaining, 1); expect(result1.currentState.failed, 0); expect(result1.isOk, true); @@ -238,6 +242,8 @@ void main() { ), ); + expect(result2.previousState.remaining, 1); + expect(result2.previousState.failed, 0); expect(result2.currentState.remaining, 0); expect(result2.currentState.failed, 0); expect(result2.isOk, true); @@ -274,6 +280,8 @@ void main() { ); expect(result.result, PresubmitGuardConclusionResult.ok); + expect(result.previousState.remaining, 2); + expect(result.previousState.failed, 0); expect(result.currentState.remaining, 1); expect(result.currentState.failed, 1); expect(result.isFailed, true); From 47cc178e8c265ffd212029cd7c60151ed47ed1e9 Mon Sep 17 00:00:00 2001 From: Dmitry Grand Date: Tue, 30 Jun 2026 16:11:57 -0700 Subject: [PATCH 04/13] fail check after first LUCI build failed only for unified check run and only for first failed build --- .../common/presubmit_guard_conclusion.dart | 7 +-- .../lib/src/model/firestore/ci_staging.dart | 10 +--- app_dart/lib/src/service/scheduler.dart | 27 +++++++--- .../firestore/unified_check_run_test.dart | 2 - app_dart/test/service/scheduler_test.dart | 49 +++++++++++++++++++ 5 files changed, 72 insertions(+), 23 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 960240a0ec..9efe454696 100644 --- a/app_dart/lib/src/model/common/presubmit_guard_conclusion.dart +++ b/app_dart/lib/src/model/common/presubmit_guard_conclusion.dart @@ -37,10 +37,7 @@ class PresubmitGuardState { final int remaining; final int failed; - const PresubmitGuardState({ - required this.remaining, - required this.failed, - }); + const PresubmitGuardState({required this.remaining, required this.failed}); @override bool operator ==(Object other) => @@ -82,8 +79,6 @@ class PresubmitGuardConclusion { bool get isFailed => isOk && currentState.failed > 0; - bool get isComplete => isOk && !isPending && !isFailed; - @override bool operator ==(Object other) => identical(this, other) || diff --git a/app_dart/lib/src/model/firestore/ci_staging.dart b/app_dart/lib/src/model/firestore/ci_staging.dart index b2f15f354c..703d20b10a 100644 --- a/app_dart/lib/src/model/firestore/ci_staging.dart +++ b/app_dart/lib/src/model/firestore/ci_staging.dart @@ -358,10 +358,7 @@ final class CiStaging extends AppDocument { return PresubmitGuardConclusion( result: PresubmitGuardConclusionResult.internalError, previousState: previousState, - currentState: PresubmitGuardState( - remaining: -1, - failed: failed, - ), + currentState: PresubmitGuardState(remaining: -1, failed: failed), checkRunGuard: null, summary: 'Internal server error', details: @@ -399,10 +396,7 @@ $stack ? PresubmitGuardConclusionResult.ok : PresubmitGuardConclusionResult.internalError, previousState: previousState, - currentState: PresubmitGuardState( - remaining: remaining, - failed: failed, - ), + currentState: PresubmitGuardState(remaining: remaining, failed: failed), checkRunGuard: checkRunGuard ?? '', summary: valid ? 'All tests passed' diff --git a/app_dart/lib/src/service/scheduler.dart b/app_dart/lib/src/service/scheduler.dart index f7fc4e2df8..00f61c906c 100644 --- a/app_dart/lib/src/service/scheduler.dart +++ b/app_dart/lib/src/service/scheduler.dart @@ -1204,6 +1204,15 @@ detailsUrl: $detailsUrl return false; } + // Are there tests remaining? Keep waiting. + // For unified check runs, we don't need to wait for the tests to complete. + if (stagingConclusion.isPending && !check.isUnifiedCheckRun) { + log.info( + '$logCrumb: not progressing, remaining work count: ${stagingConclusion.currentState.remaining}', + ); + return false; + } + if (stagingConclusion.isFailed) { // Something failed in the current CI stage: // @@ -1223,6 +1232,17 @@ detailsUrl: $detailsUrl details: stagingConclusion.details, ); } else if (check.isUnifiedCheckRun) { + // For unified check runs, we fail the guard only for the first failed job. + // Subsequent failures are ignored, but we still log them. + if (stagingConclusion.previousState.failed > 0) { + log.info( + '$logCrumb: check run remains failing, ' + 'previously failed checks ${stagingConclusion.previousState.failed}, ' + 'currently failed checks ${stagingConclusion.currentState.failed}, ' + 'remaining checks ${stagingConclusion.currentState.remaining}', + ); + return false; + } final guard = checkRunFromString(stagingConclusion.checkRunGuard!); final detailsUrl = 'https://flutter-dashboard.appspot.com/#/presubmit?repo=${check.slug.name}&sha=${check.sha}'; @@ -1242,13 +1262,6 @@ detailsUrl: $detailsUrl return true; } - // Are there tests remaining? Keep waiting. - if (stagingConclusion.isPending) { - log.info( - '$logCrumb: not progressing, remaining work count: ${stagingConclusion.currentState.remaining}', - ); - return false; - } // The logic for finishing a stage is different between build and test stages: // 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 3da464d386..c9799674c3 100644 --- a/app_dart/test/service/firestore/unified_check_run_test.dart +++ b/app_dart/test/service/firestore/unified_check_run_test.dart @@ -227,7 +227,6 @@ void main() { expect(result1.currentState.remaining, 1); expect(result1.currentState.failed, 0); expect(result1.isOk, true); - expect(result1.isComplete, false); expect(result1.isPending, true); final result2 = await UnifiedCheckRun.markConclusion( @@ -247,7 +246,6 @@ void main() { expect(result2.currentState.remaining, 0); expect(result2.currentState.failed, 0); expect(result2.isOk, true); - expect(result2.isComplete, true); expect(result2.isPending, false); final checkDoc = await PresubmitJob.fromFirestore( diff --git a/app_dart/test/service/scheduler_test.dart b/app_dart/test/service/scheduler_test.dart index 4c7937a12e..5ee3542a44 100644 --- a/app_dart/test/service/scheduler_test.dart +++ b/app_dart/test/service/scheduler_test.dart @@ -1758,6 +1758,55 @@ targets: ); }); + test( + 'does not fail immediately when a test check run fails if tests remain', + () async { + await CiStaging.initializeDocument( + firestoreService: firestore, + slug: Config.flutterSlug, + sha: 'abc123', + stage: CiStage.fusionEngineBuild, + tasks: ['Foo foo', 'Bar bar'], + checkRunGuard: '{}', + ); + + expect( + await scheduler.processCheckRunCompleted( + PresubmitCompletedJob.fromCheckRun( + createCocoonCheckRun( + name: 'Bar bar', + sha: 'abc123', + conclusion: 'failure', + ), + createGithubRepository().slug(), + ), + ), + isFalse, + ); + + expect( + firestore, + existsInStorage(CiStaging.metadata, [ + isCiStaging.hasCheckRuns({ + 'Foo foo': TaskConclusion.scheduled, + 'Bar bar': TaskConclusion.failure, + }), + ]), + ); + + verifyNever( + mockGithubChecksUtil.updateCheckRun( + any, + any, + any, + status: anyNamed('status'), + conclusion: anyNamed('conclusion'), + output: anyNamed('output'), + ), + ); + }, + ); + // The merge guard is not closed until both engine build and tests // complete and are successful. // This behavior is explained here: From ba6db95d15231788693a03f71e5bd1ca0133f2e7 Mon Sep 17 00:00:00 2001 From: Dmitry Grand Date: Tue, 30 Jun 2026 16:25:11 -0700 Subject: [PATCH 05/13] format --- app_dart/lib/src/service/scheduler.dart | 1 - 1 file changed, 1 deletion(-) diff --git a/app_dart/lib/src/service/scheduler.dart b/app_dart/lib/src/service/scheduler.dart index 00f61c906c..baac8462a1 100644 --- a/app_dart/lib/src/service/scheduler.dart +++ b/app_dart/lib/src/service/scheduler.dart @@ -1262,7 +1262,6 @@ detailsUrl: $detailsUrl return true; } - // The logic for finishing a stage is different between build and test stages: // // * If this is a build stage, then: From d93ff724b13c8cea68e60ef9f9b629c4467c3676 Mon Sep 17 00:00:00 2001 From: Dmitry Grand Date: Wed, 1 Jul 2026 13:42:14 -0700 Subject: [PATCH 06/13] refactoring --- .../presubmit_luci_subscription.dart | 2 +- app_dart/lib/src/service/scheduler.dart | 222 ++++++------ ...submit_luci_subscription_neutral_test.dart | 16 +- .../presubmit_luci_subscription_test.dart | 40 +-- app_dart/test/service/scheduler_test.dart | 320 ++++++++---------- .../lib/src/utilities/mocks.mocks.dart | 7 +- 6 files changed, 288 insertions(+), 319 deletions(-) diff --git a/app_dart/lib/src/request_handlers/presubmit_luci_subscription.dart b/app_dart/lib/src/request_handlers/presubmit_luci_subscription.dart index 5878d32117..3de615ba09 100644 --- a/app_dart/lib/src/request_handlers/presubmit_luci_subscription.dart +++ b/app_dart/lib/src/request_handlers/presubmit_luci_subscription.dart @@ -173,7 +173,7 @@ final class PresubmitLuciSubscription extends SubscriptionHandler { userData, status: override == .neutral ? .neutral : null, ); - await _scheduler.processCheckRunCompleted(check); + await _scheduler.processCheckRunStatusChange(check); } } diff --git a/app_dart/lib/src/service/scheduler.dart b/app_dart/lib/src/service/scheduler.dart index baac8462a1..2f89c149ac 100644 --- a/app_dart/lib/src/service/scheduler.dart +++ b/app_dart/lib/src/service/scheduler.dart @@ -1121,9 +1121,9 @@ detailsUrl: $detailsUrl /// /// Handles both fusion engine build and test stages, and both pull requests /// and merge groups. - Future processCheckRunCompleted(PresubmitCompletedJob check) async { + Future processCheckRunStatusChange(PresubmitCompletedJob check) async { if (kCheckRunsToIgnore.contains(check.name)) { - return true; + return; } final flow = check.isUnifiedCheckRun ? 'unified' : 'github'; final requestor = check.isMergeGroup ? 'merge group' : 'pull request'; @@ -1132,150 +1132,159 @@ detailsUrl: $detailsUrl final isFusion = check.slug == Config.flutterSlug; if (!isFusion && !check.isUnifiedCheckRun) { - return true; + return; } - late CiStage stage; - late PresubmitGuardConclusion stagingConclusion; - if (check.isUnifiedCheckRun) { - stage = check.stage!; - stagingConclusion = await _markUnifiedCheckRunConclusion( - guardId: check.guardId, - state: check.state, - ); + await _processUnifiedCheckRunPresubmit(logCrumb, check); } else { - // for github flow check runs are processed only if the build succeeded or - // some kind of failure occurred. if (!check.status.isComplete) { - return true; + return; + } + await _processLegacyPresubmitAndMergeGroup(logCrumb, check); + } + } + + Future _processUnifiedCheckRunPresubmit( + String logCrumb, + PresubmitCompletedJob check, + ) async { + final stage = check.stage!; + final conclusion = await _markUnifiedCheckRunConclusion( + guardId: check.guardId, + state: check.state, + ); + + if (!conclusion.isOk) { + return; + } + + if (conclusion.result == PresubmitGuardConclusionResult.internalError) { + return; + } + + if (conclusion.isFailed) { + // For unified check runs, we fail the guard only for the first failed job. + // Subsequent failures are ignored, but we still log them. + if (conclusion.previousState.failed > 0) { + log.info( + '$logCrumb: check run remains failing, ' + 'previously failed checks ${conclusion.previousState.failed}, ' + 'currently failed checks ${conclusion.currentState.failed}, ' + 'remaining checks ${conclusion.currentState.remaining}', + ); + return; } - // Check runs are fired at every stage. However, at this point it is unknown - // if this check run belongs in the engine build stage or in the test stage. - // So first look for it in the engine stage, and if it's missing, look for - // it in the test stage. - stage = CiStage.fusionEngineBuild; - stagingConclusion = await _recordCurrentCiStage( + final guard = checkRunFromString(conclusion.checkRunGuard!); + final detailsUrl = + 'https://flutter-dashboard.appspot.com/#/presubmit?repo=${check.slug.name}&sha=${check.sha}'; + await _requireActionForGuard( + slug: check.slug, + lock: guard, + headSha: check.sha, + summary: _githubChecksService.getGithubSummaryWithHeader(''' +**[Failed Checks Details]($detailsUrl)** + +''', kDashboardChecksDescription), + details: + 'For ${check.stage} CI stage ${conclusion.currentState.failed} checks failed', + detailsUrl: detailsUrl, + ); + return; + } + + // Are there tests remaining? Keep waiting. + if (conclusion.isPending) { + log.info( + '$logCrumb: not progressing, remaining work count: ${conclusion.currentState.remaining}', + ); + return; + } + + await _processSuccessedCheck(logCrumb, check, stage, conclusion); + } + + Future _processLegacyPresubmitAndMergeGroup( + String logCrumb, + PresubmitCompletedJob check, + ) async { + var stage = CiStage.fusionEngineBuild; + var conclusion = await _recordCurrentCiStage( + slug: check.slug, + sha: check.sha, + stage: stage, + name: check.name, + conclusion: check.status.toTaskConclusion(), + ); + + if (conclusion.result == PresubmitGuardConclusionResult.missing) { + stage = CiStage.fusionTests; + conclusion = await _recordCurrentCiStage( slug: check.slug, sha: check.sha, stage: stage, name: check.name, conclusion: check.status.toTaskConclusion(), ); - - if (stagingConclusion.result == PresubmitGuardConclusionResult.missing) { - // Check run not found in the engine stage. Look for it in the test stage. - stage = CiStage.fusionTests; - stagingConclusion = await _recordCurrentCiStage( - slug: check.slug, - sha: check.sha, - stage: stage, - name: check.name, - conclusion: check.status.toTaskConclusion(), - ); - } } - // First; check if we even recorded anything. This can occur if we've already passed the check_run and - // have moved on to running more tests (which wouldn't be present in our document). - if (!stagingConclusion.isOk) { - return false; + + if (!conclusion.isOk) { + return; } - // If an internal error happened in Cocoon, we need human assistance to - // figure out next steps. - if (stagingConclusion.result == + if (conclusion.result == PresubmitGuardConclusionResult.internalError) { - // If an internal error happened in the merge group, there may be no further - // signals from GitHub that would cause the merge group to either land or - // fail. The safest thing to do is to kick the pull request out of the queue - // and let humans sort it out. If the group is left hanging in the queue, it - // 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(conclusion.checkRunGuard!); await failGuardForMergeGroup( slug: check.slug, lock: guard, headSha: check.sha, - summary: stagingConclusion.summary, - details: stagingConclusion.details, + summary: conclusion.summary, + details: conclusion.details, ); } - return false; + return; } - // Are there tests remaining? Keep waiting. - // For unified check runs, we don't need to wait for the tests to complete. - if (stagingConclusion.isPending && !check.isUnifiedCheckRun) { + if (conclusion.isPending) { log.info( - '$logCrumb: not progressing, remaining work count: ${stagingConclusion.currentState.remaining}', + '$logCrumb: not progressing, remaining work count: ${conclusion.currentState.remaining}', ); - return false; + return; } - 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 (conclusion.isFailed) { if (check.isMergeGroup) { await _completeArtifacts(check.sha, false); - final guard = checkRunFromString(stagingConclusion.checkRunGuard!); + final guard = checkRunFromString(conclusion.checkRunGuard!); await failGuardForMergeGroup( slug: check.slug, lock: guard, headSha: check.sha, - summary: stagingConclusion.summary, - details: stagingConclusion.details, - ); - } else if (check.isUnifiedCheckRun) { - // For unified check runs, we fail the guard only for the first failed job. - // Subsequent failures are ignored, but we still log them. - if (stagingConclusion.previousState.failed > 0) { - log.info( - '$logCrumb: check run remains failing, ' - 'previously failed checks ${stagingConclusion.previousState.failed}, ' - 'currently failed checks ${stagingConclusion.currentState.failed}, ' - 'remaining checks ${stagingConclusion.currentState.remaining}', - ); - return false; - } - final guard = checkRunFromString(stagingConclusion.checkRunGuard!); - final detailsUrl = - 'https://flutter-dashboard.appspot.com/#/presubmit?repo=${check.slug.name}&sha=${check.sha}'; - await _requireActionForGuard( - slug: check.slug, - lock: guard, - headSha: check.sha, - summary: _githubChecksService.getGithubSummaryWithHeader(''' -**[Failed Checks Details]($detailsUrl)** - -''', kDashboardChecksDescription), - details: - 'For ${check.stage} CI stage ${stagingConclusion.currentState.failed} checks failed', - detailsUrl: detailsUrl, + summary: conclusion.summary, + details: conclusion.details, ); } - return true; + return; } - // The logic for finishing a stage is different between build and test stages: - // - // * If this is a build stage, then: - // * If this is a pull request presubmit, then start the test stage. - // * If this is a merge group (in MQ), then close the MQ guard, letting - // GitHub land it. - // * If this is a test stage, then close the MQ guard (allowing the PR to - // enter the MQ). + await _processSuccessedCheck(logCrumb, check, stage, conclusion); + } + + Future _processSuccessedCheck( + String logCrumb, + PresubmitCompletedJob check, + CiStage stage, + PresubmitGuardConclusion conclusion, + ) async { switch (stage) { case CiStage.fusionEngineBuild: if (check.isMergeGroup) { await _completeArtifacts(check.sha, true); await _closeMergeQueue( - mergeQueueGuard: stagingConclusion.checkRunGuard!, + mergeQueueGuard: conclusion.checkRunGuard!, slug: check.slug, sha: check.sha, stage: CiStage.fusionEngineBuild, @@ -1284,7 +1293,7 @@ detailsUrl: $detailsUrl } else { await _closeSuccessfulEngineBuildStage( checkRun: check.checkRun, - mergeQueueGuard: stagingConclusion.checkRunGuard!, + mergeQueueGuard: conclusion.checkRunGuard!, slug: check.slug, sha: check.sha, logCrumb: logCrumb, @@ -1292,7 +1301,7 @@ detailsUrl: $detailsUrl } case CiStage.fusionTests: await _closeSuccessfulTestStage( - mergeQueueGuard: stagingConclusion.checkRunGuard!, + mergeQueueGuard: conclusion.checkRunGuard!, slug: check.slug, sha: check.sha, logCrumb: logCrumb, @@ -1300,19 +1309,16 @@ detailsUrl: $detailsUrl case CiStage.genericTests: if (check.isUnifiedCheckRun) { await _closeSuccessfulTestStage( - mergeQueueGuard: stagingConclusion.checkRunGuard!, + mergeQueueGuard: conclusion.checkRunGuard!, slug: check.slug, sha: check.sha, logCrumb: logCrumb, ); } else { - // generic tests do not have a staging document nor are associated - // with a merge group - they are only used to collect commit stats. log.warn('$logCrumb: generic tests have no merge queue guard.'); } break; } - return true; } Future _completeArtifacts(String commitSha, bool successful) async { @@ -1638,7 +1644,7 @@ $stacktrace switch (checkRunEvent.action) { case 'completed': if (!_config.flags.closeMqGuardAfterPresubmit) { - await processCheckRunCompleted( + await processCheckRunStatusChange( PresubmitCompletedJob.fromCheckRun( checkRunEvent.checkRun!, checkRunEvent.repository!.slug(), diff --git a/app_dart/test/request_handlers/presubmit_luci_subscription_neutral_test.dart b/app_dart/test/request_handlers/presubmit_luci_subscription_neutral_test.dart index 1dfae51952..3d531fa0ea 100644 --- a/app_dart/test/request_handlers/presubmit_luci_subscription_neutral_test.dart +++ b/app_dart/test/request_handlers/presubmit_luci_subscription_neutral_test.dart @@ -110,8 +110,8 @@ void main() { ).thenAnswer((_) async => true); when( - mockScheduler.processCheckRunCompleted(any), - ).thenAnswer((_) async => true); + mockScheduler.processCheckRunStatusChange(any), + ).thenAnswer((_) async {}); tester.message = createPushMessage( Int64(1), @@ -141,12 +141,12 @@ void main() { // Verify that processCheckRunCompleted was called with TaskStatus.neutral final captured = verify( - mockScheduler.processCheckRunCompleted(captureAny), + mockScheduler.processCheckRunStatusChange(captureAny), ).captured; expect(captured, hasLength(1)); expect( captured[0], - isA().having( + isA().having( (e) => e.status, 'status', TaskStatus.neutral, @@ -182,8 +182,8 @@ void main() { ).thenAnswer((_) async => true); when( - mockScheduler.processCheckRunCompleted(any), - ).thenAnswer((_) async => true); + mockScheduler.processCheckRunStatusChange(any), + ).thenAnswer((_) async {}); tester.message = createPushMessage( Int64(1), @@ -208,12 +208,12 @@ void main() { // Verify that processCheckRunCompleted was called with TaskStatus.failed final captured = verify( - mockScheduler.processCheckRunCompleted(captureAny), + mockScheduler.processCheckRunStatusChange(captureAny), ).captured; expect(captured, hasLength(1)); expect( captured[0], - isA().having( + isA().having( (e) => e.status, 'status', TaskStatus.failed, 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 54f1bb0476..c888965099 100644 --- a/app_dart/test/request_handlers/presubmit_luci_subscription_test.dart +++ b/app_dart/test/request_handlers/presubmit_luci_subscription_test.dart @@ -89,8 +89,8 @@ void main() { mockGithubChecksService.conclusionForResult(any), ).thenAnswer((_) => github.CheckRunConclusion.empty); when( - mockScheduler.processCheckRunCompleted(any), - ).thenAnswer((_) async => true); + mockScheduler.processCheckRunStatusChange(any), + ).thenAnswer((_) async {}); tester.message = createPushMessage( Int64(1), @@ -117,7 +117,7 @@ void main() { ), ).called(1); - verify(mockScheduler.processCheckRunCompleted(any)).called(1); + verify(mockScheduler.processCheckRunStatusChange(any)).called(1); }); test('Requests when task failed but no need to reschedule', () async { @@ -134,8 +134,8 @@ void main() { mockGithubChecksService.conclusionForResult(any), ).thenAnswer((_) => github.CheckRunConclusion.empty); when( - mockScheduler.processCheckRunCompleted(any), - ).thenAnswer((_) async => true); + mockScheduler.processCheckRunStatusChange(any), + ).thenAnswer((_) async {}); final userData = PresubmitUserData( commit: CommitRef( @@ -176,7 +176,7 @@ void main() { slug: anyNamed('slug'), ), ).called(1); - verify(mockScheduler.processCheckRunCompleted(any)).called(1); + verify(mockScheduler.processCheckRunStatusChange(any)).called(1); }); test('Requests when task failed but need to reschedule', () async { @@ -215,7 +215,7 @@ void main() { rescheduled: true, ), ).called(1); - verifyNever(mockScheduler.processCheckRunCompleted(any)); + verifyNever(mockScheduler.processCheckRunStatusChange(any)); }); test('Build rescheduled when in merge queue', () async { @@ -297,7 +297,7 @@ void main() { rescheduled: true, ), ).called(1); - verifyNever(mockScheduler.processCheckRunCompleted(any)); + verifyNever(mockScheduler.processCheckRunStatusChange(any)); }); test('Build not rescheduled if not found in ciYaml list.', () async { @@ -315,8 +315,8 @@ void main() { mockGithubChecksService.conclusionForResult(any), ).thenAnswer((_) => github.CheckRunConclusion.empty); when( - mockScheduler.processCheckRunCompleted(any), - ).thenAnswer((_) async => true); + mockScheduler.processCheckRunStatusChange(any), + ).thenAnswer((_) async {}); final userData = PresubmitUserData( commit: CommitRef( @@ -360,7 +360,7 @@ void main() { ), ).called(1); - verify(mockScheduler.processCheckRunCompleted(any)).called(1); + verify(mockScheduler.processCheckRunStatusChange(any)).called(1); }); test('Build not rescheduled if ci.yaml fails validation.', () async { @@ -378,8 +378,8 @@ void main() { mockGithubChecksService.conclusionForResult(any), ).thenAnswer((_) => github.CheckRunConclusion.empty); when( - mockScheduler.processCheckRunCompleted(any), - ).thenAnswer((_) async => true); + mockScheduler.processCheckRunStatusChange(any), + ).thenAnswer((_) async {}); final userData = PresubmitUserData( checkRunId: 1, @@ -421,7 +421,7 @@ void main() { rescheduled: false, ), ).called(1); - verify(mockScheduler.processCheckRunCompleted(any)).called(1); + verify(mockScheduler.processCheckRunStatusChange(any)).called(1); }); test('Pubsub rejected if branch is not enabled.', () async { @@ -528,7 +528,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.processCheckRunStatusChange(any)); }); test('Close the MQ guard once presubmit compleated', () async { @@ -561,13 +561,13 @@ void main() { mockGithubChecksService.conclusionForResult(bbv2.Status.SUCCESS), ).thenAnswer((_) => github.CheckRunConclusion.success); when( - mockScheduler.processCheckRunCompleted(any), - ).thenAnswer((_) async => true); + mockScheduler.processCheckRunStatusChange(any), + ).thenAnswer((_) async {}); await tester.post(handler); final captured = verify( - mockScheduler.processCheckRunCompleted(captureAny), + mockScheduler.processCheckRunStatusChange(captureAny), ).captured; expect(captured, hasLength(1)); expect( @@ -623,8 +623,8 @@ void main() { ).thenAnswer((_) async => true); when( - mockScheduler.processCheckRunCompleted(any), - ).thenAnswer((_) async => true); + mockScheduler.processCheckRunStatusChange(any), + ).thenAnswer((_) async {}); tester.message = createPushMessage( Int64(1), diff --git a/app_dart/test/service/scheduler_test.dart b/app_dart/test/service/scheduler_test.dart index 5ee3542a44..4e4646d8a4 100644 --- a/app_dart/test/service/scheduler_test.dart +++ b/app_dart/test/service/scheduler_test.dart @@ -1652,14 +1652,11 @@ targets: ); for (final ignored in Scheduler.kCheckRunsToIgnore) { - expect( - await scheduler.processCheckRunCompleted( - PresubmitCompletedJob.fromCheckRun( - createCocoonCheckRun(name: ignored, sha: 'abc123'), - createGithubRepository().slug(), - ), + await scheduler.processCheckRunStatusChange( + PresubmitCompletedJob.fromCheckRun( + createCocoonCheckRun(name: ignored, sha: 'abc123'), + createGithubRepository().slug(), ), - isTrue, ); } @@ -1687,14 +1684,11 @@ targets: firestore.failOnWriteDocument(document); - expect( - await scheduler.processCheckRunCompleted( - PresubmitCompletedJob.fromCheckRun( - createCocoonCheckRun(name: 'Bar bar', sha: 'abc123'), - createGithubRepository().slug(), - ), + await scheduler.processCheckRunStatusChange( + PresubmitCompletedJob.fromCheckRun( + createCocoonCheckRun(name: 'Bar bar', sha: 'abc123'), + createGithubRepository().slug(), ), - isFalse, ); expect( @@ -1726,14 +1720,11 @@ targets: checkRunGuard: '{}', ); - expect( - await scheduler.processCheckRunCompleted( - PresubmitCompletedJob.fromCheckRun( - createCocoonCheckRun(name: 'Bar bar', sha: 'abc123'), - createGithubRepository().slug(), - ), + await scheduler.processCheckRunStatusChange( + PresubmitCompletedJob.fromCheckRun( + createCocoonCheckRun(name: 'Bar bar', sha: 'abc123'), + createGithubRepository().slug(), ), - isFalse, ); expect( @@ -1770,18 +1761,15 @@ targets: checkRunGuard: '{}', ); - expect( - await scheduler.processCheckRunCompleted( - PresubmitCompletedJob.fromCheckRun( - createCocoonCheckRun( - name: 'Bar bar', - sha: 'abc123', - conclusion: 'failure', - ), - createGithubRepository().slug(), + await scheduler.processCheckRunStatusChange( + PresubmitCompletedJob.fromCheckRun( + createCocoonCheckRun( + name: 'Bar bar', + sha: 'abc123', + conclusion: 'failure', ), + createGithubRepository().slug(), ), - isFalse, ); expect( @@ -1829,14 +1817,11 @@ targets: checkRunGuard: checkRunFor(name: 'GUARD TEST'), ); - expect( - await scheduler.processCheckRunCompleted( - PresubmitCompletedJob.fromCheckRun( - createCocoonCheckRun(name: 'Bar bar', sha: 'abc123'), - createGithubRepository().slug(), - ), + await scheduler.processCheckRunStatusChange( + PresubmitCompletedJob.fromCheckRun( + createCocoonCheckRun(name: 'Bar bar', sha: 'abc123'), + createGithubRepository().slug(), ), - isTrue, ); expect( @@ -1939,14 +1924,11 @@ targets: checkRunGuard: checkRunFor(name: 'GUARD TEST'), ); - expect( - await scheduler.processCheckRunCompleted( - PresubmitCompletedJob.fromCheckRun( - createCocoonCheckRun(name: 'Bar bar', sha: 'testSha'), - createGithubRepository().slug(), - ), + await scheduler.processCheckRunStatusChange( + PresubmitCompletedJob.fromCheckRun( + createCocoonCheckRun(name: 'Bar bar', sha: 'testSha'), + createGithubRepository().slug(), ), - isTrue, ); verify( @@ -2083,18 +2065,15 @@ targets: checkRunGuard: checkRunFor(name: 'GUARD TEST'), ); - expect( - await scheduler.processCheckRunCompleted( - PresubmitCompletedJob.fromCheckRun( - createCocoonCheckRun( - name: 'Bar bar', - sha: 'testSha', - checkSuiteId: 0, - ), - createGithubRepository().slug(), + await scheduler.processCheckRunStatusChange( + PresubmitCompletedJob.fromCheckRun( + createCocoonCheckRun( + name: 'Bar bar', + sha: 'testSha', + checkSuiteId: 0, ), + createGithubRepository().slug(), ), - isTrue, ); verify( @@ -2192,14 +2171,11 @@ targets: checkRunGuard: checkRunFor(name: 'GUARD TEST'), ); - expect( - await scheduler.processCheckRunCompleted( - PresubmitCompletedJob.fromCheckRun( - createCocoonCheckRun(name: 'Bar bar', sha: 'testSha'), - createGithubRepository().slug(), - ), + await scheduler.processCheckRunStatusChange( + PresubmitCompletedJob.fromCheckRun( + createCocoonCheckRun(name: 'Bar bar', sha: 'testSha'), + createGithubRepository().slug(), ), - isTrue, ); // The first invocation looks in the fusionEngineBuild stage, which @@ -2456,18 +2432,15 @@ targets: checkRunGuard: checkRunFor(name: 'GUARD TEST'), ); - expect( - await scheduler.processCheckRunCompleted( - PresubmitCompletedJob.fromCheckRun( - createCocoonCheckRun( - name: 'Bar bar', - sha: 'testSha', - conclusion: 'failure', - ), - createGithubRepository().slug(), + await scheduler.processCheckRunStatusChange( + PresubmitCompletedJob.fromCheckRun( + createCocoonCheckRun( + name: 'Bar bar', + sha: 'testSha', + conclusion: 'failure', ), + createGithubRepository().slug(), ), - isTrue, ); // The first invocation looks in the fusionEngineBuild stage, which @@ -2539,19 +2512,16 @@ targets: ), ); - expect( - await scheduler.processCheckRunCompleted( - PresubmitCompletedJob.fromCheckRun( - createCocoonCheckRun( - name: 'Bar bar', - sha: 'testSha', - conclusion: 'failure', - headBranch: headBranch, - ), - createGithubRepository().slug(), + await scheduler.processCheckRunStatusChange( + PresubmitCompletedJob.fromCheckRun( + createCocoonCheckRun( + name: 'Bar bar', + sha: 'testSha', + conclusion: 'failure', + headBranch: headBranch, ), + createGithubRepository().slug(), ), - isTrue, ); // The first invocation looks in the fusionEngineBuild stage, which @@ -2622,18 +2592,15 @@ targets: ), ); - expect( - await scheduler.processCheckRunCompleted( - PresubmitCompletedJob.fromCheckRun( - createCocoonCheckRun( - name: 'Bar bar', - sha: 'testSha', - headBranch: headBranch, - ), - createGithubRepository().slug(), + await scheduler.processCheckRunStatusChange( + PresubmitCompletedJob.fromCheckRun( + createCocoonCheckRun( + name: 'Bar bar', + sha: 'testSha', + headBranch: headBranch, ), + createGithubRepository().slug(), ), - isTrue, ); // The first invocation looks in the fusionEngineBuild stage, which @@ -2745,14 +2712,11 @@ targets: checkRunGuard: checkRunFor(name: 'GUARD TEST'), ); - expect( - await scheduler.processCheckRunCompleted( - PresubmitCompletedJob.fromCheckRun( - createCocoonCheckRun(name: 'Bar bar', sha: 'testSha'), - createGithubRepository().slug(), - ), + await scheduler.processCheckRunStatusChange( + PresubmitCompletedJob.fromCheckRun( + createCocoonCheckRun(name: 'Bar bar', sha: 'testSha'), + createGithubRepository().slug(), ), - isTrue, ); verifyNever( @@ -4293,7 +4257,7 @@ targets: final check = PresubmitCompletedJob.fromBuild(build, userData); - expect(await scheduler.processCheckRunCompleted(check), isTrue); + await scheduler.processCheckRunStatusChange(check); // Should schedule tests for the next stage (fusionTests) expect(fakeLuciBuildService.scheduledTryBuilds, isNotEmpty); @@ -4307,91 +4271,89 @@ targets: expect(guard.remainingJobs, 0); }); - test( - 'fails the merge queue guard when a test check run fails (merge group)', - () async { - final pullRequest = generatePullRequest(); - final checkRunGuard = generateCheckRun( - 1234, - name: Config.kMergeQueueLockName, - startedAt: DateTime.now(), - ); + test('fails the guard check when a test check run fails', () async { + final pullRequest = generatePullRequest(); + final checkRunGuard = generateCheckRun( + 1234, + name: Config.kDashboardCheckName, + startedAt: DateTime.now(), + ); - await PrCheckRuns.initializeDocument( - firestoreService: firestore, - checks: [checkRunGuard], - pullRequest: pullRequest, - ); + await PrCheckRuns.initializeDocument( + firestoreService: firestore, + checks: [checkRunGuard], + pullRequest: pullRequest, + ); - // Make it look like a merge group - // checkRunGuard.checkSuite!.headBranch = 'gh-readonly-queue/master/pr-123-abc'; + // Make it look like a merge group + // checkRunGuard.checkSuite!.headBranch = 'gh-readonly-queue/master/pr-123-abc'; - // Initialize presubmit guard for tests stage - firestore.putDocument( - PresubmitGuard( - checkRun: checkRunGuard, - headSha: pullRequest.head!.sha!, - slug: pullRequest.base!.repo!.slug(), - prNum: pullRequest.number!, - stage: CiStage.fusionTests, - author: pullRequest.user!.login!, - creationTime: DateTime.now().millisecondsSinceEpoch, - jobs: {'Linux test': TaskStatus.waitingForBackfill}, - remainingJobs: 1, - failedJobs: 0, - ), - ); + // Initialize presubmit guard for tests stage + firestore.putDocument( + PresubmitGuard( + checkRun: checkRunGuard, + headSha: pullRequest.head!.sha!, + slug: pullRequest.base!.repo!.slug(), + prNum: pullRequest.number!, + stage: CiStage.fusionTests, + author: pullRequest.user!.login!, + creationTime: DateTime.now().millisecondsSinceEpoch, + jobs: {'Linux test': TaskStatus.waitingForBackfill}, + remainingJobs: 1, + failedJobs: 0, + ), + ); - // Initialize check run for the task - firestore.putDocument( - PresubmitJob.init( - slug: pullRequest.base!.repo!.slug(), - jobName: 'Linux test', - checkRunId: checkRunGuard.id!, - creationTime: DateTime.now().millisecondsSinceEpoch, - ), - ); + // Initialize check run for the task + firestore.putDocument( + PresubmitJob.init( + slug: pullRequest.base!.repo!.slug(), + jobName: 'Linux test', + checkRunId: checkRunGuard.id!, + creationTime: DateTime.now().millisecondsSinceEpoch, + ), + ); - final userData = PresubmitUserData( - commit: CommitRef( - slug: pullRequest.base!.repo!.slug(), - sha: pullRequest.head!.sha!, - branch: 'gh-readonly-queue/master/pr-123-abc', - ), - guardCheckRunId: checkRunGuard.id, - stage: CiStage.fusionTests, - checkSuiteId: 2, - pullRequestNumber: pullRequest.number, - ); + final userData = PresubmitUserData( + commit: CommitRef( + slug: pullRequest.base!.repo!.slug(), + sha: pullRequest.head!.sha!, + branch: 'master', + ), + guardCheckRunId: checkRunGuard.id, + stage: CiStage.fusionTests, + checkSuiteId: 2, + pullRequestNumber: pullRequest.number, + ); - final build = generateBbv2Build( - Int64(1), - name: 'Linux test', - status: bbv2.Status.FAILURE, - tags: [bbv2.StringPair(key: 'current_attempt', value: '1')], - ); + final build = generateBbv2Build( + Int64(1), + name: 'Linux test', + status: bbv2.Status.FAILURE, + tags: [bbv2.StringPair(key: 'current_attempt', value: '1')], + ); - final check = PresubmitCompletedJob.fromBuild(build, userData); + final check = PresubmitCompletedJob.fromBuild(build, userData); - expect(await scheduler.processCheckRunCompleted(check), isTrue); + await scheduler.processCheckRunStatusChange(check); - verify( - mockGithubChecksUtil.updateCheckRun( - any, - any, - any, - status: anyNamed('status'), - conclusion: CheckRunConclusion.failure, // Merge queue failure - detailsUrl: anyNamed('detailsUrl'), - output: anyNamed('output'), - ), - ).called(1); + verify( + mockGithubChecksUtil.updateCheckRun( + any, + any, + any, + status: anyNamed('status'), + conclusion: CheckRunConclusion.actionRequired, + detailsUrl: anyNamed('detailsUrl'), + output: anyNamed('output'), + actions: anyNamed('actions'), + ), + ).called(1); - final guards = await firestore.query(PresubmitGuard.collectionId, {}); - final guard = PresubmitGuard.fromDocument(guards.single); - expect(guard.failedJobs, 1); - }, - ); + final guards = await firestore.query(PresubmitGuard.collectionId, {}); + final guard = PresubmitGuard.fromDocument(guards.single); + expect(guard.failedJobs, 1); + }); test( 'fails the guard check immediately when a test check run fails even if jobs remain', @@ -4463,7 +4425,7 @@ targets: final check = PresubmitCompletedJob.fromBuild(build, userData); - expect(await scheduler.processCheckRunCompleted(check), isTrue); + await scheduler.processCheckRunStatusChange(check); verify( mockGithubChecksUtil.updateCheckRun( @@ -4555,7 +4517,7 @@ targets: final check = PresubmitCompletedJob.fromBuild(build, userData); - expect(await scheduler.processCheckRunCompleted(check), isTrue); + await scheduler.processCheckRunStatusChange(check); verify( mockGithubChecksUtil.updateCheckRun( @@ -4642,7 +4604,7 @@ targets: final check = PresubmitCompletedJob.fromBuild(build, userData); - expect(await scheduler.processCheckRunCompleted(check), isTrue); + await scheduler.processCheckRunStatusChange(check); 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 5d69d2556d..059299c7f7 100644 --- a/packages/cocoon_integration_test/lib/src/utilities/mocks.mocks.dart +++ b/packages/cocoon_integration_test/lib/src/utilities/mocks.mocks.dart @@ -5835,14 +5835,15 @@ class MockScheduler extends _i1.Mock implements _i17.Scheduler { as _i13.Future>); @override - _i13.Future processCheckRunCompleted( + _i13.Future processCheckRunStatusChange( _i38.PresubmitCompletedJob? check, ) => (super.noSuchMethod( Invocation.method(#processCheckRunCompleted, [check]), - returnValue: _i13.Future.value(false), + returnValue: _i13.Future.value(), + returnValueForMissingStub: _i13.Future.value(), ) - as _i13.Future); + as _i13.Future); @override bool detectMergeGroup(_i30.CheckRun? checkRun) => From 3e06cdfd7c503f46edd4272a78d8a8eaa4cf6933 Mon Sep 17 00:00:00 2001 From: Dmitry Grand Date: Wed, 1 Jul 2026 15:25:44 -0700 Subject: [PATCH 07/13] refactoring --- .../common/presubmit_completed_check.dart | 167 ------------------ .../src/model/common/presubmit_job_state.dart | 148 +++++++++++++--- .../presubmit_luci_subscription.dart | 6 +- .../service/firestore/unified_check_run.dart | 22 +-- app_dart/lib/src/service/scheduler.dart | 72 ++++---- .../common/presubmit_check_state_test.dart | 34 ---- .../presubmit_completed_check_test.dart | 8 +- ...submit_luci_subscription_neutral_test.dart | 14 +- .../presubmit_luci_subscription_test.dart | 32 ++-- .../firestore/unified_check_run_test.dart | 88 +++++++-- app_dart/test/service/scheduler_test.dart | 70 ++++---- .../lib/src/utilities/mocks.mocks.dart | 7 +- 12 files changed, 305 insertions(+), 363 deletions(-) delete mode 100644 app_dart/lib/src/model/common/presubmit_completed_check.dart delete mode 100644 app_dart/test/model/common/presubmit_check_state_test.dart diff --git a/app_dart/lib/src/model/common/presubmit_completed_check.dart b/app_dart/lib/src/model/common/presubmit_completed_check.dart deleted file mode 100644 index 6aba3c4f2e..0000000000 --- a/app_dart/lib/src/model/common/presubmit_completed_check.dart +++ /dev/null @@ -1,167 +0,0 @@ -// Copyright 2024 The Flutter Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -import 'package:buildbucket/buildbucket_pb.dart'; -import 'package:cocoon_common/task_status.dart'; -import 'package:fixnum/fixnum.dart'; -import 'package:github/github.dart'; -import 'package:meta/meta.dart'; - -import '../../foundation/utils.dart'; -import '../../service/config.dart'; -import '../../service/luci_build_service/build_tags.dart'; -import '../../service/luci_build_service/user_data.dart'; -import '../bbv2_extension.dart'; -import '../firestore/base.dart'; -import '../firestore/presubmit_guard.dart'; -import '../github/checks.dart' as cocoon_checks; -import 'checks_extension.dart'; -import 'presubmit_job_state.dart'; - -/// Unified representation of a completed presubmit job. -/// -/// This class abstracts away the source of the job (GitHub CheckRun or BuildBucket Build) -/// to allow unified processing logic. -@immutable -class PresubmitCompletedJob { - final String name; - final String sha; - final RepositorySlug slug; - final TaskStatus status; - final bool isMergeGroup; - - final int checkRunId; - final int? checkSuiteId; - final String? headBranch; - final bool isUnifiedCheckRun; - final CiStage? stage; - final int? prNum; - final int attempt; - final int? startTime; - final int? endTime; - final String? summary; - final int? buildNumber; - final Int64? buildId; - - const PresubmitCompletedJob({ - required this.name, - required this.sha, - required this.slug, - required this.status, - required this.isMergeGroup, - required this.checkRunId, - required this.checkSuiteId, - required this.headBranch, - required this.isUnifiedCheckRun, - this.stage, - this.prNum, - this.attempt = 1, - this.startTime, - this.endTime, - this.summary, - this.buildNumber, - this.buildId, - }); - - /// Creates a [PresubmitCompletedJob] from a GitHub [CheckRun]. - factory PresubmitCompletedJob.fromCheckRun( - cocoon_checks.CheckRun checkRun, - RepositorySlug slug, - ) { - return PresubmitCompletedJob( - name: checkRun.name!, - sha: checkRun.headSha!, - slug: slug, - status: ChecksExtension.fromConclusion(checkRun.conclusion), - isMergeGroup: _isMergeGroup(checkRun.checkSuite?.headBranch), - checkRunId: checkRun.id!, - checkSuiteId: checkRun.checkSuite?.id, - headBranch: checkRun.checkSuite?.headBranch, - isUnifiedCheckRun: false, - // CheckRun model doesn't have time/summary fields currently - startTime: null, - endTime: null, - summary: null, - buildNumber: null, - buildId: null, - ); - } - - /// Creates a [PresubmitCompletedJob] from a BuildBucket [Build]. - factory PresubmitCompletedJob.fromBuild( - Build build, - PresubmitUserData userData, { - TaskStatus? status, - }) { - return PresubmitCompletedJob( - name: build.builder.builder, - sha: userData.commit.sha, - slug: userData.commit.slug, - status: status ?? build.status.toTaskStatus(), - isMergeGroup: _isMergeGroup(userData.commit.branch), - checkRunId: userData.guardCheckRunId ?? userData.checkRunId!, - checkSuiteId: userData.checkSuiteId, - headBranch: userData.commit.branch, - isUnifiedCheckRun: userData.guardCheckRunId != null, - stage: userData.stage, - prNum: userData.pullRequestNumber, - attempt: _getAttempt(build), - startTime: build.startTime.toDateTime().millisecondsSinceEpoch, - endTime: build.endTime.toDateTime().millisecondsSinceEpoch, - summary: build.summaryMarkdown, - buildNumber: build.number, - buildId: build.id, - ); - } - - static int _getAttempt(Build build) { - final tagSet = BuildTags.fromStringPairs(build.tags); - return tagSet.currentAttempt; - } - - cocoon_checks.CheckRun get checkRun { - return cocoon_checks.CheckRun( - id: checkRunId, - name: isUnifiedCheckRun ? Config.kDashboardCheckName : name, - headSha: sha, - conclusion: status.toConclusion(), - checkSuite: CheckSuite( - id: checkSuiteId, - headBranch: headBranch, - headSha: sha, - conclusion: CheckRunConclusion.empty, - pullRequests: [], - ), - ); - } - - PresubmitGuardId get guardId { - return PresubmitGuardId( - slug: slug, - prNum: prNum ?? 0, - checkRunId: checkRunId, - stage: stage ?? CiStage.fusionTests, - ); - } - - PresubmitJobState get state { - return PresubmitJobState( - jobName: name, - status: status, - attemptNumber: attempt, - startTime: startTime, - endTime: endTime, - summary: summary, - buildNumber: buildNumber, - buildId: buildId, - ); - } - - static bool _isMergeGroup(String? headBranch) { - if (headBranch == null) { - return false; - } - return tryParseGitHubMergeQueueBranch(headBranch).parsed; - } -} diff --git a/app_dart/lib/src/model/common/presubmit_job_state.dart b/app_dart/lib/src/model/common/presubmit_job_state.dart index 856b18d40f..c017de465b 100644 --- a/app_dart/lib/src/model/common/presubmit_job_state.dart +++ b/app_dart/lib/src/model/common/presubmit_job_state.dart @@ -1,22 +1,42 @@ -// Copyright 2025 The Flutter Authors. All rights reserved. +// Copyright 2024 The Flutter Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -/// @docImport 'presubmit_job_state.dart'; -library; - -import 'package:buildbucket/buildbucket_pb.dart' as bbv2; +import 'package:buildbucket/buildbucket_pb.dart'; import 'package:cocoon_common/task_status.dart'; import 'package:fixnum/fixnum.dart'; +import 'package:github/github.dart'; +import 'package:meta/meta.dart'; +import '../../foundation/utils.dart'; +import '../../service/config.dart'; import '../../service/luci_build_service/build_tags.dart'; +import '../../service/luci_build_service/user_data.dart'; import '../bbv2_extension.dart'; +import '../firestore/base.dart'; +import '../firestore/presubmit_guard.dart'; +import '../github/checks.dart' as cocoon_checks; +import 'checks_extension.dart'; -/// Represents the current state of a check run. +/// Unified representation of a completed presubmit job. +/// +/// This class abstracts away the source of the job (GitHub CheckRun or BuildBucket Build) +/// to allow unified processing logic. +@immutable class PresubmitJobState { - final String jobName; + final String name; + final String sha; + final RepositorySlug slug; final TaskStatus status; - final int attemptNumber; //static int _currentAttempt(BuildTags buildTags) + final bool isMergeGroup; + + final int checkRunId; + final int? checkSuiteId; + final String? headBranch; + final bool isUnifiedCheckRun; + final CiStage? stage; + final int? prNum; + final int attempt; final int? startTime; final int? endTime; final String? summary; @@ -24,26 +44,110 @@ class PresubmitJobState { final Int64? buildId; const PresubmitJobState({ - required this.jobName, + required this.name, + required this.sha, + required this.slug, required this.status, - required this.attemptNumber, + required this.isMergeGroup, + required this.checkRunId, + required this.checkSuiteId, + required this.headBranch, + required this.isUnifiedCheckRun, + this.stage, + this.prNum, + this.attempt = 1, this.startTime, this.endTime, this.summary, this.buildNumber, this.buildId, }); -} -extension BuildToPresubmitJobState on bbv2.Build { - PresubmitJobState toPresubmitJobState() => PresubmitJobState( - jobName: builder.builder, - status: status.toTaskStatus(), - attemptNumber: BuildTags.fromStringPairs(tags).currentAttempt, - startTime: startTime.toDateTime().millisecondsSinceEpoch, - endTime: endTime.toDateTime().millisecondsSinceEpoch, - summary: summaryMarkdown, - buildNumber: number, - buildId: id, - ); + /// Creates a [PresubmitJobState] from a GitHub [CheckRun]. + factory PresubmitJobState.fromCheckRun( + cocoon_checks.CheckRun checkRun, + RepositorySlug slug, + ) { + return PresubmitJobState( + name: checkRun.name!, + sha: checkRun.headSha!, + slug: slug, + status: ChecksExtension.fromConclusion(checkRun.conclusion), + isMergeGroup: _isMergeGroup(checkRun.checkSuite?.headBranch), + checkRunId: checkRun.id!, + checkSuiteId: checkRun.checkSuite?.id, + headBranch: checkRun.checkSuite?.headBranch, + isUnifiedCheckRun: false, + // CheckRun model doesn't have time/summary fields currently + startTime: null, + endTime: null, + summary: null, + buildNumber: null, + buildId: null, + ); + } + + /// Creates a [PresubmitJobState] from a BuildBucket [Build]. + factory PresubmitJobState.fromBuild( + Build build, + PresubmitUserData userData, { + TaskStatus? status, + }) { + return PresubmitJobState( + name: build.builder.builder, + sha: userData.commit.sha, + slug: userData.commit.slug, + status: status ?? build.status.toTaskStatus(), + isMergeGroup: _isMergeGroup(userData.commit.branch), + checkRunId: userData.guardCheckRunId ?? userData.checkRunId!, + checkSuiteId: userData.checkSuiteId, + headBranch: userData.commit.branch, + isUnifiedCheckRun: userData.guardCheckRunId != null, + stage: userData.stage, + prNum: userData.pullRequestNumber, + attempt: _getAttempt(build), + startTime: build.startTime.toDateTime().millisecondsSinceEpoch, + endTime: build.endTime.toDateTime().millisecondsSinceEpoch, + summary: build.summaryMarkdown, + buildNumber: build.number, + buildId: build.id, + ); + } + + static int _getAttempt(Build build) { + final tagSet = BuildTags.fromStringPairs(build.tags); + return tagSet.currentAttempt; + } + + cocoon_checks.CheckRun get checkRun { + return cocoon_checks.CheckRun( + id: checkRunId, + name: isUnifiedCheckRun ? Config.kDashboardCheckName : name, + headSha: sha, + conclusion: status.toConclusion(), + checkSuite: CheckSuite( + id: checkSuiteId, + headBranch: headBranch, + headSha: sha, + conclusion: CheckRunConclusion.empty, + pullRequests: [], + ), + ); + } + + PresubmitGuardId get guardId { + return PresubmitGuardId( + slug: slug, + prNum: prNum ?? 0, + checkRunId: checkRunId, + stage: stage ?? CiStage.fusionTests, + ); + } + + static bool _isMergeGroup(String? headBranch) { + if (headBranch == null) { + return false; + } + return tryParseGitHubMergeQueueBranch(headBranch).parsed; + } } diff --git a/app_dart/lib/src/request_handlers/presubmit_luci_subscription.dart b/app_dart/lib/src/request_handlers/presubmit_luci_subscription.dart index 3de615ba09..538cfba8c7 100644 --- a/app_dart/lib/src/request_handlers/presubmit_luci_subscription.dart +++ b/app_dart/lib/src/request_handlers/presubmit_luci_subscription.dart @@ -14,7 +14,7 @@ import '../model/bbv2_extension.dart'; import '../model/ci_yaml/ci_yaml.dart'; import '../model/ci_yaml/target.dart'; import '../model/commit_ref.dart'; -import '../model/common/presubmit_completed_check.dart'; +import '../model/common/presubmit_job_state.dart'; import '../request_handling/exceptions.dart'; import '../request_handling/subscription_handler.dart'; import '../service/extensions/cache_service_test_suppression.dart'; @@ -168,12 +168,12 @@ final class PresubmitLuciSubscription extends SubscriptionHandler { // Process to the check-run status in the merge queue document during // the LUCI callback. if (config.flags.closeMqGuardAfterPresubmit || isUnifiedCheckRun) { - final check = PresubmitCompletedJob.fromBuild( + final check = PresubmitJobState.fromBuild( build, userData, status: override == .neutral ? .neutral : null, ); - await _scheduler.processCheckRunStatusChange(check); + await _scheduler.processJobStatusUpdate(check); } } 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 ae9928f54d..2b4e241d17 100644 --- a/app_dart/lib/src/service/firestore/unified_check_run.dart +++ b/app_dart/lib/src/service/firestore/unified_check_run.dart @@ -13,8 +13,8 @@ import 'package:googleapis/firestore/v1.dart' hide Status; import 'package:meta/meta.dart'; import '../../model/common/failed_presubmit_jobs.dart'; -import '../../model/common/presubmit_guard_conclusion.dart'; import '../../model/common/presubmit_job_state.dart'; +import '../../model/common/presubmit_guard_conclusion.dart'; import '../../model/firestore/base.dart'; import '../../model/firestore/ci_staging.dart'; import '../../model/firestore/presubmit_guard.dart'; @@ -521,7 +521,7 @@ final class UnifiedCheckRun { final changeCrumb = '${guardId.slug.owner}_${guardId.slug.name}_${guardId.prNum}_${guardId.checkRunId}'; final logCrumb = - 'markConclusion(${changeCrumb}_${guardId.stage}, ${state.jobName}, ${state.status}, ${state.attemptNumber})'; + 'markConclusion(${changeCrumb}_${guardId.stage}, ${state.name}, ${state.status}, ${state.attempt})'; // Marking needs to happen while in a transaction to ensure `remaining` is // updated correctly. For that to happen correctly; we need to perform a @@ -556,9 +556,9 @@ final class UnifiedCheckRun { ); // Check if the build is present in the guard before trying to load it. - if (presubmitGuard.jobs[state.jobName] == null) { + if (presubmitGuard.jobs[state.name] == null) { log.info( - '$logCrumb: ${state.jobName} with attemptNumber ${state.attemptNumber} not present for $transaction / ${presubmitGuardDocument.fields}', + '$logCrumb: ${state.name} with attempt ${state.attempt} not present for $transaction / ${presubmitGuardDocument.fields}', ); await firestoreService.rollback(transaction); return PresubmitGuardConclusion( @@ -570,7 +570,7 @@ final class UnifiedCheckRun { ), checkRunGuard: presubmitGuard.checkRunJson, summary: - 'Check run "${state.jobName}" not present in ${guardId.stage} CI stage', + 'Check run "${state.name}" not present in ${guardId.stage} CI stage', details: 'Change $changeCrumb', ); } @@ -578,8 +578,8 @@ final class UnifiedCheckRun { final checkDocName = PresubmitJob.documentNameFor( slug: guardId.slug, checkRunId: guardId.checkRunId, - jobName: state.jobName, - attemptNumber: state.attemptNumber, + jobName: state.name, + attemptNumber: state.attempt, ); final presubmitJobDocument = await firestoreService.getDocument( checkDocName, @@ -590,7 +590,7 @@ final class UnifiedCheckRun { remaining = presubmitGuard.remainingJobs; failed = presubmitGuard.failedJobs; final jobs = presubmitGuard.jobs; - var status = jobs[state.jobName]!; + var status = jobs[state.name]!; // If job is waiting for backfill, that means its initiated by github // or re-run. So no processing needed, we should only update appropriate @@ -649,7 +649,7 @@ final class UnifiedCheckRun { valid = true; } } - jobs[state.jobName] = status; + jobs[state.name] = status; presubmitGuard.jobs = jobs; presubmitJob.status = status; } on DetailedApiRequestError catch (e, stack) { @@ -701,14 +701,14 @@ $stack checkRunGuard: presubmitGuard.checkRunJson, summary: valid ? 'Successfully updated presubmit guard status' - : 'Not a valid state transition for ${state.jobName}', + : 'Not a valid state transition for ${state.name}', details: valid ? ''' For CI stage ${guardId.stage}: Pending: $remaining Failed: $failed ''' - : 'Attempted to set the state of job ${state.jobName} ' + : 'Attempted to set the state of job ${state.name} ' 'to "${state.status.name}".', ); } catch (e) { diff --git a/app_dart/lib/src/service/scheduler.dart b/app_dart/lib/src/service/scheduler.dart index 2f89c149ac..b73ceeeb70 100644 --- a/app_dart/lib/src/service/scheduler.dart +++ b/app_dart/lib/src/service/scheduler.dart @@ -20,14 +20,12 @@ import '../model/ci_yaml/ci_yaml.dart'; import '../model/ci_yaml/target.dart'; import '../model/commit_ref.dart'; import '../model/common/checks_extension.dart'; -import '../model/common/presubmit_completed_check.dart'; -import '../model/common/presubmit_guard_conclusion.dart'; import '../model/common/presubmit_job_state.dart'; +import '../model/common/presubmit_guard_conclusion.dart'; import '../model/firestore/base.dart'; import '../model/firestore/ci_staging.dart'; import '../model/firestore/commit.dart' as fs; import '../model/firestore/pr_check_runs.dart'; -import '../model/firestore/presubmit_guard.dart'; import '../model/firestore/task.dart' as fs; import '../model/github/checks.dart' as cocoon_checks; import '../model/github/checks.dart' show MergeGroup; @@ -1121,48 +1119,43 @@ detailsUrl: $detailsUrl /// /// Handles both fusion engine build and test stages, and both pull requests /// and merge groups. - Future processCheckRunStatusChange(PresubmitCompletedJob check) async { - if (kCheckRunsToIgnore.contains(check.name)) { + Future processJobStatusUpdate(PresubmitJobState job) async { + if (kCheckRunsToIgnore.contains(job.name)) { return; } - final flow = check.isUnifiedCheckRun ? 'unified' : 'github'; - final requestor = check.isMergeGroup ? 'merge group' : 'pull request'; + final flow = job.isUnifiedCheckRun ? 'unified' : 'github'; + final requestor = job.isMergeGroup ? 'merge group' : 'pull request'; final logCrumb = - 'checkCompleted(${check.name}, $flow, $requestor, ${check.slug}, ${check.sha}, ${check.status})'; + 'checkCompleted(${job.name}, $flow, $requestor, ${job.slug}, ${job.sha}, ${job.status})'; - final isFusion = check.slug == Config.flutterSlug; - if (!isFusion && !check.isUnifiedCheckRun) { + final isFusion = job.slug == Config.flutterSlug; + if (!isFusion && !job.isUnifiedCheckRun) { return; } - if (check.isUnifiedCheckRun) { - await _processUnifiedCheckRunPresubmit(logCrumb, check); + if (job.isUnifiedCheckRun) { + await _processUnifiedCheckRunFlowJobStatusUpdate(logCrumb, job); } else { - if (!check.status.isComplete) { + if (!job.status.isComplete) { return; } - await _processLegacyPresubmitAndMergeGroup(logCrumb, check); + await _processRegularFlowCheckStatusUpdate(logCrumb, job); } } - Future _processUnifiedCheckRunPresubmit( + Future _processUnifiedCheckRunFlowJobStatusUpdate( String logCrumb, - PresubmitCompletedJob check, + PresubmitJobState job, ) async { - final stage = check.stage!; + final stage = job.stage!; final conclusion = await _markUnifiedCheckRunConclusion( - guardId: check.guardId, - state: check.state, + state: job, ); if (!conclusion.isOk) { return; } - if (conclusion.result == PresubmitGuardConclusionResult.internalError) { - return; - } - if (conclusion.isFailed) { // For unified check runs, we fail the guard only for the first failed job. // Subsequent failures are ignored, but we still log them. @@ -1175,19 +1168,20 @@ detailsUrl: $detailsUrl ); return; } + final guard = checkRunFromString(conclusion.checkRunGuard!); final detailsUrl = - 'https://flutter-dashboard.appspot.com/#/presubmit?repo=${check.slug.name}&sha=${check.sha}'; + 'https://flutter-dashboard.appspot.com/#/presubmit?repo=${job.slug.name}&sha=${job.sha}'; await _requireActionForGuard( - slug: check.slug, + slug: job.slug, lock: guard, - headSha: check.sha, + headSha: job.sha, summary: _githubChecksService.getGithubSummaryWithHeader(''' **[Failed Checks Details]($detailsUrl)** ''', kDashboardChecksDescription), details: - 'For ${check.stage} CI stage ${conclusion.currentState.failed} checks failed', + 'For ${job.stage} CI stage ${conclusion.currentState.failed} checks failed', detailsUrl: detailsUrl, ); return; @@ -1201,12 +1195,12 @@ detailsUrl: $detailsUrl return; } - await _processSuccessedCheck(logCrumb, check, stage, conclusion); + await _processSuccessedCheck(logCrumb, job, stage, conclusion); } - Future _processLegacyPresubmitAndMergeGroup( + Future _processRegularFlowCheckStatusUpdate( String logCrumb, - PresubmitCompletedJob check, + PresubmitJobState check, ) async { var stage = CiStage.fusionEngineBuild; var conclusion = await _recordCurrentCiStage( @@ -1229,12 +1223,8 @@ detailsUrl: $detailsUrl } if (!conclusion.isOk) { - return; - } - - if (conclusion.result == - PresubmitGuardConclusionResult.internalError) { - if (check.isMergeGroup) { + if (conclusion.result == PresubmitGuardConclusionResult.internalError && + check.isMergeGroup) { await _completeArtifacts(check.sha, false); final guard = checkRunFromString(conclusion.checkRunGuard!); await failGuardForMergeGroup( @@ -1275,7 +1265,7 @@ detailsUrl: $detailsUrl Future _processSuccessedCheck( String logCrumb, - PresubmitCompletedJob check, + PresubmitJobState check, CiStage stage, PresubmitGuardConclusion conclusion, ) async { @@ -1602,11 +1592,11 @@ $stacktrace } Future _markUnifiedCheckRunConclusion({ - required PresubmitGuardId guardId, required PresubmitJobState state, }) async { + final guardId = state.guardId; final logCrumb = - 'checkCompleted(${state.jobName}, ${state.buildNumber}, ${guardId.stage}, ${guardId.slug}, ${state.status})'; + 'checkCompleted(${state.name}, ${state.buildNumber}, ${guardId.stage}, ${guardId.slug}, ${state.status})'; log.info('$logCrumb: ${guardId.documentId}'); // We're doing a transactional update, which could fail if multiple tasks @@ -1644,8 +1634,8 @@ $stacktrace switch (checkRunEvent.action) { case 'completed': if (!_config.flags.closeMqGuardAfterPresubmit) { - await processCheckRunStatusChange( - PresubmitCompletedJob.fromCheckRun( + await processJobStatusUpdate( + PresubmitJobState.fromCheckRun( checkRunEvent.checkRun!, checkRunEvent.repository!.slug(), ), diff --git a/app_dart/test/model/common/presubmit_check_state_test.dart b/app_dart/test/model/common/presubmit_check_state_test.dart deleted file mode 100644 index ced2acb665..0000000000 --- a/app_dart/test/model/common/presubmit_check_state_test.dart +++ /dev/null @@ -1,34 +0,0 @@ -// Copyright 2025 The Flutter Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -import 'package:buildbucket/buildbucket_pb.dart' as bbv2; -import 'package:cocoon_common/task_status.dart'; -import 'package:cocoon_server_test/test_logging.dart'; -import 'package:cocoon_service/src/model/common/presubmit_job_state.dart'; -import 'package:fixnum/fixnum.dart'; -import 'package:test/test.dart'; - -void main() { - useTestLoggerPerTest(); - group('PresubmitJobState', () { - test('BuildToPresubmitJobState extension maps build number', () { - final build = bbv2.Build( - id: Int64.MAX_VALUE, - builder: bbv2.BuilderID(builder: 'linux_test'), - status: bbv2.Status.SUCCESS, - number: 12345, - startTime: bbv2.Timestamp(seconds: Int64(1000)), - endTime: bbv2.Timestamp(seconds: Int64(2000)), - summaryMarkdown: 'Summary', - tags: [bbv2.StringPair(key: 'github_checkrun', value: '123')], - ); - - final state = build.toPresubmitJobState(); - expect(state.jobName, 'linux_test'); - expect(state.status, TaskStatus.succeeded); - expect(state.buildNumber, 12345); - expect(state.buildId, Int64.MAX_VALUE); - }); - }); -} diff --git a/app_dart/test/model/common/presubmit_completed_check_test.dart b/app_dart/test/model/common/presubmit_completed_check_test.dart index 79f2ca50d0..8a93327056 100644 --- a/app_dart/test/model/common/presubmit_completed_check_test.dart +++ b/app_dart/test/model/common/presubmit_completed_check_test.dart @@ -6,7 +6,7 @@ import 'package:buildbucket/buildbucket_pb.dart'; import 'package:cocoon_common/task_status.dart'; import 'package:cocoon_server_test/test_logging.dart'; import 'package:cocoon_service/src/model/commit_ref.dart'; -import 'package:cocoon_service/src/model/common/presubmit_completed_check.dart'; +import 'package:cocoon_service/src/model/common/presubmit_job_state.dart'; import 'package:cocoon_service/src/model/firestore/base.dart'; import 'package:cocoon_service/src/model/github/checks.dart' as cocoon_checks; import 'package:cocoon_service/src/service/config.dart'; @@ -30,7 +30,7 @@ void main() { conclusion: 'success', ); - final check = PresubmitCompletedJob.fromCheckRun(checkRun, slug); + final check = PresubmitJobState.fromCheckRun(checkRun, slug); expect(check.name, 'test_check'); expect(check.sha, sha); @@ -63,7 +63,7 @@ void main() { checkSuiteId: 456, ); - final check = PresubmitCompletedJob.fromBuild(build, userData); + final check = PresubmitJobState.fromBuild(build, userData); expect(check.name, 'test_builder'); expect(check.sha, sha); @@ -99,7 +99,7 @@ void main() { checkSuiteId: 456, ); - final check = PresubmitCompletedJob.fromBuild(build, userData); + final check = PresubmitJobState.fromBuild(build, userData); expect(check.name, 'test_builder'); expect(check.sha, sha); diff --git a/app_dart/test/request_handlers/presubmit_luci_subscription_neutral_test.dart b/app_dart/test/request_handlers/presubmit_luci_subscription_neutral_test.dart index 3d531fa0ea..0e6f128075 100644 --- a/app_dart/test/request_handlers/presubmit_luci_subscription_neutral_test.dart +++ b/app_dart/test/request_handlers/presubmit_luci_subscription_neutral_test.dart @@ -9,7 +9,7 @@ import 'package:cocoon_server_test/mocks.dart'; import 'package:cocoon_server_test/test_logging.dart'; import 'package:cocoon_service/cocoon_service.dart'; import 'package:cocoon_service/src/model/commit_ref.dart'; -import 'package:cocoon_service/src/model/common/presubmit_completed_check.dart'; +import 'package:cocoon_service/src/model/common/presubmit_job_state.dart'; import 'package:cocoon_service/src/service/luci_build_service/user_data.dart'; import 'package:fixnum/fixnum.dart'; import 'package:github/github.dart' as github; @@ -110,7 +110,7 @@ void main() { ).thenAnswer((_) async => true); when( - mockScheduler.processCheckRunStatusChange(any), + mockScheduler.processJobStatusUpdate(any), ).thenAnswer((_) async {}); tester.message = createPushMessage( @@ -141,12 +141,12 @@ void main() { // Verify that processCheckRunCompleted was called with TaskStatus.neutral final captured = verify( - mockScheduler.processCheckRunStatusChange(captureAny), + mockScheduler.processJobStatusUpdate(captureAny), ).captured; expect(captured, hasLength(1)); expect( captured[0], - isA().having( + isA().having( (e) => e.status, 'status', TaskStatus.neutral, @@ -182,7 +182,7 @@ void main() { ).thenAnswer((_) async => true); when( - mockScheduler.processCheckRunStatusChange(any), + mockScheduler.processJobStatusUpdate(any), ).thenAnswer((_) async {}); tester.message = createPushMessage( @@ -208,12 +208,12 @@ void main() { // Verify that processCheckRunCompleted was called with TaskStatus.failed final captured = verify( - mockScheduler.processCheckRunStatusChange(captureAny), + mockScheduler.processJobStatusUpdate(captureAny), ).captured; expect(captured, hasLength(1)); expect( captured[0], - isA().having( + isA().having( (e) => e.status, 'status', TaskStatus.failed, 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 c888965099..9038acc1a5 100644 --- a/app_dart/test/request_handlers/presubmit_luci_subscription_test.dart +++ b/app_dart/test/request_handlers/presubmit_luci_subscription_test.dart @@ -9,7 +9,7 @@ import 'package:cocoon_server_test/mocks.dart'; import 'package:cocoon_server_test/test_logging.dart'; import 'package:cocoon_service/cocoon_service.dart'; import 'package:cocoon_service/src/model/commit_ref.dart'; -import 'package:cocoon_service/src/model/common/presubmit_completed_check.dart'; +import 'package:cocoon_service/src/model/common/presubmit_job_state.dart'; import 'package:cocoon_service/src/request_handling/exceptions.dart'; import 'package:cocoon_service/src/service/luci_build_service/build_tags.dart'; import 'package:cocoon_service/src/service/luci_build_service/user_data.dart'; @@ -89,7 +89,7 @@ void main() { mockGithubChecksService.conclusionForResult(any), ).thenAnswer((_) => github.CheckRunConclusion.empty); when( - mockScheduler.processCheckRunStatusChange(any), + mockScheduler.processJobStatusUpdate(any), ).thenAnswer((_) async {}); tester.message = createPushMessage( @@ -117,7 +117,7 @@ void main() { ), ).called(1); - verify(mockScheduler.processCheckRunStatusChange(any)).called(1); + verify(mockScheduler.processJobStatusUpdate(any)).called(1); }); test('Requests when task failed but no need to reschedule', () async { @@ -134,7 +134,7 @@ void main() { mockGithubChecksService.conclusionForResult(any), ).thenAnswer((_) => github.CheckRunConclusion.empty); when( - mockScheduler.processCheckRunStatusChange(any), + mockScheduler.processJobStatusUpdate(any), ).thenAnswer((_) async {}); final userData = PresubmitUserData( @@ -176,7 +176,7 @@ void main() { slug: anyNamed('slug'), ), ).called(1); - verify(mockScheduler.processCheckRunStatusChange(any)).called(1); + verify(mockScheduler.processJobStatusUpdate(any)).called(1); }); test('Requests when task failed but need to reschedule', () async { @@ -215,7 +215,7 @@ void main() { rescheduled: true, ), ).called(1); - verifyNever(mockScheduler.processCheckRunStatusChange(any)); + verifyNever(mockScheduler.processJobStatusUpdate(any)); }); test('Build rescheduled when in merge queue', () async { @@ -297,7 +297,7 @@ void main() { rescheduled: true, ), ).called(1); - verifyNever(mockScheduler.processCheckRunStatusChange(any)); + verifyNever(mockScheduler.processJobStatusUpdate(any)); }); test('Build not rescheduled if not found in ciYaml list.', () async { @@ -315,7 +315,7 @@ void main() { mockGithubChecksService.conclusionForResult(any), ).thenAnswer((_) => github.CheckRunConclusion.empty); when( - mockScheduler.processCheckRunStatusChange(any), + mockScheduler.processJobStatusUpdate(any), ).thenAnswer((_) async {}); final userData = PresubmitUserData( @@ -360,7 +360,7 @@ void main() { ), ).called(1); - verify(mockScheduler.processCheckRunStatusChange(any)).called(1); + verify(mockScheduler.processJobStatusUpdate(any)).called(1); }); test('Build not rescheduled if ci.yaml fails validation.', () async { @@ -378,7 +378,7 @@ void main() { mockGithubChecksService.conclusionForResult(any), ).thenAnswer((_) => github.CheckRunConclusion.empty); when( - mockScheduler.processCheckRunStatusChange(any), + mockScheduler.processJobStatusUpdate(any), ).thenAnswer((_) async {}); final userData = PresubmitUserData( @@ -421,7 +421,7 @@ void main() { rescheduled: false, ), ).called(1); - verify(mockScheduler.processCheckRunStatusChange(any)).called(1); + verify(mockScheduler.processJobStatusUpdate(any)).called(1); }); test('Pubsub rejected if branch is not enabled.', () async { @@ -528,7 +528,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.processCheckRunStatusChange(any)); + verifyNever(mockScheduler.processJobStatusUpdate(any)); }); test('Close the MQ guard once presubmit compleated', () async { @@ -561,18 +561,18 @@ void main() { mockGithubChecksService.conclusionForResult(bbv2.Status.SUCCESS), ).thenAnswer((_) => github.CheckRunConclusion.success); when( - mockScheduler.processCheckRunStatusChange(any), + mockScheduler.processJobStatusUpdate(any), ).thenAnswer((_) async {}); await tester.post(handler); final captured = verify( - mockScheduler.processCheckRunStatusChange(captureAny), + mockScheduler.processJobStatusUpdate(captureAny), ).captured; expect(captured, hasLength(1)); expect( captured[0], - isA() + isA() .having((e) => e.name, 'name', 'Linux C') .having((e) => e.sha, 'sha', 'abc') .having((e) => e.checkRunId, 'checkRunId', 1) @@ -623,7 +623,7 @@ void main() { ).thenAnswer((_) async => true); when( - mockScheduler.processCheckRunStatusChange(any), + mockScheduler.processJobStatusUpdate(any), ).thenAnswer((_) async {}); tester.message = createPushMessage( 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 c9799674c3..6211b3a202 100644 --- a/app_dart/test/service/firestore/unified_check_run_test.dart +++ b/app_dart/test/service/firestore/unified_check_run_test.dart @@ -5,8 +5,8 @@ import 'package:cocoon_common/task_status.dart'; import 'package:cocoon_integration_test/testing.dart'; import 'package:cocoon_server_test/test_logging.dart'; -import 'package:cocoon_service/src/model/common/presubmit_guard_conclusion.dart'; import 'package:cocoon_service/src/model/common/presubmit_job_state.dart'; +import 'package:cocoon_service/src/model/common/presubmit_guard_conclusion.dart'; import 'package:cocoon_service/src/model/firestore/base.dart'; import 'package:cocoon_service/src/model/firestore/presubmit_guard.dart'; import 'package:cocoon_service/src/model/firestore/presubmit_job.dart'; @@ -170,14 +170,21 @@ void main() { }); test('updates check status and remaining count on success', () async { - final state = const PresubmitJobState( - jobName: 'linux', + final state = PresubmitJobState( + name: 'linux', status: TaskStatus.succeeded, - attemptNumber: 1, + attempt: 1, startTime: 2000, endTime: 3000, buildNumber: 456, buildId: Int64.MAX_VALUE, + checkRunId: 123, + checkSuiteId: 2, + headBranch: 'master', + isMergeGroup: false, + sha: '', + slug: slug, + isUnifiedCheckRun: false, ); final result = await UnifiedCheckRun.markConclusion( @@ -213,12 +220,21 @@ void main() { final result1 = await UnifiedCheckRun.markConclusion( firestoreService: firestoreService, guardId: guardId, - state: const PresubmitJobState( - jobName: 'linux', + state: PresubmitJobState( + name: 'linux', status: TaskStatus.succeeded, - attemptNumber: 1, + attempt: 1, startTime: 2000, endTime: 3000, + buildId: Int64.MAX_VALUE, + buildNumber: 456, + checkRunId: 123, + checkSuiteId: 2, + headBranch: 'master', + isMergeGroup: false, + sha: '', + slug: slug, + isUnifiedCheckRun: false, ), ); @@ -232,12 +248,21 @@ void main() { final result2 = await UnifiedCheckRun.markConclusion( firestoreService: firestoreService, guardId: guardId, - state: const PresubmitJobState( - jobName: 'mac', + state: PresubmitJobState( + name: 'mac', status: TaskStatus.succeeded, - attemptNumber: 1, + attempt: 1, startTime: 2000, endTime: 3000, + buildId: Int64.MAX_VALUE, + buildNumber: 456, + checkRunId: 123, + checkSuiteId: 2, + headBranch: 'master', + isMergeGroup: false, + sha: '', + slug: slug, + isUnifiedCheckRun: false, ), ); @@ -263,12 +288,21 @@ void main() { ); test('updates check status and failed count on failure', () async { - final state = const PresubmitJobState( - jobName: 'linux', + final state = PresubmitJobState( + name: 'linux', status: TaskStatus.failed, - attemptNumber: 1, + attempt: 1, startTime: 2000, endTime: 3000, + buildId: Int64.MAX_VALUE, + buildNumber: 456, + checkRunId: 123, + checkSuiteId: 2, + headBranch: 'master', + isMergeGroup: false, + sha: '', + slug: slug, + isUnifiedCheckRun: false, ); final result = await UnifiedCheckRun.markConclusion( @@ -287,10 +321,19 @@ void main() { }); test('handles missing check gracefully', () async { - final state = const PresubmitJobState( - jobName: 'windows', // Missing + final state = PresubmitJobState( + name: 'windows', // Missing status: TaskStatus.succeeded, - attemptNumber: 1, + attempt: 1, + buildId: Int64.MAX_VALUE, + buildNumber: 456, + checkRunId: 123, + checkSuiteId: 2, + headBranch: 'master', + isMergeGroup: false, + sha: '', + slug: slug, + isUnifiedCheckRun: false, ); final result = await UnifiedCheckRun.markConclusion( @@ -302,13 +345,20 @@ void main() { expect(result.result, PresubmitGuardConclusionResult.missing); }); test('updates check status and build number on inProgress', () async { - final state = const PresubmitJobState( - jobName: 'linux', + final state = PresubmitJobState( + name: 'linux', status: TaskStatus.inProgress, - attemptNumber: 1, + attempt: 1, startTime: 2000, buildNumber: 456, buildId: Int64.MAX_VALUE, + checkRunId: 123, + checkSuiteId: 2, + headBranch: 'master', + isMergeGroup: false, + sha: '', + slug: slug, + isUnifiedCheckRun: false, ); final result = await UnifiedCheckRun.markConclusion( diff --git a/app_dart/test/service/scheduler_test.dart b/app_dart/test/service/scheduler_test.dart index 4e4646d8a4..01f0cbd8aa 100644 --- a/app_dart/test/service/scheduler_test.dart +++ b/app_dart/test/service/scheduler_test.dart @@ -14,7 +14,7 @@ import 'package:cocoon_service/cocoon_service.dart'; import 'package:cocoon_service/src/model/ci_yaml/ci_yaml.dart'; import 'package:cocoon_service/src/model/ci_yaml/target.dart'; import 'package:cocoon_service/src/model/commit_ref.dart'; -import 'package:cocoon_service/src/model/common/presubmit_completed_check.dart'; +import 'package:cocoon_service/src/model/common/presubmit_job_state.dart'; import 'package:cocoon_service/src/model/firestore/ci_staging.dart'; import 'package:cocoon_service/src/model/firestore/commit.dart' as fs; import 'package:cocoon_service/src/model/firestore/task.dart' as fs; @@ -1652,8 +1652,8 @@ targets: ); for (final ignored in Scheduler.kCheckRunsToIgnore) { - await scheduler.processCheckRunStatusChange( - PresubmitCompletedJob.fromCheckRun( + await scheduler.processJobStatusUpdate( + PresubmitJobState.fromCheckRun( createCocoonCheckRun(name: ignored, sha: 'abc123'), createGithubRepository().slug(), ), @@ -1684,8 +1684,8 @@ targets: firestore.failOnWriteDocument(document); - await scheduler.processCheckRunStatusChange( - PresubmitCompletedJob.fromCheckRun( + await scheduler.processJobStatusUpdate( + PresubmitJobState.fromCheckRun( createCocoonCheckRun(name: 'Bar bar', sha: 'abc123'), createGithubRepository().slug(), ), @@ -1720,8 +1720,8 @@ targets: checkRunGuard: '{}', ); - await scheduler.processCheckRunStatusChange( - PresubmitCompletedJob.fromCheckRun( + await scheduler.processJobStatusUpdate( + PresubmitJobState.fromCheckRun( createCocoonCheckRun(name: 'Bar bar', sha: 'abc123'), createGithubRepository().slug(), ), @@ -1761,8 +1761,8 @@ targets: checkRunGuard: '{}', ); - await scheduler.processCheckRunStatusChange( - PresubmitCompletedJob.fromCheckRun( + await scheduler.processJobStatusUpdate( + PresubmitJobState.fromCheckRun( createCocoonCheckRun( name: 'Bar bar', sha: 'abc123', @@ -1817,8 +1817,8 @@ targets: checkRunGuard: checkRunFor(name: 'GUARD TEST'), ); - await scheduler.processCheckRunStatusChange( - PresubmitCompletedJob.fromCheckRun( + await scheduler.processJobStatusUpdate( + PresubmitJobState.fromCheckRun( createCocoonCheckRun(name: 'Bar bar', sha: 'abc123'), createGithubRepository().slug(), ), @@ -1924,8 +1924,8 @@ targets: checkRunGuard: checkRunFor(name: 'GUARD TEST'), ); - await scheduler.processCheckRunStatusChange( - PresubmitCompletedJob.fromCheckRun( + await scheduler.processJobStatusUpdate( + PresubmitJobState.fromCheckRun( createCocoonCheckRun(name: 'Bar bar', sha: 'testSha'), createGithubRepository().slug(), ), @@ -2065,8 +2065,8 @@ targets: checkRunGuard: checkRunFor(name: 'GUARD TEST'), ); - await scheduler.processCheckRunStatusChange( - PresubmitCompletedJob.fromCheckRun( + await scheduler.processJobStatusUpdate( + PresubmitJobState.fromCheckRun( createCocoonCheckRun( name: 'Bar bar', sha: 'testSha', @@ -2171,8 +2171,8 @@ targets: checkRunGuard: checkRunFor(name: 'GUARD TEST'), ); - await scheduler.processCheckRunStatusChange( - PresubmitCompletedJob.fromCheckRun( + await scheduler.processJobStatusUpdate( + PresubmitJobState.fromCheckRun( createCocoonCheckRun(name: 'Bar bar', sha: 'testSha'), createGithubRepository().slug(), ), @@ -2432,8 +2432,8 @@ targets: checkRunGuard: checkRunFor(name: 'GUARD TEST'), ); - await scheduler.processCheckRunStatusChange( - PresubmitCompletedJob.fromCheckRun( + await scheduler.processJobStatusUpdate( + PresubmitJobState.fromCheckRun( createCocoonCheckRun( name: 'Bar bar', sha: 'testSha', @@ -2512,8 +2512,8 @@ targets: ), ); - await scheduler.processCheckRunStatusChange( - PresubmitCompletedJob.fromCheckRun( + await scheduler.processJobStatusUpdate( + PresubmitJobState.fromCheckRun( createCocoonCheckRun( name: 'Bar bar', sha: 'testSha', @@ -2592,8 +2592,8 @@ targets: ), ); - await scheduler.processCheckRunStatusChange( - PresubmitCompletedJob.fromCheckRun( + await scheduler.processJobStatusUpdate( + PresubmitJobState.fromCheckRun( createCocoonCheckRun( name: 'Bar bar', sha: 'testSha', @@ -2712,8 +2712,8 @@ targets: checkRunGuard: checkRunFor(name: 'GUARD TEST'), ); - await scheduler.processCheckRunStatusChange( - PresubmitCompletedJob.fromCheckRun( + await scheduler.processJobStatusUpdate( + PresubmitJobState.fromCheckRun( createCocoonCheckRun(name: 'Bar bar', sha: 'testSha'), createGithubRepository().slug(), ), @@ -4255,9 +4255,9 @@ targets: ], ); - final check = PresubmitCompletedJob.fromBuild(build, userData); + final check = PresubmitJobState.fromBuild(build, userData); - await scheduler.processCheckRunStatusChange(check); + await scheduler.processJobStatusUpdate(check); // Should schedule tests for the next stage (fusionTests) expect(fakeLuciBuildService.scheduledTryBuilds, isNotEmpty); @@ -4333,9 +4333,9 @@ targets: tags: [bbv2.StringPair(key: 'current_attempt', value: '1')], ); - final check = PresubmitCompletedJob.fromBuild(build, userData); + final check = PresubmitJobState.fromBuild(build, userData); - await scheduler.processCheckRunStatusChange(check); + await scheduler.processJobStatusUpdate(check); verify( mockGithubChecksUtil.updateCheckRun( @@ -4423,9 +4423,9 @@ targets: ], ); - final check = PresubmitCompletedJob.fromBuild(build, userData); + final check = PresubmitJobState.fromBuild(build, userData); - await scheduler.processCheckRunStatusChange(check); + await scheduler.processJobStatusUpdate(check); verify( mockGithubChecksUtil.updateCheckRun( @@ -4515,9 +4515,9 @@ targets: ], ); - final check = PresubmitCompletedJob.fromBuild(build, userData); + final check = PresubmitJobState.fromBuild(build, userData); - await scheduler.processCheckRunStatusChange(check); + await scheduler.processJobStatusUpdate(check); verify( mockGithubChecksUtil.updateCheckRun( @@ -4602,9 +4602,9 @@ targets: ], ); - final check = PresubmitCompletedJob.fromBuild(build, userData); + final check = PresubmitJobState.fromBuild(build, userData); - await scheduler.processCheckRunStatusChange(check); + await scheduler.processJobStatusUpdate(check); 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 059299c7f7..404b8cb542 100644 --- a/packages/cocoon_integration_test/lib/src/utilities/mocks.mocks.dart +++ b/packages/cocoon_integration_test/lib/src/utilities/mocks.mocks.dart @@ -16,7 +16,7 @@ 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; import 'package:cocoon_service/src/model/commit_ref.dart' as _i31; -import 'package:cocoon_service/src/model/common/presubmit_completed_check.dart' +import 'package:cocoon_service/src/model/common/presubmit_job_state.dart' as _i38; import 'package:cocoon_service/src/model/github/checks.dart' as _i30; import 'package:cocoon_service/src/model/github/workflow_job.dart' as _i36; @@ -5835,11 +5835,10 @@ class MockScheduler extends _i1.Mock implements _i17.Scheduler { as _i13.Future>); @override - _i13.Future processCheckRunStatusChange( - _i38.PresubmitCompletedJob? check, + _i13.Future processJobStatusUpdate(_i38.PresubmitJobState? job, ) => (super.noSuchMethod( - Invocation.method(#processCheckRunCompleted, [check]), + Invocation.method(#processCheckRunCompleted, [job]), returnValue: _i13.Future.value(), returnValueForMissingStub: _i13.Future.value(), ) From 2ded19c93fcf932ae1f3ce5d9a5dc256b528f8d1 Mon Sep 17 00:00:00 2001 From: Dmitry Grand Date: Wed, 1 Jul 2026 15:27:00 -0700 Subject: [PATCH 08/13] spell check --- app_dart/lib/src/service/scheduler.dart | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app_dart/lib/src/service/scheduler.dart b/app_dart/lib/src/service/scheduler.dart index b73ceeeb70..ec7724a5f2 100644 --- a/app_dart/lib/src/service/scheduler.dart +++ b/app_dart/lib/src/service/scheduler.dart @@ -1195,7 +1195,7 @@ detailsUrl: $detailsUrl return; } - await _processSuccessedCheck(logCrumb, job, stage, conclusion); + await _processSucceededCheck(logCrumb, job, stage, conclusion); } Future _processRegularFlowCheckStatusUpdate( @@ -1260,10 +1260,10 @@ detailsUrl: $detailsUrl return; } - await _processSuccessedCheck(logCrumb, check, stage, conclusion); + await _processSucceededCheck(logCrumb, check, stage, conclusion); } - Future _processSuccessedCheck( + Future _processSucceededCheck( String logCrumb, PresubmitJobState check, CiStage stage, From b85c6ba0f0bfbd64114e6998cad73241006527f6 Mon Sep 17 00:00:00 2001 From: Dmitry Grand Date: Wed, 1 Jul 2026 15:27:32 -0700 Subject: [PATCH 09/13] format --- app_dart/lib/src/service/scheduler.dart | 4 +--- ...submit_luci_subscription_neutral_test.dart | 8 ++----- .../presubmit_luci_subscription_test.dart | 24 +++++-------------- .../firestore/unified_check_run_test.dart | 2 +- .../lib/src/utilities/mocks.mocks.dart | 3 +-- 5 files changed, 11 insertions(+), 30 deletions(-) diff --git a/app_dart/lib/src/service/scheduler.dart b/app_dart/lib/src/service/scheduler.dart index ec7724a5f2..8591bd9c57 100644 --- a/app_dart/lib/src/service/scheduler.dart +++ b/app_dart/lib/src/service/scheduler.dart @@ -1148,9 +1148,7 @@ detailsUrl: $detailsUrl PresubmitJobState job, ) async { final stage = job.stage!; - final conclusion = await _markUnifiedCheckRunConclusion( - state: job, - ); + final conclusion = await _markUnifiedCheckRunConclusion(state: job); if (!conclusion.isOk) { return; diff --git a/app_dart/test/request_handlers/presubmit_luci_subscription_neutral_test.dart b/app_dart/test/request_handlers/presubmit_luci_subscription_neutral_test.dart index 0e6f128075..d880a9d22c 100644 --- a/app_dart/test/request_handlers/presubmit_luci_subscription_neutral_test.dart +++ b/app_dart/test/request_handlers/presubmit_luci_subscription_neutral_test.dart @@ -109,9 +109,7 @@ void main() { ), ).thenAnswer((_) async => true); - when( - mockScheduler.processJobStatusUpdate(any), - ).thenAnswer((_) async {}); + when(mockScheduler.processJobStatusUpdate(any)).thenAnswer((_) async {}); tester.message = createPushMessage( Int64(1), @@ -181,9 +179,7 @@ void main() { ), ).thenAnswer((_) async => true); - when( - mockScheduler.processJobStatusUpdate(any), - ).thenAnswer((_) async {}); + when(mockScheduler.processJobStatusUpdate(any)).thenAnswer((_) async {}); tester.message = createPushMessage( Int64(1), 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 9038acc1a5..8041ded437 100644 --- a/app_dart/test/request_handlers/presubmit_luci_subscription_test.dart +++ b/app_dart/test/request_handlers/presubmit_luci_subscription_test.dart @@ -88,9 +88,7 @@ void main() { when( mockGithubChecksService.conclusionForResult(any), ).thenAnswer((_) => github.CheckRunConclusion.empty); - when( - mockScheduler.processJobStatusUpdate(any), - ).thenAnswer((_) async {}); + when(mockScheduler.processJobStatusUpdate(any)).thenAnswer((_) async {}); tester.message = createPushMessage( Int64(1), @@ -133,9 +131,7 @@ void main() { when( mockGithubChecksService.conclusionForResult(any), ).thenAnswer((_) => github.CheckRunConclusion.empty); - when( - mockScheduler.processJobStatusUpdate(any), - ).thenAnswer((_) async {}); + when(mockScheduler.processJobStatusUpdate(any)).thenAnswer((_) async {}); final userData = PresubmitUserData( commit: CommitRef( @@ -314,9 +310,7 @@ void main() { when( mockGithubChecksService.conclusionForResult(any), ).thenAnswer((_) => github.CheckRunConclusion.empty); - when( - mockScheduler.processJobStatusUpdate(any), - ).thenAnswer((_) async {}); + when(mockScheduler.processJobStatusUpdate(any)).thenAnswer((_) async {}); final userData = PresubmitUserData( commit: CommitRef( @@ -377,9 +371,7 @@ void main() { when( mockGithubChecksService.conclusionForResult(any), ).thenAnswer((_) => github.CheckRunConclusion.empty); - when( - mockScheduler.processJobStatusUpdate(any), - ).thenAnswer((_) async {}); + when(mockScheduler.processJobStatusUpdate(any)).thenAnswer((_) async {}); final userData = PresubmitUserData( checkRunId: 1, @@ -560,9 +552,7 @@ void main() { when( mockGithubChecksService.conclusionForResult(bbv2.Status.SUCCESS), ).thenAnswer((_) => github.CheckRunConclusion.success); - when( - mockScheduler.processJobStatusUpdate(any), - ).thenAnswer((_) async {}); + when(mockScheduler.processJobStatusUpdate(any)).thenAnswer((_) async {}); await tester.post(handler); @@ -622,9 +612,7 @@ void main() { ), ).thenAnswer((_) async => true); - when( - mockScheduler.processJobStatusUpdate(any), - ).thenAnswer((_) async {}); + when(mockScheduler.processJobStatusUpdate(any)).thenAnswer((_) async {}); tester.message = createPushMessage( Int64(1), 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 6211b3a202..d530450100 100644 --- a/app_dart/test/service/firestore/unified_check_run_test.dart +++ b/app_dart/test/service/firestore/unified_check_run_test.dart @@ -184,7 +184,7 @@ void main() { isMergeGroup: false, sha: '', slug: slug, - isUnifiedCheckRun: false, + isUnifiedCheckRun: false, ); final result = await UnifiedCheckRun.markConclusion( 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 404b8cb542..095e7f9b83 100644 --- a/packages/cocoon_integration_test/lib/src/utilities/mocks.mocks.dart +++ b/packages/cocoon_integration_test/lib/src/utilities/mocks.mocks.dart @@ -5835,8 +5835,7 @@ class MockScheduler extends _i1.Mock implements _i17.Scheduler { as _i13.Future>); @override - _i13.Future processJobStatusUpdate(_i38.PresubmitJobState? job, - ) => + _i13.Future processJobStatusUpdate(_i38.PresubmitJobState? job) => (super.noSuchMethod( Invocation.method(#processCheckRunCompleted, [job]), returnValue: _i13.Future.value(), From 51e457b8d0a99373000bee351840947a13702781 Mon Sep 17 00:00:00 2001 From: Dmitry Grand Date: Wed, 1 Jul 2026 16:10:31 -0700 Subject: [PATCH 10/13] refactoring --- app_dart/lib/src/service/scheduler.dart | 163 +++++++++++++++--------- 1 file changed, 105 insertions(+), 58 deletions(-) diff --git a/app_dart/lib/src/service/scheduler.dart b/app_dart/lib/src/service/scheduler.dart index 8591bd9c57..5c5c9019ea 100644 --- a/app_dart/lib/src/service/scheduler.dart +++ b/app_dart/lib/src/service/scheduler.dart @@ -1136,10 +1136,7 @@ detailsUrl: $detailsUrl if (job.isUnifiedCheckRun) { await _processUnifiedCheckRunFlowJobStatusUpdate(logCrumb, job); } else { - if (!job.status.isComplete) { - return; - } - await _processRegularFlowCheckStatusUpdate(logCrumb, job); + await _processGitHubFlowCheckStatusUpdate(logCrumb, job); } } @@ -1193,13 +1190,34 @@ detailsUrl: $detailsUrl return; } - await _processSucceededCheck(logCrumb, job, stage, conclusion); + switch (stage) { + case CiStage.fusionEngineBuild: + await _closeSuccessfulEngineBuildStage( + checkRun: job.checkRun, + mergeQueueGuard: conclusion.checkRunGuard!, + slug: job.slug, + sha: job.sha, + logCrumb: logCrumb, + ); + case CiStage.fusionTests: + case CiStage.genericTests: + await _closeSuccessfulTestStage( + mergeQueueGuard: conclusion.checkRunGuard!, + slug: job.slug, + sha: job.sha, + logCrumb: logCrumb, + ); + } } - Future _processRegularFlowCheckStatusUpdate( + Future _processGitHubFlowCheckStatusUpdate( String logCrumb, PresubmitJobState check, ) async { + if (!check.status.isComplete) { + return; + } + var stage = CiStage.fusionEngineBuild; var conclusion = await _recordCurrentCiStage( slug: check.slug, @@ -1220,9 +1238,72 @@ detailsUrl: $detailsUrl ); } + if (check.isMergeGroup) { + await _processMergeGroupCheckStatusUpdate( + logCrumb, + check, + stage, + conclusion, + ); + } else { + await _processRegularCheckStatusUpdate( + logCrumb, + check, + stage, + conclusion, + ); + } + } + + Future _processRegularCheckStatusUpdate( + String logCrumb, + PresubmitJobState check, + CiStage stage, + PresubmitGuardConclusion conclusion, + ) async { + if (!conclusion.isOk) { + return; + } + + if (conclusion.isPending || conclusion.isFailed) { + log.info( + '$logCrumb: not progressing, ' + 'remaining checks count: ${conclusion.currentState.remaining}, ' + 'failed checks count: ${conclusion.currentState.failed}', + ); + return; + } + + switch (stage) { + case CiStage.fusionEngineBuild: + await _closeSuccessfulEngineBuildStage( + checkRun: check.checkRun, + mergeQueueGuard: conclusion.checkRunGuard!, + slug: check.slug, + sha: check.sha, + logCrumb: logCrumb, + ); + case CiStage.fusionTests: + await _closeSuccessfulTestStage( + mergeQueueGuard: conclusion.checkRunGuard!, + slug: check.slug, + sha: check.sha, + logCrumb: logCrumb, + ); + case CiStage.genericTests: + log.warn('$logCrumb: generic tests have no merge queue guard.'); + break; + } + } + + Future _processMergeGroupCheckStatusUpdate( + String logCrumb, + PresubmitJobState check, + CiStage stage, + PresubmitGuardConclusion conclusion, + ) async { if (!conclusion.isOk) { - if (conclusion.result == PresubmitGuardConclusionResult.internalError && - check.isMergeGroup) { + if (conclusion.result == PresubmitGuardConclusionResult.internalError) { await _completeArtifacts(check.sha, false); final guard = checkRunFromString(conclusion.checkRunGuard!); await failGuardForMergeGroup( @@ -1244,67 +1325,33 @@ detailsUrl: $detailsUrl } if (conclusion.isFailed) { - if (check.isMergeGroup) { - await _completeArtifacts(check.sha, false); - final guard = checkRunFromString(conclusion.checkRunGuard!); - await failGuardForMergeGroup( - slug: check.slug, - lock: guard, - headSha: check.sha, - summary: conclusion.summary, - details: conclusion.details, - ); - } + await _completeArtifacts(check.sha, false); + final guard = checkRunFromString(conclusion.checkRunGuard!); + await failGuardForMergeGroup( + slug: check.slug, + lock: guard, + headSha: check.sha, + summary: conclusion.summary, + details: conclusion.details, + ); return; } - await _processSucceededCheck(logCrumb, check, stage, conclusion); - } - - Future _processSucceededCheck( - String logCrumb, - PresubmitJobState check, - CiStage stage, - PresubmitGuardConclusion conclusion, - ) async { switch (stage) { case CiStage.fusionEngineBuild: - if (check.isMergeGroup) { - await _completeArtifacts(check.sha, true); - await _closeMergeQueue( - mergeQueueGuard: conclusion.checkRunGuard!, - slug: check.slug, - sha: check.sha, - stage: CiStage.fusionEngineBuild, - logCrumb: logCrumb, - ); - } else { - await _closeSuccessfulEngineBuildStage( - checkRun: check.checkRun, - mergeQueueGuard: conclusion.checkRunGuard!, - slug: check.slug, - sha: check.sha, - logCrumb: logCrumb, - ); - } - case CiStage.fusionTests: - await _closeSuccessfulTestStage( + await _completeArtifacts(check.sha, true); + await _closeMergeQueue( mergeQueueGuard: conclusion.checkRunGuard!, slug: check.slug, sha: check.sha, + stage: CiStage.fusionEngineBuild, logCrumb: logCrumb, ); + case CiStage.fusionTests: + log.warn('$logCrumb: fusion tests have no merge group.'); + break; case CiStage.genericTests: - if (check.isUnifiedCheckRun) { - await _closeSuccessfulTestStage( - mergeQueueGuard: conclusion.checkRunGuard!, - slug: check.slug, - sha: check.sha, - logCrumb: logCrumb, - ); - } else { - log.warn('$logCrumb: generic tests have no merge queue guard.'); - } + log.warn('$logCrumb: generic tests have no merge group.'); break; } } From d3a689c63341cefce1515bf5ae63c34178532e8c Mon Sep 17 00:00:00 2001 From: Dmitry Grand Date: Wed, 1 Jul 2026 16:14:37 -0700 Subject: [PATCH 11/13] fix analizer --- app_dart/lib/src/service/firestore/unified_check_run.dart | 2 +- app_dart/lib/src/service/scheduler.dart | 2 +- app_dart/test/service/firestore/unified_check_run_test.dart | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) 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 2b4e241d17..9f504bdb09 100644 --- a/app_dart/lib/src/service/firestore/unified_check_run.dart +++ b/app_dart/lib/src/service/firestore/unified_check_run.dart @@ -13,8 +13,8 @@ import 'package:googleapis/firestore/v1.dart' hide Status; import 'package:meta/meta.dart'; import '../../model/common/failed_presubmit_jobs.dart'; -import '../../model/common/presubmit_job_state.dart'; import '../../model/common/presubmit_guard_conclusion.dart'; +import '../../model/common/presubmit_job_state.dart'; import '../../model/firestore/base.dart'; import '../../model/firestore/ci_staging.dart'; import '../../model/firestore/presubmit_guard.dart'; diff --git a/app_dart/lib/src/service/scheduler.dart b/app_dart/lib/src/service/scheduler.dart index 5c5c9019ea..352f253bd6 100644 --- a/app_dart/lib/src/service/scheduler.dart +++ b/app_dart/lib/src/service/scheduler.dart @@ -20,8 +20,8 @@ import '../model/ci_yaml/ci_yaml.dart'; import '../model/ci_yaml/target.dart'; import '../model/commit_ref.dart'; import '../model/common/checks_extension.dart'; -import '../model/common/presubmit_job_state.dart'; import '../model/common/presubmit_guard_conclusion.dart'; +import '../model/common/presubmit_job_state.dart'; import '../model/firestore/base.dart'; import '../model/firestore/ci_staging.dart'; import '../model/firestore/commit.dart' as fs; 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 d530450100..5e64cef324 100644 --- a/app_dart/test/service/firestore/unified_check_run_test.dart +++ b/app_dart/test/service/firestore/unified_check_run_test.dart @@ -5,8 +5,8 @@ import 'package:cocoon_common/task_status.dart'; import 'package:cocoon_integration_test/testing.dart'; import 'package:cocoon_server_test/test_logging.dart'; -import 'package:cocoon_service/src/model/common/presubmit_job_state.dart'; import 'package:cocoon_service/src/model/common/presubmit_guard_conclusion.dart'; +import 'package:cocoon_service/src/model/common/presubmit_job_state.dart'; import 'package:cocoon_service/src/model/firestore/base.dart'; import 'package:cocoon_service/src/model/firestore/presubmit_guard.dart'; import 'package:cocoon_service/src/model/firestore/presubmit_job.dart'; From 8c6d1eadb79f4846eac3fb3b8920cc289ebd06ff Mon Sep 17 00:00:00 2001 From: Dmitry Grand Date: Wed, 1 Jul 2026 16:24:20 -0700 Subject: [PATCH 12/13] fixes --- app_dart/lib/src/service/scheduler.dart | 61 ++++++++++--------- .../lib/src/utilities/mocks.mocks.dart | 2 +- 2 files changed, 32 insertions(+), 31 deletions(-) diff --git a/app_dart/lib/src/service/scheduler.dart b/app_dart/lib/src/service/scheduler.dart index 352f253bd6..e0eaa08def 100644 --- a/app_dart/lib/src/service/scheduler.dart +++ b/app_dart/lib/src/service/scheduler.dart @@ -1212,43 +1212,43 @@ detailsUrl: $detailsUrl Future _processGitHubFlowCheckStatusUpdate( String logCrumb, - PresubmitJobState check, + PresubmitJobState job, ) async { - if (!check.status.isComplete) { + if (!job.status.isComplete) { return; } var stage = CiStage.fusionEngineBuild; var conclusion = await _recordCurrentCiStage( - slug: check.slug, - sha: check.sha, + slug: job.slug, + sha: job.sha, stage: stage, - name: check.name, - conclusion: check.status.toTaskConclusion(), + name: job.name, + conclusion: job.status.toTaskConclusion(), ); if (conclusion.result == PresubmitGuardConclusionResult.missing) { stage = CiStage.fusionTests; conclusion = await _recordCurrentCiStage( - slug: check.slug, - sha: check.sha, + slug: job.slug, + sha: job.sha, stage: stage, - name: check.name, - conclusion: check.status.toTaskConclusion(), + name: job.name, + conclusion: job.status.toTaskConclusion(), ); } - if (check.isMergeGroup) { + if (job.isMergeGroup) { await _processMergeGroupCheckStatusUpdate( logCrumb, - check, + job, stage, conclusion, ); } else { await _processRegularCheckStatusUpdate( logCrumb, - check, + job, stage, conclusion, ); @@ -1257,7 +1257,7 @@ detailsUrl: $detailsUrl Future _processRegularCheckStatusUpdate( String logCrumb, - PresubmitJobState check, + PresubmitJobState job, CiStage stage, PresubmitGuardConclusion conclusion, ) async { @@ -1277,17 +1277,17 @@ detailsUrl: $detailsUrl switch (stage) { case CiStage.fusionEngineBuild: await _closeSuccessfulEngineBuildStage( - checkRun: check.checkRun, + checkRun: job.checkRun, mergeQueueGuard: conclusion.checkRunGuard!, - slug: check.slug, - sha: check.sha, + slug: job.slug, + sha: job.sha, logCrumb: logCrumb, ); case CiStage.fusionTests: await _closeSuccessfulTestStage( mergeQueueGuard: conclusion.checkRunGuard!, - slug: check.slug, - sha: check.sha, + slug: job.slug, + sha: job.sha, logCrumb: logCrumb, ); case CiStage.genericTests: @@ -1298,18 +1298,19 @@ detailsUrl: $detailsUrl Future _processMergeGroupCheckStatusUpdate( String logCrumb, - PresubmitJobState check, + PresubmitJobState job, CiStage stage, PresubmitGuardConclusion conclusion, ) async { if (!conclusion.isOk) { - if (conclusion.result == PresubmitGuardConclusionResult.internalError) { - await _completeArtifacts(check.sha, false); + if (conclusion.result == PresubmitGuardConclusionResult.internalError && + conclusion.checkRunGuard != null) { + await _completeArtifacts(job.sha, false); final guard = checkRunFromString(conclusion.checkRunGuard!); await failGuardForMergeGroup( - slug: check.slug, + slug: job.slug, lock: guard, - headSha: check.sha, + headSha: job.sha, summary: conclusion.summary, details: conclusion.details, ); @@ -1325,12 +1326,12 @@ detailsUrl: $detailsUrl } if (conclusion.isFailed) { - await _completeArtifacts(check.sha, false); + await _completeArtifacts(job.sha, false); final guard = checkRunFromString(conclusion.checkRunGuard!); await failGuardForMergeGroup( - slug: check.slug, + slug: job.slug, lock: guard, - headSha: check.sha, + headSha: job.sha, summary: conclusion.summary, details: conclusion.details, ); @@ -1339,11 +1340,11 @@ detailsUrl: $detailsUrl switch (stage) { case CiStage.fusionEngineBuild: - await _completeArtifacts(check.sha, true); + await _completeArtifacts(job.sha, true); await _closeMergeQueue( mergeQueueGuard: conclusion.checkRunGuard!, - slug: check.slug, - sha: check.sha, + slug: job.slug, + sha: job.sha, stage: CiStage.fusionEngineBuild, logCrumb: logCrumb, ); 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 095e7f9b83..f52ce291d8 100644 --- a/packages/cocoon_integration_test/lib/src/utilities/mocks.mocks.dart +++ b/packages/cocoon_integration_test/lib/src/utilities/mocks.mocks.dart @@ -5837,7 +5837,7 @@ class MockScheduler extends _i1.Mock implements _i17.Scheduler { @override _i13.Future processJobStatusUpdate(_i38.PresubmitJobState? job) => (super.noSuchMethod( - Invocation.method(#processCheckRunCompleted, [job]), + Invocation.method(#processJobStatusUpdate, [job]), returnValue: _i13.Future.value(), returnValueForMissingStub: _i13.Future.value(), ) From 5942af3de35177e2aa635db195107ed5dcbbea91 Mon Sep 17 00:00:00 2001 From: Dmitry Grand Date: Wed, 1 Jul 2026 16:28:09 -0700 Subject: [PATCH 13/13] format --- app_dart/lib/src/service/scheduler.dart | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/app_dart/lib/src/service/scheduler.dart b/app_dart/lib/src/service/scheduler.dart index e0eaa08def..3c32ccf980 100644 --- a/app_dart/lib/src/service/scheduler.dart +++ b/app_dart/lib/src/service/scheduler.dart @@ -1246,12 +1246,7 @@ detailsUrl: $detailsUrl conclusion, ); } else { - await _processRegularCheckStatusUpdate( - logCrumb, - job, - stage, - conclusion, - ); + await _processRegularCheckStatusUpdate(logCrumb, job, stage, conclusion); } }