Fixing review state to APPROVED whe 'LGTM' in COMMENTED review

lib/args.js

@@ -37,6 +37,12 @@ function buildYargs(args = null) {
       describe: 'File to write the metadata in',
       type: 'string'
     })
+    .option('comments', {
+      alias: 'c',
+      demandOption: false,
+      describe: 'Check for\'LGTM\' in comments',
+      type: 'boolean'
+    })
     .help()
     .alias('help', 'h')
     .argv;
@@ -47,8 +53,8 @@ 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, comments } = args;
+  const result = { owner, repo, file, comments };
   if (!isNaN(identifier)) {
     result.prid = +identifier;
   } else {

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;
@@ -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`);
+          }
         }
       }
 

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 = {

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.comments);
   return {
     status,
     request,

test/fixtures/README/README.md

@@ -252,6 +252,8 @@ For more information about the governance of the Node.js project, see
 **Foo User** <[email protected]> (she/her)
 * [Quo](https://github.com/quo) -
 **Quo User** <[email protected]> (she/her)
+* [Quux](https://github.com/quux) -
+**Quux User** <[email protected]> (he/him)
 
 ### Collaborator Emeriti
 

test/fixtures/collaborators.json

@@ -22,5 +22,11 @@
     "name": "Quo User",
     "email": "[email protected]",
     "type": "COLLABORATOR"
+  },
+  {
+    "login": "Quux",
+    "name": "Quux User",
+    "email": "[email protected]",
+    "type": "COLLABORATOR"
   }
 ]

test/fixtures/reviewers_approved.json

@@ -13,6 +13,20 @@
       "source": "review"
     }
   },
+  {
+    "reviewer": {
+      "login": "Quux",
+      "name": "Quux User",
+      "email": "[email protected]",
+      "type": "COLLABORATOR"
+    },
+    "review": {
+      "state": "APPROVED",
+      "date": "2017-10-24T14:49:52Z",
+      "ref": "LGTM",
+      "source": "review_comment"
+    }
+  },
   {
     "reviewer": {
       "login": "Baz",

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",

test/unit/args.test.js

@@ -4,6 +4,7 @@ const parseArgs = require('../../lib/args');
 const assert = require('assert');
 
 const expected = {
+  comments: false,
   owner: `nodejs`,
   repo: `node`,
   prid: 16637,

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 <[email protected]>
+Reviewed-By: Quux User <[email protected]>
 Reviewed-By: Baz User <[email protected]>
 Reviewed-By: Bar User <[email protected]>
 `;

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);
     });