Skip to content

Commit 93db417

Browse files
Piinksjtmcdole
andauthored
[flutter-gold] Consider neutral checks completed (#5036)
Fixes flutter/flutter#185832 This PR improves how Flutter Gold status updates are computed by accounting for completed checks that end in a `NEUTRAL` conclusion (alongside `SUCCESS`). It also streamlines logging and fortifies the check status evaluation. * Updates the evaluation logic so that PR checks ending in NEUTRAL are no longer grouped alongside incomplete or failing tests. * Added explicit status verification for check runs. If a check run does not have the status 'COMPLETED', it is now safely and reliably classified as incomplete. * Moved the log.debug statement to only capture check runs that are truly incomplete or failing, significantly reducing log output noise. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read the [Flutter Style Guide] _recently_, and have followed its advice. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/master/docs/contributing/Style-guide-for-Flutter-repo.md [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/master/docs/contributing/Chat.md --------- Co-authored-by: John "codefu" McDole <john@mcdole.org> Co-authored-by: John McDole <codefu@google.com>
1 parent b375de7 commit 93db417

2 files changed

Lines changed: 66 additions & 4 deletions

File tree

app_dart/lib/src/request_handlers/push_gold_status_to_github.dart

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,6 @@ final class PushGoldStatusToGithub extends ApiRequestHandler {
136136
checkRuns = checkRuns ?? <Map<String, dynamic>>[];
137137
log.debug('This PR has ${checkRuns.length} checks.');
138138
for (var checkRun in checkRuns) {
139-
log.debug('Check run: $checkRun');
140139
final name = checkRun['name'].toLowerCase() as String;
141140
if (slug == Config.flutterSlug) {
142141
if (const <String>[
@@ -160,8 +159,13 @@ final class PushGoldStatusToGithub extends ApiRequestHandler {
160159
runsGoldenFileTests = true;
161160
}
162161
}
163-
if (checkRun['conclusion'] == null ||
164-
checkRun['conclusion'].toUpperCase() != 'SUCCESS') {
162+
const successfulConclusion = {'SUCCESS', 'NEUTRAL'};
163+
164+
if (checkRun['status']?.toUpperCase() != 'COMPLETED' ||
165+
!successfulConclusion.contains(
166+
checkRun['conclusion']?.toUpperCase(),
167+
)) {
168+
log.debug('Incomplete check run: $checkRun');
165169
incompleteChecks.add(name);
166170
}
167171
}

app_dart/test/request_handlers/push_gold_status_to_github_test.dart

Lines changed: 59 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -361,6 +361,64 @@ void main() {
361361
);
362362
});
363363

364+
test(
365+
'same commit, neutral checks complete, last status complete',
366+
() async {
367+
// Same commit
368+
final pr = newPullRequest(123, 'abc', 'master');
369+
prsFromGitHub = <PullRequest>[pr];
370+
371+
firestore.putDocument(
372+
newGithubGoldStatus(
373+
slug,
374+
pr,
375+
GithubGoldStatus.statusCompleted,
376+
'abc',
377+
config.flutterGoldSuccessValue!,
378+
),
379+
);
380+
381+
// Neutral checks complete
382+
checkRuns = <dynamic>[
383+
<String, String>{
384+
'name': 'framework',
385+
'status': 'completed',
386+
'conclusion': 'neutral',
387+
},
388+
<String, String>{
389+
'name': 'web engine',
390+
'status': 'completed',
391+
'conclusion': 'neutral',
392+
},
393+
];
394+
395+
final body = await tester.get(handler);
396+
expect(body, same(Response.emptyOk));
397+
expect(
398+
firestore,
399+
existsInStorage(fs.GithubGoldStatus.metadata, [
400+
isGithubGoldStatus.hasUpdates(0),
401+
]),
402+
);
403+
expect(log, hasNoWarningsOrHigher);
404+
405+
// Should not apply labels or make comments
406+
verifyNever(
407+
issuesService.addLabelsToIssue(slug, pr.number!, <String>[
408+
kGoldenFileLabel,
409+
]),
410+
);
411+
412+
verifyNever(
413+
issuesService.createComment(
414+
slug,
415+
pr.number!,
416+
argThat(contains(config.flutterGoldCommentID(pr))),
417+
),
418+
);
419+
},
420+
);
421+
364422
test(
365423
'same commit, checks complete, last status & gold status is running/awaiting triage, should not comment',
366424
() async {
@@ -1160,7 +1218,7 @@ void main() {
11601218
checkRuns = <dynamic>[
11611219
<String, String>{
11621220
'name': 'framework',
1163-
'completed': 'in_progress',
1221+
'status': 'completed',
11641222
'conclusion': 'success',
11651223
},
11661224
];

0 commit comments

Comments
 (0)