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 56f3b3992..5eebad2a9 100644 --- a/app_dart/lib/src/service/firestore/unified_check_run.dart +++ b/app_dart/lib/src/service/firestore/unified_check_run.dart @@ -37,17 +37,21 @@ final class UnifiedCheckRun { config.flags.isUnifiedCheckRunFlowEnabledForUser( pullRequest.user!.login!, )) { + // Create the UnifiedCheckRun and UnifiedCheckRunBuilds. log.info( 'Storing UnifiedCheckRun data for ${slug.fullName}#${pullRequest.number} as it enabled for user ${pullRequest.user!.login}.', ); - // Create the UnifiedCheckRun and UnifiedCheckRunBuilds. + // We store the creation time of the guard since there might be several + // guards for the same PR created and each new one created after previous + // was succeeded so we are interested in a state of the latest one. + final creationTime = DateTime.now().toUtc().microsecondsSinceEpoch; final guard = PresubmitGuard( checkRun: checkRun, commitSha: sha, slug: slug, pullRequestId: pullRequest.number!, stage: stage, - creationTime: pullRequest.createdAt!.microsecondsSinceEpoch, + creationTime: creationTime, author: pullRequest.user!.login!, remainingBuilds: tasks.length, failedBuilds: 0, @@ -58,7 +62,7 @@ final class UnifiedCheckRun { PresubmitCheck.init( buildName: task, checkRunId: checkRun.id!, - creationTime: pullRequest.createdAt!.microsecondsSinceEpoch, + creationTime: creationTime, ), ]; await firestoreService.writeViaTransaction( @@ -89,7 +93,9 @@ final class UnifiedCheckRun { log.info('$logCrumb Re-Running failed checks.'); final transaction = await firestoreService.beginTransaction(); - final guards = await getPresubmitGuardsForCheckRun( + // New guard created only if previous is succeeded so failed checks might be + // only in latest guard. + final guard = await getLatestPresubmitGuardForCheckRun( firestoreService: firestoreService, slug: slug, pullRequestId: pullRequestId, @@ -97,52 +103,55 @@ final class UnifiedCheckRun { transaction: transaction, ); - for (final guard in guards) { - // Copy the failed build names to a local variable to avoid losing the - // failed build names after resetting the failed guard.builds. - final failedBuildNames = guard.failedBuildNames; - if (failedBuildNames.isNotEmpty) { - guard.failedBuilds = 0; - guard.remainingBuilds = failedBuildNames.length; - final builds = guard.builds; - for (final buildName in failedBuildNames) { - builds[buildName] = TaskStatus.waitingForBackfill; - } - guard.builds = builds; - final checks = [ - for (final buildName in failedBuildNames) - PresubmitCheck.init( - buildName: buildName, - checkRunId: checkRunId, - creationTime: DateTime.now().toUtc().microsecondsSinceEpoch, - attemptNumber: - ((await getLatestPresubmitCheck( - firestoreService: firestoreService, - checkRunId: checkRunId, - buildName: buildName, - transaction: transaction, - ))?.attemptNumber ?? - 0) + - 1, // Increment the latest attempt number. - ), - ]; - try { - final response = await firestoreService.commit( - transaction, - documentsToWrites([...checks, guard]), - ); - log.info( - '$logCrumb: results = ${response.writeResults?.map((e) => e.toJson())}', - ); - return FailedChecksForRerun( - checkRunGuard: guard.checkRun, - checkNames: failedBuildNames, - stage: guard.stage, - ); - } catch (e) { - log.info('$logCrumb: failed to update presubmit check', e); - rethrow; - } + if (guard == null) { + return null; + } + + // Copy the failed build names to a local variable to avoid losing the + // failed build names after resetting the failed guard.builds. + final creationTime = DateTime.now().toUtc().microsecondsSinceEpoch; + final failedBuildNames = guard.failedBuildNames; + if (failedBuildNames.isNotEmpty) { + guard.failedBuilds = 0; + guard.remainingBuilds = failedBuildNames.length; + final builds = guard.builds; + for (final buildName in failedBuildNames) { + builds[buildName] = TaskStatus.waitingForBackfill; + } + guard.builds = builds; + final checks = [ + for (final buildName in failedBuildNames) + PresubmitCheck.init( + buildName: buildName, + checkRunId: checkRunId, + creationTime: creationTime, + attemptNumber: + ((await getLatestPresubmitCheck( + firestoreService: firestoreService, + checkRunId: checkRunId, + buildName: buildName, + transaction: transaction, + ))?.attemptNumber ?? + 0) + + 1, // Increment the latest attempt number. + ), + ]; + try { + final response = await firestoreService.commit( + transaction, + documentsToWrites([...checks, guard]), + ); + log.info( + '$logCrumb: results = ${response.writeResults?.map((e) => e.toJson())}', + ); + return FailedChecksForRerun( + checkRunGuard: guard.checkRun, + checkNames: failedBuildNames, + stage: guard.stage, + ); + } catch (e) { + log.info('$logCrumb: failed to update presubmit check', e); + rethrow; } } return null; @@ -184,7 +193,7 @@ final class UnifiedCheckRun { )).firstOrNull; } - /// Returns the latest check for the specified github [checkRunId] and + /// Returns the latest [PresubmitCheck] for the specified github [checkRunId] and /// [buildName]. static Future getLatestPresubmitCheck({ required FirestoreService firestoreService, @@ -202,20 +211,20 @@ final class UnifiedCheckRun { )).firstOrNull; } - /// Returns [PresubmitGuard]s for the specified github [checkRunId]. - static Future> getPresubmitGuardsForCheckRun({ + /// Returns the latest [PresubmitGuard] for the specified github [checkRunId]. + static Future getLatestPresubmitGuardForCheckRun({ required FirestoreService firestoreService, required RepositorySlug slug, required int pullRequestId, required int checkRunId, Transaction? transaction, }) async { - return await _queryPresubmitGuards( + return (await _queryPresubmitGuards( firestoreService: firestoreService, checkRunId: checkRunId, transaction: transaction, - orderMap: const {PresubmitGuard.fieldStage: kQueryOrderAscending}, - ); + limit: 1, + )).firstOrNull; } static Future> _queryPresubmitGuards({ @@ -332,9 +341,9 @@ final class UnifiedCheckRun { await firestoreService.rollback(transaction); return PresubmitGuardConclusion( result: PresubmitGuardConclusionResult.missing, - remaining: presubmitGuard.remainingBuilds!, + remaining: presubmitGuard.remainingBuilds, checkRunGuard: presubmitGuard.checkRunJson, - failed: presubmitGuard.failedBuilds!, + failed: presubmitGuard.failedBuilds, summary: 'Check run "${state.buildName}" not present in ${guardId.stage} CI stage', details: 'Change $changeCrumb', @@ -352,8 +361,8 @@ final class UnifiedCheckRun { ); presubmitCheck = PresubmitCheck.fromDocument(presubmitCheckDocument); - remaining = presubmitGuard.remainingBuilds!; - failed = presubmitGuard.failedBuilds!; + remaining = presubmitGuard.remainingBuilds; + failed = presubmitGuard.failedBuilds; final builds = presubmitGuard.builds; var status = builds[state.buildName]!; @@ -369,7 +378,7 @@ final class UnifiedCheckRun { } else if (state.status == TaskStatus.inProgress) { presubmitCheck.startTime = state.startTime!; // If the build is not completed, update the status. - if (!status!.isBuildCompleted) { + if (!status.isBuildCompleted) { status = state.status; } valid = 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 51141394c..26a64f632 100644 --- a/app_dart/test/service/firestore/unified_check_run_test.dart +++ b/app_dart/test/service/firestore/unified_check_run_test.dart @@ -484,7 +484,7 @@ void main() { }); }); - test('getPresubmitGuardsForCheckRun returns correct guards', () async { + test('getLatestPresubmitGuardForCheckRun returns latest guard', () async { final sha = 'sha'; final slug = RepositorySlug('flutter', 'flutter'); final checkRun = CheckRun.fromJson(const { @@ -513,7 +513,7 @@ void main() { slug: slug, pullRequestId: 1, stage: CiStage.fusionTests, - creationTime: 1000, + creationTime: 2000, author: 'dash', remainingBuilds: 1, failedBuilds: 0, @@ -527,17 +527,16 @@ void main() { ], exists: false), ); - final guards = await UnifiedCheckRun.getPresubmitGuardsForCheckRun( + final guard = await UnifiedCheckRun.getLatestPresubmitGuardForCheckRun( firestoreService: firestoreService, slug: slug, pullRequestId: 1, checkRunId: 123, ); - expect(guards, hasLength(2)); - expect(guards[0].stage, CiStage.fusionEngineBuild); - expect(guards[1].stage, CiStage.fusionTests); - expect(guards.every((g) => g.checkRunId == 123), isTrue); + expect(guard, isNotNull); + expect(guard!.stage, CiStage.fusionTests); + expect(guard.checkRunId, 123); }); }); } diff --git a/app_dart/test/service/scheduler_test.dart b/app_dart/test/service/scheduler_test.dart index f5a04072b..f0bdd3027 100644 --- a/app_dart/test/service/scheduler_test.dart +++ b/app_dart/test/service/scheduler_test.dart @@ -1290,8 +1290,6 @@ targets: expect(updatedGuard.builds['Linux A'], TaskStatus.waitingForBackfill); }); - - test('rerequested action reschedules failed checks for fusion', () async { final mockGithubChecksService = MockGithubChecksService(); when( @@ -1323,7 +1321,7 @@ targets: slug: slug, pullRequestId: 1, stage: CiStage.fusionTests, - creationTime: 1000, + creationTime: 2000, author: 'dash', builds: {'Linux A': TaskStatus.failed}, failedBuilds: 1, @@ -1333,7 +1331,7 @@ targets: final check = PresubmitCheck( buildName: 'Linux A', checkRunId: 1, - creationTime: 1000, + creationTime: 2000, status: TaskStatus.failed, attemptNumber: 1, );