diff --git a/app_dart/lib/src/model/firestore/ci_staging.dart b/app_dart/lib/src/model/firestore/ci_staging.dart index 75dc596184..7a7e145ad3 100644 --- a/app_dart/lib/src/model/firestore/ci_staging.dart +++ b/app_dart/lib/src/model/firestore/ci_staging.dart @@ -88,8 +88,7 @@ class CiStaging extends Document { required String checkRun, required String conclusion, }) async { - final changeCrumb = '${slug.owner}_${slug.name}_$sha'; - final logCrumb = 'markConclusion(${changeCrumb}_$stage, $checkRun, $conclusion)'; + final logCrumb = 'markConclusion(${slug.owner}_${slug.name}_${sha}_$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 @@ -111,10 +110,8 @@ class CiStaging extends Document { var remaining = -1; var failed = -1; - var total = -1; bool valid = false; String? checkRunGuard; - String? recordedConclusion; late final Document doc; @@ -149,15 +146,9 @@ 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. - recordedConclusion = fields[checkRun]?.stringValue; + final 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); @@ -166,8 +157,6 @@ class CiStaging extends Document { remaining: remaining, checkRunGuard: null, failed: failed, - summary: 'Check run "$checkRun" not present in $stage CI stage', - details: 'Change $changeCrumb', ); } @@ -218,7 +207,7 @@ class CiStaging extends Document { fields[checkRun] = Value(stringValue: conclusion); fields[kRemainingField] = Value(integerValue: '$remaining'); fields[kFailedField] = Value(integerValue: '$failed'); - } on DetailedApiRequestError catch (e, stack) { + } on DetailedApiRequestError catch (e) { 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'); @@ -228,15 +217,6 @@ 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. @@ -259,15 +239,6 @@ $stack 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".', ); } @@ -372,16 +343,12 @@ 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; @@ -399,21 +366,8 @@ class StagingConclusion { other.result == result && other.remaining == remaining && other.checkRunGuard == checkRunGuard && - other.failed == failed && - other.summary == summary && - other.details == details); - - @override - int get hashCode => Object.hashAll([ - result, - remaining, - checkRunGuard, - failed, - summary, - details, - ]); + other.failed == failed); @override - String toString() => - 'StagingConclusion("$result", "$remaining", "$checkRunGuard", "$failed", "$summary", "$details")'; + int get hashCode => Object.hashAll([result, remaining, checkRunGuard, failed]); } diff --git a/app_dart/lib/src/service/config.dart b/app_dart/lib/src/service/config.dart index 0350580152..4f46b7a9f9 100644 --- a/app_dart/lib/src/service/config.dart +++ b/app_dart/lib/src/service/config.dart @@ -32,37 +32,11 @@ const String kDefaultBranchName = 'master'; class Config { Config(this._db, this._cache); - /// When present on a pull request, instructs Cocoon to submit it - /// automatically as soon as all the required checks pass. + /// Labels autosubmit looks for on pull requests /// /// 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; @@ -176,6 +150,7 @@ 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 22558103ec..1d48b1ce5b 100644 --- a/app_dart/lib/src/service/scheduler.dart +++ b/app_dart/lib/src/service/scheduler.dart @@ -101,9 +101,28 @@ 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 = [Config.kMergeQueueLockName, Config.kCiYamlCheckName]; + static const kCheckRunsToIgnore = [kMergeQueueLockName, kCiYamlCheckName]; /// Briefly describes what the "Merge Queue Guard" check is for. /// @@ -464,11 +483,9 @@ class Scheduler { // Update validate ci.yaml check await closeCiYamlCheckRun('PR ${pullRequest.number}', exception, slug, ciValidationCheckRun); - // 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. + // The 'lock' will be unlocked later in processCheckRunCompletion after all engine builds are processed. if (unlockMergeGroup) { - await unlockMergeQueueGuard(slug, pullRequest.head!.sha!, lock); + await unlockMergeGroupChecks(slug, pullRequest.head!.sha!, lock, exception); } log.info( 'Finished triggering builds for: pr ${pullRequest.number}, commit ${pullRequest.base!.sha}, branch ${pullRequest.head!.ref} and slug $slug}', @@ -493,7 +510,7 @@ class Scheduler { conclusion: CheckRunConclusion.success, ); } else { - log.warning('Marking $description ${Config.kCiYamlCheckName} as failed', e); + log.warning('Marking $description $kCiYamlCheckName as failed', e); // Failure when validating ci.yaml await githubChecksService.githubChecksUtil.updateCheckRun( config, @@ -502,7 +519,7 @@ class Scheduler { status: CheckRunStatus.completed, conclusion: CheckRunConclusion.failure, output: CheckRunOutput( - title: Config.kCiYamlCheckName, + title: kCiYamlCheckName, summary: '.ci.yaml has failures', text: exception.toString(), ), @@ -516,9 +533,9 @@ class Scheduler { config, slug, pullRequest.head!.sha!, - Config.kCiYamlCheckName, + kCiYamlCheckName, output: const CheckRunOutput( - title: Config.kCiYamlCheckName, + title: kCiYamlCheckName, summary: 'If this check is stuck pending, push an empty commit to retrigger the checks', ), ); @@ -562,7 +579,7 @@ class Scheduler { // close the ci.yaml validation and merge group guard. if (!isFusion) { await closeCiYamlCheckRun('MQ $slug/$headSha', null, slug, ciValidationCheckRun); - await unlockMergeQueueGuard(slug, headSha, lock); + await unlockMergeGroupChecks(slug, headSha, lock, null); return; } @@ -692,57 +709,51 @@ class Scheduler { config, slug, headSha, - Config.kMergeQueueLockName, + kMergeQueueLockName, output: const CheckRunOutput( - title: Config.kMergeQueueLockName, + title: kMergeQueueLockName, summary: kMergeQueueLockDescription, ), ); } - /// Completes the "Merge Queue Guard" check run. + /// Completes the "Merge Queue Guard" check that was scheduled using + /// [lockMergeGroupChecks] with either success or 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. - /// - /// 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. + /// If [exception] is null completed the check with success. Otherwise, + /// completes the check with failure. /// - /// 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, - ), - ); + /// 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 [builderTriggerList] is specificed, return only builders that are contained in [presubmitTarget]. @@ -877,29 +888,22 @@ class Scheduler { return false; } - /// Process a completed GitHub `check_run`. - /// - /// Handles both fusion engine build and test stages, and both pull requests - /// and merge groups. + /// Process completed GitHub `check_run` to enable fusion engine builds. Future processCheckRunCompletion(cocoon_checks.CheckRunEvent checkRunEvent) async { - final checkRun = checkRunEvent.checkRun!; - final name = checkRun.name; - final sha = checkRun.headSha; + final name = checkRunEvent.checkRun?.name; + final sha = checkRunEvent.checkRun?.headSha; final slug = checkRunEvent.repository?.slug(); - final conclusion = checkRun.conclusion; + final conclusion = checkRunEvent.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 isMergeGroup = detectMergeGroup(checkRun); + final logCrumb = 'checkCompleted($name, $slug, $sha, $conclusion)'; firestoreService = await config.createFirestoreService(); @@ -934,50 +938,23 @@ class Scheduler { return false; } - // 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. + // Are their tests remaining? Then we shouldn't unblock guard yet. if (stagingConclusion.isPending) { log.info('$logCrumb: not progressing, remaining work count: ${stagingConclusion.remaining}'); return false; } - if (stagingConclusion.isFailed) { - // Something failed in the current CI stage: - // - // * 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, - ); - } + // 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, + ); return true; } @@ -985,41 +962,30 @@ 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, letting - // GitHub land it. + // * If this is a merge group (in MQ), then close the MQ guard and 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: - await _closeSuccessfulEngineBuildStage( - checkRun: checkRun, + return _closeSuccessfulEngineBuildStage( + checkRunEvent: checkRunEvent, mergeQueueGuard: stagingConclusion.checkRunGuard!, slug: slug, sha: sha, logCrumb: logCrumb, ); case CiStage.fusionTests: - await _closeSuccessfulTestStage( + return _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.CheckRun checkRun, + Future _closeSuccessfulEngineBuildStage({ + required cocoon_checks.CheckRunEvent checkRunEvent, required String mergeQueueGuard, required RepositorySlug slug, required String sha, @@ -1028,7 +994,9 @@ 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 - if (detectMergeGroup(checkRun)) { + final headBranch = checkRunEvent.checkRun?.checkSuite?.headBranch; + final isInMergeQueue = headBranch?.startsWith('gh-readonly-queue/') ?? false; + if (isInMergeQueue) { await _closeMergeQueue( mergeQueueGuard: mergeQueueGuard, slug: slug, @@ -1036,28 +1004,46 @@ class Scheduler { stage: CiStage.fusionEngineBuild, logCrumb: logCrumb, ); - return; + return true; } log.info('$logCrumb: Stage completed successfully: ${CiStage.fusionEngineBuild}'); await _proceedToCiTestingStage( - checkRun: checkRun, + checkRunEvent: checkRunEvent, 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}'); - await unlockMergeQueueGuard(slug, sha, checkRunFromString(mergeQueueGuard)); + + // 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; } /// Returns the presubmit targets for the fusion repo [pullRequest] that should run for the given [stage]. @@ -1085,11 +1071,25 @@ class Scheduler { // Unlock the guarding check_run. final checkRunGuard = checkRunFromString(mergeQueueGuard); - await unlockMergeQueueGuard(slug, sha, checkRunGuard); + 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'); } Future _proceedToCiTestingStage({ - required cocoon_checks.CheckRun checkRun, + required cocoon_checks.CheckRunEvent checkRunEvent, required RepositorySlug slug, required String sha, required String mergeQueueGuard, @@ -1099,8 +1099,8 @@ class Scheduler { // Look up the PR in our cache first. This reduces github quota and requires less calls. PullRequest? pullRequest; - final id = checkRun.id!; - final name = checkRun.name!; + final id = checkRunEvent.checkRun!.id!; + final name = checkRunEvent.checkRun!.name!; try { pullRequest = await findPullRequestFor( firestoreService, @@ -1113,7 +1113,7 @@ class Scheduler { // We'va failed to find the pull request; try a reverse look it from the check suite. if (pullRequest == null) { - final int checkSuiteId = checkRun.checkSuite!.id!; + final int checkSuiteId = checkRunEvent.checkRun!.checkSuite!.id!; pullRequest = await githubChecksService.findMatchingPullRequest(slug, sha, checkSuiteId); } @@ -1122,6 +1122,7 @@ class Scheduler { 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. @@ -1154,15 +1155,18 @@ class Scheduler { error, backtrace, ); - rethrow; + exception = error; } catch (error, backtrace) { log.warning( '$logCrumb: Exception encountered when scheduling presubmit targets for ${pullRequest.number}', error, backtrace, ); - rethrow; + exception = error; } + + // Unlock the guarding check_run. + await unlockMergeGroupChecks(slug, sha, checkRunGuard, exception); } Future _recordCurrentCiStage({ @@ -1201,7 +1205,7 @@ class Scheduler { /// generated when someone clicks the re-run button from a failed build from /// the Github UI. /// - /// If the rerequested check is for [Config.kCiYamlCheckName], all presubmit jobs are retried. + /// If the rerequested check is for [kCiYamlCheckName], all presubmit jobs are retried. /// Otherwise, the specific check will be retried. /// /// Relevant APIs: @@ -1216,12 +1220,12 @@ class Scheduler { log.fine('Rerun requested by GitHub user: ${checkRunEvent.sender?.login}'); final String? name = checkRunEvent.checkRun!.name; bool success = false; - if (name == Config.kMergeQueueLockName) { + if (name == kMergeQueueLockName) { final RepositorySlug slug = checkRunEvent.repository!.slug(); final int checkSuiteId = checkRunEvent.checkRun!.checkSuite!.id!; - log.fine('Requested re-run of "${Config.kMergeQueueLockName}" for $slug / $checkSuiteId - ignoring'); + log.fine('Requested re-run of "$kMergeQueueLockName" for $slug / $checkSuiteId - ignoring'); success = true; - } else if (name == Config.kCiYamlCheckName) { + } else if (name == 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 ed5dfc0a4b..1659ae3821 100644 --- a/app_dart/test/model/firestore/ci_staging_test.dart +++ b/app_dart/test/model/firestore/ci_staging_test.dart @@ -186,8 +186,6 @@ 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)) @@ -210,7 +208,6 @@ 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: '{}'), @@ -237,7 +234,7 @@ void main() { predicate((CommitRequest t) { return t.transaction == kTransaction && t.writes!.length == 1 && - t.writes!.first.update!.fields!.length == 5 && + t.writes!.first.update!.fields!.length == 4 && t.writes!.first.update!.fields!['Linux build_test']!.stringValue == 'mulligan' && t.writes!.first.update!.fields![CiStaging.kRemainingField]!.integerValue == '0'; }), @@ -266,7 +263,6 @@ 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), }, @@ -287,19 +283,7 @@ void main() { final result = await future; expect( result, - 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 -''', - ), + const StagingConclusion(remaining: 0, result: StagingConclusionResult.ok, failed: 0, checkRunGuard: '{}'), ); verify( docRes.commit( @@ -307,7 +291,7 @@ For CI stage engine: predicate((CommitRequest t) { return t.transaction == kTransaction && t.writes!.length == 1 && - t.writes!.first.update!.fields!.length == 5 && + t.writes!.first.update!.fields!.length == 4 && t.writes!.first.update!.fields!['Linux build_test']!.stringValue == 'mulligan' && t.writes!.first.update!.fields![CiStaging.kRemainingField]!.integerValue == '0'; }), @@ -336,7 +320,6 @@ For CI stage engine: 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), }, @@ -362,8 +345,6 @@ For CI stage engine: 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( @@ -372,7 +353,7 @@ For CI stage engine: predicate((CommitRequest t) { return t.transaction == kTransaction && t.writes!.length == 1 && - t.writes!.first.update!.fields!.length == 5 && + t.writes!.first.update!.fields!.length == 4 && t.writes!.first.update!.fields!['MacOS build_test']!.stringValue == 'mulligan' && t.writes!.first.update!.fields![CiStaging.kRemainingField]!.integerValue == '1'; }), @@ -401,7 +382,6 @@ For CI stage engine: 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), }, @@ -423,19 +403,7 @@ For CI stage engine: // Remaining == 1 because our test was already concluded. expect( result, - 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 -''', - ), + const StagingConclusion(remaining: 1, result: StagingConclusionResult.ok, failed: 0, checkRunGuard: '{}'), ); verify( docRes.commit( @@ -443,7 +411,7 @@ For CI stage engine: predicate((CommitRequest t) { return t.transaction == kTransaction && t.writes!.length == 1 && - t.writes!.first.update!.fields!.length == 5 && + t.writes!.first.update!.fields!.length == 4 && 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'; @@ -473,7 +441,6 @@ For CI stage engine: 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), }, @@ -499,8 +466,6 @@ For CI stage engine: 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( @@ -509,7 +474,7 @@ For CI stage engine: predicate((CommitRequest t) { return t.transaction == kTransaction && t.writes!.length == 1 && - t.writes!.first.update!.fields!.length == 5 && + t.writes!.first.update!.fields!.length == 4 && 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'; @@ -539,7 +504,6 @@ For CI stage engine: 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), }, @@ -560,19 +524,7 @@ For CI stage engine: final result = await future; expect( result, - 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 -''', - ), + const StagingConclusion(remaining: 1, result: StagingConclusionResult.ok, failed: 1, checkRunGuard: '{}'), ); verify( docRes.commit( @@ -580,7 +532,7 @@ For CI stage engine: predicate((CommitRequest t) { return t.transaction == kTransaction && t.writes!.length == 1 && - t.writes!.first.update!.fields!.length == 5 && + t.writes!.first.update!.fields!.length == 4 && 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 21a5727cd1..760f091b21 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', - 'Unlocking Merge Queue Guard for flutter/flutter/c9affbbb12aa40cb3afbe94b9ea6b119a256bebf', + 'All required tests passed for c9affbbb12aa40cb3afbe94b9ea6b119a256bebf', ], ); }); diff --git a/app_dart/test/service/scheduler_test.dart b/app_dart/test/service/scheduler_test.dart index 4645a45ed7..e615b8ecbb 100644 --- a/app_dart/test/service/scheduler_test.dart +++ b/app_dart/test/service/scheduler_test.dart @@ -184,12 +184,7 @@ 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 { - return generateCheckRun( - invocation.positionalArguments[2].hashCode, - name: invocation.positionalArguments[3], - ); - }); + .thenAnswer((Invocation invocation) async => generateCheckRun(invocation.positionalArguments[2].hashCode)); fakeFusion = FakeFusionTester(); callbacks = MockCallbacks(); @@ -731,12 +726,12 @@ targets: return CheckRun.fromJson(const { 'id': 1, 'started_at': '2020-05-10T02:49:31Z', - 'name': Config.kCiYamlCheckName, + 'name': Scheduler.kCiYamlCheckName, 'check_suite': {'id': 2}, }); }); final Map checkRunEventJson = jsonDecode(checkRunString) as Map; - checkRunEventJson['check_run']['name'] = Config.kCiYamlCheckName; + checkRunEventJson['check_run']['name'] = Scheduler.kCiYamlCheckName; final cocoon_checks.CheckRunEvent checkRunEvent = cocoon_checks.CheckRunEvent.fromJson(checkRunEventJson); expect(await scheduler.processCheckRun(checkRunEvent), true); verify( @@ -744,7 +739,7 @@ targets: any, any, any, - Config.kCiYamlCheckName, + Scheduler.kCiYamlCheckName, output: anyNamed('output'), ), ); @@ -796,12 +791,12 @@ targets: return CheckRun.fromJson(const { 'id': 1, 'started_at': '2020-05-10T02:49:31Z', - 'name': Config.kCiYamlCheckName, + 'name': Scheduler.kCiYamlCheckName, 'check_suite': {'id': 2}, }); }); final Map checkRunEventJson = jsonDecode(checkRunString) as Map; - checkRunEventJson['check_run']['name'] = Config.kMergeQueueLockName; + checkRunEventJson['check_run']['name'] = Scheduler.kMergeQueueLockName; final cocoon_checks.CheckRunEvent checkRunEvent = cocoon_checks.CheckRunEvent.fromJson(checkRunEventJson); expect(await scheduler.processCheckRun(checkRunEvent), true); verifyNever( @@ -809,7 +804,7 @@ targets: any, any, any, - Config.kMergeQueueLockName, + Scheduler.kMergeQueueLockName, output: anyNamed('output'), ), ); @@ -1010,8 +1005,6 @@ targets: remaining: 1, checkRunGuard: '{}', failed: 0, - summary: 'Field missing', - details: 'Some details', ); }); @@ -1054,8 +1047,6 @@ targets: remaining: 1, checkRunGuard: '{}', failed: 0, - summary: 'Internal error', - details: 'Some details', ); }); @@ -1106,8 +1097,6 @@ targets: remaining: 1, checkRunGuard: '{}', failed: 0, - summary: 'OK', - details: 'Some details', ); }); @@ -1142,11 +1131,7 @@ targets: ); }); - // 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 { + test('failed tests unlocks but does not schedule more', () async { when( callbacks.markCheckRunConclusion( firestoreService: anyNamed('firestoreService'), @@ -1162,8 +1147,6 @@ targets: remaining: 0, checkRunGuard: checkRunFor(name: 'GUARD TEST'), failed: 1, - summary: 'OK', - details: 'Some details', ); }); @@ -1186,16 +1169,21 @@ targets: ), ).called(1); - verifyNever( + verify( mockGithubChecksUtil.updateCheckRun( any, - any, - any, - status: anyNamed('status'), - conclusion: anyNamed('conclusion'), + 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'), output: anyNamed('output'), ), - ); + ).called(1); }); test('schedules tests after engine stage', () async { @@ -1276,8 +1264,6 @@ targets: remaining: 0, checkRunGuard: checkRunFor(name: 'GUARD TEST'), failed: 0, - summary: 'OK', - details: 'Some details', ); }); @@ -1303,16 +1289,21 @@ targets: ), ).called(1); - verifyNever( + verify( mockGithubChecksUtil.updateCheckRun( any, - any, - any, - status: anyNamed('status'), - conclusion: anyNamed('conclusion'), + 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); final result = verify( luci.scheduleTryBuilds( @@ -1332,11 +1323,39 @@ 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(); - final gitHubChecksService = MockGithubChecksService(); + when(luci.scheduleTryBuilds(targets: anyNamed('targets'), pullRequest: anyNamed('pullRequest'))) + .thenAnswer((inv) async { + return []; + }); - when(githubService.github).thenReturn(githubClient); + final gitHubChecksService = MockGithubChecksService(); when(gitHubChecksService.githubChecksUtil).thenReturn(mockGithubChecksUtil); + when(gitHubChecksService.findMatchingPullRequest(any, any, any)).thenAnswer((inv) async { + return pullRequest; + }); scheduler = Scheduler( cache: cache, @@ -1369,8 +1388,6 @@ targets: remaining: 0, checkRunGuard: checkRunFor(name: 'GUARD TEST'), failed: 0, - summary: 'Field missing or OK', - details: 'Some details', ); }); @@ -1408,34 +1425,44 @@ 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(); - final gitHubChecksService = MockGithubChecksService(); + when(luci.scheduleTryBuilds(targets: anyNamed('targets'), pullRequest: anyNamed('pullRequest'))) + .thenAnswer((inv) async { + return []; + }); - when(githubService.github).thenReturn(githubClient); + final gitHubChecksService = MockGithubChecksService(); when(gitHubChecksService.githubChecksUtil).thenReturn(mockGithubChecksUtil); + when(gitHubChecksService.findMatchingPullRequest(any, any, any)).thenAnswer((inv) async { + return pullRequest; + }); scheduler = Scheduler( cache: cache, @@ -1471,8 +1498,6 @@ targets: CiStage.fusionEngineBuild => 0, CiStage.fusionTests => 1, }, - summary: 'Field missing or OK', - details: 'Some details', ); }); @@ -1511,8 +1536,9 @@ targets: ), ).called(1); - // The test stage completed, but with failures. The merge queue - // guard should stay open to prevent the pull request from landing. + // 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. verifyNever( mockGithubChecksUtil.updateCheckRun( any, @@ -1605,8 +1631,6 @@ targets: remaining: 0, checkRunGuard: checkRunFor(name: 'GUARD TEST'), failed: 0, - summary: 'OK', - details: 'Some details', ); }); @@ -1633,16 +1657,21 @@ targets: ), ).called(1); - verifyNever( + verify( mockGithubChecksUtil.updateCheckRun( any, - any, - any, - status: anyNamed('status'), - conclusion: anyNamed('conclusion'), + 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); final result = verify( luci.scheduleTryBuilds( @@ -1854,14 +1883,14 @@ targets: verify(mockGithubChecksUtil.createCheckRun(any, any, any, captureAny, output: captureAnyNamed('output'))) .captured, [ - Config.kMergeQueueLockName, + Scheduler.kMergeQueueLockName, const CheckRunOutput( - title: Config.kMergeQueueLockName, + title: Scheduler.kMergeQueueLockName, summary: Scheduler.kMergeQueueLockDescription, ), - Config.kCiYamlCheckName, + Scheduler.kCiYamlCheckName, const CheckRunOutput( - title: Config.kCiYamlCheckName, + title: Scheduler.kCiYamlCheckName, summary: 'If this check is stuck pending, push an empty commit to retrigger the checks', ), 'Linux A', @@ -1883,15 +1912,15 @@ targets: verify(mockGithubChecksUtil.createCheckRun(any, any, any, captureAny, output: captureAnyNamed('output'))) .captured, [ - Config.kMergeQueueLockName, + Scheduler.kMergeQueueLockName, const CheckRunOutput( - title: Config.kMergeQueueLockName, + title: Scheduler.kMergeQueueLockName, summary: Scheduler.kMergeQueueLockDescription, ), - Config.kCiYamlCheckName, + Scheduler.kCiYamlCheckName, // No other targets should be created. const CheckRunOutput( - title: Config.kCiYamlCheckName, + title: Scheduler.kCiYamlCheckName, summary: 'If this check is stuck pending, push an empty commit to retrigger the checks', ), ], @@ -2028,14 +2057,14 @@ targets: verify(mockGithubChecksUtil.createCheckRun(any, any, any, captureAny, output: captureAnyNamed('output'))) .captured, [ - Config.kMergeQueueLockName, + Scheduler.kMergeQueueLockName, const CheckRunOutput( - title: Config.kMergeQueueLockName, + title: Scheduler.kMergeQueueLockName, summary: Scheduler.kMergeQueueLockDescription, ), - Config.kCiYamlCheckName, + Scheduler.kCiYamlCheckName, const CheckRunOutput( - title: Config.kCiYamlCheckName, + title: Scheduler.kCiYamlCheckName, summary: 'If this check is stuck pending, push an empty commit to retrigger the checks', ), 'Linux A', @@ -2056,14 +2085,14 @@ targets: verify(mockGithubChecksUtil.createCheckRun(any, any, any, captureAny, output: captureAnyNamed('output'))) .captured, [ - Config.kMergeQueueLockName, + Scheduler.kMergeQueueLockName, const CheckRunOutput( - title: Config.kMergeQueueLockName, + title: Scheduler.kMergeQueueLockName, summary: Scheduler.kMergeQueueLockDescription, ), - Config.kCiYamlCheckName, + Scheduler.kCiYamlCheckName, const CheckRunOutput( - title: Config.kCiYamlCheckName, + title: Scheduler.kCiYamlCheckName, summary: 'If this check is stuck pending, push an empty commit to retrigger the checks', ), 'Linux A', @@ -2105,44 +2134,23 @@ 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( - capturedUpdates, - <(String, CheckRunStatus, CheckRunConclusion)>[ - ( - 'ci.yaml validation', - CheckRunStatus.completed, - CheckRunConclusion.failure, - ), - ( - 'Merge Queue Guard', - CheckRunStatus.completed, - CheckRunConclusion.success, + verify( + mockGithubChecksUtil.updateCheckRun( + any, + any, + any, + status: captureAnyNamed('status'), + conclusion: captureAnyNamed('conclusion'), + output: anyNamed('output'), ), + ).captured, + [ + CheckRunStatus.completed, + CheckRunConclusion.failure, + CheckRunStatus.completed, + CheckRunConclusion.failure, ], ); }); @@ -2165,7 +2173,7 @@ targets: CheckRunStatus.completed, CheckRunConclusion.failure, CheckRunStatus.completed, - CheckRunConclusion.success, + CheckRunConclusion.failure, ], ); }); diff --git a/app_dart/test/src/datastore/fake_config.dart b/app_dart/test/src/datastore/fake_config.dart index 96a37c5044..aa9c12d66f 100644 --- a/app_dart/test/src/datastore/fake_config.dart +++ b/app_dart/test/src/datastore/fake_config.dart @@ -101,6 +101,7 @@ class FakeConfig implements Config { String? flutterGoldDraftChangeValue; String? flutterGoldStalePRValue; List? supportedBranchesValue; + String? overrideTreeStatusLabelValue; Set? supportedReposValue; Set? postsubmitSupportedReposValue; Duration? githubRequestDelayValue; @@ -260,6 +261,9 @@ 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 9d82b7ca32..ecfd92b1ee 100644 --- a/auto_submit/lib/requests/check_pull_request.dart +++ b/auto_submit/lib/requests/check_pull_request.dart @@ -56,6 +56,8 @@ class CheckPullRequest extends CheckRequest { log.info('Processing ${messageList.length} messages'); + final PullRequestValidationService validationService = PullRequestValidationService(config); + final List> futures = >[]; for (pub.ReceivedMessage message in messageList) { @@ -90,7 +92,6 @@ 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 466533a0ce..2c4225a621 100644 --- a/auto_submit/lib/service/config.dart +++ b/auto_submit/lib/service/config.dart @@ -59,37 +59,11 @@ class Config { static const String kFlutterGitHubBotKey = 'AUTO_SUBMIT_FLUTTER_GITHUB_TOKEN'; static const String kTreeStatusDiscordUrl = 'TREE_STATUS_DISCORD_WEBHOOK_URL'; - /// When present on a pull request, instructs Cocoon to submit it - /// automatically as soon as all the required checks pass. + /// Labels autosubmit looks for on pull requests /// /// 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 @@ -108,6 +82,9 @@ 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 64b1251028..ca6d0247fd 100644 --- a/auto_submit/lib/service/github_service.dart +++ b/auto_submit/lib/service/github_service.dart @@ -43,34 +43,6 @@ 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 bf7e8aa583..3a7eea63fc 100644 --- a/auto_submit/lib/service/process_method.dart +++ b/auto_submit/lib/service/process_method.dart @@ -6,7 +6,6 @@ /// 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 30b2dbb371..b4677d0de4 100644 --- a/auto_submit/lib/service/pull_request_validation_service.dart +++ b/auto_submit/lib/service/pull_request_validation_service.dart @@ -28,13 +28,11 @@ class PullRequestValidationService extends ValidationService { /// Processes a pub/sub message associated with PullRequest event. Future processMessage(github.PullRequest messagePullRequest, String ackId, PubSub pubsub) async { - final slug = messagePullRequest.base!.repo!.slug(); - final fullPullRequest = await getFullPullRequest(slug, messagePullRequest.number!); - if (shouldProcess(fullPullRequest)) { + if (await shouldProcess(messagePullRequest)) { await processPullRequest( config: config, result: await getNewestPullRequestInfo(config, messagePullRequest), - pullRequest: fullPullRequest, + messagePullRequest: messagePullRequest, ackId: ackId, pubsub: pubsub, ); @@ -44,11 +42,9 @@ class PullRequestValidationService extends ValidationService { } } - bool shouldProcess(github.PullRequest pullRequest) { - final labelNames = pullRequest.labelNames; - final containsLabelsNeedingValidation = - labelNames.contains(Config.kAutosubmitLabel) || labelNames.contains(Config.kEmergencyLabel); - return pullRequest.state == 'open' && containsLabelsNeedingValidation; + Future shouldProcess(github.PullRequest pullRequest) async { + final (currentPullRequest, labelNames) = await getPrWithLabels(pullRequest); + return (currentPullRequest.state == 'open' && labelNames.contains(Config.kAutosubmitLabel)); } /// Processes a PullRequest running several validations to decide whether to @@ -56,16 +52,16 @@ class PullRequestValidationService extends ValidationService { Future processPullRequest({ required Config config, required QueryResult result, - required github.PullRequest pullRequest, + required github.PullRequest messagePullRequest, required String ackId, required PubSub pubsub, }) async { - final github.RepositorySlug slug = pullRequest.base!.repo!.slug(); - final int prNumber = pullRequest.number!; + final github.RepositorySlug slug = messagePullRequest.base!.repo!.slug(); + final int prNumber = messagePullRequest.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 (pullRequest.isMergeQueueEnabled) { + if (messagePullRequest.isMergeQueueEnabled) { if (result.repository!.pullRequest!.isInMergeQueue) { log.info( '${slug.fullName}/$prNumber is already in the merge queue. Skipping.', @@ -75,91 +71,22 @@ 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.processEmergency, + processMethod: ProcessMethod.processAutosubmit, 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. @@ -167,102 +94,24 @@ class _PullRequestValidationProcessor { log.info('${slug.fullName}/$prNumber running validation ${validation.name}'); final ValidationResult validationResult = await validation.validate( result, - pullRequest, + updatedPullRequest, ); 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 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); + 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); 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.', @@ -273,19 +122,19 @@ class _PullRequestValidationProcessor { } // 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) { + for (MapEntry result in validationsMap.entries) { + if (!result.value.result && result.value.action == Action.IGNORE_TEMPORARILY) { log.info( - 'Temporarily ignoring processing of ${slug.fullName}/$prNumber due to $key failing validation.', + 'Temporarily ignoring processing of ${slug.fullName}/$prNumber due to ${result.key} failing validation.', ); return; } } // If we got to this point it means we are ready to submit the PR. - final MergeResult processed = await validationService.submitPullRequest( + final MergeResult processed = await submitPullRequest( config: config, - pullRequest: pullRequest, + messagePullRequest: messagePullRequest, ); if (!processed.result) { @@ -296,9 +145,9 @@ class _PullRequestValidationProcessor { } 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 validationService.insertPullRequestRecord( + await insertPullRequestRecord( config: config, - pullRequest: pullRequest, + pullRequest: messagePullRequest, pullRequestType: PullRequestChangeType.change, ); } @@ -306,12 +155,4 @@ class _PullRequestValidationProcessor { 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 5a79d26616..5db94ff72a 100644 --- a/auto_submit/lib/service/revert_request_validation_service.dart +++ b/auto_submit/lib/service/revert_request_validation_service.dart @@ -57,12 +57,11 @@ class RevertRequestValidationService extends ValidationService { ) async { // Make sure the pull request still contains the labels. final github.PullRequest messagePullRequest = githubPullRequestEvent.pullRequest!; - final slug = messagePullRequest.base!.repo!.slug(); - final fullPullRequest = await getFullPullRequest(slug, messagePullRequest.number!); - final revertProcessMethod = await shouldProcess(fullPullRequest); + final (currentPullRequest, labelNames) = await getPrWithLabels(messagePullRequest); + final RevertProcessMethod revertProcessMethod = await shouldProcess(currentPullRequest, labelNames); - final updatedGithubPullRequestEvent = GithubPullRequestEvent( - pullRequest: fullPullRequest, + final GithubPullRequestEvent updatedGithubPullRequestEvent = GithubPullRequestEvent( + pullRequest: currentPullRequest, action: githubPullRequestEvent.action, sender: githubPullRequestEvent.sender, ); @@ -81,7 +80,7 @@ class RevertRequestValidationService extends ValidationService { case RevertProcessMethod.revertOf: await processRevertOfRequest( result: await getNewestPullRequestInfo(config, messagePullRequest), - githubPullRequestEvent: githubPullRequestEvent, + githubPullRequestEvent: updatedGithubPullRequestEvent, ackId: ackId, pubsub: pubsub, ); @@ -127,8 +126,10 @@ class RevertRequestValidationService extends ValidationService { } /// Determine if we should process the incoming pull request webhook event. - Future shouldProcess(github.PullRequest pullRequest) async { - final labelNames = pullRequest.labelNames; + Future shouldProcess( + github.PullRequest pullRequest, + List labelNames, + ) async { // This is the initial revert request state. if (pullRequest.state == 'closed' && labelNames.contains(Config.kRevertLabel) && pullRequest.mergedAt != null) { return RevertProcessMethod.revert; @@ -213,14 +214,14 @@ class RevertRequestValidationService extends ValidationService { required String ackId, required PubSub pubsub, }) async { - final pullRequest = githubPullRequestEvent.pullRequest!; - final github.RepositorySlug slug = pullRequest.base!.repo!.slug(); + final github.PullRequest messagePullRequest = githubPullRequestEvent.pullRequest!; + final github.RepositorySlug slug = messagePullRequest.base!.repo!.slug(); final GithubService githubService = await config.createGithubService(slug); - final int prNumber = pullRequest.number!; + final int prNumber = messagePullRequest.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 (pullRequest.isMergeQueueEnabled) { + if (messagePullRequest.isMergeQueueEnabled) { if (result.repository!.pullRequest!.isInMergeQueue) { log.info( '${slug.fullName}/$prNumber is already in the merge queue. Skipping.', @@ -263,15 +264,15 @@ class RevertRequestValidationService extends ValidationService { validationsMap[validation.name] = await validation.validate( result, // this needs to be the newly opened pull request. - pullRequest, + messagePullRequest, ); } /// 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 commmentMessage = value.message.isEmpty ? 'Validations Fail.' : value.message; + 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.kRevertOfLabel); await githubService.createComment(slug, prNumber, message); @@ -290,10 +291,10 @@ class RevertRequestValidationService extends ValidationService { } // 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) { + for (MapEntry result in validationsMap.entries) { + if (!result.value.result && result.value.action == Action.IGNORE_TEMPORARILY) { log.info( - 'Temporarily ignoring processing of ${slug.fullName}/$prNumber due to $key failing validation.', + 'Temporarily ignoring processing of ${slug.fullName}/$prNumber due to ${result.key} failing validation.', ); return; } @@ -302,7 +303,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, - pullRequest: pullRequest, + messagePullRequest: messagePullRequest, ); if (!processed.result) { @@ -313,14 +314,14 @@ class RevertRequestValidationService extends ValidationService { } else { // Need to add the discord notification here. final DiscordNotification discordNotification = await discordNotificationClient; - final Message discordMessage = craftDiscordRevertMessage(pullRequest); + final Message discordMessage = craftDiscordRevertMessage(messagePullRequest); 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: pullRequest, + pullRequest: messagePullRequest, pullRequestType: PullRequestChangeType.revert, ); } diff --git a/auto_submit/lib/service/validation_service.dart b/auto_submit/lib/service/validation_service.dart index 74747a7f76..cfc3308f95 100644 --- a/auto_submit/lib/service/validation_service.dart +++ b/auto_submit/lib/service/validation_service.dart @@ -45,36 +45,41 @@ class ValidationService { return QueryResult.fromJson(data); } - Future getFullPullRequest(github.RepositorySlug slug, int pullRequestNumber) async { + Future<(github.PullRequest, List)> getPrWithLabels(github.PullRequest pullRequest) async { + final github.RepositorySlug slug = pullRequest.base!.repo!.slug(); final GithubService githubService = await config.createGithubService(slug); - return githubService.getPullRequest(slug, pullRequestNumber); + 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); } /// Merges the commit if the PullRequest passes all the validations. Future submitPullRequest({ required Config config, - required github.PullRequest pullRequest, + required github.PullRequest messagePullRequest, }) async { - final github.RepositorySlug slug = pullRequest.base!.repo!.slug(); - final int number = pullRequest.number!; + final github.RepositorySlug slug = messagePullRequest.base!.repo!.slug(); + final int number = messagePullRequest.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 (pullRequest.title!.contains(revertPattern)) { + if (messagePullRequest.title!.contains(revertPattern)) { // Cleanup auto-generated revert messages. messagePrefix = ''' -${pullRequest.title!.replaceFirst('Revert "Revert', 'Reland')} +${messagePullRequest.title!.replaceFirst('Revert "Revert', 'Reland')} '''; } - final String prBody = _sanitizePrBody(pullRequest.body ?? ''); + final String prBody = _sanitizePrBody(messagePullRequest.body ?? ''); final String commitMessage = '$messagePrefix$prBody'; - if (pullRequest.isMergeQueueEnabled) { - return _enqueuePullRequest(slug, pullRequest); + if (messagePullRequest.isMergeQueueEnabled) { + return _enqueuePullRequest(slug, messagePullRequest); } else { return _mergePullRequest(number, commitMessage, slug); } @@ -83,7 +88,7 @@ ${pullRequest.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 == Config.kEmergencyLabel).isNotEmpty ?? false; + restPullRequest.labels?.where((label) => label.name == 'emergency').isNotEmpty ?? false; try { await retryOptions.retry( @@ -112,6 +117,7 @@ ${pullRequest.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, ); }, @@ -282,14 +288,4 @@ 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 21fe05023a..82ba156dcb 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 (!isTreeStatusReporting(slug, prNumber, statuses)) { + if (!treeStatusCheck(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,22 +95,17 @@ class CiSuccessful extends Validation { 'issues identified (or deflake) before re-applying this label.'); } } - final Action action = labelNames.contains(Config.kEmergencyLabel) ? Action.IGNORE_FAILURE : Action.REMOVE_LABEL; + final Action action = + labelNames.contains(config.overrideTreeStatusLabel) ? Action.IGNORE_FAILURE : Action.REMOVE_LABEL; return ValidationResult(allSuccess, action, buffer.toString()); } - /// 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. + /// Check the tree status. /// /// 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 isTreeStatusReporting(github.RepositorySlug slug, int prNumber, List statuses) { + bool treeStatusCheck(github.RepositorySlug slug, int prNumber, List statuses) { bool treeStatusValid = false; if (!Config.reposWithTreeStatus.contains(slug)) { return true; @@ -146,6 +141,7 @@ 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 = []; @@ -154,7 +150,7 @@ class CiSuccessful extends Validation { final String? name = status.context; if (status.state != STATUS_SUCCESS) { - if (notInAuthorsControl.contains(name) && labelNames.contains(Config.kEmergencyLabel)) { + if (notInAuthorsControl.contains(name) && labelNames.contains(overrideTreeStatusLabel)) { continue; } allSuccess = false; @@ -198,11 +194,6 @@ 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 7459a467e2..f65b60eba1 100644 --- a/auto_submit/lib/validations/validation_filter.dart +++ b/auto_submit/lib/validations/validation_filter.dart @@ -23,8 +23,6 @@ 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: @@ -60,20 +58,6 @@ 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 { @@ -83,9 +67,13 @@ class RevertRequestValidationFilter implements ValidationFilter { final RepositoryConfiguration repositoryConfiguration; @override - Set getValidations() => { - Approval(config: config), - RequiredCheckRuns(config: config), - Mergeable(config: config), - }; + Set getValidations() { + final Set validationsToRun = {}; + + validationsToRun.add(Approval(config: config)); + validationsToRun.add(RequiredCheckRuns(config: config)); + validationsToRun.add(Mergeable(config: config)); + + return validationsToRun; + } } diff --git a/auto_submit/test/requests/check_pull_request_test.dart b/auto_submit/test/requests/check_pull_request_test.dart index bbeef08198..d28dcaf0b0 100644 --- a/auto_submit/test/requests/check_pull_request_test.dart +++ b/auto_submit/test/requests/check_pull_request_test.dart @@ -55,6 +55,7 @@ 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'; @@ -319,10 +320,7 @@ void main() { prNumber: 1, repoName: cocoonRepo, ); - - githubService.usePullRequestList = true; - githubService.pullRequestMockList = [pullRequest1, pullRequest2]; - + githubService.pullRequestData = pullRequest1; // 'octocat' is the pr author from generatePullRequest calls. // 'member' is in the review nodes and 'author1' is the pr author. githubService.isTeamMemberMockMap['member'] = true; @@ -420,10 +418,10 @@ void main() { assert(pubsub.messagesQueue.isEmpty); }); - test('Merges PR with failed tree status if the "emergency" label is provided', () async { + test('Merges PR with failed tree status if override tree status label is provided', () async { final PullRequest pullRequest = generatePullRequest( prNumber: 0, - labelName: Config.kEmergencyLabel, + labelName: labelName, ); // 'member' is in the review nodes and 'author1' is the pr author. githubService.isTeamMemberMockMap['member'] = true; @@ -436,37 +434,6 @@ 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, @@ -486,18 +453,18 @@ void main() { verifyQueries(expectedOptions); final Map expectedMergeRequestMap = {}; - expectedMergeRequestMap[0] = RepositorySlug('flutter', 'flutter'); + expectedMergeRequestMap[0] = RepositorySlug( + 'flutter', + 'flutter', + ); - githubService.verifyMergePullRequests(expectedMergeRequestMap); + githubService.mergeRequestMock = PullRequestMerge( + merged: true, + sha: 'sha1', + message: 'Pull request merged successfully', + ); - // 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); + githubService.verifyMergePullRequests(expectedMergeRequestMap); 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 e2c1038252..07c2c3da8a 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, - pullRequest: pullRequest, + messagePullRequest: pullRequest, ackId: 'test', pubsub: pubsub, ); @@ -131,7 +131,7 @@ void main() { await validationService.processPullRequest( config: config, result: queryResult, - pullRequest: pullRequest, + messagePullRequest: pullRequest, ackId: 'test', pubsub: pubsub, ); @@ -173,7 +173,7 @@ void main() { await validationService.processPullRequest( config: config, result: queryResult, - pullRequest: pullRequest, + messagePullRequest: 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(validationService.shouldProcess(pullRequest), true); + expect(await 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(validationService.shouldProcess(pullRequest), false); + expect(await 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(validationService.shouldProcess(pullRequest), false); + expect(await 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(validationService.shouldProcess(pullRequest), false); + expect(await 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(validationService.shouldProcess(pullRequest), false); + expect(await validationService.shouldProcess(pullRequest), false); }); }); @@ -238,7 +238,7 @@ void main() { final MergeResult result = await validationService.submitPullRequest( config: config, - pullRequest: pullRequest, + messagePullRequest: pullRequest, ); expect(result.message, contains('Reland "My first PR!"')); @@ -263,7 +263,7 @@ void main() { await validationService.processPullRequest( config: config, result: queryResult, - pullRequest: pullRequest, + messagePullRequest: pullRequest, ackId: 'test', pubsub: pubsub, ); @@ -302,7 +302,7 @@ void main() { await validationService.processPullRequest( config: config, result: queryResult, - pullRequest: pullRequest, + messagePullRequest: pullRequest, ackId: 'test', pubsub: pubsub, ); @@ -348,7 +348,7 @@ void main() { await validationService.processPullRequest( config: config, result: queryResult, - pullRequest: pullRequest, + messagePullRequest: pullRequest, ackId: 'test', pubsub: pubsub, ); @@ -379,7 +379,7 @@ void main() { ); final MergeResult result = await validationService.submitPullRequest( config: config, - pullRequest: pullRequest, + messagePullRequest: 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, - pullRequest: pullRequest, + messagePullRequest: 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, - pullRequest: pullRequest, + messagePullRequest: 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, - pullRequest: pullRequest, + messagePullRequest: 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, - pullRequest: pullRequest, + messagePullRequest: 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, - pullRequest: pullRequest, + messagePullRequest: 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, - pullRequest: pullRequest, + messagePullRequest: pullRequest, ); expect( @@ -650,7 +650,7 @@ This is the second line in a paragraph.'''); final MergeResult result = await validationService.submitPullRequest( config: config, - pullRequest: pullRequest, + messagePullRequest: pullRequest, ); expect( @@ -700,7 +700,7 @@ This is the second line in a paragraph.'''); await validationService.processPullRequest( config: config, result: createQueryResult(flutterRequest), - pullRequest: pullRequest, + messagePullRequest: 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 3f62791dc8..4517ed4e2d 100644 --- a/auto_submit/test/service/revert_request_validation_service_test.dart +++ b/auto_submit/test/service/revert_request_validation_service_test.dart @@ -124,9 +124,10 @@ 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); + final RevertProcessMethod revertProcessMethod = await validationService.shouldProcess(pullRequest, labelNames); expect(revertProcessMethod, RevertProcessMethod.revert); }); @@ -135,9 +136,10 @@ 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); + final RevertProcessMethod revertProcessMethod = await validationService.shouldProcess(pullRequest, labelNames); expect(revertProcessMethod, RevertProcessMethod.revertOf); @@ -150,9 +152,10 @@ 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); + final RevertProcessMethod revertProcessMethod = await validationService.shouldProcess(pullRequest, labelNames); expect(revertProcessMethod, RevertProcessMethod.none); }); @@ -161,9 +164,10 @@ 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); + final RevertProcessMethod revertProcessMethod = await validationService.shouldProcess(pullRequest, labelNames); expect(revertProcessMethod, RevertProcessMethod.none); }); @@ -172,9 +176,10 @@ 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); + final RevertProcessMethod revertProcessMethod = await validationService.shouldProcess(pullRequest, labelNames); expect(revertProcessMethod, RevertProcessMethod.none); }); @@ -187,9 +192,10 @@ 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); + final RevertProcessMethod revertProcessMethod = await validationService.shouldProcess(pullRequest, labelNames); expect(revertProcessMethod, RevertProcessMethod.none); }); @@ -1326,7 +1332,7 @@ Reason for reverting: comment was added by mistake.'''; final MergeResult result = await validationService.submitPullRequest( config: config, - pullRequest: pullRequest, + messagePullRequest: pullRequest, ); expect(result.message, contains('Reland "My first PR!"')); @@ -1350,7 +1356,7 @@ Reason for reverting: comment was added by mistake.'''; ); final MergeResult result = await validationService.submitPullRequest( config: config, - pullRequest: pullRequest, + messagePullRequest: pullRequest, ); expect(result.message, ''' @@ -1409,7 +1415,7 @@ If you need help, consider asking for advice on the #hackers-new channel on [Dis final MergeResult result = await validationService.submitPullRequest( config: config, - pullRequest: pullRequest, + messagePullRequest: 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 d3aac3e28d..7570c0e6d8 100644 --- a/auto_submit/test/src/service/fake_config.dart +++ b/auto_submit/test/src/service/fake_config.dart @@ -22,6 +22,7 @@ class FakeConfig extends Config { this.githubGraphQLClient, this.githubService, this.rollerAccountsValue, + this.overrideTreeStatusLabelValue, this.webhookKey, this.kPubsubPullNumberValue, this.bigqueryService, @@ -34,6 +35,7 @@ class FakeConfig extends Config { GraphQLClient? githubGraphQLClient; GithubService? githubService = FakeGithubService(); Set? rollerAccountsValue; + String? overrideTreeStatusLabelValue; String? webhookKey; int? kPubsubPullNumberValue; BigqueryService? bigqueryService; @@ -73,6 +75,9 @@ 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 d4bc37ad00..e6ed6bd63f 100644 --- a/auto_submit/test/src/service/fake_github_service.dart +++ b/auto_submit/test/src/service/fake_github_service.dart @@ -8,7 +8,6 @@ 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 { @@ -136,64 +135,6 @@ 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); @@ -328,13 +269,10 @@ class FakeGithubService implements GithubService { } void verifyMergePullRequests(Map expected) { - expect( - reason: 'Pull request numbers in mergePullRequest invocations do not match', - verifyPullRequestMergeCallMap.keys.toList(), - expected.keys.toList(), - ); + assert(verifyPullRequestMergeCallMap.length == expected.length); verifyPullRequestMergeCallMap.forEach((key, value) { - expect(value, expected[key]); + assert(expected.containsKey(key)); + assert(expected[key] == value); }); } diff --git a/auto_submit/test/validations/ci_successful_test.dart b/auto_submit/test/validations/ci_successful_test.dart index e100d2babe..8489d39808 100644 --- a/auto_submit/test/validations/ci_successful_test.dart +++ b/auto_submit/test/validations/ci_successful_test.dart @@ -9,7 +9,6 @@ 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'; @@ -278,7 +277,7 @@ void main() { final Author author = Author(login: 'ricardoamador'); final List labelNames = []; - labelNames.add(Config.kEmergencyLabel); + labelNames.add('warning: land on red to fix tree breakage'); labelNames.add('Other label'); convertContextNodeStatuses(contextNodeList); @@ -520,7 +519,7 @@ void main() { /// The status must be uppercase as the original code is expecting this. convertContextNodeStatuses(contextNodeList); - final bool treeStatusFlag = ciSuccessful.isTreeStatusReporting(slug, prNumber, contextNodeList); + final bool treeStatusFlag = ciSuccessful.treeStatusCheck(slug, prNumber, contextNodeList); expect(treeStatusFlag, true); }); @@ -531,7 +530,7 @@ void main() { /// The status must be uppercase as the original code is expecting this. convertContextNodeStatuses(contextNodeList); - final bool treeStatusFlag = ciSuccessful.isTreeStatusReporting(slug, prNumber, contextNodeList); + final bool treeStatusFlag = ciSuccessful.treeStatusCheck(slug, prNumber, contextNodeList); expect(treeStatusFlag, true); }); @@ -541,7 +540,7 @@ void main() { /// The status must be uppercase as the original code is expecting this. convertContextNodeStatuses(contextNodeList); - final bool treeStatusFlag = ciSuccessful.isTreeStatusReporting(slug, prNumber, contextNodeList); + final bool treeStatusFlag = ciSuccessful.treeStatusCheck(slug, prNumber, contextNodeList); expect(treeStatusFlag, false); }); }); @@ -664,7 +663,7 @@ void main() { expect(commit, isNotNull); expect(commit.status, isNotNull); - final github.PullRequest npr = generatePullRequest(labelName: Config.kEmergencyLabel); + final github.PullRequest npr = generatePullRequest(labelName: 'warning: land on red to fix tree breakage'); githubService.checkRunsData = checkRunsMock; ciSuccessful.validate(queryResult, npr).then((value) {