todo: warn new commits after review (nodejs#65)

priyank-p · joyeecheung · commit e829262b1449 · 2017-11-06T21:05:48.000+08:00
warn new commits after review
also added `lint-fix` command
diff --git a/README.md b/README.md
@@ -27,7 +27,7 @@ Then create a file named `.ncurc` under your `$HOME` directory (`~/.ncurc`);
 
 ```
 {
-  "username": "you_github_username"
+  "username": "you_github_username",
   "token": "token_that_you_created"
 }
 ```
@@ -90,7 +90,7 @@ $ git commit --amend -F msg.txt
 - [x] Check if commiters match authors
 - [x] Check 48-hour wait
 - [x] Check two TSC approval for semver-major
-- [ ] Warn new commits after reviews
+- [x] Warn new commits after reviews
 - [ ] Check number of files changed (request pre-backport)
 
 ### Contributing
diff --git a/lib/pr_checker.js b/lib/pr_checker.js
@@ -34,6 +34,10 @@ class PRChecker {
     this.reviewers = reviewers;
     this.pr = pr;
     this.comments = comments;
+    // this.reviews and this.commits must
+    // be in order as received from github api
+    // to check if new commits were pushed after
+    // the last review.
     this.reviews = reviews;
     this.commits = commits;
     this.collaboratorEmails = new Set(
@@ -44,14 +48,15 @@ class PRChecker {
   checkAll() {
     const status = [
       this.checkReviews(),
+      this.checkCommitsAfterReview(),
       this.checkPRWait(new Date()),
       this.checkCI()
     ];
 
     if (this.authorIsNew()) {
       status.push(this.checkAuthor());
     }
-    // TODO: maybe invalidate review after new commits?
+
     // TODO: check for pre-backport, Github API v4
     // does not support reading files changed
 
@@ -231,6 +236,38 @@ class PRChecker {
     // 3. is not authored by the people opening the PR
     return true;
   }
+
+  checkCommitsAfterReview() {
+    const {
+      commits, reviews, logger
+    } = this;
+
+    if (reviews.length < 1) {
+      return true;
+    }
+
+    const commitIndex = commits.length - 1;
+    const reviewIndex = reviews.length - 1;
+    const lastCommit = commits[commitIndex].commit;
+    const lastReview = reviews[reviewIndex];
+
+    const commitDate = lastCommit.committedDate;
+    const reviewDate = lastReview.publishedAt;
+
+    let status = true;
+    if (commitDate > reviewDate) {
+      logger.warn('Changes were pushed since the last review:');
+      commits.forEach((commit) => {
+        commit = commit.commit;
+        if (commit.committedDate > reviewDate) {
+          status = false;
+          logger.warn(`- ${commit.message.split('\n')[0]}`);
+        }
+      });
+    }
+
+    return status;
+  }
 }
 
 module.exports = PRChecker;
diff --git a/package.json b/package.json
@@ -10,6 +10,7 @@
     "test": "npm run test-unit && npm run lint",
     "test-unit": "mocha --require intelli-espower-loader test/unit/*.test.js --exit",
     "test-all": "mocha --require intelli-espower-loader test/**/*.test.js --exit",
+    "lint-fix": "eslint . --fix",
     "coverage": "nyc --reporter=html --reporter=text --reporter=text-summary npm test",
     "coverage-all": "nyc --reporter=lcov --reporter=text --reporter=text-summary npm run test-all",
     "lint": "eslint . --cache",
diff --git a/test/fixtures/data.js b/test/fixtures/data.js
@@ -31,6 +31,15 @@ const simpleCommits = readJSON('simple_commits.json');
 
 const collabArr = readJSON('collaborators.json');
 
+const singleCommitAfterReview = {
+  commits: readJSON('single_commit_after_review_commits.json'),
+  reviews: readJSON('single_commit_after_review_reviews.json')
+};
+const multipleCommitsAfterReview = {
+  commits: readJSON('multiple_commits_after_review_commits.json'),
+  reviews: readJSON('multiple_commits_after_review_reviews.json')
+};
+
 collabArr.forEach((c) => {
   Object.setPrototypeOf(c, Collaborator.prototype);
 });
@@ -59,6 +68,8 @@ module.exports = {
   commentsWithLGTM,
   oddCommits,
   simpleCommits,
+  singleCommitAfterReview,
+  multipleCommitsAfterReview,
   collaborators,
   firstTimerPR,
   semverMajorPR,
diff --git a/test/fixtures/multiple_commits_after_review_commits.json b/test/fixtures/multiple_commits_after_review_commits.json
@@ -0,0 +1,21 @@
+[{
+  "commit": {
+    "committedDate": "2017-10-25T11:27:02Z",
+    "oid": "f230e691459f8b0f448e1eec1b83fbf7f708eba3",
+    "bodyText": "src: add requested feature",
+    "message": "src: add requested feature\nkjlks jskld ksa",
+    "author": {
+      "login": "bar"
+    }
+  }
+},{
+  "commit": {
+    "committedDate": "2017-10-25T12:42:02Z",
+    "oid": "9416475a6dc1b27c3e343dcbc07674a439c88db1",
+    "bodyText": "nit: edit mistakes",
+    "message": "nit: fix errors\n fixed requested errors",
+    "author": {
+      "login": "bar"
+    }
+  }
+}]
diff --git a/test/fixtures/multiple_commits_after_review_reviews.json b/test/fixtures/multiple_commits_after_review_reviews.json
@@ -0,0 +1,9 @@
+[{
+  "bodyText": "Good idea",
+  "state": "APPROVED",
+  "author": {
+    "login": "foo"
+  },
+  "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-89923489",
+  "publishedAt": "2017-09-23T11:19:25Z"
+}]
diff --git a/test/fixtures/single_commit_after_review_commits.json b/test/fixtures/single_commit_after_review_commits.json
@@ -0,0 +1,11 @@
+[{
+  "commit": {
+    "committedDate": "2017-10-25T11:27:02Z",
+    "oid": "6c0945cbeea2cbbc97d13a3d9e3fe68bd145b985",
+    "bodyText": "src: fix issue with es-modules",
+    "message": "single commit was pushed after review",
+    "author": {
+      "login": "bar"
+    }
+  }
+}]
diff --git a/test/fixtures/single_commit_after_review_reviews.json b/test/fixtures/single_commit_after_review_reviews.json
@@ -0,0 +1,9 @@
+[{
+  "bodyText": "Good idea",
+  "state": "APPROVED",
+  "author": {
+    "login": "foo"
+  },
+  "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71480624",
+  "publishedAt": "2017-10-24T11:19:25Z"
+}]
diff --git a/test/unit/pr_checker.test.js b/test/unit/pr_checker.test.js
@@ -14,6 +14,8 @@ const {
   rejectingReviews,
   commentsWithCI,
   commentsWithLGTM,
+  singleCommitAfterReview,
+  multipleCommitsAfterReview,
   oddCommits,
   simpleCommits,
   collaborators,
@@ -38,13 +40,16 @@ describe('PRChecker', () => {
     let checkCIStub;
     let authorIsNewStub;
     let checkAuthorStub;
+    let checkCommitsAfterReviewStub;
 
     before(() => {
       checkReviewsStub = sinon.stub(checker, 'checkReviews');
       checkPRWaitStub = sinon.stub(checker, 'checkPRWait');
       checkCIStub = sinon.stub(checker, 'checkCI');
       authorIsNewStub = sinon.stub(checker, 'authorIsNew').returns(true);
       checkAuthorStub = sinon.stub(checker, 'checkAuthor');
+      checkCommitsAfterReviewStub =
+        sinon.stub(checker, 'checkCommitsAfterReview');
     });
 
     after(() => {
@@ -53,6 +58,7 @@ describe('PRChecker', () => {
       checkCIStub.restore();
       authorIsNewStub.restore();
       checkAuthorStub.restore();
+      checkCommitsAfterReviewStub.restore();
     });
 
     it('should run necessary checks', () => {
@@ -63,6 +69,7 @@ describe('PRChecker', () => {
       assert.strictEqual(checkCIStub.calledOnce, true);
       assert.strictEqual(authorIsNewStub.calledOnce, true);
       assert.strictEqual(checkAuthorStub.calledOnce, true);
+      assert.strictEqual(checkCommitsAfterReviewStub.calledOnce, true);
     });
   });
 
@@ -300,4 +307,91 @@ describe('PRChecker', () => {
       assert.deepStrictEqual(logger.logs, expectedLogs);
     });
   });
+
+  describe('checkCommitsAfterReview', () => {
+    let logger = new TestLogger();
+
+    afterEach(() => {
+      logger.clear();
+    });
+
+    it('should warn about commit pushed since the last review', () => {
+      const { commits, reviews } = singleCommitAfterReview;
+
+      const expectedLogs = {
+        warn: [
+          [ 'Changes were pushed since the last review:' ],
+          [ '- single commit was pushed after review' ]
+        ],
+        info: [],
+        trace: [],
+        error: []
+      };
+
+      const checker = new PRChecker(logger, {
+        pr: firstTimerPR,
+        reviewers: allGreenReviewers,
+        comments: commentsWithLGTM,
+        collaborators,
+        reviews,
+        commits
+      });
+
+      let status = checker.checkCommitsAfterReview();
+      assert.deepStrictEqual(status, false);
+      assert.deepStrictEqual(logger.logs, expectedLogs);
+    });
+
+    it('should warn about multiple commits since the last review', () => {
+      const { commits, reviews } = multipleCommitsAfterReview;
+
+      const expectedLogs = {
+        warn: [
+          [ 'Changes were pushed since the last review:' ],
+          [ '- src: add requested feature' ],
+          [ '- nit: fix errors' ]
+        ],
+        info: [],
+        trace: [],
+        error: []
+      };
+
+      const checker = new PRChecker(logger, {
+        pr: firstTimerPR,
+        reviewers: allGreenReviewers,
+        comments: commentsWithLGTM,
+        collaborators,
+        reviews,
+        commits
+      });
+
+      let status = checker.checkCommitsAfterReview();
+      assert.deepStrictEqual(status, false);
+      assert.deepStrictEqual(logger.logs, expectedLogs);
+    });
+
+    it('should skip the check if there are no reviews', () => {
+      const { commits } = multipleCommitsAfterReview;
+
+      const expectedLogs = {
+        warn: [],
+        info: [],
+        trace: [],
+        error: []
+      };
+
+      const checker = new PRChecker(logger, {
+        pr: firstTimerPR,
+        reviewers: allGreenReviewers,
+        comments: commentsWithLGTM,
+        reviews: [],
+        collaborators,
+        commits
+      });
+
+      let status = checker.checkCommitsAfterReview();
+      assert.deepStrictEqual(status, true);
+      assert.deepStrictEqual(logger.logs, expectedLogs);
+    });
+  });
 });
