Fixing review state to APPROVED whe 'LGTM' in COMMENTED review (#72)

* Fixing review state to APPROVED whe 'LGTM' in COMMENTED review * Add tests cases and comments flag * Update README.md for new option
nodejs · Nov 8, 2017 · 1d05665 · 1d05665
1 parent 744a34d
commit 1d05665
Show file tree

Hide file tree

Showing 12 changed files with 103 additions and 27 deletions.
diff --git a/README.md b/README.md
@@ -52,11 +52,12 @@ get-metadata <identifier>
 Retrieves metadata for a PR and validates them against nodejs/node PR rules
 
 Options:
-  --version    Show version number                                     [boolean]
-  --owner, -o  GitHub owner of the PR repository                        [string]
-  --repo, -r   GitHub repository of the PR                              [string]
-  --file, -f   File to write the metadata in                            [string]
-  --help, -h   Show help                                               [boolean]
+  --version         Show version number                                [boolean]
+  --owner, -o       GitHub owner of the PR repository                   [string]
+  --repo, -r        GitHub repository of the PR                         [string]
+  --file, -f        File to write the metadata in                       [string]
+  --check-comments  Check for 'LGTM' in comments                        [boolean]
+  --help, -h        Show help                                          [boolean]
 ```
 
 Examples:

diff --git a/lib/args.js b/lib/args.js
@@ -37,6 +37,11 @@ function buildYargs(args = null) {
       describe: 'File to write the metadata in',
       type: 'string'
     })
+    .option('check-comments', {
+      demandOption: false,
+      describe: 'Check for \'LGTM\' in comments',
+      type: 'boolean'
+    })
     .help()
     .alias('help', 'h')
     .argv;
@@ -47,8 +52,10 @@ const PR_RE = new RegExp(
   '([0-9]+)(?:/(?:files)?)?$');
 
 function checkAndParseArgs(args) {
-  const { owner = 'nodejs', repo = 'node', identifier, file } = args;
-  const result = { owner, repo, file };
+  const {
+    owner = 'nodejs', repo = 'node', identifier, file, checkComments
+  } = args;
+  const result = { owner, repo, file, checkComments };
   if (!isNaN(identifier)) {
     result.prid = +identifier;
   } else {

diff --git a/lib/pr_checker.js b/lib/pr_checker.js
@@ -11,7 +11,7 @@ const WEEKDAY_WAIT = 48;
 const WEEKEND_WAIT = 72;
 
 const {
-  REVIEW_SOURCES: { FROM_COMMENT }
+  REVIEW_SOURCES: { FROM_COMMENT, FROM_REVIEW_COMMENT }
 } = require('./reviews');
 const {
   FIRST_TIME_CONTRIBUTOR, FIRST_TIMER
@@ -45,9 +45,9 @@ class PRChecker {
     );
   }
 
-  checkAll() {
+  checkAll(comments = false) {
     const status = [
-      this.checkReviews(),
+      this.checkReviews(comments),
       this.checkCommitsAfterReview(),
       this.checkPRWait(new Date()),
       this.checkCI()
@@ -75,7 +75,7 @@ class PRChecker {
     return hint;
   }
 
-  checkReviews() {
+  checkReviews(comments = false) {
     const {
       pr, logger, reviewers: { rejected, approved }
     } = this;
@@ -88,7 +88,7 @@ class PRChecker {
       let hint = this.getTSCHint(rejected);
       logger.warn(`Rejections: ${rejected.length}${hint}`);
       for (const { reviewer, review } of rejected) {
-        logger.warn(`${reviewer.getName()}) rejected in ${review.ref}`);
+        logger.warn(`${reviewer.getName()} rejected in ${review.ref}`);
       }
     }
     if (approved.length === 0) {
@@ -98,10 +98,13 @@ class PRChecker {
       let hint = this.getTSCHint(approved);
       logger.info(`Approvals: ${approved.length}${hint}`);
 
-      for (const { reviewer, review } of approved) {
-        if (review.source === FROM_COMMENT) {
-          logger.info(
-            `${reviewer.getName()}) approved in via LGTM in comments`);
+      if (comments) {
+        for (const {reviewer, review} of approved) {
+          if (review.source === FROM_COMMENT ||
+            review.source === FROM_REVIEW_COMMENT) {
+            logger.warn(
+              `${reviewer.getName()} approved in via LGTM in comments`);
+          }
         }
       }
 

diff --git a/lib/reviews.js b/lib/reviews.js
@@ -4,9 +4,10 @@ const {
 } = require('./review_state');
 const { isCollaborator } = require('./collaborators');
 const { ascending } = require('./comp');
-const LGTM_RE = /(\W|^)lgtm(\W|$)/i;
+const LGTM_RE = /^lgtm\W?$/i;
 const FROM_REVIEW = 'review';
 const FROM_COMMENT = 'comment';
+const FROM_REVIEW_COMMENT = 'review_comment';
 
 class Review {
   /**
@@ -55,7 +56,14 @@ class ReviewAnalyzer {
     const map = new Map();
     const collaborators = this.collaborators;
     const list = this.reviews
-      .filter((r) => r.state !== PENDING && r.state !== COMMENTED)
+      .filter((r) => r.state !== PENDING)
+      .filter((r) => {
+        if (r.state === COMMENTED) {
+          return this.isApprovedInComment(r);
+        } else {
+          return true;
+        }
+      })
       .filter((r) => {
         return (isCollaborator(collaborators, r.author));
       }).sort((a, b) => {
@@ -80,6 +88,12 @@ class ReviewAnalyzer {
             new Review(r.state, r.publishedAt, r.url, FROM_REVIEW)
           );
           break;
+        case COMMENTED:
+          map.set(
+            login,
+            new Review(APPROVED, r.publishedAt, r.bodyText, FROM_REVIEW_COMMENT)
+          );
+          break;
         case DISMISSED:
           // TODO: check the state of the dismissed review?
           map.delete(login);
@@ -97,7 +111,7 @@ class ReviewAnalyzer {
   updateMapByRawReviews(oldMap) {
     const comments = this.comments;
     const collaborators = this.collaborators;
-    const withLgtm = comments.filter((c) => LGTM_RE.test(c.bodyText))
+    const withLgtm = comments.filter((c) => this.hasLGTM(c))
       .filter((c) => {
         return (isCollaborator(collaborators, c.author));
       }).sort((a, b) => {
@@ -133,17 +147,34 @@ class ReviewAnalyzer {
     for (const [ login, review ] of reviewers) {
       const reviewer = collaborators.get(login.toLowerCase());
       if (review.state === APPROVED) {
-        result.approved.push({ reviewer, review });
+        result.approved.push({reviewer, review});
       } else if (review.state === CHANGES_REQUESTED) {
         result.rejected.push({ reviewer, review });
       }
     }
     return result;
   }
+
+  /**
+   * @param review
+   * @returns {boolean}
+   */
+  isApprovedInComment(review) {
+    return review.state === COMMENTED && this.hasLGTM(review);
+  }
+
+  /**
+   * @param object
+   * @param prop: string
+   * @returns {boolean}
+   */
+  hasLGTM(object) {
+    return LGTM_RE.test(object.bodyText.trim());
+  }
 }
 
 const REVIEW_SOURCES = {
-  FROM_COMMENT, FROM_REVIEW
+  FROM_COMMENT, FROM_REVIEW, FROM_REVIEW_COMMENT
 };
 
 module.exports = {

diff --git a/steps/metadata.js b/steps/metadata.js
@@ -34,7 +34,7 @@ module.exports = async function getMetadata(argv, logger) {
   }, 'Generated metadata:');
 
   const checker = new PRChecker(logger, data);
-  const status = checker.checkAll();
+  const status = checker.checkAll(argv.checkComments);
   return {
     status,
     request,

diff --git a/test/fixtures/README/README.md b/test/fixtures/README/README.md
@@ -252,6 +252,8 @@ For more information about the governance of the Node.js project, see
 **Foo User** &lt;foo@example.com&gt; (she/her)
 * [Quo](https://github.com/quo) -
 **Quo User** &lt;quo@example.com&gt; (she/her)
+* [Quux](https://github.com/quux) -
+**Quux User** &lt;quux@example.com&gt; (he/him)
 
 ### Collaborator Emeriti
 

diff --git a/test/fixtures/collaborators.json b/test/fixtures/collaborators.json
@@ -22,5 +22,11 @@
     "name": "Quo User",
     "email": "quo@example.com",
     "type": "COLLABORATOR"
+  },
+  {
+    "login": "Quux",
+    "name": "Quux User",
+    "email": "quux@example.com",
+    "type": "COLLABORATOR"
   }
 ]
diff --git a/test/fixtures/reviewers_approved.json b/test/fixtures/reviewers_approved.json
@@ -13,6 +13,20 @@
       "source": "review"
     }
   },
+  {
+    "reviewer": {
+      "login": "Quux",
+      "name": "Quux User",
+      "email": "quux@example.com",
+      "type": "COLLABORATOR"
+    },
+    "review": {
+      "state": "APPROVED",
+      "date": "2017-10-24T14:49:52Z",
+      "ref": "LGTM",
+      "source": "review_comment"
+    }
+  },
   {
     "reviewer": {
       "login": "Baz",

diff --git a/test/fixtures/reviews_approved.json b/test/fixtures/reviews_approved.json
@@ -35,6 +35,15 @@
   "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71488236",
   "publishedAt": "2017-10-24T14:49:52Z"
 },
+{
+  "bodyText": "LGTM",
+  "state": "COMMENTED",
+  "author": {
+    "login": "Quux"
+  },
+  "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71817236",
+  "publishedAt": "2017-10-24T14:49:52Z"
+},
 {
   "bodyText": "A few nits",
   "state": "COMMENTED",

diff --git a/test/unit/args.test.js b/test/unit/args.test.js
@@ -4,6 +4,7 @@ const parseArgs = require('../../lib/args');
 const assert = require('assert');
 
 const expected = {
+  checkComments: false,
   owner: `nodejs`,
   repo: `node`,
   prid: 16637,

diff --git a/test/unit/metadata_gen.test.js b/test/unit/metadata_gen.test.js
@@ -19,6 +19,7 @@ const expected = `PR-URL: https://github.com/nodejs/node/pull/16438
 Fixes: https://github.com/nodejs/node/issues/16437
 Refs: https://github.com/nodejs/node/pull/15148
 Reviewed-By: Foo User <foo@example.com>
+Reviewed-By: Quux User <quux@example.com>
 Reviewed-By: Baz User <baz@example.com>
 Reviewed-By: Bar User <bar@example.com>
 `;

diff --git a/test/unit/pr_checker.test.js b/test/unit/pr_checker.test.js
@@ -80,12 +80,13 @@ describe('PRChecker', () => {
 
       const expectedLogs = {
         warn: [
+          ['Quux User(Quux) approved in via LGTM in comments'],
+          ['Bar User(bar) approved in via LGTM in comments'],
           ['semver-major requires at least two TSC approvals']
         ],
         info: [
           ['Rejections: 0'],
-          ['Approvals: 3, 1 from TSC (bar)'],
-          ['Bar User(bar)) approved in via LGTM in comments']
+          ['Approvals: 4, 1 from TSC (bar)']
         ],
         error: [],
         trace: []
@@ -100,7 +101,7 @@ describe('PRChecker', () => {
         collaborators
       });
 
-      const status = checker.checkReviews();
+      const status = checker.checkReviews(true);
       assert(!status);
       assert.deepStrictEqual(logger.logs, expectedLogs);
     });
@@ -111,8 +112,8 @@ describe('PRChecker', () => {
       const expectedLogs = {
         warn: [
           ['Rejections: 2, 1 from TSC (bar)'],
-          ['Foo User(foo)) rejected in https://github.com/nodejs/node/pull/16438#pullrequestreview-71480624'],
-          ['Bar User(bar)) rejected in https://github.com/nodejs/node/pull/16438#pullrequestreview-71482624'],
+          ['Foo User(foo) rejected in https://github.com/nodejs/node/pull/16438#pullrequestreview-71480624'],
+          ['Bar User(bar) rejected in https://github.com/nodejs/node/pull/16438#pullrequestreview-71482624'],
           ['Approvals: 0']
         ],
         info: [],