From 7506bbe8976ef765037eb6077503774a69dc02dc Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Wed, 8 Mar 2023 16:42:09 +0100 Subject: [PATCH] fix(pr_checker): do not count non-approving reviews Before this commit, any review after the last commit would disable the warning and let the CQ land the PR. This commit makes sure that only approving reviews from collaborators are taken into account to decide if a PR is ready to land. --- lib/pr_checker.js | 18 +++++++++++++++--- lib/queries/Reviews.gql | 1 + ...an_three_commits_after_review_reviews.json | 1 + ...multiple_commits_after_review_reviews.json | 1 + test/fixtures/reviews_approved.json | 7 +++++++ test/fixtures/reviews_requesting_changes.json | 6 ++++++ .../single_commit_after_review_reviews.json | 19 +++++++++++++++++++ test/unit/pr_checker.test.js | 16 +++++++--------- 8 files changed, 57 insertions(+), 12 deletions(-) diff --git a/lib/pr_checker.js b/lib/pr_checker.js index e48bf2d5a..5f223d3eb 100644 --- a/lib/pr_checker.js +++ b/lib/pr_checker.js @@ -30,6 +30,15 @@ const FAST_TRACK_RE = /^Fast-track has been requested by @(.+?)\. Please 👍 to 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'; +// eslint-disable-next-line no-extend-native +Array.prototype.findLastIndex ??= function findLastIndex(fn) { + const reversedIndex = Reflect.apply( + Array.prototype.findIndex, + this.slice().reverse(), + arguments); + return reversedIndex === -1 ? -1 : this.length - reversedIndex - 1; +}; + export default class PRChecker { /** * @param {{}} cli @@ -520,11 +529,14 @@ export default class PRChecker { } = this; const { maxCommits } = argv; - if (reviews.length < 1) { + const reviewIndex = reviews.findLastIndex( + review => review.authorCanPushToRepository && review.state === 'APPROVED' + ); + + if (reviewIndex === -1) { return false; } - const reviewIndex = reviews.length - 1; const reviewDate = reviews[reviewIndex].publishedAt; const afterCommits = []; @@ -537,7 +549,7 @@ export default class PRChecker { const totalCommits = afterCommits.length; if (totalCommits > 0) { - cli.warn('Commits were pushed since the last review:'); + cli.warn('Commits were pushed since the last approving review:'); const sliceLength = maxCommits === 0 ? totalCommits : -maxCommits; afterCommits.slice(sliceLength) .forEach(commit => { diff --git a/lib/queries/Reviews.gql b/lib/queries/Reviews.gql index cb9374f9d..4761fdcbe 100644 --- a/lib/queries/Reviews.gql +++ b/lib/queries/Reviews.gql @@ -13,6 +13,7 @@ query Reviews($prid: Int!, $owner: String!, $repo: String!, $after: String) { author { login } + authorCanPushToRepository url publishedAt } diff --git a/test/fixtures/more_than_three_commits_after_review_reviews.json b/test/fixtures/more_than_three_commits_after_review_reviews.json index 842cb67d7..039b51d34 100644 --- a/test/fixtures/more_than_three_commits_after_review_reviews.json +++ b/test/fixtures/more_than_three_commits_after_review_reviews.json @@ -4,6 +4,7 @@ "author": { "login": "foo" }, + "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-89923489", "publishedAt": "2017-07-23T11:19:25Z" }] diff --git a/test/fixtures/multiple_commits_after_review_reviews.json b/test/fixtures/multiple_commits_after_review_reviews.json index 7b5ae2d8a..1307b1ebd 100644 --- a/test/fixtures/multiple_commits_after_review_reviews.json +++ b/test/fixtures/multiple_commits_after_review_reviews.json @@ -4,6 +4,7 @@ "author": { "login": "foo" }, + "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-89923489", "publishedAt": "2017-09-23T11:19:25Z" }] diff --git a/test/fixtures/reviews_approved.json b/test/fixtures/reviews_approved.json index 179521643..4024c5ad9 100644 --- a/test/fixtures/reviews_approved.json +++ b/test/fixtures/reviews_approved.json @@ -4,6 +4,7 @@ "author": { "login": "foo" }, + "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71480624", "publishedAt": "2017-10-24T11:19:00Z" }, @@ -13,6 +14,7 @@ "author": { "login": "Baz" }, + "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71488392", "publishedAt": "2017-10-24T11:50:52Z" }, @@ -22,6 +24,7 @@ "author": { "login": "Baz" }, + "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-714882992", "publishedAt": "2017-10-24T12:30:52Z" }, @@ -31,6 +34,7 @@ "author": { "login": "Quux" }, + "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71817236", "publishedAt": "2017-10-24T14:49:01Z" }, @@ -40,6 +44,7 @@ "author": { "login": "Baz" }, + "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71488236", "publishedAt": "2017-10-24T14:49:02Z" }, @@ -49,6 +54,7 @@ "author": { "login": "Quo" }, + "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71817236", "publishedAt": "2017-10-24T19:09:52Z" }, @@ -58,6 +64,7 @@ "author": { "login": "bot" }, + "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71839232", "publishedAt": "2017-10-28T19:21:52Z" }] diff --git a/test/fixtures/reviews_requesting_changes.json b/test/fixtures/reviews_requesting_changes.json index 18a5417cf..7684e4ec4 100644 --- a/test/fixtures/reviews_requesting_changes.json +++ b/test/fixtures/reviews_requesting_changes.json @@ -5,6 +5,7 @@ "author": { "login": "foo" }, + "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71480624", "publishedAt": "2017-10-24T11:19:25Z" }, @@ -14,6 +15,7 @@ "author": { "login": "Baz" }, + "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71488392", "publishedAt": "2017-10-24T11:50:52Z" }, @@ -23,6 +25,7 @@ "author": { "login": "Baz" }, + "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-714882992", "publishedAt": "2017-10-24T12:30:52Z" }, @@ -32,6 +35,7 @@ "author": { "login": "Quo" }, + "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71817236", "publishedAt": "2017-10-24T19:09:52Z" }, @@ -41,6 +45,7 @@ "author": { "login": "bot" }, + "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71839232", "publishedAt": "2017-10-24T19:21:52Z" }, @@ -50,6 +55,7 @@ "author": { "login": "bar" }, + "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71482624", "publishedAt": "2017-10-24T11:27:02Z" }] diff --git a/test/fixtures/single_commit_after_review_reviews.json b/test/fixtures/single_commit_after_review_reviews.json index 7fdae6e26..c37c880c6 100644 --- a/test/fixtures/single_commit_after_review_reviews.json +++ b/test/fixtures/single_commit_after_review_reviews.json @@ -4,6 +4,25 @@ "author": { "login": "foo" }, + "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71480624", "publishedAt": "2017-10-24T11:19:25Z" +},{ + "bodyText": "Not sure about that last change", + "state": "COMMENTED", + "author": { + "login": "foo" + }, + "authorCanPushToRepository": true, + "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71480625", + "publishedAt": "2017-10-26T11:19:25Z" +},{ + "bodyText": "Good idea", + "state": "APPROVED", + "author": { + "login": "bar" + }, + "authorCanPushToRepository": false, + "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71480626", + "publishedAt": "2017-10-26T11:19:25Z" }] diff --git a/test/unit/pr_checker.test.js b/test/unit/pr_checker.test.js index 441200e50..74a6e92ee 100644 --- a/test/unit/pr_checker.test.js +++ b/test/unit/pr_checker.test.js @@ -2059,7 +2059,7 @@ describe('PRChecker', () => { const expectedLogs = { warn: [ - ['Commits were pushed since the last review:'], + ['Commits were pushed since the last approving review:'], ['- src: fix issue with es-modules'] ], info: [], @@ -2091,7 +2091,7 @@ describe('PRChecker', () => { const expectedLogs = { warn: [ - ['Commits were pushed since the last review:'], + ['Commits were pushed since the last approving review:'], ['- src: add requested feature'], ['- nit: edit mistakes'] ], @@ -2122,7 +2122,7 @@ describe('PRChecker', () => { const { commits, reviews } = moreThanThreeCommitsAfterReview; const expectedLogs = { warn: [ - ['Commits were pushed since the last review:'], + ['Commits were pushed since the last approving review:'], ['- src: add requested feature'], ['- nit: edit mistakes'], ['- final: we should be good to go'], @@ -2180,20 +2180,18 @@ describe('PRChecker', () => { commits: simpleCommits, collaborators, authorIsNew: () => true, - getThread() { - return PRData.prototype.getThread.call(this); - } + getThread: PRData.prototype.getThread }, {}, argv); const status = checker.checkCommitsAfterReview(); - assert.deepStrictEqual(status, true); + assert.strictEqual(status, true); }); it('should log as expected if passed 1 as flag', () => { const { commits, reviews } = moreThanThreeCommitsAfterReview; const expectedLogs = { warn: [ - ['Commits were pushed since the last review:'], + ['Commits were pushed since the last approving review:'], ['- final: we should be good to go'], ['...(use `--max-commits 4` to see the full list of commits)'] ], @@ -2224,7 +2222,7 @@ describe('PRChecker', () => { const { commits, reviews } = moreThanThreeCommitsAfterReview; const expectedLogs = { warn: [ - ['Commits were pushed since the last review:'], + ['Commits were pushed since the last approving review:'], ['...(use `--max-commits 4` to see the full list of commits)'] ], info: [],