Add tests cases and comments flag

Author: Tiriel

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

@@ -56,7 +56,14 @@ class ReviewAnalyzer {
     const map = new Map();
     const collaborators = this.collaborators;
     const list = this.reviews
-      .filter((r) => r.state !== PENDING || this.isApprovedInComment(r))
+      .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) => {
@@ -84,7 +91,7 @@ class ReviewAnalyzer {
         case COMMENTED:
           map.set(
             login,
-            new Review(r.state, r.publishedAt, r.bodyText, FROM_REVIEW_COMMENT)
+            new Review(APPROVED, r.publishedAt, r.bodyText, FROM_REVIEW_COMMENT)
           );
           break;
         case DISMISSED:
@@ -104,7 +111,7 @@ class ReviewAnalyzer {
   updateMapByRawReviews(oldMap) {
     const comments = this.comments;
     const collaborators = this.collaborators;
-    const withLgtm = comments.filter((c) => this.hasLGTM(c, 'bodyText'))
+    const withLgtm = comments.filter((c) => this.hasLGTM(c))
       .filter((c) => {
         return (isCollaborator(collaborators, c.author));
       }).sort((a, b) => {
@@ -139,7 +146,7 @@ class ReviewAnalyzer {
     const collaborators = this.collaborators;
     for (const [ login, review ] of reviewers) {
       const reviewer = collaborators.get(login.toLowerCase());
-      if (review.state === APPROVED || this.isApprovedInComment(review)) {
+      if (review.state === APPROVED) {
         result.approved.push({reviewer, review});
       } else if (review.state === CHANGES_REQUESTED) {
         result.rejected.push({ reviewer, review });
@@ -153,16 +160,16 @@ class ReviewAnalyzer {
    * @returns {boolean}
    */
   isApprovedInComment(review) {
-    return review.state === COMMENTED && this.hasLGTM(review, 'ref');
+    return review.state === COMMENTED && this.hasLGTM(review);
   }
 
   /**
    * @param object
    * @param prop: string
    * @returns {boolean}
    */
-  hasLGTM(object, prop) {
-    return LGTM_RE.test(object[prop].trim());
+  hasLGTM(object) {
+    return LGTM_RE.test(object.bodyText.trim());
   }
 }
 

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

@@ -39,7 +39,7 @@
   "bodyText": "LGTM",
   "state": "COMMENTED",
   "author": {
-    "login": "Baz"
+    "login": "Quux"
   },
   "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71817236",
   "publishedAt": "2017-10-24T14:49:52Z"

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