From 8ad4b377c270e8495a8cf8fece1e6a439304e327 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Tue, 8 Nov 2022 05:03:25 -0800 Subject: [PATCH] fix: use case-insensitive string comparison for fast-track approvals (#658) I noticed today that a PR that had sufficient fast-track approvals was being rejected as needing one more approval. This is because one of the approvers' had a GitHub handle where the case in the README (`linkgoron`) did not match the case returned by GitHub (`Linkgoron`). Since GitHub handles are case-insensitive, this change makes the comparison case-insensitive. One might be tempted to think that we need to do the same for the list of TSC members, but handles are never compared there so it is not necessary. --- lib/pr_checker.js | 4 +- ...ts_with_two_fast_track_different_case.json | 16 +++++++ test/fixtures/data.js | 2 + test/unit/pr_checker.test.js | 48 +++++++++++++++++++ 4 files changed, 68 insertions(+), 2 deletions(-) create mode 100644 test/fixtures/comments_with_two_fast_track_different_case.json diff --git a/lib/pr_checker.js b/lib/pr_checker.js index 8128ef0d..10968da5 100644 --- a/lib/pr_checker.js +++ b/lib/pr_checker.js @@ -177,11 +177,11 @@ export default class PRChecker { } const [, requester] = comment.bodyText.match(FAST_TRACK_RE); const collaborators = Array.from(this.data.collaborators.values(), - (c) => c.login); + (c) => c.login.toLowerCase()); const approvals = comment.reactions.nodes.filter((r) => r.user.login !== requester && r.user.login !== pr.author.login && - collaborators.includes(r.user.login)).length; + collaborators.includes(r.user.login.toLowerCase())).length; if (requester === pr.author.login && approvals < 2) { cli.error('The fast-track request requires' + diff --git a/test/fixtures/comments_with_two_fast_track_different_case.json b/test/fixtures/comments_with_two_fast_track_different_case.json new file mode 100644 index 00000000..19dc7db5 --- /dev/null +++ b/test/fixtures/comments_with_two_fast_track_different_case.json @@ -0,0 +1,16 @@ +[ + { + "publishedAt": "2017-10-22T05:16:36.458Z", + "bodyText": "Fast-track has been requested by @bar. Please 👍 to approve.", + "author": { + "login": "github-actions" + }, + "reactions": { + "nodes": [ + { "user": { "login": "Bar" } }, + { "user": { "login": "Foo" } }, + { "user": { "login": "BAZ" } } + ] + } + } +] diff --git a/test/fixtures/data.js b/test/fixtures/data.js index f53f5182..b60fe933 100644 --- a/test/fixtures/data.js +++ b/test/fixtures/data.js @@ -37,6 +37,8 @@ export const requestingChangesReviews = export const commentsWithFastTrack = readJSON('comments_with_fast_track.json'); export const commentsWithTwoFastTrack = readJSON('comments_with_two_fast_track.json'); +export const commentsWithTwoFastTrackDifferentCase = + readJSON('comments_with_two_fast_track_different_case.json'); export const commentsWithFastTrackInsuffientApprovals = readJSON('comments_with_fast_track_insufficient_approvals.json'); export const commentsWithCI = readJSON('comments_with_ci.json'); diff --git a/test/unit/pr_checker.test.js b/test/unit/pr_checker.test.js index 761bb83b..86ff437c 100644 --- a/test/unit/pr_checker.test.js +++ b/test/unit/pr_checker.test.js @@ -18,6 +18,7 @@ import { noReviewers, commentsWithFastTrack, commentsWithTwoFastTrack, + commentsWithTwoFastTrackDifferentCase, commentsWithFastTrackInsuffientApprovals, commentsWithCI, commentsWithFailedCI, @@ -582,6 +583,53 @@ describe('PRChecker', () => { cli.assertCalledWith(expectedLogs); }); + it('should compare collaborator handles as case-insensitive', () => { + const cli = new TestCLI(); + + const expectedLogs = { + ok: + [['Approvals: 4'], + ['- Foo User (@foo): https://github.com/nodejs/node/pull/16438#pullrequestreview-71480624'], + ['- Quux User (@Quux): LGTM'], + ['- Baz User (@Baz): https://github.com/nodejs/node/pull/16438#pullrequestreview-71488236'], + ['- Bar User (@bar) (TSC): lgtm']], + info: + [['This PR was created on Fri, 30 Nov 2018 17:50:44 GMT'], + ['This PR is being fast-tracked']] + }; + + const pr = Object.assign({}, firstTimerPR, { + author: { + login: 'bar' + }, + createdAt: LT_48H, + labels: { + nodes: [ + { name: 'fast-track' } + ] + } + }); + + const data = { + pr, + reviewers: allGreenReviewers, + comments: commentsWithTwoFastTrackDifferentCase, + 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 with 1 fast-track approval from the pr author', () => { const cli = new TestCLI();