diff --git a/app_dart/lib/src/model/firestore/presubmit_check.dart b/app_dart/lib/src/model/firestore/presubmit_check.dart index 80fa78b26..ee57a2850 100644 --- a/app_dart/lib/src/model/firestore/presubmit_check.dart +++ b/app_dart/lib/src/model/firestore/presubmit_check.dart @@ -233,8 +233,12 @@ final class PresubmitCheck extends AppDocument { fields[fieldEndTime] = endTime.toValue(); } - set summary(String summary) { - fields[fieldSummary] = summary.toValue(); + set summary(String? summary) { + if (summary == null) { + fields.remove(fieldSummary); + } else { + fields[fieldSummary] = summary.toValue(); + } } void updateFromBuild(bbv2.Build build) { 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 e19e450d5..ef336ab4a 100644 --- a/app_dart/lib/src/service/firestore/unified_check_run.dart +++ b/app_dart/lib/src/service/firestore/unified_check_run.dart @@ -76,66 +76,6 @@ final class UnifiedCheckRun { } } - /// Initializes a new document for the given [tasks] in Firestore so that stage-tracking can succeed. - /// - /// The list of tasks will be written as fields of a document with additional fields for tracking the creationTime - /// number of tasks, remaining count. It is required to include [commitSha] as a json encoded [CheckRun] as this - /// will be used to unlock any check runs blocking progress. - /// - /// Returns the created document or throws an error. - static Future initializePresubmitGuardDocument({ - required FirestoreService firestoreService, - - required RepositorySlug slug, - required int pullRequestId, - required CheckRun checkRun, - required CiStage stage, - required String commitSha, - required int creationTime, - required String author, - required List tasks, - }) async { - final buildCount = tasks.length; - final logCrumb = - 'initializeDocument(${slug.owner}_${slug.name}_${pullRequestId}_${checkRun.id}_$stage, $buildCount builds)'; - - final presubmitGuard = PresubmitGuard( - checkRun: checkRun, - commitSha: commitSha, - slug: slug, - pullRequestId: pullRequestId, - stage: stage, - creationTime: creationTime, - author: author, - remainingBuilds: buildCount, - failedBuilds: 0, - builds: {for (final task in tasks) task: TaskStatus.waitingForBackfill}, - ); - - final presubmitGuardId = PresubmitGuard.documentIdFor( - slug: slug, - pullRequestId: pullRequestId, - checkRunId: checkRun.id!, - stage: stage, - ); - - try { - // Calling createDocument multiple times for the same documentId will return a 409 - ALREADY_EXISTS error; - // this is good because it means we don't have to do any transactions. - // curl -X POST -H "Content-Type: application/json" -H "Authorization: Bearer " "https://firestore.googleapis.com/v1beta1/projects/flutter-dashboard/databases/cocoon/documents/presubmit_guard?documentId=foo_bar_baz" -d '{"fields": {"test": {"stringValue": "baz"}}}' - final newDoc = await firestoreService.createDocument( - presubmitGuard, - collectionId: presubmitGuard.runtimeMetadata.collectionId, - documentId: presubmitGuardId.documentId, - ); - log.info('$logCrumb: document created'); - return newDoc; - } catch (e) { - log.warn('$logCrumb: failed to create document', e); - rethrow; - } - } - /// Returns _all_ checks running against the specified github [checkRunId]. static Future> queryAllPresubmitChecksForGuard({ required FirestoreService firestoreService, @@ -248,9 +188,9 @@ final class UnifiedCheckRun { await firestoreService.rollback(transaction); return PresubmitGuardConclusion( result: PresubmitGuardConclusionResult.missing, - remaining: presubmitGuard.remainingBuilds ?? -1, + remaining: presubmitGuard.remainingBuilds!, checkRunGuard: presubmitGuard.checkRunJson, - failed: presubmitGuard.failedBuilds ?? -1, + failed: presubmitGuard.failedBuilds!, summary: 'Check run "${state.buildName}" not present in ${guardId.stage} CI stage', details: 'Change $changeCrumb', @@ -271,11 +211,24 @@ final class UnifiedCheckRun { remaining = presubmitGuard.remainingBuilds!; failed = presubmitGuard.failedBuilds!; final builds = presubmitGuard.builds; + var status = builds?[state.buildName]!; - // If build is in progress, we should only update appropriate checks with - // that [TaskStatus] - if (state.status.isBuildInProgress) { + // If build is waiting for backfill, that means its initiated by github + // or re-run. So no processing needed, we should only update appropriate + // checks with that [TaskStatus] + if (state.status == TaskStatus.waitingForBackfill) { + status = state.status; + valid = true; + // If build is in progress, we should update apropriate checks with start + // time and their status to that [TaskStatus] only if the build is not + // completed. + } else if (state.status == TaskStatus.inProgress) { presubmitCheck.startTime = state.startTime!; + // If the build is not completed, update the status. + if (!status!.isBuildCompleted) { + status = state.status; + } + valid = true; } else { // "remaining" should go down if buildSuccessed of any attempt // or buildFailed a first attempt. @@ -284,35 +237,35 @@ final class UnifiedCheckRun { // attemptNumber = 1 && buildFailed: up (+1) // So if the test existed and either remaining or failed_count is changed; // the response is valid. - - if (state.status.isBuildSuccessed || - state.attemptNumber == 1 && state.status.isBuildFailed) { + status = state.status; + if (status.isBuildSuccessed) { // Guard against going negative and log enough info so we can debug. if (remaining == 0) { throw '$logCrumb: field "${PresubmitGuard.fieldRemainingBuilds}" is already zero for $transaction / ${presubmitGuardDocument.fields}'; } remaining = remaining - 1; - valid = true; - } - // Only rollback the "failed" counter if this is a successful test run, - // i.e. the test failed, the user requested a rerun, and now it passes. - if (state.attemptNumber > 1 && state.status.isBuildSuccessed) { - log.info( - '$logCrumb: conclusion flipped to positive - assuming test was re-run', - ); - if (failed == 0) { - throw '$logCrumb: field "${PresubmitGuard.fieldFailedBuilds}" is already zero for $transaction / ${presubmitGuardDocument.fields}'; + // Only rollback the "failed" counter if this is a successful test run, + // i.e. the test failed, the user requested a rerun, and now it passes. + if (state.attemptNumber > 1) { + log.info( + '$logCrumb: conclusion flipped to positive - assuming test was re-run', + ); + if (failed == 0) { + throw '$logCrumb: field "${PresubmitGuard.fieldFailedBuilds}" is already zero for $transaction / ${presubmitGuardDocument.fields}'; + } + failed = failed - 1; } valid = true; - failed = failed - 1; } // Only increment the "failed" counter if the conclusion failed for the first attempt. - if (state.attemptNumber == 1 && state.status.isBuildFailed) { - log.info('$logCrumb: test failed'); + if (status.isBuildFailed) { + if (state.attemptNumber == 1) { + log.info('$logCrumb: test failed'); + failed = failed + 1; + } valid = true; - failed = failed + 1; } // All checks pass. "valid" is only set to true if there was a change in either the remaining or failed count. @@ -322,10 +275,11 @@ final class UnifiedCheckRun { presubmitGuard.remainingBuilds = remaining; presubmitGuard.failedBuilds = failed; presubmitCheck.endTime = state.endTime!; + presubmitCheck.summary = state.summary; } - builds![state.buildName] = state.status; + builds![state.buildName] = status; presubmitGuard.builds = builds; - presubmitCheck.status = state.status; + presubmitCheck.status = status; } on DetailedApiRequestError catch (e, stack) { if (e.status == 404) { // An attempt to read a document not in firestore should not be retried. diff --git a/app_dart/lib/src/service/scheduler.dart b/app_dart/lib/src/service/scheduler.dart index 4a0771ebd..05759634d 100644 --- a/app_dart/lib/src/service/scheduler.dart +++ b/app_dart/lib/src/service/scheduler.dart @@ -670,16 +670,17 @@ class Scheduler { // of the queue. To do that, the merge queue guard must fail as it's the // only required GitHub check. await failGuardForMergeGroup( - slug, - headSha, - 'Failed to schedule checks for merge group', - ''' + slug: slug, + lock: lock, + headSha: headSha, + summary: 'Failed to schedule checks for merge group', + details: + ''' $logCrumb ERROR: $e $s ''', - lock, ); } } @@ -852,13 +853,14 @@ $s /// /// This removes the merge group from the merge queue without landing it. The /// corresponding pull request will have to be fixed and re-enqueued again. - Future failGuardForMergeGroup( - RepositorySlug slug, - String headSha, - String summary, - String details, - CheckRun lock, - ) async { + Future failGuardForMergeGroup({ + required RepositorySlug slug, + required CheckRun lock, + required String headSha, + required String summary, + required String details, + String? detailsUrl, + }) async { log.info('Failing merge group guard for merge group $headSha in $slug'); await _githubChecksService.githubChecksUtil.updateCheckRun( _config, @@ -871,6 +873,7 @@ $s summary: summary, text: details, ), + detailsUrl: detailsUrl, ); } @@ -1017,16 +1020,34 @@ $s await _completeArtifacts(check.sha, false); final guard = checkRunFromString(stagingConclusion.checkRunGuard!); await failGuardForMergeGroup( - check.slug, - check.sha, - stagingConclusion.summary, - stagingConclusion.details, - guard, + slug: check.slug, + lock: guard, + headSha: check.sha, + summary: stagingConclusion.summary, + details: stagingConclusion.details, ); } return false; } + // If the number of failed checks is equal to the number of remaining checks, then all remaining checks have failed. + if (check.isUnifiedCheckRun && + stagingConclusion.failed > 0 && + stagingConclusion.failed == stagingConclusion.remaining) { + final guard = checkRunFromString(stagingConclusion.checkRunGuard!); + await failGuardForMergeGroup( + slug: check.slug, + lock: guard, + headSha: check.sha, + summary: kMergeQueueLockDescription, + details: + 'For CI stage ${check.stage} ${stagingConclusion.failed} checks failed', + detailsUrl: + 'https://flutter-dashboard.appspot.com/repo/${check.slug.name}/checkrun/${check.guardId}', + ); + return true; + } + // Are there tests remaining? Keep waiting. if (stagingConclusion.isPending) { log.info( @@ -1046,11 +1067,11 @@ $s await _completeArtifacts(check.sha, false); final guard = checkRunFromString(stagingConclusion.checkRunGuard!); await failGuardForMergeGroup( - check.slug, - check.sha, - stagingConclusion.summary, - stagingConclusion.details, - guard, + slug: check.slug, + lock: guard, + headSha: check.sha, + summary: stagingConclusion.summary, + details: stagingConclusion.details, ); } return true; 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 ed01d1be1..de188fa54 100644 --- a/app_dart/test/service/firestore/unified_check_run_test.dart +++ b/app_dart/test/service/firestore/unified_check_run_test.dart @@ -132,31 +132,36 @@ void main() { ); // Initialize documents - await UnifiedCheckRun.initializePresubmitGuardDocument( - firestoreService: firestoreService, + final guard = PresubmitGuard( + checkRun: checkRun, + commitSha: sha, slug: slug, pullRequestId: 1, - checkRun: checkRun, stage: CiStage.fusionEngineBuild, - commitSha: sha, creationTime: 1000, author: 'dash', - tasks: ['linux', 'mac'], + builds: { + for (final task in ['linux', 'mac']) + task: TaskStatus.waitingForBackfill, + }, + remainingBuilds: 2, + failedBuilds: 0, ); final check1 = PresubmitCheck.init( buildName: 'linux', - checkRunId: 123, + checkRunId: guardId.checkRunId, creationTime: 1000, ); final check2 = PresubmitCheck.init( buildName: 'mac', - checkRunId: 123, + checkRunId: guardId.checkRunId, creationTime: 1000, ); await firestoreService.writeViaTransaction( documentsToWrites([ + Document(name: guard.name, fields: guard.fields), Document(name: check1.name, fields: check1.fields), Document(name: check2.name, fields: check2.fields), ], exists: false), @@ -262,7 +267,7 @@ void main() { ); expect(result.result, PresubmitGuardConclusionResult.ok); - expect(result.remaining, 1); + expect(result.remaining, 2); expect(result.failed, 1); }); diff --git a/app_dart/test/service/scheduler_test.dart b/app_dart/test/service/scheduler_test.dart index 713b27f37..0eff54c77 100644 --- a/app_dart/test/service/scheduler_test.dart +++ b/app_dart/test/service/scheduler_test.dart @@ -23,7 +23,6 @@ import 'package:cocoon_service/src/model/firestore/presubmit_guard.dart'; import 'package:cocoon_service/src/model/firestore/task.dart' as fs; import 'package:cocoon_service/src/model/github/checks.dart' as cocoon_checks; import 'package:cocoon_service/src/service/big_query.dart'; -import 'package:cocoon_service/src/service/firestore/unified_check_run.dart'; import 'package:cocoon_service/src/service/flags/dynamic_config.dart'; import 'package:cocoon_service/src/service/flags/unified_check_run_flow_flags.dart'; import 'package:cocoon_service/src/service/luci_build_service/engine_artifacts.dart'; @@ -3590,16 +3589,19 @@ targets: ); // Initialize presubmit guard for engine stage - await UnifiedCheckRun.initializePresubmitGuardDocument( - firestoreService: firestore, - slug: pullRequest.base!.repo!.slug(), - pullRequestId: pullRequest.number!, - checkRun: checkRunGuard, - stage: CiStage.fusionEngineBuild, - commitSha: pullRequest.head!.sha!, - creationTime: DateTime.now().millisecondsSinceEpoch, - author: pullRequest.user!.login!, - tasks: ['Linux engine_build'], + firestore.putDocument( + PresubmitGuard( + checkRun: checkRunGuard, + commitSha: pullRequest.head!.sha!, + slug: pullRequest.base!.repo!.slug(), + pullRequestId: pullRequest.number!, + stage: CiStage.fusionEngineBuild, + author: pullRequest.user!.login!, + creationTime: DateTime.now().millisecondsSinceEpoch, + builds: {'Linux engine_build': TaskStatus.waitingForBackfill}, + remainingBuilds: 1, + failedBuilds: 0, + ), ); // Initialize check run for the task @@ -3677,16 +3679,19 @@ targets: // checkRunGuard.checkSuite!.headBranch = 'gh-readonly-queue/master/pr-123-abc'; // Initialize presubmit guard for tests stage - await UnifiedCheckRun.initializePresubmitGuardDocument( - firestoreService: firestore, - slug: pullRequest.base!.repo!.slug(), - pullRequestId: pullRequest.number!, - checkRun: checkRunGuard, - stage: CiStage.fusionTests, - commitSha: pullRequest.head!.sha!, - creationTime: DateTime.now().millisecondsSinceEpoch, - author: pullRequest.user!.login!, - tasks: ['Linux test'], + firestore.putDocument( + PresubmitGuard( + checkRun: checkRunGuard, + commitSha: pullRequest.head!.sha!, + slug: pullRequest.base!.repo!.slug(), + pullRequestId: pullRequest.number!, + stage: CiStage.fusionTests, + author: pullRequest.user!.login!, + creationTime: DateTime.now().millisecondsSinceEpoch, + builds: {'Linux test': TaskStatus.waitingForBackfill}, + remainingBuilds: 1, + failedBuilds: 0, + ), ); // Initialize check run for the task @@ -3728,6 +3733,7 @@ targets: any, status: anyNamed('status'), conclusion: CheckRunConclusion.failure, // Merge queue failure + detailsUrl: anyNamed('detailsUrl'), output: anyNamed('output'), ), ).called(1); @@ -3756,16 +3762,19 @@ targets: // checkRunGuard.checkSuite!.headBranch = 'gh-readonly-queue/master/pr-123-abc'; // Initialize presubmit guard for tests stage - await UnifiedCheckRun.initializePresubmitGuardDocument( - firestoreService: firestore, - slug: pullRequest.base!.repo!.slug(), - pullRequestId: pullRequest.number!, - checkRun: checkRunGuard, - stage: CiStage.fusionTests, - commitSha: pullRequest.head!.sha!, - creationTime: DateTime.now().millisecondsSinceEpoch, - author: pullRequest.user!.login!, - tasks: ['Linux test'], + firestore.putDocument( + PresubmitGuard( + checkRun: checkRunGuard, + commitSha: pullRequest.head!.sha!, + slug: pullRequest.base!.repo!.slug(), + pullRequestId: pullRequest.number!, + stage: CiStage.fusionTests, + author: pullRequest.user!.login!, + creationTime: DateTime.now().millisecondsSinceEpoch, + builds: {'Linux test': TaskStatus.waitingForBackfill}, + remainingBuilds: 1, + failedBuilds: 0, + ), ); // Initialize check run for the task diff --git a/app_dart/test/src/utilities/mocks.mocks.dart b/app_dart/test/src/utilities/mocks.mocks.dart index 662983c2d..2c1bd60fd 100644 --- a/app_dart/test/src/utilities/mocks.mocks.dart +++ b/app_dart/test/src/utilities/mocks.mocks.dart @@ -5626,21 +5626,23 @@ class MockScheduler extends _i1.Mock implements _i17.Scheduler { as _i13.Future); @override - _i13.Future failGuardForMergeGroup( - _i7.RepositorySlug? slug, - String? headSha, - String? summary, - String? details, - _i7.CheckRun? lock, - ) => + _i13.Future failGuardForMergeGroup({ + required _i7.RepositorySlug? slug, + required _i7.CheckRun? lock, + required String? headSha, + required String? summary, + required String? details, + String? detailsUrl, + }) => (super.noSuchMethod( - Invocation.method(#failGuardForMergeGroup, [ - slug, - headSha, - summary, - details, - lock, - ]), + Invocation.method(#failGuardForMergeGroup, [], { + #slug: slug, + #lock: lock, + #headSha: headSha, + #summary: summary, + #details: details, + #detailsUrl: detailsUrl, + }), returnValue: _i13.Future.value(), returnValueForMissingStub: _i13.Future.value(), ) @@ -5685,16 +5687,6 @@ class MockScheduler extends _i1.Mock implements _i17.Scheduler { ) as _i13.Future); - @override - _i13.Future processUnifiedCheckRunCompleted( - _i42.PresubmitCompletedCheck? check, - ) => - (super.noSuchMethod( - Invocation.method(#processUnifiedCheckRunCompleted, [check]), - returnValue: _i13.Future.value(false), - ) - as _i13.Future); - @override bool detectMergeGroup(_i32.CheckRun? checkRun) => (super.noSuchMethod(