diff --git a/app_dart/lib/src/model/common/presubmit_completed_check.dart b/app_dart/lib/src/model/common/presubmit_completed_check.dart index 2026da1c3..b5102732d 100644 --- a/app_dart/lib/src/model/common/presubmit_completed_check.dart +++ b/app_dart/lib/src/model/common/presubmit_completed_check.dart @@ -118,7 +118,7 @@ class PresubmitCompletedJob { cocoon_checks.CheckRun get checkRun { return cocoon_checks.CheckRun( id: checkRunId, - name: isUnifiedCheckRun ? Config.kMergeQueueLockName : name, + name: isUnifiedCheckRun ? Config.kFlutterPresubmitsName : name, headSha: sha, conclusion: status.toConclusion(), checkSuite: CheckSuite( diff --git a/app_dart/lib/src/service/config.dart b/app_dart/lib/src/service/config.dart index 95cefda5b..bf76c4d13 100644 --- a/app_dart/lib/src/service/config.dart +++ b/app_dart/lib/src/service/config.dart @@ -81,6 +81,10 @@ interface class Config extends DynamicallyUpdatedConfig { /// workflow. static const String kMergeQueueLockName = 'Merge Queue Guard'; + /// A required check that stays in pending state until all presubmit checks pass + /// for users opted into the unified checkrun flow. + static const String kFlutterPresubmitsName = 'Flutter Presubmits'; + final CacheService _cache; final SecretManager _secrets; final http.Client _httpClient; diff --git a/app_dart/lib/src/service/scheduler.dart b/app_dart/lib/src/service/scheduler.dart index d74ed83d9..18fcdb704 100644 --- a/app_dart/lib/src/service/scheduler.dart +++ b/app_dart/lib/src/service/scheduler.dart @@ -117,6 +117,10 @@ class Scheduler { 'If you need to merge your PR without tests (a rare situation, typically ' 'an emergency), then you can use the `emergency` label.'; + /// Briefly describes what the "Flutter Presubmits" check is for. + static const String kFlutterPresubmitsDescription = + 'Flutter Presubmits unified check run.'; + /// Ensure [commits] exist in Cocoon. /// /// If the commit already exists, it is ignored. @@ -332,6 +336,10 @@ class Scheduler { final slug = pullRequest.base!.repo!.slug(); + final isUnifiedCheckRun = _config.flags.isUnifiedCheckRunFlowEnabledForUser( + pullRequest.user!.login!, + ); + // The MQ only waits for "required status checks" before deciding whether to // merge the PR into the target branch. This required check added to both // the PR and to the merge group, and so it must be completed in both cases. @@ -340,12 +348,10 @@ class Scheduler { pullRequest.head!.sha!, // Override details url of merge queue guard check for users with unified // check run flow enabled - detailsUrl: - _config.flags.isUnifiedCheckRunFlowEnabledForUser( - pullRequest.user!.login!, - ) + detailsUrl: isUnifiedCheckRun ? 'https://flutter-dashboard.appspot.com/#/presubmit?repo=${slug.name}&sha=${pullRequest.head!.sha}' : null, + isUnifiedCheckRun: isUnifiedCheckRun, ); // Track if we should unlock the merge group lock in case of non-fusion or @@ -611,7 +617,11 @@ class Scheduler { '${contentHash != null ? ', contentHash: $contentHash' : ''})'; log.info('$logCrumb: scheduling merge group checks'); - final lock = await lockMergeGroupChecks(slug, headSha); + final lock = await lockMergeGroupChecks( + slug, + headSha, + isUnifiedCheckRun: false, + ); // If the repo is not fusion, it doesn't run anything in the MQ, so just // close the merge group guard. @@ -827,8 +837,9 @@ $s RepositorySlug slug, String headSha, { String? detailsUrl, + required bool isUnifiedCheckRun, }) async { - return _githubChecksService.githubChecksUtil.createCheckRun( + final mqGuard = await _githubChecksService.githubChecksUtil.createCheckRun( _config, slug, headSha, @@ -837,8 +848,43 @@ $s title: Config.kMergeQueueLockName, summary: kMergeQueueLockDescription, ), - detailsUrl: detailsUrl, + detailsUrl: isUnifiedCheckRun ? null : detailsUrl, ); + + final flutterPresubmits = await _githubChecksService.githubChecksUtil + .createCheckRun( + _config, + slug, + headSha, + Config.kFlutterPresubmitsName, + output: const CheckRunOutput( + title: Config.kFlutterPresubmitsName, + summary: kFlutterPresubmitsDescription, + ), + detailsUrl: isUnifiedCheckRun ? detailsUrl : null, + ); + + if (isUnifiedCheckRun) { + // Skip MQ Guard + await _githubChecksService.githubChecksUtil.updateCheckRun( + _config, + slug, + mqGuard, + status: CheckRunStatus.completed, + conclusion: CheckRunConclusion.success, + ); + return flutterPresubmits; + } else { + // Skip Flutter Presubmits + await _githubChecksService.githubChecksUtil.updateCheckRun( + _config, + slug, + flutterPresubmits, + status: CheckRunStatus.completed, + conclusion: CheckRunConclusion.success, + ); + return mqGuard; + } } /// Creates a pending check run for "Awaiting CICD label" if it doesn't exist. @@ -1592,11 +1638,12 @@ $stacktrace ); final name = checkRunEvent.checkRun!.name; var success = false; - if (name == Config.kMergeQueueLockName) { + if (name == Config.kMergeQueueLockName || + name == Config.kFlutterPresubmitsName) { final slug = checkRunEvent.repository!.slug(); final checkSuiteId = checkRunEvent.checkRun!.checkSuite!.id!; log.debug( - '$logCrumb: Requested re-run of "${Config.kMergeQueueLockName}" for ' + '$logCrumb: Requested re-run of "$name" for ' '$slug / $checkSuiteId - ignoring', ); success = true; diff --git a/app_dart/test/model/common/presubmit_completed_check_test.dart b/app_dart/test/model/common/presubmit_completed_check_test.dart index 2f74c1d9d..ad9a33a31 100644 --- a/app_dart/test/model/common/presubmit_completed_check_test.dart +++ b/app_dart/test/model/common/presubmit_completed_check_test.dart @@ -72,7 +72,7 @@ void main() { expect(check.checkSuiteId, 456); expect(check.headBranch, 'gh-readonly-queue/master/pr-123-abc'); expect(check.isUnifiedCheckRun, true); - expect(check.checkRun.name, Config.kMergeQueueLockName); + expect(check.checkRun.name, Config.kFlutterPresubmitsName); expect(check.buildNumber, 0); }); 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 0bf8786f1..854a6c1c0 100644 --- a/app_dart/test/request_handlers/github/webhook_subscription_test.dart +++ b/app_dart/test/request_handlers/github/webhook_subscription_test.dart @@ -3351,7 +3351,7 @@ void foo() { status: CheckRunStatus.completed, conclusion: CheckRunConclusion.success, ), - ).called(1); + ).called(2); expect( log, diff --git a/app_dart/test/service/scheduler_test.dart b/app_dart/test/service/scheduler_test.dart index e2aaff219..fb293e1bf 100644 --- a/app_dart/test/service/scheduler_test.dart +++ b/app_dart/test/service/scheduler_test.dart @@ -938,6 +938,93 @@ void main() { verifyNever(mockGithubChecksUtil.createCheckRun(any, any, any, any)); }); + test('rerequested flutter presubmits check is ignored', () async { + final mockGithubService = MockGithubService(); + final mockGithubClient = MockGitHub(); + config = FakeConfig(githubService: mockGithubService); + scheduler = Scheduler( + githubService: config.githubService ?? FakeGithubService(), + cache: cache, + config: config, + githubChecksService: GithubChecksService( + config, + githubChecksUtil: mockGithubChecksUtil, + ), + getFilesChanged: getFilesChanged, + ciYamlFetcher: ciYamlFetcher, + luciBuildService: FakeLuciBuildService( + config: config, + githubChecksUtil: mockGithubChecksUtil, + firestore: firestore, + ), + contentAwareHash: fakeContentAwareHash, + firestore: firestore, + bigQuery: bigQuery, + ); + when(mockGithubService.github).thenReturn(mockGithubClient); + when( + mockGithubService.searchIssuesAndPRs( + any, + any, + sort: anyNamed('sort'), + pages: anyNamed('pages'), + ), + ).thenAnswer((_) async => [generateIssue(3)]); + when( + mockGithubChecksUtil.listCheckSuitesForRef( + any, + any, + ref: anyNamed('ref'), + ), + ).thenAnswer( + (_) async => [ + // From check_run.check_suite.id in [checkRunString]. + generateCheckSuite(668083231), + ], + ); + when( + mockGithubService.getPullRequest(any, any), + ).thenAnswer((_) async => generatePullRequest()); + getFilesChanged.cannedFiles = ['abc/def']; + when( + mockGithubChecksUtil.createCheckRun( + any, + any, + any, + any, + output: anyNamed('output'), + ), + ).thenAnswer((_) async { + return CheckRun.fromJson(const { + 'id': 1, + 'started_at': '2020-05-10T02:49:31Z', + 'name': Config.kCiYamlCheckName, + 'check_suite': {'id': 2}, + }); + }); + final checkRunEventJson = + jsonDecode(checkRunString()) as Map; + checkRunEventJson['check_run']['name'] = Config.kFlutterPresubmitsName; + final checkRunEvent = cocoon_checks.CheckRunEvent.fromJson( + checkRunEventJson, + ); + expect( + await scheduler.processCheckRun(checkRunEvent), + const ProcessCheckRunResult.success(), + ); + verifyNever( + mockGithubChecksUtil.createCheckRun( + any, + any, + any, + Config.kFlutterPresubmitsName, + output: anyNamed('output'), + ), + ); + // Verifies no checks were created + verifyNever(mockGithubChecksUtil.createCheckRun(any, any, any, any)); + }); + test('rerequested presubmit job triggers presubmit build', () async { // Note that we're not inserting any commits into the db, because // only postsubmit commits are stored in the Firestore. @@ -2670,6 +2757,11 @@ targets: title: Config.kMergeQueueLockName, summary: Scheduler.kMergeQueueLockDescription, ), + Config.kFlutterPresubmitsName, + const CheckRunOutput( + title: Config.kFlutterPresubmitsName, + summary: Scheduler.kFlutterPresubmitsDescription, + ), Config.kCiYamlCheckName, const CheckRunOutput( title: Config.kCiYamlCheckName, @@ -2709,6 +2801,11 @@ targets: title: Config.kMergeQueueLockName, summary: Scheduler.kMergeQueueLockDescription, ), + Config.kFlutterPresubmitsName, + const CheckRunOutput( + title: Config.kFlutterPresubmitsName, + summary: Scheduler.kFlutterPresubmitsDescription, + ), Config.kCiYamlCheckName, // No other targets should be created. const CheckRunOutput( @@ -2748,6 +2845,9 @@ targets: CheckRunStatus.completed, CheckRunConclusion.success, null, + CheckRunStatus.completed, + CheckRunConclusion.success, + null, ], ); }); @@ -2858,6 +2958,11 @@ targets: title: Config.kMergeQueueLockName, summary: Scheduler.kMergeQueueLockDescription, ), + Config.kFlutterPresubmitsName, + const CheckRunOutput( + title: Config.kFlutterPresubmitsName, + summary: Scheduler.kFlutterPresubmitsDescription, + ), Config.kCiYamlCheckName, const CheckRunOutput( title: Config.kCiYamlCheckName, @@ -2899,6 +3004,11 @@ targets: title: Config.kMergeQueueLockName, summary: Scheduler.kMergeQueueLockDescription, ), + Config.kFlutterPresubmitsName, + const CheckRunOutput( + title: Config.kFlutterPresubmitsName, + summary: Scheduler.kFlutterPresubmitsDescription, + ), Config.kCiYamlCheckName, const CheckRunOutput( title: Config.kCiYamlCheckName, @@ -2934,6 +3044,8 @@ targets: CheckRunConclusion.success, CheckRunStatus.completed, CheckRunConclusion.success, + CheckRunStatus.completed, + CheckRunConclusion.success, ], ); }); @@ -2965,6 +3077,11 @@ targets: await scheduler.triggerPresubmitTargets(pullRequest: pullRequest); expect(capturedUpdates, <(String, CheckRunStatus, CheckRunConclusion)>[ + ( + Config.kFlutterPresubmitsName, + CheckRunStatus.completed, + CheckRunConclusion.success, + ), ( 'ci.yaml validation', CheckRunStatus.completed, @@ -2987,7 +3104,12 @@ targets: output: anyNamed('output'), ), ).captured, - [CheckRunStatus.completed, CheckRunConclusion.failure], + [ + CheckRunStatus.completed, + CheckRunConclusion.success, + CheckRunStatus.completed, + CheckRunConclusion.failure, + ], ); }); @@ -3135,7 +3257,7 @@ targets: // see the blend of fusionCiYaml and singleCiYaml expect(captured[0][0].name, 'Linux engine_build'); - expect(checkRuns, hasLength(2)); + expect(checkRuns, hasLength(3)); verify( mockGithubChecksUtil.updateCheckRun( any, @@ -3289,7 +3411,7 @@ targets: captureAny, output: captureAnyNamed('output'), ), - ).called(1); + ).called(2); final result = verify( luci.scheduleMergeGroupBuilds( targets: captureAnyNamed('targets'), @@ -3302,7 +3424,7 @@ targets: ['Linux engine_build', 'Mac engine_build'], ); - expect(checkRuns, hasLength(1)); + expect(checkRuns, hasLength(2)); verifyNever( mockGithubChecksUtil.updateCheckRun( any, @@ -3548,7 +3670,7 @@ targets: captureAny, output: captureAnyNamed('output'), ), - ).called(1); + ).called(2); final result = verify( luci.scheduleMergeGroupBuilds( targets: captureAnyNamed('targets'), @@ -3561,7 +3683,7 @@ targets: ['Mac engine_build'], ); - expect(checkRuns, hasLength(1)); + expect(checkRuns, hasLength(2)); verifyNever( mockGithubChecksUtil.updateCheckRun( any, @@ -3684,7 +3806,7 @@ targets: captureAny, output: captureAnyNamed('output'), ), - ).called(1); + ).called(2); final result = verify( luci.scheduleMergeGroupBuilds( targets: captureAnyNamed('targets'), @@ -3697,11 +3819,12 @@ targets: ['Linux engine_build', 'Mac engine_build'], ); - expect(checkRuns, hasLength(1)); + expect(checkRuns, hasLength(2)); // Expect the merge queue guard to be completed with failure. - final mergeQueueGuard = checkRuns.single; - expect(mergeQueueGuard.name, Config.kMergeQueueLockName); + final mergeQueueGuard = checkRuns.firstWhere( + (element) => element.name == Config.kMergeQueueLockName, + ); expect( verify( await mockGithubChecksUtil.updateCheckRun( @@ -3881,7 +4004,7 @@ targets: final pullRequest = generatePullRequest(); final checkRunGuard = generateCheckRun( 1234, - name: Config.kMergeQueueLockName, + name: Config.kFlutterPresubmitsName, ); await PrCheckRuns.initializeDocument( diff --git a/packages/cocoon_integration_test/lib/src/utilities/mocks.mocks.dart b/packages/cocoon_integration_test/lib/src/utilities/mocks.mocks.dart index 37efe1c58..7ca7667c8 100644 --- a/packages/cocoon_integration_test/lib/src/utilities/mocks.mocks.dart +++ b/packages/cocoon_integration_test/lib/src/utilities/mocks.mocks.dart @@ -733,6 +733,14 @@ class MockConfig extends _i1.Mock implements _i2.Config { _i1.throwOnMissingStub(this); } + @override + _i5.Client get httpClient => + (super.noSuchMethod( + Invocation.getter(#httpClient), + returnValue: _FakeClient_3(this, Invocation.getter(#httpClient)), + ) + as _i5.Client); + @override Set<_i7.RepositorySlug> get supportedRepos => (super.noSuchMethod( @@ -2164,6 +2172,14 @@ class MockGithubService extends _i1.Mock implements _i9.GithubService { ) as _i13.Future>); + @override + _i13.Future isTeamMember(String? team, String? user, String? org) => + (super.noSuchMethod( + Invocation.method(#isTeamMember, [team, user, org]), + returnValue: _i13.Future.value(false), + ) + as _i13.Future); + @override _i13.Future<_i7.PullRequest> createPullRequest( _i7.RepositorySlug? slug, { @@ -5684,12 +5700,13 @@ class MockScheduler extends _i1.Mock implements _i17.Scheduler { _i7.RepositorySlug? slug, String? headSha, { String? detailsUrl, + required bool? isUnifiedCheckRun, }) => (super.noSuchMethod( Invocation.method( #lockMergeGroupChecks, [slug, headSha], - {#detailsUrl: detailsUrl}, + {#detailsUrl: detailsUrl, #isUnifiedCheckRun: isUnifiedCheckRun}, ), returnValue: _i13.Future<_i7.CheckRun>.value( _FakeCheckRun_20( @@ -5697,13 +5714,45 @@ class MockScheduler extends _i1.Mock implements _i17.Scheduler { Invocation.method( #lockMergeGroupChecks, [slug, headSha], - {#detailsUrl: detailsUrl}, + { + #detailsUrl: detailsUrl, + #isUnifiedCheckRun: isUnifiedCheckRun, + }, ), ), ), ) as _i13.Future<_i7.CheckRun>); + @override + _i13.Future<_i7.CheckRun?> createAwaitingCicdLabelCheckRun( + _i7.RepositorySlug? slug, + String? headSha, + ) => + (super.noSuchMethod( + Invocation.method(#createAwaitingCicdLabelCheckRun, [ + slug, + headSha, + ]), + returnValue: _i13.Future<_i7.CheckRun?>.value(), + ) + as _i13.Future<_i7.CheckRun?>); + + @override + _i13.Future resolveAwaitingCicdLabelCheckRun( + _i7.RepositorySlug? slug, + String? headSha, + ) => + (super.noSuchMethod( + Invocation.method(#resolveAwaitingCicdLabelCheckRun, [ + slug, + headSha, + ]), + returnValue: _i13.Future.value(), + returnValueForMissingStub: _i13.Future.value(), + ) + as _i13.Future); + @override _i13.Future unlockMergeQueueGuard( _i7.RepositorySlug? slug,