From 3cc326aecbb645aef7de7ce90a31dd290cd3e03d Mon Sep 17 00:00:00 2001 From: LiviaMedeiros Date: Wed, 1 Mar 2023 20:44:21 +0800 Subject: [PATCH 1/5] fix: treat `fast-track` with not enough approvals as non-fatal --- lib/pr_checker.js | 10 +++++----- test/unit/pr_checker.test.js | 12 ++++++++---- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/lib/pr_checker.js b/lib/pr_checker.js index cb47da14..6f39c51e 100644 --- a/lib/pr_checker.js +++ b/lib/pr_checker.js @@ -138,7 +138,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'); @@ -184,13 +184,13 @@ export default class PRChecker { collaborators.includes(r.user.login.toLowerCase())).length; if (requester === pr.author.login && approvals < 2) { - cli.error('The fast-track request requires' + + cli.warn('The fast-track request requires' + " at least two collaborators' approvals (👍)."); - return false; + isFastTracked = false; } else if (approvals === 0) { - cli.error('The fast-track request requires' + + cli.warn('The fast-track request requires' + " at least one collaborator's approval (👍)."); - return false; + isFastTracked = false; } } diff --git a/test/unit/pr_checker.test.js b/test/unit/pr_checker.test.js index 356b2937..050894b9 100644 --- a/test/unit/pr_checker.test.js +++ b/test/unit/pr_checker.test.js @@ -643,9 +643,11 @@ describe('PRChecker', () => { info: [['This PR was created on Fri, 30 Nov 2018 17:50:44 GMT'], ['This PR is being fast-tracked']], - error: + warn: [['The fast-track request requires at' + - " least two collaborators' approvals (👍)."]] + " least two collaborators' approvals (👍)."]], + error: + [['This PR needs to wait 24 more hours to land']] }; const pr = Object.assign({}, firstTimerPR, { @@ -693,9 +695,11 @@ describe('PRChecker', () => { info: [['This PR was created on Fri, 30 Nov 2018 17:50:44 GMT'], ['This PR is being fast-tracked']], - error: + warn: [['The fast-track request requires at' + - " least one collaborator's approval (👍)."]] + " least one collaborator's approval (👍)."]], + error: + [['This PR needs to wait 24 more hours to land']] }; const pr = Object.assign({}, firstTimerPR, { From 77cb59b443dec86fd78f70af4e42e2ed88f38c8f Mon Sep 17 00:00:00 2001 From: LiviaMedeiros Date: Wed, 1 Mar 2023 23:41:19 +0800 Subject: [PATCH 2/5] fix: update error messages --- lib/pr_checker.js | 24 ++++++++++++++---------- test/unit/pr_checker.test.js | 14 ++++++-------- 2 files changed, 20 insertions(+), 18 deletions(-) diff --git a/lib/pr_checker.js b/lib/pr_checker.js index 6f39c51e..afc80a73 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 { @@ -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,13 @@ 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.warn('The fast-track request requires' + - " at least two collaborators' approvals (👍)."); - isFastTracked = false; - } else if (approvals === 0) { - cli.warn('The fast-track request requires' + - " at least one collaborator's approval (👍)."); + const missingFastTrackApprovals = FAST_TRACK_MIN_APPROVALS - approvals - + (requester === pr.author.login ? 0 : 1); + if (missingFastTrackApprovals > 0) { isFastTracked = false; + fastTrackAppendix = ' (or 0 minutes if there is ' + + `${missingFastTrackApprovals} more approval (👍) of ` + + 'the fast-track request from collaborators).'; } } @@ -211,10 +212,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 +227,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 050894b9..63f67685 100644 --- a/test/unit/pr_checker.test.js +++ b/test/unit/pr_checker.test.js @@ -643,11 +643,10 @@ describe('PRChecker', () => { info: [['This PR was created on Fri, 30 Nov 2018 17:50:44 GMT'], ['This PR is being fast-tracked']], - warn: - [['The fast-track request requires at' + - " least two collaborators' approvals (👍)."]], error: - [['This PR needs to wait 24 more hours to land']] + [['This PR needs to wait 24 more hours to land (or 0 minutes if ' + + 'there is 2 more approval (👍) of the fast-track request from ' + + 'collaborators).']] }; const pr = Object.assign({}, firstTimerPR, { @@ -695,11 +694,10 @@ describe('PRChecker', () => { info: [['This PR was created on Fri, 30 Nov 2018 17:50:44 GMT'], ['This PR is being fast-tracked']], - warn: - [['The fast-track request requires at' + - " least one collaborator's approval (👍)."]], error: - [['This PR needs to wait 24 more hours to land']] + [['This PR needs to wait 24 more hours to land (or 0 minutes if ' + + 'there is 1 more approval (👍) of the fast-track request from ' + + 'collaborators).']] }; const pr = Object.assign({}, firstTimerPR, { From cb6778e30d13208be88b2d116a4b5aa0db26b5bb Mon Sep 17 00:00:00 2001 From: LiviaMedeiros Date: Wed, 1 Mar 2023 23:55:52 +0800 Subject: [PATCH 3/5] fix: error message fixup --- lib/pr_checker.js | 4 ++-- test/unit/pr_checker.test.js | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/pr_checker.js b/lib/pr_checker.js index afc80a73..55c90821 100644 --- a/lib/pr_checker.js +++ b/lib/pr_checker.js @@ -189,8 +189,8 @@ export default class PRChecker { (requester === pr.author.login ? 0 : 1); if (missingFastTrackApprovals > 0) { isFastTracked = false; - fastTrackAppendix = ' (or 0 minutes if there is ' + - `${missingFastTrackApprovals} more approval (👍) of ` + + fastTrackAppendix = ' (or 0 hours if there is ' + + `${missingFastTrackApprovals} more approval(s) (👍) of ` + 'the fast-track request from collaborators).'; } } diff --git a/test/unit/pr_checker.test.js b/test/unit/pr_checker.test.js index 63f67685..1bcdf671 100644 --- a/test/unit/pr_checker.test.js +++ b/test/unit/pr_checker.test.js @@ -644,8 +644,8 @@ describe('PRChecker', () => { [['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 24 more hours to land (or 0 minutes if ' + - 'there is 2 more approval (👍) of the fast-track request from ' + + [['This PR needs to wait 24 more hours to land (or 0 hours if ' + + 'there is 2 more approval(s) (👍) of the fast-track request from ' + 'collaborators).']] }; @@ -695,8 +695,8 @@ describe('PRChecker', () => { [['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 24 more hours to land (or 0 minutes if ' + - 'there is 1 more approval (👍) of the fast-track request from ' + + [['This PR needs to wait 24 more hours to land (or 0 hours if ' + + 'there is 1 more approval(s) (👍) of the fast-track request from ' + 'collaborators).']] }; From b22cb6aae197bce55b165f621f5dca02c06e7a3a Mon Sep 17 00:00:00 2001 From: LiviaMedeiros Date: Thu, 2 Mar 2023 00:09:37 +0800 Subject: [PATCH 4/5] fix: error message fixup --- lib/pr_checker.js | 6 ++++-- test/unit/pr_checker.test.js | 4 ++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/pr_checker.js b/lib/pr_checker.js index 55c90821..e48bf2d5 100644 --- a/lib/pr_checker.js +++ b/lib/pr_checker.js @@ -189,8 +189,10 @@ export default class PRChecker { (requester === pr.author.login ? 0 : 1); if (missingFastTrackApprovals > 0) { isFastTracked = false; - fastTrackAppendix = ' (or 0 hours if there is ' + - `${missingFastTrackApprovals} more approval(s) (👍) of ` + + fastTrackAppendix = ' (or 0 hours if there ' + + `${missingFastTrackApprovals === 1 ? 'is' : 'are'} ` + + `${missingFastTrackApprovals} more approval` + + `${missingFastTrackApprovals === 1 ? '' : 's'} (👍) of ` + 'the fast-track request from collaborators).'; } } diff --git a/test/unit/pr_checker.test.js b/test/unit/pr_checker.test.js index 1bcdf671..81aa87df 100644 --- a/test/unit/pr_checker.test.js +++ b/test/unit/pr_checker.test.js @@ -645,7 +645,7 @@ describe('PRChecker', () => { ['This PR is being fast-tracked']], error: [['This PR needs to wait 24 more hours to land (or 0 hours if ' + - 'there is 2 more approval(s) (👍) of the fast-track request from ' + + 'there are 2 more approvals (👍) of the fast-track request from ' + 'collaborators).']] }; @@ -696,7 +696,7 @@ describe('PRChecker', () => { ['This PR is being fast-tracked']], error: [['This PR needs to wait 24 more hours to land (or 0 hours if ' + - 'there is 1 more approval(s) (👍) of the fast-track request from ' + + 'there is 1 more approval (👍) of the fast-track request from ' + 'collaborators).']] }; From 14a94b6505c4b806fe95543875ea8213665b1bce Mon Sep 17 00:00:00 2001 From: LiviaMedeiros Date: Thu, 2 Mar 2023 14:36:04 +0800 Subject: [PATCH 5/5] test: add test for fast-track with insufficient approvals --- test/unit/pr_checker.test.js | 45 ++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/test/unit/pr_checker.test.js b/test/unit/pr_checker.test.js index 81aa87df..441200e5 100644 --- a/test/unit/pr_checker.test.js +++ b/test/unit/pr_checker.test.js @@ -729,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();