fix(pr_checker): do not count non-approving reviews (#680)

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.
nodejs · Mar 18, 2023 · b9c443b · b9c443b
1 parent e0d6d0d
commit b9c443b
Show file tree

Hide file tree

Showing 8 changed files with 57 additions and 12 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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: [],