From b324c99bca3a77be9076a208598b34196cf9413b Mon Sep 17 00:00:00 2001 From: Livia Medeiros Date: Sat, 4 Mar 2023 05:04:03 +0900 Subject: [PATCH] fix: treat `fast-track` with not enough approvals as non-fatal (#676) * fix: treat `fast-track` with not enough approvals as non-fatal * fix: update error messages * fix: error message fixup * fix: error message fixup * test: add test for fast-track with insufficient approvals --- lib/pr_checker.js | 30 ++++++++++++-------- test/unit/pr_checker.test.js | 55 +++++++++++++++++++++++++++++++++--- 2 files changed, 69 insertions(+), 16 deletions(-) diff --git a/lib/pr_checker.js b/lib/pr_checker.js index cb47da14..e48bf2d5 100644 --- a/lib/pr_checker.js +++ b/lib/pr_checker.js @@ -27,6 +27,7 @@ const WAIT_TIME_SINGLE_APPROVAL = 24 * 7; const GITHUB_SUCCESS_CONCLUSIONS = ['SUCCESS', 'NEUTRAL', 'SKIPPED']; const FAST_TRACK_RE = /^Fast-track has been requested by @(.+?)\. Please 👍 to approve\.$/; +const FAST_TRACK_MIN_APPROVALS = 2; const GIT_CONFIG_GUIDE_URL = 'https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork'; export default class PRChecker { @@ -138,7 +139,7 @@ export default class PRChecker { const { requestedChanges, approved } = reviewers; const labels = pr.labels.nodes.map((l) => l.name); - const isFastTracked = labels.includes('fast-track'); + let isFastTracked = labels.includes('fast-track'); const isCodeAndLearn = labels.includes('code-and-learn'); const isSemverMajor = labels.includes('semver-major'); @@ -168,6 +169,7 @@ export default class PRChecker { } } + let fastTrackAppendix = ''; if (isFastTracked) { const comment = [...this.comments].reverse().find((c) => FAST_TRACK_RE.test(c.bodyText)); @@ -183,14 +185,15 @@ export default class PRChecker { r.user.login !== pr.author.login && collaborators.includes(r.user.login.toLowerCase())).length; - if (requester === pr.author.login && approvals < 2) { - cli.error('The fast-track request requires' + - " at least two collaborators' approvals (👍)."); - return false; - } else if (approvals === 0) { - cli.error('The fast-track request requires' + - " at least one collaborator's approval (👍)."); - return false; + const missingFastTrackApprovals = FAST_TRACK_MIN_APPROVALS - approvals - + (requester === pr.author.login ? 0 : 1); + if (missingFastTrackApprovals > 0) { + isFastTracked = false; + fastTrackAppendix = ' (or 0 hours if there ' + + `${missingFastTrackApprovals === 1 ? 'is' : 'are'} ` + + `${missingFastTrackApprovals} more approval` + + `${missingFastTrackApprovals === 1 ? '' : 's'} (👍) of ` + + 'the fast-track request from collaborators).'; } } @@ -211,10 +214,12 @@ export default class PRChecker { if (timeLeftMulti === 0) { const timeLeftMins = this.waitTimeMultiApproval * 60 - minutesFromCreateTime; - cli.error(`This PR needs to wait ${timeLeftMins} more minutes to land`); + cli.error(`This PR needs to wait ${timeLeftMins} ` + + `more minutes to land${fastTrackAppendix}`); return false; } - cli.error(`This PR needs to wait ${timeLeftMulti} more hours to land`); + cli.error(`This PR needs to wait ${timeLeftMulti} more ` + + `hours to land${fastTrackAppendix}`); return false; } @@ -224,7 +229,8 @@ export default class PRChecker { } timeLeftMulti = timeLeftMulti < 0 || isFastTracked ? 0 : timeLeftMulti; cli.error(`This PR needs to wait ${timeLeftSingle} more hours to land ` + - `(or ${timeLeftMulti} hours if there is one more approval)`); + `(or ${timeLeftMulti} hours if there is one more approval)` + + fastTrackAppendix); return false; } } diff --git a/test/unit/pr_checker.test.js b/test/unit/pr_checker.test.js index 356b2937..441200e5 100644 --- a/test/unit/pr_checker.test.js +++ b/test/unit/pr_checker.test.js @@ -644,8 +644,9 @@ describe('PRChecker', () => { [['This PR was created on Fri, 30 Nov 2018 17:50:44 GMT'], ['This PR is being fast-tracked']], error: - [['The fast-track request requires at' + - " least two collaborators' approvals (👍)."]] + [['This PR needs to wait 24 more hours to land (or 0 hours if ' + + 'there are 2 more approvals (👍) of the fast-track request from ' + + 'collaborators).']] }; const pr = Object.assign({}, firstTimerPR, { @@ -694,8 +695,9 @@ describe('PRChecker', () => { [['This PR was created on Fri, 30 Nov 2018 17:50:44 GMT'], ['This PR is being fast-tracked']], error: - [['The fast-track request requires at' + - " least one collaborator's approval (👍)."]] + [['This PR needs to wait 24 more hours to land (or 0 hours if ' + + 'there is 1 more approval (👍) of the fast-track request from ' + + 'collaborators).']] }; const pr = Object.assign({}, firstTimerPR, { @@ -727,6 +729,51 @@ describe('PRChecker', () => { cli.assertCalledWith(expectedLogs); }); + it('should error when not enough approvals or fast-track approvals', () => { + const cli = new TestCLI(); + + const expectedLogs = { + ok: + [['Approvals: 1'], + ['- Foo User (@foo): https://github.com/nodejs/node/pull/16438#pullrequestreview-71480624']], + info: + [['This PR was created on Fri, 30 Nov 2018 17:50:44 GMT'], + ['This PR is being fast-tracked']], + error: + [['This PR needs to wait 144 more hours to land (or 24 hours if ' + + 'there is one more approval) (or 0 hours if there is 1 more ' + + 'approval (👍) of the fast-track request from collaborators).']] + }; + + const pr = Object.assign({}, firstTimerPR, { + createdAt: LT_48H, + labels: { + nodes: [ + { name: 'fast-track' } + ] + } + }); + + const data = { + pr, + reviewers: singleGreenReviewer, + comments: commentsWithFastTrackInsuffientApprovals, + reviews: approvingReviews, + commits: [], + collaborators, + authorIsNew: () => true, + getThread() { + return PRData.prototype.getThread.call(this); + } + }; + const checker = new PRChecker(cli, data, {}, argv); + + cli.clearCalls(); + const status = checker.checkReviewsAndWait(new Date(NOW)); + assert(!status); + cli.assertCalledWith(expectedLogs); + }); + it('should error when missing fast-track request comment', () => { const cli = new TestCLI();