diff --git a/app_dart/lib/src/model/firestore/ci_staging.dart b/app_dart/lib/src/model/firestore/ci_staging.dart index 7a7e145ad3..75dc596184 100644 --- a/app_dart/lib/src/model/firestore/ci_staging.dart +++ b/app_dart/lib/src/model/firestore/ci_staging.dart @@ -88,7 +88,8 @@ class CiStaging extends Document { required String checkRun, required String conclusion, }) async { - final logCrumb = 'markConclusion(${slug.owner}_${slug.name}_${sha}_$stage, $checkRun, $conclusion)'; + final changeCrumb = '${slug.owner}_${slug.name}_$sha'; + final logCrumb = 'markConclusion(${changeCrumb}_$stage, $checkRun, $conclusion)'; // Marking needs to happen while in a transaction to ensure `remaining` is // updated correctly. For that to happen correctly; we need to perform a @@ -110,8 +111,10 @@ class CiStaging extends Document { var remaining = -1; var failed = -1; + var total = -1; bool valid = false; String? checkRunGuard; + String? recordedConclusion; late final Document doc; @@ -146,9 +149,15 @@ class CiStaging extends Document { } failed = maybeFailed; + final maybeTotal = int.tryParse(fields[kTotalField]?.integerValue ?? ''); + if (maybeTotal == null) { + throw '$logCrumb: missing field "$kTotalField" for $transaction / ${doc.fields}'; + } + total = maybeTotal; + // 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. - final recordedConclusion = fields[checkRun]?.stringValue; + recordedConclusion = fields[checkRun]?.stringValue; if (recordedConclusion == null) { log.info('$logCrumb: $checkRun not present in doc for $transaction / $doc'); await docRes.rollback(RollbackRequest(transaction: transaction), kDatabase); @@ -157,6 +166,8 @@ class CiStaging extends Document { remaining: remaining, checkRunGuard: null, failed: failed, + summary: 'Check run "$checkRun" not present in $stage CI stage', + details: 'Change $changeCrumb', ); } @@ -207,7 +218,7 @@ class CiStaging extends Document { fields[checkRun] = Value(stringValue: conclusion); fields[kRemainingField] = Value(integerValue: '$remaining'); fields[kFailedField] = Value(integerValue: '$failed'); - } on DetailedApiRequestError catch (e) { + } on DetailedApiRequestError catch (e, stack) { if (e.status == 404) { // An attempt to read a document not in firestore should not be retried. log.info('$logCrumb: staging document not found for $transaction'); @@ -217,6 +228,15 @@ class CiStaging extends Document { remaining: -1, checkRunGuard: null, failed: failed, + summary: 'Internal server error', + details: ''' +Staging document not found for CI stage "$stage" for $changeCrumb. Got 404 from +Firestore. + +Error: +${e.toString()} +$stack +''', ); } // All other errors should bubble up and be retried. @@ -239,6 +259,15 @@ class CiStaging extends Document { remaining: remaining, checkRunGuard: checkRunGuard, failed: failed, + summary: valid ? 'All tests passed' : 'Not a valid state transition for $checkRun', + details: valid + ? ''' +For CI stage $stage: + Total check runs scheduled: $total + Pending: $remaining + Failed: $failed +''' + : 'Attempted to transition the state of check run $checkRun from "$recordedConclusion" to "$conclusion".', ); } @@ -343,12 +372,16 @@ class StagingConclusion { final int remaining; final String? checkRunGuard; final int failed; + final String summary; + final String details; const StagingConclusion({ required this.result, required this.remaining, required this.checkRunGuard, required this.failed, + required this.summary, + required this.details, }); bool get isOk => result == StagingConclusionResult.ok; @@ -366,8 +399,21 @@ class StagingConclusion { other.result == result && other.remaining == remaining && other.checkRunGuard == checkRunGuard && - other.failed == failed); + other.failed == failed && + other.summary == summary && + other.details == details); + + @override + int get hashCode => Object.hashAll([ + result, + remaining, + checkRunGuard, + failed, + summary, + details, + ]); @override - int get hashCode => Object.hashAll([result, remaining, checkRunGuard, failed]); + String toString() => + 'StagingConclusion("$result", "$remaining", "$checkRunGuard", "$failed", "$summary", "$details")'; } diff --git a/app_dart/lib/src/service/config.dart b/app_dart/lib/src/service/config.dart index 4f46b7a9f9..0350580152 100644 --- a/app_dart/lib/src/service/config.dart +++ b/app_dart/lib/src/service/config.dart @@ -32,11 +32,37 @@ const String kDefaultBranchName = 'master'; class Config { Config(this._db, this._cache); - /// Labels autosubmit looks for on pull requests + /// When present on a pull request, instructs Cocoon to submit it + /// automatically as soon as all the required checks pass. /// /// Keep this in sync with the similar `Config` class in `auto_submit`. static const String kAutosubmitLabel = 'autosubmit'; + /// When present on a pull request, allows it to land without passing all the + /// checks, and jumps the merge queue. + /// + /// Keep this in sync with the similar `Config` class in `auto_submit`. + static const String kEmergencyLabel = 'emergency'; + + /// Validates that CI tasks were successfully created from the .ci.yaml file. + /// + /// If this check fails, it means Cocoon failed to fully populate the list of + /// CI checks and the PR/commit should be treated as failing. + static const String kCiYamlCheckName = 'ci.yaml validation'; + + /// A required check that stays in pending state until a sufficient subset of + /// checks pass. + /// + /// This check is "required", meaning that it must pass before Github will + /// allow a PR to land in the merge queue, or a merge group to land on the + /// target branch (main or master). + /// + /// IMPORTANT: the name of this task - "Merge Queue Guard" - must strictly + /// match the name of the required check configured in the repo settings. + /// Changing the name here or in the settings alone will break the PR + /// workflow. + static const String kMergeQueueLockName = 'Merge Queue Guard'; + final DatastoreDB _db; final CacheService _cache; @@ -150,7 +176,6 @@ class Config { // GitHub App properties. Future get githubPrivateKey => _getSingleValue('githubapp_private_pem'); - Future get overrideTreeStatusLabel => _getSingleValue('override_tree_status_label'); Future get githubPublicKey => _getSingleValue('githubapp_public_pem'); Future get githubAppId => _getSingleValue('githubapp_id'); Future> get githubAppInstallations async { diff --git a/app_dart/lib/src/service/scheduler.dart b/app_dart/lib/src/service/scheduler.dart index 1d48b1ce5b..22558103ec 100644 --- a/app_dart/lib/src/service/scheduler.dart +++ b/app_dart/lib/src/service/scheduler.dart @@ -101,28 +101,9 @@ class Scheduler { /// Name of the subcache to store scheduler related values in redis. static const String subcacheName = 'scheduler'; - /// Validates that CI tasks were successfully created from the .ci.yaml file. - /// - /// If this check fails, it means Cocoon failed to fully populate the list of - /// CI checks and the PR/commit should be treated as failing. - static const String kCiYamlCheckName = 'ci.yaml validation'; - - /// A required check that stays in pending state until a sufficient subset of - /// checks pass. - /// - /// This check is "required", meaning that it must pass before Github will - /// allow a PR to land in the merge queue, or a merge group to land on the - /// target branch (main or master). - /// - /// IMPORTANT: the name of this task - "Merge Queue Guard" - must strictly - /// match the name of the required check configured in the repo settings. - /// Changing the name here or in the settings alone will break the PR - /// workflow. - static const String kMergeQueueLockName = 'Merge Queue Guard'; - /// List of check runs that do not need to be tracked or looked up in /// any staging logic. - static const kCheckRunsToIgnore = [kMergeQueueLockName, kCiYamlCheckName]; + static const kCheckRunsToIgnore = [Config.kMergeQueueLockName, Config.kCiYamlCheckName]; /// Briefly describes what the "Merge Queue Guard" check is for. /// @@ -483,9 +464,11 @@ class Scheduler { // Update validate ci.yaml check await closeCiYamlCheckRun('PR ${pullRequest.number}', exception, slug, ciValidationCheckRun); - // The 'lock' will be unlocked later in processCheckRunCompletion after all engine builds are processed. + // Normally the lock stays pending until the PR is ready to be enqueued, but + // there are situations (see code above) when it needs to be unlocked + // immediately. if (unlockMergeGroup) { - await unlockMergeGroupChecks(slug, pullRequest.head!.sha!, lock, exception); + await unlockMergeQueueGuard(slug, pullRequest.head!.sha!, lock); } log.info( 'Finished triggering builds for: pr ${pullRequest.number}, commit ${pullRequest.base!.sha}, branch ${pullRequest.head!.ref} and slug $slug}', @@ -510,7 +493,7 @@ class Scheduler { conclusion: CheckRunConclusion.success, ); } else { - log.warning('Marking $description $kCiYamlCheckName as failed', e); + log.warning('Marking $description ${Config.kCiYamlCheckName} as failed', e); // Failure when validating ci.yaml await githubChecksService.githubChecksUtil.updateCheckRun( config, @@ -519,7 +502,7 @@ class Scheduler { status: CheckRunStatus.completed, conclusion: CheckRunConclusion.failure, output: CheckRunOutput( - title: kCiYamlCheckName, + title: Config.kCiYamlCheckName, summary: '.ci.yaml has failures', text: exception.toString(), ), @@ -533,9 +516,9 @@ class Scheduler { config, slug, pullRequest.head!.sha!, - kCiYamlCheckName, + Config.kCiYamlCheckName, output: const CheckRunOutput( - title: kCiYamlCheckName, + title: Config.kCiYamlCheckName, summary: 'If this check is stuck pending, push an empty commit to retrigger the checks', ), ); @@ -579,7 +562,7 @@ class Scheduler { // close the ci.yaml validation and merge group guard. if (!isFusion) { await closeCiYamlCheckRun('MQ $slug/$headSha', null, slug, ciValidationCheckRun); - await unlockMergeGroupChecks(slug, headSha, lock, null); + await unlockMergeQueueGuard(slug, headSha, lock); return; } @@ -709,51 +692,57 @@ class Scheduler { config, slug, headSha, - kMergeQueueLockName, + Config.kMergeQueueLockName, output: const CheckRunOutput( - title: kMergeQueueLockName, + title: Config.kMergeQueueLockName, summary: kMergeQueueLockDescription, ), ); } - /// Completes the "Merge Queue Guard" check that was scheduled using - /// [lockMergeGroupChecks] with either success or failure. + /// Completes the "Merge Queue Guard" check run. /// - /// If [exception] is null completed the check with success. Otherwise, - /// completes the check with failure. + /// If the guard is guarding a merge group, this immediately makes the merge + /// group eligible for landing onto the target branch (e.g. master), depending + /// on the success of the merge groups queued in front of this one. /// - /// Calling this method unlocks the merge group, allowing Github to either - /// merge the respective PR into the target branch (if success), or remove the - /// PR from the merge queue (if failure). - Future unlockMergeGroupChecks(RepositorySlug slug, String headSha, CheckRun lock, Object? exception) async { - if (exception == null) { - // All checks have passed. Unlocking Github with success. - log.info('All required tests passed for $headSha'); - await githubChecksService.githubChecksUtil.updateCheckRun( - config, - slug, - lock, - status: CheckRunStatus.completed, - conclusion: CheckRunConclusion.success, - ); - } else { - // Some checks failed. Unlocking Github with failure. - log.info('Some required tests failed for $headSha'); - log.warning(exception.toString()); - await githubChecksService.githubChecksUtil.updateCheckRun( - config, - slug, - lock, - status: CheckRunStatus.completed, - conclusion: CheckRunConclusion.failure, - output: CheckRunOutput( - title: kCiYamlCheckName, - summary: 'Some required tests failed for $headSha', - text: exception.toString(), - ), - ); - } + /// If the guard is guarding a pull request, this immediately makes the pull + /// request eligible for enqueuing into the merge queue. + Future unlockMergeQueueGuard(RepositorySlug slug, String headSha, CheckRun lock) async { + log.info('Unlocking Merge Queue Guard for $slug/$headSha'); + await githubChecksService.githubChecksUtil.updateCheckRun( + config, + slug, + lock, + status: CheckRunStatus.completed, + conclusion: CheckRunConclusion.success, + ); + } + + /// Fails the "Merge Queue Guard" check for a merge group. + /// + /// 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 { + log.info('Failing merge group guard for merge group $headSha in $slug'); + await githubChecksService.githubChecksUtil.updateCheckRun( + config, + slug, + lock, + status: CheckRunStatus.completed, + conclusion: CheckRunConclusion.failure, + output: CheckRunOutput( + title: Config.kCiYamlCheckName, + summary: summary, + text: details, + ), + ); } /// If [builderTriggerList] is specificed, return only builders that are contained in [presubmitTarget]. @@ -888,22 +877,29 @@ class Scheduler { return false; } - /// Process completed GitHub `check_run` to enable fusion engine builds. + /// Process a completed GitHub `check_run`. + /// + /// Handles both fusion engine build and test stages, and both pull requests + /// and merge groups. Future processCheckRunCompletion(cocoon_checks.CheckRunEvent checkRunEvent) async { - final name = checkRunEvent.checkRun?.name; - final sha = checkRunEvent.checkRun?.headSha; + final checkRun = checkRunEvent.checkRun!; + final name = checkRun.name; + final sha = checkRun.headSha; final slug = checkRunEvent.repository?.slug(); - final conclusion = checkRunEvent.checkRun?.conclusion; + final conclusion = checkRun.conclusion; if (name == null || sha == null || slug == null || conclusion == null || kCheckRunsToIgnore.contains(name)) { return true; } + final logCrumb = 'checkCompleted($name, $slug, $sha, $conclusion)'; + final isFusion = await fusionTester.isFusionBasedRef(slug, sha); if (!isFusion) { return true; } - final logCrumb = 'checkCompleted($name, $slug, $sha, $conclusion)'; + + final isMergeGroup = detectMergeGroup(checkRun); firestoreService = await config.createFirestoreService(); @@ -938,23 +934,50 @@ class Scheduler { return false; } - // Are their tests remaining? Then we shouldn't unblock guard yet. + // If an internal error happened in Cocoon, we need human assistance to + // figure out next steps. + if (stagingConclusion.result == StagingConclusionResult.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 (isMergeGroup) { + final guard = checkRunFromString(stagingConclusion.checkRunGuard!); + await failGuardForMergeGroup( + slug, + sha, + stagingConclusion.summary, + stagingConclusion.details, + guard, + ); + } + return false; + } + + // Are there tests remaining? Keep waiting. if (stagingConclusion.isPending) { log.info('$logCrumb: not progressing, remaining work count: ${stagingConclusion.remaining}'); return false; } - // Only report failure into the merge queue guard for engine build stage. - // Until https://github.com/flutter/flutter/issues/159898 is fixed, the - // merge queue guard ignores the `fusionTests` stage. - if (stage == CiStage.fusionEngineBuild && stagingConclusion.isFailed) { - await _reportCiStageFailure( - conclusion: stagingConclusion, - slug: slug, - sha: sha, - stage: stage, - logCrumb: logCrumb, - ); + 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 (isMergeGroup) { + final guard = checkRunFromString(stagingConclusion.checkRunGuard!); + await failGuardForMergeGroup( + slug, + sha, + stagingConclusion.summary, + stagingConclusion.details, + guard, + ); + } return true; } @@ -962,30 +985,41 @@ class Scheduler { // // * 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 and land it. + // * 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). switch (stage) { case CiStage.fusionEngineBuild: - return _closeSuccessfulEngineBuildStage( - checkRunEvent: checkRunEvent, + await _closeSuccessfulEngineBuildStage( + checkRun: checkRun, mergeQueueGuard: stagingConclusion.checkRunGuard!, slug: slug, sha: sha, logCrumb: logCrumb, ); case CiStage.fusionTests: - return _closeSuccessfulTestStage( + await _closeSuccessfulTestStage( mergeQueueGuard: stagingConclusion.checkRunGuard!, slug: slug, sha: sha, logCrumb: logCrumb, ); } + return true; + } + + /// Whether the [checkRunEvent] is for a merge group (rather than a pull request). + bool detectMergeGroup(cocoon_checks.CheckRun checkRun) { + final headBranch = checkRun.checkSuite?.headBranch; + if (headBranch == null) { + return false; + } + return tryParseGitHubMergeQueueBranch(headBranch).parsed; } - Future _closeSuccessfulEngineBuildStage({ - required cocoon_checks.CheckRunEvent checkRunEvent, + Future _closeSuccessfulEngineBuildStage({ + required cocoon_checks.CheckRun checkRun, required String mergeQueueGuard, required RepositorySlug slug, required String sha, @@ -994,9 +1028,7 @@ class Scheduler { // We know that we're in a fusion repo; now we need to figure out if we are // 1) in a presubmit test or // 2) in the merge queue - final headBranch = checkRunEvent.checkRun?.checkSuite?.headBranch; - final isInMergeQueue = headBranch?.startsWith('gh-readonly-queue/') ?? false; - if (isInMergeQueue) { + if (detectMergeGroup(checkRun)) { await _closeMergeQueue( mergeQueueGuard: mergeQueueGuard, slug: slug, @@ -1004,46 +1036,28 @@ class Scheduler { stage: CiStage.fusionEngineBuild, logCrumb: logCrumb, ); - return true; + return; } log.info('$logCrumb: Stage completed successfully: ${CiStage.fusionEngineBuild}'); await _proceedToCiTestingStage( - checkRunEvent: checkRunEvent, + checkRun: checkRun, mergeQueueGuard: mergeQueueGuard, slug: slug, sha: sha, logCrumb: logCrumb, ); - return true; } - Future _closeSuccessfulTestStage({ + Future _closeSuccessfulTestStage({ required String mergeQueueGuard, required RepositorySlug slug, required String sha, required String logCrumb, }) async { log.info('$logCrumb: Stage completed: ${CiStage.fusionTests}'); - - // TODO: Unlock the guarding check_run after confirming that the test stage - // document is tracking all check runs correctly. - // IMPORTANT: when moving the unlock here, REMEMBER to remove the unlock in - // _proceedToCiTestingStage for non-MQ runs. MQ should unlock the - // guard right after the build stage. - log.info(''' -Emulate: - -await unlockMergeGroupChecks( - slug = $slug, - sha = $sha, - mergeQueueGuard = $mergeQueueGuard, - null, -); -'''); - - return true; + await unlockMergeQueueGuard(slug, sha, checkRunFromString(mergeQueueGuard)); } /// Returns the presubmit targets for the fusion repo [pullRequest] that should run for the given [stage]. @@ -1071,25 +1085,11 @@ await unlockMergeGroupChecks( // Unlock the guarding check_run. final checkRunGuard = checkRunFromString(mergeQueueGuard); - await unlockMergeGroupChecks(slug, sha, checkRunGuard, null); - } - - Future _reportCiStageFailure({ - required RepositorySlug slug, - required String sha, - required StagingConclusion conclusion, - required CiStage stage, - required String logCrumb, - }) async { - log.info('$logCrumb: Stage failed: $stage with failed=${conclusion.failed}'); - - // Unlock the guarding check_run. - final checkRunGuard = checkRunFromString(conclusion.checkRunGuard!); - await unlockMergeGroupChecks(slug, sha, checkRunGuard, 'failed ${conclusion.failed} tests'); + await unlockMergeQueueGuard(slug, sha, checkRunGuard); } Future _proceedToCiTestingStage({ - required cocoon_checks.CheckRunEvent checkRunEvent, + required cocoon_checks.CheckRun checkRun, required RepositorySlug slug, required String sha, required String mergeQueueGuard, @@ -1099,8 +1099,8 @@ await unlockMergeGroupChecks( // Look up the PR in our cache first. This reduces github quota and requires less calls. PullRequest? pullRequest; - final id = checkRunEvent.checkRun!.id!; - final name = checkRunEvent.checkRun!.name!; + final id = checkRun.id!; + final name = checkRun.name!; try { pullRequest = await findPullRequestFor( firestoreService, @@ -1113,7 +1113,7 @@ await unlockMergeGroupChecks( // We'va failed to find the pull request; try a reverse look it from the check suite. if (pullRequest == null) { - final int checkSuiteId = checkRunEvent.checkRun!.checkSuite!.id!; + final int checkSuiteId = checkRun.checkSuite!.id!; pullRequest = await githubChecksService.findMatchingPullRequest(slug, sha, checkSuiteId); } @@ -1122,7 +1122,6 @@ await unlockMergeGroupChecks( throw 'No PR found matching this check_run($id, $name)'; } - Object? exception; try { // Both the author and label should be checked to make sure that no one is // attempting to get a pull request without check through. @@ -1155,18 +1154,15 @@ await unlockMergeGroupChecks( error, backtrace, ); - exception = error; + rethrow; } catch (error, backtrace) { log.warning( '$logCrumb: Exception encountered when scheduling presubmit targets for ${pullRequest.number}', error, backtrace, ); - exception = error; + rethrow; } - - // Unlock the guarding check_run. - await unlockMergeGroupChecks(slug, sha, checkRunGuard, exception); } Future _recordCurrentCiStage({ @@ -1205,7 +1201,7 @@ await unlockMergeGroupChecks( /// generated when someone clicks the re-run button from a failed build from /// the Github UI. /// - /// If the rerequested check is for [kCiYamlCheckName], all presubmit jobs are retried. + /// If the rerequested check is for [Config.kCiYamlCheckName], all presubmit jobs are retried. /// Otherwise, the specific check will be retried. /// /// Relevant APIs: @@ -1220,12 +1216,12 @@ await unlockMergeGroupChecks( log.fine('Rerun requested by GitHub user: ${checkRunEvent.sender?.login}'); final String? name = checkRunEvent.checkRun!.name; bool success = false; - if (name == kMergeQueueLockName) { + if (name == Config.kMergeQueueLockName) { final RepositorySlug slug = checkRunEvent.repository!.slug(); final int checkSuiteId = checkRunEvent.checkRun!.checkSuite!.id!; - log.fine('Requested re-run of "$kMergeQueueLockName" for $slug / $checkSuiteId - ignoring'); + log.fine('Requested re-run of "${Config.kMergeQueueLockName}" for $slug / $checkSuiteId - ignoring'); success = true; - } else if (name == kCiYamlCheckName) { + } else if (name == Config.kCiYamlCheckName) { // The CheckRunEvent.checkRun.pullRequests array is empty for this // event, so we need to find the matching pull request. final RepositorySlug slug = checkRunEvent.repository!.slug(); diff --git a/app_dart/test/model/firestore/ci_staging_test.dart b/app_dart/test/model/firestore/ci_staging_test.dart index 1659ae3821..ed5dfc0a4b 100644 --- a/app_dart/test/model/firestore/ci_staging_test.dart +++ b/app_dart/test/model/firestore/ci_staging_test.dart @@ -186,6 +186,8 @@ void main() { result: StagingConclusionResult.missing, failed: 0, checkRunGuard: null, + summary: 'Check run "test" not present in engine CI stage', + details: 'Change flutter_flutter_1234', ), ); verify(docRes.rollback(argThat(predicate((RollbackRequest t) => t.transaction == kTransaction)), kDatabase)) @@ -208,6 +210,7 @@ void main() { (_) async => Document( name: expectedName, fields: { + CiStaging.kTotalField: Value(integerValue: '1'), CiStaging.kRemainingField: Value(integerValue: '1'), CiStaging.kFailedField: Value(integerValue: '0'), CiStaging.kCheckRunGuardField: Value(stringValue: '{}'), @@ -234,7 +237,7 @@ void main() { predicate((CommitRequest t) { return t.transaction == kTransaction && t.writes!.length == 1 && - t.writes!.first.update!.fields!.length == 4 && + t.writes!.first.update!.fields!.length == 5 && t.writes!.first.update!.fields!['Linux build_test']!.stringValue == 'mulligan' && t.writes!.first.update!.fields![CiStaging.kRemainingField]!.integerValue == '0'; }), @@ -263,6 +266,7 @@ void main() { fields: { CiStaging.kRemainingField: Value(integerValue: '1'), CiStaging.kFailedField: Value(integerValue: '0'), + CiStaging.kTotalField: Value(integerValue: '1'), CiStaging.kCheckRunGuardField: Value(stringValue: '{}'), 'Linux build_test': Value(stringValue: CiStaging.kScheduledValue), }, @@ -283,7 +287,19 @@ void main() { final result = await future; expect( result, - const StagingConclusion(remaining: 0, result: StagingConclusionResult.ok, failed: 0, checkRunGuard: '{}'), + const StagingConclusion( + remaining: 0, + result: StagingConclusionResult.ok, + failed: 0, + checkRunGuard: '{}', + summary: 'All tests passed', + details: ''' +For CI stage engine: + Total check runs scheduled: 1 + Pending: 0 + Failed: 0 +''', + ), ); verify( docRes.commit( @@ -291,7 +307,7 @@ void main() { predicate((CommitRequest t) { return t.transaction == kTransaction && t.writes!.length == 1 && - t.writes!.first.update!.fields!.length == 4 && + t.writes!.first.update!.fields!.length == 5 && t.writes!.first.update!.fields!['Linux build_test']!.stringValue == 'mulligan' && t.writes!.first.update!.fields![CiStaging.kRemainingField]!.integerValue == '0'; }), @@ -320,6 +336,7 @@ void main() { fields: { CiStaging.kRemainingField: Value(integerValue: '1'), CiStaging.kFailedField: Value(integerValue: '0'), + CiStaging.kTotalField: Value(integerValue: '1'), CiStaging.kCheckRunGuardField: Value(stringValue: '{}'), 'MacOS build_test': Value(stringValue: CiStaging.kSuccessValue), }, @@ -345,6 +362,8 @@ void main() { result: StagingConclusionResult.internalError, failed: 0, checkRunGuard: '{}', + summary: 'Not a valid state transition for MacOS build_test', + details: 'Attempted to transition the state of check run MacOS build_test from "success" to "mulligan".', ), ); verify( @@ -353,7 +372,7 @@ void main() { predicate((CommitRequest t) { return t.transaction == kTransaction && t.writes!.length == 1 && - t.writes!.first.update!.fields!.length == 4 && + t.writes!.first.update!.fields!.length == 5 && t.writes!.first.update!.fields!['MacOS build_test']!.stringValue == 'mulligan' && t.writes!.first.update!.fields![CiStaging.kRemainingField]!.integerValue == '1'; }), @@ -382,6 +401,7 @@ void main() { fields: { CiStaging.kRemainingField: Value(integerValue: '1'), CiStaging.kFailedField: Value(integerValue: '1'), + CiStaging.kTotalField: Value(integerValue: '1'), CiStaging.kCheckRunGuardField: Value(stringValue: '{}'), 'MacOS build_test': Value(stringValue: CiStaging.kFailureValue), }, @@ -403,7 +423,19 @@ void main() { // Remaining == 1 because our test was already concluded. expect( result, - const StagingConclusion(remaining: 1, result: StagingConclusionResult.ok, failed: 0, checkRunGuard: '{}'), + const StagingConclusion( + remaining: 1, + result: StagingConclusionResult.ok, + failed: 0, + checkRunGuard: '{}', + summary: 'All tests passed', + details: ''' +For CI stage engine: + Total check runs scheduled: 1 + Pending: 1 + Failed: 0 +''', + ), ); verify( docRes.commit( @@ -411,7 +443,7 @@ void main() { predicate((CommitRequest t) { return t.transaction == kTransaction && t.writes!.length == 1 && - t.writes!.first.update!.fields!.length == 4 && + t.writes!.first.update!.fields!.length == 5 && t.writes!.first.update!.fields!['MacOS build_test']!.stringValue == CiStaging.kSuccessValue && t.writes!.first.update!.fields![CiStaging.kRemainingField]!.integerValue == '1' && t.writes!.first.update!.fields![CiStaging.kFailedField]!.integerValue == '0'; @@ -441,6 +473,7 @@ void main() { fields: { CiStaging.kRemainingField: Value(integerValue: '1'), CiStaging.kFailedField: Value(integerValue: '1'), + CiStaging.kTotalField: Value(integerValue: '1'), CiStaging.kCheckRunGuardField: Value(stringValue: '{}'), 'MacOS build_test': Value(stringValue: CiStaging.kFailureValue), }, @@ -466,6 +499,8 @@ void main() { result: StagingConclusionResult.internalError, failed: 1, checkRunGuard: '{}', + summary: 'Not a valid state transition for MacOS build_test', + details: 'Attempted to transition the state of check run MacOS build_test from "failure" to "failure".', ), ); verify( @@ -474,7 +509,7 @@ void main() { predicate((CommitRequest t) { return t.transaction == kTransaction && t.writes!.length == 1 && - t.writes!.first.update!.fields!.length == 4 && + t.writes!.first.update!.fields!.length == 5 && t.writes!.first.update!.fields!['MacOS build_test']!.stringValue == CiStaging.kFailureValue && t.writes!.first.update!.fields![CiStaging.kRemainingField]!.integerValue == '1' && t.writes!.first.update!.fields![CiStaging.kFailedField]!.integerValue == '1'; @@ -504,6 +539,7 @@ void main() { fields: { CiStaging.kRemainingField: Value(integerValue: '1'), CiStaging.kFailedField: Value(integerValue: '0'), + CiStaging.kTotalField: Value(integerValue: '1'), CiStaging.kCheckRunGuardField: Value(stringValue: '{}'), 'MacOS build_test': Value(stringValue: CiStaging.kSuccessValue), }, @@ -524,7 +560,19 @@ void main() { final result = await future; expect( result, - const StagingConclusion(remaining: 1, result: StagingConclusionResult.ok, failed: 1, checkRunGuard: '{}'), + const StagingConclusion( + remaining: 1, + result: StagingConclusionResult.ok, + failed: 1, + checkRunGuard: '{}', + summary: 'All tests passed', + details: ''' +For CI stage engine: + Total check runs scheduled: 1 + Pending: 1 + Failed: 1 +''', + ), ); verify( docRes.commit( @@ -532,7 +580,7 @@ void main() { predicate((CommitRequest t) { return t.transaction == kTransaction && t.writes!.length == 1 && - t.writes!.first.update!.fields!.length == 4 && + t.writes!.first.update!.fields!.length == 5 && t.writes!.first.update!.fields!['MacOS build_test']!.stringValue == CiStaging.kFailureValue && t.writes!.first.update!.fields![CiStaging.kRemainingField]!.integerValue == '1' && t.writes!.first.update!.fields![CiStaging.kFailedField]!.integerValue == '1'; diff --git a/app_dart/test/request_handlers/github/webhook_subscription_test.dart b/app_dart/test/request_handlers/github/webhook_subscription_test.dart index 760f091b21..21a5727cd1 100644 --- a/app_dart/test/request_handlers/github/webhook_subscription_test.dart +++ b/app_dart/test/request_handlers/github/webhook_subscription_test.dart @@ -2693,7 +2693,7 @@ void foo() { 'triggerMergeGroupTargets(flutter/flutter, c9affbbb12aa40cb3afbe94b9ea6b119a256bebf, simulated): Scheduling merge group checks', 'Updating ci.yaml validation check for MQ flutter/flutter/c9affbbb12aa40cb3afbe94b9ea6b119a256bebf', 'ci.yaml validation check was successful for MQ flutter/flutter/c9affbbb12aa40cb3afbe94b9ea6b119a256bebf', - 'All required tests passed for c9affbbb12aa40cb3afbe94b9ea6b119a256bebf', + 'Unlocking Merge Queue Guard for flutter/flutter/c9affbbb12aa40cb3afbe94b9ea6b119a256bebf', ], ); }); diff --git a/app_dart/test/service/scheduler_test.dart b/app_dart/test/service/scheduler_test.dart index e615b8ecbb..4645a45ed7 100644 --- a/app_dart/test/service/scheduler_test.dart +++ b/app_dart/test/service/scheduler_test.dart @@ -184,7 +184,12 @@ void main() { mockGithubChecksUtil = MockGithubChecksUtil(); // Generate check runs based on the name hash code when(mockGithubChecksUtil.createCheckRun(any, any, any, any, output: anyNamed('output'))) - .thenAnswer((Invocation invocation) async => generateCheckRun(invocation.positionalArguments[2].hashCode)); + .thenAnswer((Invocation invocation) async { + return generateCheckRun( + invocation.positionalArguments[2].hashCode, + name: invocation.positionalArguments[3], + ); + }); fakeFusion = FakeFusionTester(); callbacks = MockCallbacks(); @@ -726,12 +731,12 @@ targets: return CheckRun.fromJson(const { 'id': 1, 'started_at': '2020-05-10T02:49:31Z', - 'name': Scheduler.kCiYamlCheckName, + 'name': Config.kCiYamlCheckName, 'check_suite': {'id': 2}, }); }); final Map checkRunEventJson = jsonDecode(checkRunString) as Map; - checkRunEventJson['check_run']['name'] = Scheduler.kCiYamlCheckName; + checkRunEventJson['check_run']['name'] = Config.kCiYamlCheckName; final cocoon_checks.CheckRunEvent checkRunEvent = cocoon_checks.CheckRunEvent.fromJson(checkRunEventJson); expect(await scheduler.processCheckRun(checkRunEvent), true); verify( @@ -739,7 +744,7 @@ targets: any, any, any, - Scheduler.kCiYamlCheckName, + Config.kCiYamlCheckName, output: anyNamed('output'), ), ); @@ -791,12 +796,12 @@ targets: return CheckRun.fromJson(const { 'id': 1, 'started_at': '2020-05-10T02:49:31Z', - 'name': Scheduler.kCiYamlCheckName, + 'name': Config.kCiYamlCheckName, 'check_suite': {'id': 2}, }); }); final Map checkRunEventJson = jsonDecode(checkRunString) as Map; - checkRunEventJson['check_run']['name'] = Scheduler.kMergeQueueLockName; + checkRunEventJson['check_run']['name'] = Config.kMergeQueueLockName; final cocoon_checks.CheckRunEvent checkRunEvent = cocoon_checks.CheckRunEvent.fromJson(checkRunEventJson); expect(await scheduler.processCheckRun(checkRunEvent), true); verifyNever( @@ -804,7 +809,7 @@ targets: any, any, any, - Scheduler.kMergeQueueLockName, + Config.kMergeQueueLockName, output: anyNamed('output'), ), ); @@ -1005,6 +1010,8 @@ targets: remaining: 1, checkRunGuard: '{}', failed: 0, + summary: 'Field missing', + details: 'Some details', ); }); @@ -1047,6 +1054,8 @@ targets: remaining: 1, checkRunGuard: '{}', failed: 0, + summary: 'Internal error', + details: 'Some details', ); }); @@ -1097,6 +1106,8 @@ targets: remaining: 1, checkRunGuard: '{}', failed: 0, + summary: 'OK', + details: 'Some details', ); }); @@ -1131,7 +1142,11 @@ targets: ); }); - test('failed tests unlocks but does not schedule more', () async { + // The merge guard is not closed until both engine build and tests + // complete and are successful. + // This behavior is explained here: + // https://github.com/flutter/flutter/issues/159898#issuecomment-2597209435 + test('failed tests neither unlock merge queue guard nor schedule test stage', () async { when( callbacks.markCheckRunConclusion( firestoreService: anyNamed('firestoreService'), @@ -1147,6 +1162,8 @@ targets: remaining: 0, checkRunGuard: checkRunFor(name: 'GUARD TEST'), failed: 1, + summary: 'OK', + details: 'Some details', ); }); @@ -1169,21 +1186,16 @@ targets: ), ).called(1); - verify( + verifyNever( mockGithubChecksUtil.updateCheckRun( any, - argThat(equals(RepositorySlug('flutter', 'flutter'))), - argThat( - predicate((arg) { - expect(arg.name, 'GUARD TEST'); - return true; - }), - ), - status: argThat(equals(CheckRunStatus.completed), named: 'status'), - conclusion: argThat(equals(CheckRunConclusion.failure), named: 'conclusion'), + any, + any, + status: anyNamed('status'), + conclusion: anyNamed('conclusion'), output: anyNamed('output'), ), - ).called(1); + ); }); test('schedules tests after engine stage', () async { @@ -1264,6 +1276,8 @@ targets: remaining: 0, checkRunGuard: checkRunFor(name: 'GUARD TEST'), failed: 0, + summary: 'OK', + details: 'Some details', ); }); @@ -1289,21 +1303,16 @@ targets: ), ).called(1); - verify( + verifyNever( mockGithubChecksUtil.updateCheckRun( any, - argThat(equals(RepositorySlug('flutter', 'flutter'))), - argThat( - predicate((arg) { - expect(arg.name, 'GUARD TEST'); - return true; - }), - ), - status: argThat(equals(CheckRunStatus.completed), named: 'status'), - conclusion: argThat(equals(CheckRunConclusion.success), named: 'conclusion'), + any, + any, + status: anyNamed('status'), + conclusion: anyNamed('conclusion'), output: anyNamed('output'), ), - ).called(1); + ); final result = verify( luci.scheduleTryBuilds( @@ -1323,39 +1332,11 @@ targets: test('tracks test check runs in firestore', () async { final githubService = config.githubService = MockGithubService(); final githubClient = MockGitHub(); - when(githubService.github).thenReturn(githubClient); - when(githubService.searchIssuesAndPRs(any, any, sort: anyNamed('sort'), pages: anyNamed('pages'))) - .thenAnswer((_) async => [generateIssue(42)]); - - final pullRequest = generatePullRequest(); - when(githubService.getPullRequest(any, any)).thenAnswer((_) async => pullRequest); - when(githubService.listFiles(any)).thenAnswer((_) async => ['abc/def']); - when(mockGithubChecksUtil.listCheckSuitesForRef(any, any, ref: anyNamed('ref'))).thenAnswer( - (_) async => [ - // From check_run.check_suite.id in [checkRunString]. - generateCheckSuite(668083231), - ], - ); - - httpClient = MockClient((http.Request request) async { - if (request.url.path.endsWith('engine/src/flutter/.ci.yaml')) { - return http.Response(fusionCiYaml, 200); - } else if (request.url.path.endsWith('.ci.yaml')) { - return http.Response(singleCiYaml, 200); - } - throw Exception('Failed to find ${request.url.path}'); - }); final luci = MockLuciBuildService(); - when(luci.scheduleTryBuilds(targets: anyNamed('targets'), pullRequest: anyNamed('pullRequest'))) - .thenAnswer((inv) async { - return []; - }); - final gitHubChecksService = MockGithubChecksService(); + + when(githubService.github).thenReturn(githubClient); when(gitHubChecksService.githubChecksUtil).thenReturn(mockGithubChecksUtil); - when(gitHubChecksService.findMatchingPullRequest(any, any, any)).thenAnswer((inv) async { - return pullRequest; - }); scheduler = Scheduler( cache: cache, @@ -1388,6 +1369,8 @@ targets: remaining: 0, checkRunGuard: checkRunFor(name: 'GUARD TEST'), failed: 0, + summary: 'Field missing or OK', + details: 'Some details', ); }); @@ -1425,44 +1408,34 @@ targets: conclusion: argThat(equals('success'), named: 'conclusion'), ), ).called(1); + + // Because tests completed, and completed successfully, the guard is + // unlocked, allowing the PR to land. + verify( + mockGithubChecksUtil.updateCheckRun( + any, + argThat(equals(RepositorySlug('flutter', 'flutter'))), + argThat( + predicate((arg) { + expect(arg.name, 'GUARD TEST'); + return true; + }), + ), + status: argThat(equals(CheckRunStatus.completed), named: 'status'), + conclusion: argThat(equals(CheckRunConclusion.success), named: 'conclusion'), + output: anyNamed('output'), + ), + ).called(1); }); test('does not fail the merge queue guard when a test check run fails', () async { final githubService = config.githubService = MockGithubService(); final githubClient = MockGitHub(); - when(githubService.github).thenReturn(githubClient); - when(githubService.searchIssuesAndPRs(any, any, sort: anyNamed('sort'), pages: anyNamed('pages'))) - .thenAnswer((_) async => [generateIssue(42)]); - - final pullRequest = generatePullRequest(); - when(githubService.getPullRequest(any, any)).thenAnswer((_) async => pullRequest); - when(githubService.listFiles(any)).thenAnswer((_) async => ['abc/def']); - when(mockGithubChecksUtil.listCheckSuitesForRef(any, any, ref: anyNamed('ref'))).thenAnswer( - (_) async => [ - // From check_run.check_suite.id in [checkRunString]. - generateCheckSuite(668083231), - ], - ); - - httpClient = MockClient((http.Request request) async { - if (request.url.path.endsWith('engine/src/flutter/.ci.yaml')) { - return http.Response(fusionCiYaml, 200); - } else if (request.url.path.endsWith('.ci.yaml')) { - return http.Response(singleCiYaml, 200); - } - throw Exception('Failed to find ${request.url.path}'); - }); final luci = MockLuciBuildService(); - when(luci.scheduleTryBuilds(targets: anyNamed('targets'), pullRequest: anyNamed('pullRequest'))) - .thenAnswer((inv) async { - return []; - }); - final gitHubChecksService = MockGithubChecksService(); + + when(githubService.github).thenReturn(githubClient); when(gitHubChecksService.githubChecksUtil).thenReturn(mockGithubChecksUtil); - when(gitHubChecksService.findMatchingPullRequest(any, any, any)).thenAnswer((inv) async { - return pullRequest; - }); scheduler = Scheduler( cache: cache, @@ -1498,6 +1471,8 @@ targets: CiStage.fusionEngineBuild => 0, CiStage.fusionTests => 1, }, + summary: 'Field missing or OK', + details: 'Some details', ); }); @@ -1536,9 +1511,8 @@ targets: ), ).called(1); - // Only report failure into the merge queue guard for engine build stage. - // Until https://github.com/flutter/flutter/issues/159898 is fixed, the - // merge queue guard ignores the `fusionTests` stage. + // The test stage completed, but with failures. The merge queue + // guard should stay open to prevent the pull request from landing. verifyNever( mockGithubChecksUtil.updateCheckRun( any, @@ -1631,6 +1605,8 @@ targets: remaining: 0, checkRunGuard: checkRunFor(name: 'GUARD TEST'), failed: 0, + summary: 'OK', + details: 'Some details', ); }); @@ -1657,21 +1633,16 @@ targets: ), ).called(1); - verify( + verifyNever( mockGithubChecksUtil.updateCheckRun( any, - argThat(equals(RepositorySlug('flutter', 'flutter'))), - argThat( - predicate((arg) { - expect(arg.name, 'GUARD TEST'); - return true; - }), - ), - status: argThat(equals(CheckRunStatus.completed), named: 'status'), - conclusion: argThat(equals(CheckRunConclusion.success), named: 'conclusion'), + any, + any, + status: anyNamed('status'), + conclusion: anyNamed('conclusion'), output: anyNamed('output'), ), - ).called(1); + ); final result = verify( luci.scheduleTryBuilds( @@ -1883,14 +1854,14 @@ targets: verify(mockGithubChecksUtil.createCheckRun(any, any, any, captureAny, output: captureAnyNamed('output'))) .captured, [ - Scheduler.kMergeQueueLockName, + Config.kMergeQueueLockName, const CheckRunOutput( - title: Scheduler.kMergeQueueLockName, + title: Config.kMergeQueueLockName, summary: Scheduler.kMergeQueueLockDescription, ), - Scheduler.kCiYamlCheckName, + Config.kCiYamlCheckName, const CheckRunOutput( - title: Scheduler.kCiYamlCheckName, + title: Config.kCiYamlCheckName, summary: 'If this check is stuck pending, push an empty commit to retrigger the checks', ), 'Linux A', @@ -1912,15 +1883,15 @@ targets: verify(mockGithubChecksUtil.createCheckRun(any, any, any, captureAny, output: captureAnyNamed('output'))) .captured, [ - Scheduler.kMergeQueueLockName, + Config.kMergeQueueLockName, const CheckRunOutput( - title: Scheduler.kMergeQueueLockName, + title: Config.kMergeQueueLockName, summary: Scheduler.kMergeQueueLockDescription, ), - Scheduler.kCiYamlCheckName, + Config.kCiYamlCheckName, // No other targets should be created. const CheckRunOutput( - title: Scheduler.kCiYamlCheckName, + title: Config.kCiYamlCheckName, summary: 'If this check is stuck pending, push an empty commit to retrigger the checks', ), ], @@ -2057,14 +2028,14 @@ targets: verify(mockGithubChecksUtil.createCheckRun(any, any, any, captureAny, output: captureAnyNamed('output'))) .captured, [ - Scheduler.kMergeQueueLockName, + Config.kMergeQueueLockName, const CheckRunOutput( - title: Scheduler.kMergeQueueLockName, + title: Config.kMergeQueueLockName, summary: Scheduler.kMergeQueueLockDescription, ), - Scheduler.kCiYamlCheckName, + Config.kCiYamlCheckName, const CheckRunOutput( - title: Scheduler.kCiYamlCheckName, + title: Config.kCiYamlCheckName, summary: 'If this check is stuck pending, push an empty commit to retrigger the checks', ), 'Linux A', @@ -2085,14 +2056,14 @@ targets: verify(mockGithubChecksUtil.createCheckRun(any, any, any, captureAny, output: captureAnyNamed('output'))) .captured, [ - Scheduler.kMergeQueueLockName, + Config.kMergeQueueLockName, const CheckRunOutput( - title: Scheduler.kMergeQueueLockName, + title: Config.kMergeQueueLockName, summary: Scheduler.kMergeQueueLockDescription, ), - Scheduler.kCiYamlCheckName, + Config.kCiYamlCheckName, const CheckRunOutput( - title: Scheduler.kCiYamlCheckName, + title: Config.kCiYamlCheckName, summary: 'If this check is stuck pending, push an empty commit to retrigger the checks', ), 'Linux A', @@ -2134,23 +2105,44 @@ targets: } throw Exception('Failed to find ${request.url.path}'); }); + + final capturedUpdates = <(String, CheckRunStatus, CheckRunConclusion)>[]; + + when( + mockGithubChecksUtil.updateCheckRun( + any, + any, + any, + status: anyNamed('status'), + conclusion: anyNamed('conclusion'), + output: anyNamed('output'), + ), + ).thenAnswer((inv) async { + final CheckRun checkRun = inv.positionalArguments[2]; + capturedUpdates.add( + ( + checkRun.name!, + inv.namedArguments[#status], + inv.namedArguments[#conclusion], + ), + ); + }); + await scheduler.triggerPresubmitTargets(pullRequest: pullRequest); + expect( - verify( - mockGithubChecksUtil.updateCheckRun( - any, - any, - any, - status: captureAnyNamed('status'), - conclusion: captureAnyNamed('conclusion'), - output: anyNamed('output'), + capturedUpdates, + <(String, CheckRunStatus, CheckRunConclusion)>[ + ( + 'ci.yaml validation', + CheckRunStatus.completed, + CheckRunConclusion.failure, + ), + ( + 'Merge Queue Guard', + CheckRunStatus.completed, + CheckRunConclusion.success, ), - ).captured, - [ - CheckRunStatus.completed, - CheckRunConclusion.failure, - CheckRunStatus.completed, - CheckRunConclusion.failure, ], ); }); @@ -2173,7 +2165,7 @@ targets: CheckRunStatus.completed, CheckRunConclusion.failure, CheckRunStatus.completed, - CheckRunConclusion.failure, + CheckRunConclusion.success, ], ); }); diff --git a/app_dart/test/src/datastore/fake_config.dart b/app_dart/test/src/datastore/fake_config.dart index aa9c12d66f..96a37c5044 100644 --- a/app_dart/test/src/datastore/fake_config.dart +++ b/app_dart/test/src/datastore/fake_config.dart @@ -101,7 +101,6 @@ class FakeConfig implements Config { String? flutterGoldDraftChangeValue; String? flutterGoldStalePRValue; List? supportedBranchesValue; - String? overrideTreeStatusLabelValue; Set? supportedReposValue; Set? postsubmitSupportedReposValue; Duration? githubRequestDelayValue; @@ -261,9 +260,6 @@ class FakeConfig implements Config { @override Future createDefaultGitHubService() async => githubService!; - @override - Future get overrideTreeStatusLabel async => overrideTreeStatusLabelValue!; - @override String get defaultRecipeBundleRef => 'refs/heads/main'; diff --git a/auto_submit/lib/requests/check_pull_request.dart b/auto_submit/lib/requests/check_pull_request.dart index ecfd92b1ee..9d82b7ca32 100644 --- a/auto_submit/lib/requests/check_pull_request.dart +++ b/auto_submit/lib/requests/check_pull_request.dart @@ -56,8 +56,6 @@ class CheckPullRequest extends CheckRequest { log.info('Processing ${messageList.length} messages'); - final PullRequestValidationService validationService = PullRequestValidationService(config); - final List> futures = >[]; for (pub.ReceivedMessage message in messageList) { @@ -92,6 +90,7 @@ class CheckPullRequest extends CheckRequest { processingLog.add(pullRequest.number!); } + final PullRequestValidationService validationService = PullRequestValidationService(config); futures.add( validationService.processMessage(pullRequest, message.ackId!, pubsub), ); diff --git a/auto_submit/lib/service/config.dart b/auto_submit/lib/service/config.dart index 2c4225a621..466533a0ce 100644 --- a/auto_submit/lib/service/config.dart +++ b/auto_submit/lib/service/config.dart @@ -59,11 +59,37 @@ class Config { static const String kFlutterGitHubBotKey = 'AUTO_SUBMIT_FLUTTER_GITHUB_TOKEN'; static const String kTreeStatusDiscordUrl = 'TREE_STATUS_DISCORD_WEBHOOK_URL'; - /// Labels autosubmit looks for on pull requests + /// When present on a pull request, instructs Cocoon to submit it + /// automatically as soon as all the required checks pass. /// /// Keep this in sync with the similar `Config` class in `app_dart`. static const String kAutosubmitLabel = 'autosubmit'; + /// When present on a pull request, allows it to land without passing all the + /// checks, and jumps the merge queue. + /// + /// Keep this in sync with the similar `Config` class in `app_dart`. + static const String kEmergencyLabel = 'emergency'; + + /// Validates that CI tasks were successfully created from the .ci.yaml file. + /// + /// If this check fails, it means Cocoon failed to fully populate the list of + /// CI checks and the PR/commit should be treated as failing. + static const String kCiYamlCheckName = 'ci.yaml validation'; + + /// A required check that stays in pending state until a sufficient subset of + /// checks pass. + /// + /// This check is "required", meaning that it must pass before Github will + /// allow a PR to land in the merge queue, or a merge group to land on the + /// target branch (main or master). + /// + /// IMPORTANT: the name of this task - "Merge Queue Guard" - must strictly + /// match the name of the required check configured in the repo settings. + /// Changing the name here or in the settings alone will break the PR + /// workflow. + static const String kMergeQueueLockName = 'Merge Queue Guard'; + /// GitHub check stale threshold. static const int kGitHubCheckStaleThreshold = 2; // hours @@ -82,9 +108,6 @@ class Config { /// as a revert. static const String kRevertOfLabel = 'revert of'; - /// The label which shows the overrideTree Status. - String get overrideTreeStatusLabel => 'warning: land on red to fix tree breakage'; - /// Repository Slug data /// GitHub repositories that use CI status to determine if pull requests can be submitted. static Set reposWithTreeStatus = { diff --git a/auto_submit/lib/service/github_service.dart b/auto_submit/lib/service/github_service.dart index ca6d0247fd..64b1251028 100644 --- a/auto_submit/lib/service/github_service.dart +++ b/auto_submit/lib/service/github_service.dart @@ -43,6 +43,34 @@ class GithubService { .toList(); } + Future updateCheckRun({ + required RepositorySlug slug, + required CheckRun checkRun, + String? name, + String? detailsUrl, + String? externalId, + DateTime? startedAt, + CheckRunStatus status = CheckRunStatus.queued, + CheckRunConclusion? conclusion, + DateTime? completedAt, + CheckRunOutput? output, + List? actions, + }) async { + return github.checks.checkRuns.updateCheckRun( + slug, + checkRun, + name: name, + detailsUrl: detailsUrl, + externalId: externalId, + startedAt: startedAt, + status: status, + conclusion: conclusion, + completedAt: completedAt, + output: output, + actions: actions, + ); + } + /// Fetches the specified commit. Future getCommit( RepositorySlug slug, diff --git a/auto_submit/lib/service/process_method.dart b/auto_submit/lib/service/process_method.dart index 3a7eea63fc..bf7e8aa583 100644 --- a/auto_submit/lib/service/process_method.dart +++ b/auto_submit/lib/service/process_method.dart @@ -6,6 +6,7 @@ /// found. enum ProcessMethod { processAutosubmit, + processEmergency, processRevert, doNotProcess, } diff --git a/auto_submit/lib/service/pull_request_validation_service.dart b/auto_submit/lib/service/pull_request_validation_service.dart index b4677d0de4..30b2dbb371 100644 --- a/auto_submit/lib/service/pull_request_validation_service.dart +++ b/auto_submit/lib/service/pull_request_validation_service.dart @@ -28,11 +28,13 @@ class PullRequestValidationService extends ValidationService { /// Processes a pub/sub message associated with PullRequest event. Future processMessage(github.PullRequest messagePullRequest, String ackId, PubSub pubsub) async { - if (await shouldProcess(messagePullRequest)) { + final slug = messagePullRequest.base!.repo!.slug(); + final fullPullRequest = await getFullPullRequest(slug, messagePullRequest.number!); + if (shouldProcess(fullPullRequest)) { await processPullRequest( config: config, result: await getNewestPullRequestInfo(config, messagePullRequest), - messagePullRequest: messagePullRequest, + pullRequest: fullPullRequest, ackId: ackId, pubsub: pubsub, ); @@ -42,9 +44,11 @@ class PullRequestValidationService extends ValidationService { } } - Future shouldProcess(github.PullRequest pullRequest) async { - final (currentPullRequest, labelNames) = await getPrWithLabels(pullRequest); - return (currentPullRequest.state == 'open' && labelNames.contains(Config.kAutosubmitLabel)); + bool shouldProcess(github.PullRequest pullRequest) { + final labelNames = pullRequest.labelNames; + final containsLabelsNeedingValidation = + labelNames.contains(Config.kAutosubmitLabel) || labelNames.contains(Config.kEmergencyLabel); + return pullRequest.state == 'open' && containsLabelsNeedingValidation; } /// Processes a PullRequest running several validations to decide whether to @@ -52,16 +56,16 @@ class PullRequestValidationService extends ValidationService { Future processPullRequest({ required Config config, required QueryResult result, - required github.PullRequest messagePullRequest, + required github.PullRequest pullRequest, required String ackId, required PubSub pubsub, }) async { - final github.RepositorySlug slug = messagePullRequest.base!.repo!.slug(); - final int prNumber = messagePullRequest.number!; + final github.RepositorySlug slug = pullRequest.base!.repo!.slug(); + final int prNumber = pullRequest.number!; // If a pull request is currently in the merge queue do not touch it. Let // the merge queue merge it, or kick it out of the merge queue. - if (messagePullRequest.isMergeQueueEnabled) { + if (pullRequest.isMergeQueueEnabled) { if (result.repository!.pullRequest!.isInMergeQueue) { log.info( '${slug.fullName}/$prNumber is already in the merge queue. Skipping.', @@ -71,22 +75,91 @@ class PullRequestValidationService extends ValidationService { } } + final processor = _PullRequestValidationProcessor( + validationService: this, + githubService: await config.createGithubService(slug), + config: config, + result: result, + pullRequest: pullRequest, + ackId: ackId, + pubsub: pubsub, + ); + await processor.process(); + } +} + +/// A helper class that breaks down the logic into multiple smaller methods, and +/// provides common objects to all methods that need them. +class _PullRequestValidationProcessor { + _PullRequestValidationProcessor({ + required this.validationService, + required this.githubService, + required this.config, + required this.result, + required this.pullRequest, + required this.ackId, + required this.pubsub, + }) : slug = pullRequest.base!.repo!.slug(), + prNumber = pullRequest.number!; + + final PullRequestValidationService validationService; + final GithubService githubService; + final Config config; + final QueryResult result; + final github.PullRequest pullRequest; + final String ackId; + final PubSub pubsub; + final github.RepositorySlug slug; + final int prNumber; + + Future process() async { + final hasEmergencyLabel = pullRequest.labelNames.contains(Config.kEmergencyLabel); + final hasAutosubmitLabel = pullRequest.labelNames.contains(Config.kAutosubmitLabel); + + if (hasEmergencyLabel) { + final didEmergencyProcessCleanly = await _processEmergency(); + if (!didEmergencyProcessCleanly) { + // The emergency label failed to process cleanly. Do not continue processing + // the "autosubmit" label, as it may not be safe. The author assumed that + // the combination of both labels would cleanly land, and it didn't. + if (hasAutosubmitLabel) { + await _removeAutosubmitLabel('emergency label processing failed'); + } + await pubsub.acknowledge('auto-submit-queue-sub', ackId); + return; + } + } + + if (hasAutosubmitLabel) { + await _processAutosubmit(); + } else { + log.info('Ack the processed message : $ackId.'); + await pubsub.acknowledge('auto-submit-queue-sub', ackId); + } + } + + /// Processes the "emergency" label. + /// + /// Returns true, if the processing succeeded and the validation should move + /// onto the "autosubmit" label. The primary result of a successful processing + /// of the "emergency" label is the unlocking of the "Merge Queue Guard" check + /// run, which allows the respective PR to be enqueued either manually using + /// GitHub UI, or via the "autosubmit" label. + /// + /// Returns false, if the processing failed, the "Merge Queue Guard" was not + /// unlocked, and any further validation should stop. + Future _processEmergency() async { final RepositoryConfiguration repositoryConfiguration = await config.getRepositoryConfiguration(slug); // filter out validations here final ValidationFilter validationFilter = ValidationFilter( config: config, - processMethod: ProcessMethod.processAutosubmit, + processMethod: ProcessMethod.processEmergency, repositoryConfiguration: repositoryConfiguration, ); final Set validations = validationFilter.getValidations(); final Map validationsMap = {}; - final GithubService githubService = await config.createGithubService(slug); - - // get the labels before validation so that we can detect all labels. - // TODO (https://github.com/flutter/flutter/issues/132811) remove this after graphql is removed. - final github.PullRequest updatedPullRequest = await githubService.getPullRequest(slug, messagePullRequest.number!); /// Runs all the validation defined in the service. /// If the runCi flag is false then we need a way to not run the ciSuccessful validation. @@ -94,24 +167,102 @@ class PullRequestValidationService extends ValidationService { log.info('${slug.fullName}/$prNumber running validation ${validation.name}'); final ValidationResult validationResult = await validation.validate( result, - updatedPullRequest, + pullRequest, ); validationsMap[validation.name] = validationResult; } /// If there is at least one action that requires to remove label do so and add comments for all the failures. bool shouldReturn = false; - for (MapEntry result in validationsMap.entries) { - if (!result.value.result && result.value.action == Action.REMOVE_LABEL) { - final String commmentMessage = result.value.message.isEmpty ? 'Validations Fail.' : result.value.message; - final String message = 'auto label is removed for ${slug.fullName}/$prNumber, due to $commmentMessage'; - await githubService.removeLabel(slug, prNumber, Config.kAutosubmitLabel); + for (final MapEntry(key: _, :value) in validationsMap.entries) { + if (!value.result && value.action == Action.REMOVE_LABEL) { + final String commmentMessage = value.message.isEmpty ? 'Validations Fail.' : value.message; + final String message = + '${Config.kEmergencyLabel} label is removed for ${slug.fullName}/$prNumber, due to $commmentMessage'; + await githubService.removeLabel(slug, prNumber, Config.kEmergencyLabel); await githubService.createComment(slug, prNumber, message); log.info(message); shouldReturn = true; } } + if (shouldReturn) { + log.info('The pr ${slug.fullName}/$prNumber is not eligible for emergency landing.'); + return false; + } + + // If PR has some failures to ignore temporarily do nothing and continue. + for (final MapEntry(:key, :value) in validationsMap.entries) { + if (!value.result && value.action == Action.IGNORE_TEMPORARILY) { + log.info( + 'Temporarily ignoring processing of ${slug.fullName}/$prNumber due to $key failing validation.', + ); + return true; + } + } + + // At this point all validations passed, and the PR can proceed to landing + // as an emergency. + final guard = (await githubService.getCheckRunsFiltered( + slug: slug, + ref: pullRequest.base!.ref!, + checkName: Config.kMergeQueueLockName, + )) + .singleOrNull; + + if (guard == null) { + log.severe( + 'Failed to process the emergency label in ${slug.fullName}/$prNumber. ' + '"kMergeQueueLockName" check run is missing.', + ); + return false; + } + + await githubService.updateCheckRun( + slug: slug, + checkRun: guard, + status: github.CheckRunStatus.completed, + conclusion: github.CheckRunConclusion.success, + ); + + log.info('Unlocked merge guard for ${slug.fullName}/$prNumber to allow it to land as an emergency.'); + return true; + } + + Future _processAutosubmit() async { + final RepositoryConfiguration repositoryConfiguration = await config.getRepositoryConfiguration(slug); + + // filter out validations here + final ValidationFilter validationFilter = ValidationFilter( + config: config, + processMethod: ProcessMethod.processAutosubmit, + repositoryConfiguration: repositoryConfiguration, + ); + final Set validations = validationFilter.getValidations(); + + final Map validationsMap = {}; + + /// Runs all the validation defined in the service. + /// If the runCi flag is false then we need a way to not run the ciSuccessful validation. + for (Validation validation in validations) { + log.info('${slug.fullName}/$prNumber running validation ${validation.name}'); + final ValidationResult validationResult = await validation.validate( + result, + pullRequest, + ); + validationsMap[validation.name] = validationResult; + } + + /// If there is at least one action that requires to remove label do so and add comments for all the failures. + bool shouldReturn = false; + for (final MapEntry(key: _, :value) in validationsMap.entries) { + if (!value.result && value.action == Action.REMOVE_LABEL) { + final String commentMessage = value.message.isEmpty ? 'Validations Fail.' : value.message; + await _removeAutosubmitLabel(commentMessage); + shouldReturn = true; + } + } + if (shouldReturn) { log.info( 'The pr ${slug.fullName}/$prNumber with message: $ackId should be acknowledged due to validation failure.', @@ -122,19 +273,19 @@ class PullRequestValidationService extends ValidationService { } // If PR has some failures to ignore temporarily do nothing and continue. - for (MapEntry result in validationsMap.entries) { - if (!result.value.result && result.value.action == Action.IGNORE_TEMPORARILY) { + for (final MapEntry(:key, :value) in validationsMap.entries) { + if (!value.result && value.action == Action.IGNORE_TEMPORARILY) { log.info( - 'Temporarily ignoring processing of ${slug.fullName}/$prNumber due to ${result.key} failing validation.', + 'Temporarily ignoring processing of ${slug.fullName}/$prNumber due to $key failing validation.', ); return; } } // If we got to this point it means we are ready to submit the PR. - final MergeResult processed = await submitPullRequest( + final MergeResult processed = await validationService.submitPullRequest( config: config, - messagePullRequest: messagePullRequest, + pullRequest: pullRequest, ); if (!processed.result) { @@ -145,9 +296,9 @@ class PullRequestValidationService extends ValidationService { } else { log.info('Pull Request ${slug.fullName}/$prNumber was ${processed.method.pastTenseLabel} successfully!'); log.info('Attempting to insert a pull request record into the database for $prNumber'); - await insertPullRequestRecord( + await validationService.insertPullRequestRecord( config: config, - pullRequest: messagePullRequest, + pullRequest: pullRequest, pullRequestType: PullRequestChangeType.change, ); } @@ -155,4 +306,12 @@ class PullRequestValidationService extends ValidationService { log.info('Ack the processed message : $ackId.'); await pubsub.acknowledge('auto-submit-queue-sub', ackId); } + + Future _removeAutosubmitLabel(String reason) async { + final String message = + '${Config.kAutosubmitLabel} label was removed for ${slug.fullName}/$prNumber, because $reason'; + await githubService.removeLabel(slug, prNumber, Config.kAutosubmitLabel); + await githubService.createComment(slug, prNumber, message); + log.info(message); + } } diff --git a/auto_submit/lib/service/revert_request_validation_service.dart b/auto_submit/lib/service/revert_request_validation_service.dart index 5db94ff72a..5a79d26616 100644 --- a/auto_submit/lib/service/revert_request_validation_service.dart +++ b/auto_submit/lib/service/revert_request_validation_service.dart @@ -57,11 +57,12 @@ class RevertRequestValidationService extends ValidationService { ) async { // Make sure the pull request still contains the labels. final github.PullRequest messagePullRequest = githubPullRequestEvent.pullRequest!; - final (currentPullRequest, labelNames) = await getPrWithLabels(messagePullRequest); - final RevertProcessMethod revertProcessMethod = await shouldProcess(currentPullRequest, labelNames); + final slug = messagePullRequest.base!.repo!.slug(); + final fullPullRequest = await getFullPullRequest(slug, messagePullRequest.number!); + final revertProcessMethod = await shouldProcess(fullPullRequest); - final GithubPullRequestEvent updatedGithubPullRequestEvent = GithubPullRequestEvent( - pullRequest: currentPullRequest, + final updatedGithubPullRequestEvent = GithubPullRequestEvent( + pullRequest: fullPullRequest, action: githubPullRequestEvent.action, sender: githubPullRequestEvent.sender, ); @@ -80,7 +81,7 @@ class RevertRequestValidationService extends ValidationService { case RevertProcessMethod.revertOf: await processRevertOfRequest( result: await getNewestPullRequestInfo(config, messagePullRequest), - githubPullRequestEvent: updatedGithubPullRequestEvent, + githubPullRequestEvent: githubPullRequestEvent, ackId: ackId, pubsub: pubsub, ); @@ -126,10 +127,8 @@ class RevertRequestValidationService extends ValidationService { } /// Determine if we should process the incoming pull request webhook event. - Future shouldProcess( - github.PullRequest pullRequest, - List labelNames, - ) async { + Future shouldProcess(github.PullRequest pullRequest) async { + final labelNames = pullRequest.labelNames; // This is the initial revert request state. if (pullRequest.state == 'closed' && labelNames.contains(Config.kRevertLabel) && pullRequest.mergedAt != null) { return RevertProcessMethod.revert; @@ -214,14 +213,14 @@ class RevertRequestValidationService extends ValidationService { required String ackId, required PubSub pubsub, }) async { - final github.PullRequest messagePullRequest = githubPullRequestEvent.pullRequest!; - final github.RepositorySlug slug = messagePullRequest.base!.repo!.slug(); + final pullRequest = githubPullRequestEvent.pullRequest!; + final github.RepositorySlug slug = pullRequest.base!.repo!.slug(); final GithubService githubService = await config.createGithubService(slug); - final int prNumber = messagePullRequest.number!; + final int prNumber = pullRequest.number!; // If a pull request is currently in the merge queue do not touch it. Let // the merge queue merge it, or kick it out of the merge queue. - if (messagePullRequest.isMergeQueueEnabled) { + if (pullRequest.isMergeQueueEnabled) { if (result.repository!.pullRequest!.isInMergeQueue) { log.info( '${slug.fullName}/$prNumber is already in the merge queue. Skipping.', @@ -264,15 +263,15 @@ class RevertRequestValidationService extends ValidationService { validationsMap[validation.name] = await validation.validate( result, // this needs to be the newly opened pull request. - messagePullRequest, + pullRequest, ); } /// If there is at least one action that requires to remove label do so and add comments for all the failures. bool shouldReturn = false; - for (MapEntry result in validationsMap.entries) { - if (!result.value.result && result.value.action == Action.REMOVE_LABEL) { - final String commmentMessage = result.value.message.isEmpty ? 'Validations Fail.' : result.value.message; + for (final MapEntry(key: _, :value) in validationsMap.entries) { + if (!value.result && value.action == Action.REMOVE_LABEL) { + final String commmentMessage = value.message.isEmpty ? 'Validations Fail.' : value.message; final String message = 'auto label is removed for ${slug.fullName}/$prNumber, due to $commmentMessage'; await githubService.removeLabel(slug, prNumber, Config.kRevertOfLabel); await githubService.createComment(slug, prNumber, message); @@ -291,10 +290,10 @@ class RevertRequestValidationService extends ValidationService { } // If PR has some failures to ignore temporarily do nothing and continue. - for (MapEntry result in validationsMap.entries) { - if (!result.value.result && result.value.action == Action.IGNORE_TEMPORARILY) { + for (final MapEntry(:key, :value) in validationsMap.entries) { + if (!value.result && value.action == Action.IGNORE_TEMPORARILY) { log.info( - 'Temporarily ignoring processing of ${slug.fullName}/$prNumber due to ${result.key} failing validation.', + 'Temporarily ignoring processing of ${slug.fullName}/$prNumber due to $key failing validation.', ); return; } @@ -303,7 +302,7 @@ class RevertRequestValidationService extends ValidationService { // If we got to this point it means we are ready to submit the PR. final MergeResult processed = await submitPullRequest( config: config, - messagePullRequest: messagePullRequest, + pullRequest: pullRequest, ); if (!processed.result) { @@ -314,14 +313,14 @@ class RevertRequestValidationService extends ValidationService { } else { // Need to add the discord notification here. final DiscordNotification discordNotification = await discordNotificationClient; - final Message discordMessage = craftDiscordRevertMessage(messagePullRequest); + final Message discordMessage = craftDiscordRevertMessage(pullRequest); discordNotification.notifyDiscordChannelWebhook(jsonEncode(discordMessage.toJson())); log.info('Pull Request ${slug.fullName}/$prNumber was ${processed.method.pastTenseLabel} successfully!'); log.info('Attempting to insert a pull request record into the database for $prNumber'); await insertPullRequestRecord( config: config, - pullRequest: messagePullRequest, + pullRequest: pullRequest, pullRequestType: PullRequestChangeType.revert, ); } diff --git a/auto_submit/lib/service/validation_service.dart b/auto_submit/lib/service/validation_service.dart index cfc3308f95..74747a7f76 100644 --- a/auto_submit/lib/service/validation_service.dart +++ b/auto_submit/lib/service/validation_service.dart @@ -45,41 +45,36 @@ class ValidationService { return QueryResult.fromJson(data); } - Future<(github.PullRequest, List)> getPrWithLabels(github.PullRequest pullRequest) async { - final github.RepositorySlug slug = pullRequest.base!.repo!.slug(); + Future getFullPullRequest(github.RepositorySlug slug, int pullRequestNumber) async { final GithubService githubService = await config.createGithubService(slug); - final github.PullRequest currentPullRequest = await githubService.getPullRequest(slug, pullRequest.number!); - final List labelNames = (currentPullRequest.labels as List) - .map((github.IssueLabel labelMap) => labelMap.name) - .toList(); - return (currentPullRequest, labelNames); + return githubService.getPullRequest(slug, pullRequestNumber); } /// Merges the commit if the PullRequest passes all the validations. Future submitPullRequest({ required Config config, - required github.PullRequest messagePullRequest, + required github.PullRequest pullRequest, }) async { - final github.RepositorySlug slug = messagePullRequest.base!.repo!.slug(); - final int number = messagePullRequest.number!; + final github.RepositorySlug slug = pullRequest.base!.repo!.slug(); + final int number = pullRequest.number!; // Pass an explicit commit message from the PR title otherwise the GitHub API will use the first commit message. const String revertPattern = 'Revert "Revert'; String messagePrefix = ''; - if (messagePullRequest.title!.contains(revertPattern)) { + if (pullRequest.title!.contains(revertPattern)) { // Cleanup auto-generated revert messages. messagePrefix = ''' -${messagePullRequest.title!.replaceFirst('Revert "Revert', 'Reland')} +${pullRequest.title!.replaceFirst('Revert "Revert', 'Reland')} '''; } - final String prBody = _sanitizePrBody(messagePullRequest.body ?? ''); + final String prBody = _sanitizePrBody(pullRequest.body ?? ''); final String commitMessage = '$messagePrefix$prBody'; - if (messagePullRequest.isMergeQueueEnabled) { - return _enqueuePullRequest(slug, messagePullRequest); + if (pullRequest.isMergeQueueEnabled) { + return _enqueuePullRequest(slug, pullRequest); } else { return _mergePullRequest(number, commitMessage, slug); } @@ -88,7 +83,7 @@ ${messagePullRequest.title!.replaceFirst('Revert "Revert', 'Reland')} Future _enqueuePullRequest(github.RepositorySlug slug, github.PullRequest restPullRequest) async { final graphQlService = await GraphQlService.forRepo(config, slug); final isEmergencyPullRequest = - restPullRequest.labels?.where((label) => label.name == 'emergency').isNotEmpty ?? false; + restPullRequest.labels?.where((label) => label.name == Config.kEmergencyLabel).isNotEmpty ?? false; try { await retryOptions.retry( @@ -117,7 +112,6 @@ ${messagePullRequest.title!.replaceFirst('Revert "Revert', 'Reland')} commitMessage: commitMessage, slug: slug, number: number, - // TODO(ricardoamador): make this configurable per repository, https://github.com/flutter/flutter/issues/114557 mergeMethod: github.MergeMethod.squash, ); }, @@ -288,4 +282,14 @@ extension PullRequestExtension on github.PullRequest { final slug = base!.repo!.slug(); return mqEnabledRepos.contains(slug.fullName); } + + /// Extracts label names from the `IssueLabel` list [labels]. + List get labelNames { + final labels = this.labels; + if (labels == null) { + return const []; + } + + return labels.map((label) => label.name).toList(); + } } diff --git a/auto_submit/lib/validations/ci_successful.dart b/auto_submit/lib/validations/ci_successful.dart index 82ba156dcb..21fe05023a 100644 --- a/auto_submit/lib/validations/ci_successful.dart +++ b/auto_submit/lib/validations/ci_successful.dart @@ -57,7 +57,7 @@ class CiSuccessful extends Validation { final String? baseBranch = messagePullRequest.base!.ref; if (baseBranch == targetBranch) { // Only validate tree status where base branch is the default branch. - if (!treeStatusCheck(slug, prNumber, statuses)) { + if (!isTreeStatusReporting(slug, prNumber, statuses)) { log.warning('Statuses were not ready for ${slug.fullName}/$prNumber, sha: $commit.'); return ValidationResult(false, Action.IGNORE_TEMPORARILY, 'Hold to wait for the tree status ready.'); } @@ -95,17 +95,22 @@ class CiSuccessful extends Validation { 'issues identified (or deflake) before re-applying this label.'); } } - final Action action = - labelNames.contains(config.overrideTreeStatusLabel) ? Action.IGNORE_FAILURE : Action.REMOVE_LABEL; + final Action action = labelNames.contains(Config.kEmergencyLabel) ? Action.IGNORE_FAILURE : Action.REMOVE_LABEL; return ValidationResult(allSuccess, action, buffer.toString()); } - /// Check the tree status. + /// Return true if the tree status check has been reported, or if doesn't have + /// to be reported. + /// + /// Tree status is pushed by [PushBuildStatusToGithub], which is asynchronous + /// relative to the creation of the PR. At the time the CI status is being + /// checked, the tree status may not have been reported yet. /// /// If a repo has a tree status, we should wait for it to show up instead of posting /// a failure to GitHub pull request. + /// /// If a repo doesn't have a tree status, simply return `true`. - bool treeStatusCheck(github.RepositorySlug slug, int prNumber, List statuses) { + bool isTreeStatusReporting(github.RepositorySlug slug, int prNumber, List statuses) { bool treeStatusValid = false; if (!Config.reposWithTreeStatus.contains(slug)) { return true; @@ -141,7 +146,6 @@ class CiSuccessful extends Validation { Set failures, bool allSuccess, ) { - final String overrideTreeStatusLabel = config.overrideTreeStatusLabel; log.info('Validating name: ${slug.name}/$prNumber, statuses: $statuses'); final List staleStatuses = []; @@ -150,7 +154,7 @@ class CiSuccessful extends Validation { final String? name = status.context; if (status.state != STATUS_SUCCESS) { - if (notInAuthorsControl.contains(name) && labelNames.contains(overrideTreeStatusLabel)) { + if (notInAuthorsControl.contains(name) && labelNames.contains(Config.kEmergencyLabel)) { continue; } allSuccess = false; @@ -194,6 +198,11 @@ class CiSuccessful extends Validation { for (github.CheckRun checkRun in checkRuns) { final String? name = checkRun.name; + if (checkRun.name == Config.kMergeQueueLockName) { + // Merge Queue Guard is not used to determine the status of CI. + continue; + } + if (checkRun.conclusion == github.CheckRunConclusion.skipped || checkRun.conclusion == github.CheckRunConclusion.success || (checkRun.status == github.CheckRunStatus.completed && diff --git a/auto_submit/lib/validations/validation_filter.dart b/auto_submit/lib/validations/validation_filter.dart index f65b60eba1..7459a467e2 100644 --- a/auto_submit/lib/validations/validation_filter.dart +++ b/auto_submit/lib/validations/validation_filter.dart @@ -23,6 +23,8 @@ abstract class ValidationFilter { switch (processMethod) { case ProcessMethod.processAutosubmit: return PullRequestValidationFilter(config, repositoryConfiguration); + case ProcessMethod.processEmergency: + return EmergencyValidationFilter(config, repositoryConfiguration); case ProcessMethod.processRevert: return RevertRequestValidationFilter(config, repositoryConfiguration); default: @@ -58,6 +60,20 @@ class PullRequestValidationFilter implements ValidationFilter { } } +/// Provides validations for applying the `emergency` label. +class EmergencyValidationFilter implements ValidationFilter { + EmergencyValidationFilter(this.config, this.repositoryConfiguration); + + final Config config; + final RepositoryConfiguration repositoryConfiguration; + + @override + Set getValidations() => { + Approval(config: config), + Mergeable(config: config), + }; +} + /// [RevertRequestValidationFilter] returns a Set of validations that we run on /// all revert pull requests. class RevertRequestValidationFilter implements ValidationFilter { @@ -67,13 +83,9 @@ class RevertRequestValidationFilter implements ValidationFilter { final RepositoryConfiguration repositoryConfiguration; @override - Set getValidations() { - final Set validationsToRun = {}; - - validationsToRun.add(Approval(config: config)); - validationsToRun.add(RequiredCheckRuns(config: config)); - validationsToRun.add(Mergeable(config: config)); - - return validationsToRun; - } + Set getValidations() => { + Approval(config: config), + RequiredCheckRuns(config: config), + Mergeable(config: config), + }; } diff --git a/auto_submit/test/requests/check_pull_request_test.dart b/auto_submit/test/requests/check_pull_request_test.dart index d28dcaf0b0..bbeef08198 100644 --- a/auto_submit/test/requests/check_pull_request_test.dart +++ b/auto_submit/test/requests/check_pull_request_test.dart @@ -55,7 +55,6 @@ void main() { const String testSubscription = 'test-sub'; const String testTopic = 'test-topic'; const String rollorAuthor = 'engine-flutter-autoroll'; - const String labelName = 'warning: land on red to fix tree breakage'; const String cocoonRepo = 'cocoon'; const String noAutosubmitLabel = 'no_autosubmit'; @@ -320,7 +319,10 @@ void main() { prNumber: 1, repoName: cocoonRepo, ); - githubService.pullRequestData = pullRequest1; + + githubService.usePullRequestList = true; + githubService.pullRequestMockList = [pullRequest1, pullRequest2]; + // 'octocat' is the pr author from generatePullRequest calls. // 'member' is in the review nodes and 'author1' is the pr author. githubService.isTeamMemberMockMap['member'] = true; @@ -418,10 +420,10 @@ void main() { assert(pubsub.messagesQueue.isEmpty); }); - test('Merges PR with failed tree status if override tree status label is provided', () async { + test('Merges PR with failed tree status if the "emergency" label is provided', () async { final PullRequest pullRequest = generatePullRequest( prNumber: 0, - labelName: labelName, + labelName: Config.kEmergencyLabel, ); // 'member' is in the review nodes and 'author1' is the pr author. githubService.isTeamMemberMockMap['member'] = true; @@ -434,6 +436,37 @@ void main() { ), ); + githubService.checkRunsData = '''{ + "total_count": 2, + "check_runs": [ + { + "id": 1, + "head_sha": "be6ff099a4ee56e152a5fa2f37edd10f79d1269a", + "external_id": "", + "details_url": "https://example.com", + "status": "completed", + "conclusion": "success", + "started_at": "2018-05-04T01:14:52Z", + "name": "mighty_readme", + "check_suite": { + "id": 5 + } + }, + { + "id": 2, + "head_sha": "be6ff099a4ee56e152a5fa2f37edd10f79d1269a", + "external_id": "", + "details_url": "https://example.com", + "status": "in_progress", + "started_at": "2018-05-04T01:14:52Z", + "name": "Merge Queue Guard", + "check_suite": { + "id": 5 + } + } + ] +}'''; + checkPullRequest = CheckPullRequest( config: config, pubsub: pubsub, @@ -453,19 +486,19 @@ void main() { verifyQueries(expectedOptions); final Map expectedMergeRequestMap = {}; - expectedMergeRequestMap[0] = RepositorySlug( - 'flutter', - 'flutter', - ); - - githubService.mergeRequestMock = PullRequestMerge( - merged: true, - sha: 'sha1', - message: 'Pull request merged successfully', - ); + expectedMergeRequestMap[0] = RepositorySlug('flutter', 'flutter'); githubService.verifyMergePullRequests(expectedMergeRequestMap); + // Verify that the "Merge Queue Guard" was unlocked. + expect(githubService.checkRunUpdates, hasLength(1)); + final checkRunUpdate = githubService.checkRunUpdates.single; + expect(checkRunUpdate.slug, RepositorySlug('flutter', 'flutter')); + expect(checkRunUpdate.checkRun.id, 2); + expect(checkRunUpdate.checkRun.name, 'Merge Queue Guard'); + expect(checkRunUpdate.status, CheckRunStatus.completed); + expect(checkRunUpdate.conclusion, CheckRunConclusion.success); + assert(pubsub.messagesQueue.isEmpty); }); diff --git a/auto_submit/test/service/pull_request_validation_service_test.dart b/auto_submit/test/service/pull_request_validation_service_test.dart index 07c2c3da8a..e2c1038252 100644 --- a/auto_submit/test/service/pull_request_validation_service_test.dart +++ b/auto_submit/test/service/pull_request_validation_service_test.dart @@ -83,7 +83,7 @@ void main() { await validationService.processPullRequest( config: config, result: queryResult, - messagePullRequest: pullRequest, + pullRequest: pullRequest, ackId: 'test', pubsub: pubsub, ); @@ -131,7 +131,7 @@ void main() { await validationService.processPullRequest( config: config, result: queryResult, - messagePullRequest: pullRequest, + pullRequest: pullRequest, ackId: 'test', pubsub: pubsub, ); @@ -173,7 +173,7 @@ void main() { await validationService.processPullRequest( config: config, result: queryResult, - messagePullRequest: pullRequest, + pullRequest: pullRequest, ackId: 'test', pubsub: pubsub, ); @@ -187,21 +187,21 @@ void main() { test('Should process message when autosubmit label exists and pr is open', () async { final PullRequest pullRequest = generatePullRequest(prNumber: 0, repoName: slug.name); githubService.pullRequestData = pullRequest; - expect(await validationService.shouldProcess(pullRequest), true); + expect(validationService.shouldProcess(pullRequest), true); }); test('Skip processing message when autosubmit label does not exist anymore', () async { final PullRequest pullRequest = generatePullRequest(prNumber: 0, repoName: slug.name); pullRequest.labels = []; githubService.pullRequestData = pullRequest; - expect(await validationService.shouldProcess(pullRequest), false); + expect(validationService.shouldProcess(pullRequest), false); }); test('Skip processing message when the pull request is closed', () async { final PullRequest pullRequest = generatePullRequest(prNumber: 0, repoName: slug.name); pullRequest.state = 'closed'; githubService.pullRequestData = pullRequest; - expect(await validationService.shouldProcess(pullRequest), false); + expect(validationService.shouldProcess(pullRequest), false); }); test('Should not process message when revert label exists and pr is open', () async { @@ -209,7 +209,7 @@ void main() { final IssueLabel issueLabel = IssueLabel(name: 'revert'); pullRequest.labels = [issueLabel]; githubService.pullRequestData = pullRequest; - expect(await validationService.shouldProcess(pullRequest), false); + expect(validationService.shouldProcess(pullRequest), false); }); test('Skip processing message when revert label exists and pr is closed', () async { @@ -218,7 +218,7 @@ void main() { final IssueLabel issueLabel = IssueLabel(name: 'revert'); pullRequest.labels = [issueLabel]; githubService.pullRequestData = pullRequest; - expect(await validationService.shouldProcess(pullRequest), false); + expect(validationService.shouldProcess(pullRequest), false); }); }); @@ -238,7 +238,7 @@ void main() { final MergeResult result = await validationService.submitPullRequest( config: config, - messagePullRequest: pullRequest, + pullRequest: pullRequest, ); expect(result.message, contains('Reland "My first PR!"')); @@ -263,7 +263,7 @@ void main() { await validationService.processPullRequest( config: config, result: queryResult, - messagePullRequest: pullRequest, + pullRequest: pullRequest, ackId: 'test', pubsub: pubsub, ); @@ -302,7 +302,7 @@ void main() { await validationService.processPullRequest( config: config, result: queryResult, - messagePullRequest: pullRequest, + pullRequest: pullRequest, ackId: 'test', pubsub: pubsub, ); @@ -348,7 +348,7 @@ void main() { await validationService.processPullRequest( config: config, result: queryResult, - messagePullRequest: pullRequest, + pullRequest: pullRequest, ackId: 'test', pubsub: pubsub, ); @@ -379,7 +379,7 @@ void main() { ); final MergeResult result = await validationService.submitPullRequest( config: config, - messagePullRequest: pullRequest, + pullRequest: pullRequest, ); expect(result.message, ''' @@ -438,7 +438,7 @@ If you need help, consider asking for advice on the #hackers-new channel on [Dis final MergeResult result = await validationService.submitPullRequest( config: config, - messagePullRequest: pullRequest, + pullRequest: pullRequest, ); expect(result.result, isTrue); @@ -488,7 +488,7 @@ This is the second line in a paragraph.'''); final MergeResult result = await validationService.submitPullRequest( config: config, - messagePullRequest: pullRequest, + pullRequest: pullRequest, ); expect(result.method, SubmitMethod.enqueue); @@ -522,7 +522,7 @@ This is the second line in a paragraph.'''); final MergeResult result = await validationService.submitPullRequest( config: config, - messagePullRequest: pullRequest, + pullRequest: pullRequest, ); expect(result.method, SubmitMethod.merge); @@ -537,7 +537,7 @@ This is the second line in a paragraph.'''); final MergeResult result = await validationService.submitPullRequest( config: config, - messagePullRequest: pullRequest, + pullRequest: pullRequest, ); expect(result.method, SubmitMethod.enqueue); @@ -552,7 +552,7 @@ This is the second line in a paragraph.'''); final MergeResult result = await validationService.submitPullRequest( config: config, - messagePullRequest: pullRequest, + pullRequest: pullRequest, ); expect(result.method, SubmitMethod.enqueue); @@ -595,7 +595,7 @@ This is the second line in a paragraph.'''); final MergeResult result = await validationService.submitPullRequest( config: config, - messagePullRequest: pullRequest, + pullRequest: pullRequest, ); expect( @@ -650,7 +650,7 @@ This is the second line in a paragraph.'''); final MergeResult result = await validationService.submitPullRequest( config: config, - messagePullRequest: pullRequest, + pullRequest: pullRequest, ); expect( @@ -700,7 +700,7 @@ This is the second line in a paragraph.'''); await validationService.processPullRequest( config: config, result: createQueryResult(flutterRequest), - messagePullRequest: pullRequest, + pullRequest: pullRequest, ackId: 'test', pubsub: pubsub, ); diff --git a/auto_submit/test/service/revert_request_validation_service_test.dart b/auto_submit/test/service/revert_request_validation_service_test.dart index 4517ed4e2d..3f62791dc8 100644 --- a/auto_submit/test/service/revert_request_validation_service_test.dart +++ b/auto_submit/test/service/revert_request_validation_service_test.dart @@ -124,10 +124,9 @@ void main() { test('Process revert from closed as "revert"', () async { final PullRequest pullRequest = generatePullRequest(prNumber: 0, repoName: slug.name, state: 'closed'); final IssueLabel issueLabel = IssueLabel(name: 'revert'); - final List labelNames = ['revert']; pullRequest.labels = [issueLabel]; githubService.pullRequestData = pullRequest; - final RevertProcessMethod revertProcessMethod = await validationService.shouldProcess(pullRequest, labelNames); + final RevertProcessMethod revertProcessMethod = await validationService.shouldProcess(pullRequest); expect(revertProcessMethod, RevertProcessMethod.revert); }); @@ -136,10 +135,9 @@ void main() { final PullRequest pullRequest = generatePullRequest(prNumber: 0, repoName: slug.name, state: 'open', author: config.autosubmitBot); final IssueLabel issueLabel = IssueLabel(name: 'revert of'); - final List labelNames = ['revert of']; pullRequest.labels = [issueLabel]; githubService.pullRequestData = pullRequest; - final RevertProcessMethod revertProcessMethod = await validationService.shouldProcess(pullRequest, labelNames); + final RevertProcessMethod revertProcessMethod = await validationService.shouldProcess(pullRequest); expect(revertProcessMethod, RevertProcessMethod.revertOf); @@ -152,10 +150,9 @@ void main() { test('Pull request state is open with revert label is not processed', () async { final PullRequest pullRequest = generatePullRequest(prNumber: 0, repoName: slug.name, state: 'open'); final IssueLabel issueLabel = IssueLabel(name: 'revert'); - final List labelNames = ['revert']; pullRequest.labels = [issueLabel]; githubService.pullRequestData = pullRequest; - final RevertProcessMethod revertProcessMethod = await validationService.shouldProcess(pullRequest, labelNames); + final RevertProcessMethod revertProcessMethod = await validationService.shouldProcess(pullRequest); expect(revertProcessMethod, RevertProcessMethod.none); }); @@ -164,10 +161,9 @@ void main() { final PullRequest pullRequest = generatePullRequest(prNumber: 0, repoName: slug.name, state: 'closed', author: config.autosubmitBot); final IssueLabel issueLabel = IssueLabel(name: 'revert of'); - final List labelNames = ['revert of']; pullRequest.labels = [issueLabel]; githubService.pullRequestData = pullRequest; - final RevertProcessMethod revertProcessMethod = await validationService.shouldProcess(pullRequest, labelNames); + final RevertProcessMethod revertProcessMethod = await validationService.shouldProcess(pullRequest); expect(revertProcessMethod, RevertProcessMethod.none); }); @@ -176,10 +172,9 @@ void main() { final PullRequest pullRequest = generatePullRequest(prNumber: 0, repoName: slug.name, state: 'open', author: 'octocat'); final IssueLabel issueLabel = IssueLabel(name: 'revert of'); - final List labelNames = ['revert of']; pullRequest.labels = [issueLabel]; githubService.pullRequestData = pullRequest; - final RevertProcessMethod revertProcessMethod = await validationService.shouldProcess(pullRequest, labelNames); + final RevertProcessMethod revertProcessMethod = await validationService.shouldProcess(pullRequest); expect(revertProcessMethod, RevertProcessMethod.none); }); @@ -192,10 +187,9 @@ void main() { mergedAt: null, ); final IssueLabel issueLabel = IssueLabel(name: 'revert'); - final List labelNames = ['revert']; pullRequest.labels = [issueLabel]; githubService.pullRequestData = pullRequest; - final RevertProcessMethod revertProcessMethod = await validationService.shouldProcess(pullRequest, labelNames); + final RevertProcessMethod revertProcessMethod = await validationService.shouldProcess(pullRequest); expect(revertProcessMethod, RevertProcessMethod.none); }); @@ -1332,7 +1326,7 @@ Reason for reverting: comment was added by mistake.'''; final MergeResult result = await validationService.submitPullRequest( config: config, - messagePullRequest: pullRequest, + pullRequest: pullRequest, ); expect(result.message, contains('Reland "My first PR!"')); @@ -1356,7 +1350,7 @@ Reason for reverting: comment was added by mistake.'''; ); final MergeResult result = await validationService.submitPullRequest( config: config, - messagePullRequest: pullRequest, + pullRequest: pullRequest, ); expect(result.message, ''' @@ -1415,7 +1409,7 @@ If you need help, consider asking for advice on the #hackers-new channel on [Dis final MergeResult result = await validationService.submitPullRequest( config: config, - messagePullRequest: pullRequest, + pullRequest: pullRequest, ); expect(result.result, isTrue); diff --git a/auto_submit/test/src/service/fake_config.dart b/auto_submit/test/src/service/fake_config.dart index 7570c0e6d8..d3aac3e28d 100644 --- a/auto_submit/test/src/service/fake_config.dart +++ b/auto_submit/test/src/service/fake_config.dart @@ -22,7 +22,6 @@ class FakeConfig extends Config { this.githubGraphQLClient, this.githubService, this.rollerAccountsValue, - this.overrideTreeStatusLabelValue, this.webhookKey, this.kPubsubPullNumberValue, this.bigqueryService, @@ -35,7 +34,6 @@ class FakeConfig extends Config { GraphQLClient? githubGraphQLClient; GithubService? githubService = FakeGithubService(); Set? rollerAccountsValue; - String? overrideTreeStatusLabelValue; String? webhookKey; int? kPubsubPullNumberValue; BigqueryService? bigqueryService; @@ -75,9 +73,6 @@ class FakeConfig extends Config { 'dependabot', }; - @override - String get overrideTreeStatusLabel => overrideTreeStatusLabelValue ?? 'warning: land on red to fix tree breakage'; - @override Future getWebhookKey() async { return webhookKey ?? 'not_a_real_key'; diff --git a/auto_submit/test/src/service/fake_github_service.dart b/auto_submit/test/src/service/fake_github_service.dart index e6ed6bd63f..d4bc37ad00 100644 --- a/auto_submit/test/src/service/fake_github_service.dart +++ b/auto_submit/test/src/service/fake_github_service.dart @@ -8,6 +8,7 @@ import 'package:auto_submit/service/github_service.dart'; import 'package:cocoon_server/testing/mocks.dart'; import 'package:github/github.dart'; import 'package:shelf/src/response.dart'; +import 'package:test/test.dart'; /// A fake GithubService implementation. class FakeGithubService implements GithubService { @@ -135,6 +136,64 @@ class FakeGithubService implements GithubService { return checkRuns; } + final List< + ({ + RepositorySlug slug, + CheckRun checkRun, + String? name, + String? detailsUrl, + String? externalId, + DateTime? startedAt, + CheckRunStatus status, + CheckRunConclusion? conclusion, + DateTime? completedAt, + CheckRunOutput? output, + List? actions, + })> checkRunUpdates = []; + + @override + Future updateCheckRun({ + required RepositorySlug slug, + required CheckRun checkRun, + String? name, + String? detailsUrl, + String? externalId, + DateTime? startedAt, + CheckRunStatus status = CheckRunStatus.queued, + CheckRunConclusion? conclusion, + DateTime? completedAt, + CheckRunOutput? output, + List? actions, + }) async { + final Map json = checkRun.toJson(); + + checkRunUpdates.add( + ( + slug: slug, + checkRun: checkRun, + name: name, + detailsUrl: detailsUrl, + externalId: externalId, + startedAt: startedAt, + status: status, + conclusion: conclusion, + completedAt: completedAt, + output: output, + actions: actions, + ), + ); + + if (conclusion != null) { + json['conclusion'] = conclusion.value; + } + + if (status != checkRun.status) { + json['status'] = status.value; + } + + return CheckRun.fromJson(json); + } + @override Future getCommit(RepositorySlug slug, String sha) async { final RepositoryCommit commit = RepositoryCommit.fromJson(jsonDecode(commitMock!) as Map); @@ -269,10 +328,13 @@ class FakeGithubService implements GithubService { } void verifyMergePullRequests(Map expected) { - assert(verifyPullRequestMergeCallMap.length == expected.length); + expect( + reason: 'Pull request numbers in mergePullRequest invocations do not match', + verifyPullRequestMergeCallMap.keys.toList(), + expected.keys.toList(), + ); verifyPullRequestMergeCallMap.forEach((key, value) { - assert(expected.containsKey(key)); - assert(expected[key] == value); + expect(value, expected[key]); }); } diff --git a/auto_submit/test/validations/ci_successful_test.dart b/auto_submit/test/validations/ci_successful_test.dart index 8489d39808..e100d2babe 100644 --- a/auto_submit/test/validations/ci_successful_test.dart +++ b/auto_submit/test/validations/ci_successful_test.dart @@ -9,6 +9,7 @@ import 'dart:core'; import 'package:auto_submit/configuration/repository_configuration.dart'; import 'package:auto_submit/model/auto_submit_query_result.dart'; import 'package:auto_submit/model/pull_request_data_types.dart'; +import 'package:auto_submit/service/config.dart'; import 'package:auto_submit/validations/ci_successful.dart'; import 'package:auto_submit/validations/validation.dart'; import 'package:cocoon_server/logging.dart'; @@ -277,7 +278,7 @@ void main() { final Author author = Author(login: 'ricardoamador'); final List labelNames = []; - labelNames.add('warning: land on red to fix tree breakage'); + labelNames.add(Config.kEmergencyLabel); labelNames.add('Other label'); convertContextNodeStatuses(contextNodeList); @@ -519,7 +520,7 @@ void main() { /// The status must be uppercase as the original code is expecting this. convertContextNodeStatuses(contextNodeList); - final bool treeStatusFlag = ciSuccessful.treeStatusCheck(slug, prNumber, contextNodeList); + final bool treeStatusFlag = ciSuccessful.isTreeStatusReporting(slug, prNumber, contextNodeList); expect(treeStatusFlag, true); }); @@ -530,7 +531,7 @@ void main() { /// The status must be uppercase as the original code is expecting this. convertContextNodeStatuses(contextNodeList); - final bool treeStatusFlag = ciSuccessful.treeStatusCheck(slug, prNumber, contextNodeList); + final bool treeStatusFlag = ciSuccessful.isTreeStatusReporting(slug, prNumber, contextNodeList); expect(treeStatusFlag, true); }); @@ -540,7 +541,7 @@ void main() { /// The status must be uppercase as the original code is expecting this. convertContextNodeStatuses(contextNodeList); - final bool treeStatusFlag = ciSuccessful.treeStatusCheck(slug, prNumber, contextNodeList); + final bool treeStatusFlag = ciSuccessful.isTreeStatusReporting(slug, prNumber, contextNodeList); expect(treeStatusFlag, false); }); }); @@ -663,7 +664,7 @@ void main() { expect(commit, isNotNull); expect(commit.status, isNotNull); - final github.PullRequest npr = generatePullRequest(labelName: 'warning: land on red to fix tree breakage'); + final github.PullRequest npr = generatePullRequest(labelName: Config.kEmergencyLabel); githubService.checkRunsData = checkRunsMock; ciSuccessful.validate(queryResult, npr).then((value) {