lib: clean up PR and CI url parsing logic (#242)

- Put the PR URL parsing logic in links.js and reuse it
- Differentiate PR and commit jobs in preparation for #161
- Updates a few CI link patterns

Author: joyeecheung

.eslintrc

@@ -5,7 +5,8 @@
     "space-before-function-paren": ["error", "never"],
     "no-multi-spaces": ["error", { "ignoreEOLComments": true }],
     "camelcase": "off",
-    "max-len": [2, 80, 4, {"ignoreUrls": true}]
+    "max-len": [2, 80, 4, {"ignoreUrls": true}],
+    "object-property-newline": "off"
   },
   "env": {
     "node": true

components/git/land.js

@@ -1,5 +1,6 @@
 'use strict';
 
+const { parsePRFromURL } = require('../../lib/links');
 const getMetadata = require('../metadata');
 const CLI = require('../../lib/cli');
 const Request = require('../../lib/request');
@@ -59,30 +60,35 @@ const FINAL = 'final';
 const CONTINUE = 'continue';
 const ABORT = 'abort';
 
-const GITHUB_PULL_REQUEST_URL = /github.com\/[^/]+\/[^/]+\/pull\/(\d+)/;
-
 function handler(argv) {
-  if (argv.prid &&
-    (Number.isInteger(argv.prid) || argv.prid.match(GITHUB_PULL_REQUEST_URL))
-  ) {
-    return land(START, argv);
+  if (argv.prid) {
+    if (Number.isInteger(argv.prid)) {
+      return land(START, argv);
+    } else {
+      const parsed = parsePRFromURL(argv.prid);
+      if (parsed) {
+        Object.assign(argv, parsed);
+        return land(START, argv);
+      }
+    }
+    yargs.showHelp();
+    return;
   }
+
   const provided = [];
   for (const type of Object.keys(landOptions)) {
     if (argv[type]) {
       provided.push(type);
     }
   }
-  if (provided.length === 0) {
-    yargs.showHelp();
-    return;
-  }
-  if (provided.length > 1) {
-    yargs.showHelp();
-    return;
+
+  if (provided.length === 1) {
+    return land(provided[0], argv);
   }
 
-  return land(provided[0], argv);
+  // If the more than one action is provided or no valid action
+  // is provided, show help.
+  yargs.showHelp();
 }
 
 function land(state, argv) {
@@ -129,10 +135,6 @@ async function main(state, argv, cli, req, dir) {
   }
 
   if (state === START) {
-    if (argv.prid.match && argv.prid.match(GITHUB_PULL_REQUEST_URL)) {
-      argv.prid = Number(argv.prid.split('/').pop());
-    }
-
     if (session.hasStarted()) {
       cli.warn(
         'Previous `git node land` session for ' +

components/git/metadata.js

@@ -2,6 +2,7 @@
 
 const yargs = require('yargs');
 
+const { parsePRFromURL } = require('../../lib/links');
 const getMetadata = require('../metadata');
 const CLI = require('../../lib/cli');
 const config = require('../../lib/config').getMergedConfig();
@@ -59,25 +60,16 @@ function builder(yargs) {
     .wrap(90);
 }
 
-const PR_RE = new RegExp(
-  '^https://github.com/(\\w+)/([a-zA-Z.-]+)/pull/' +
-  '([0-9]+)(?:/(?:files)?)?$');
-
 function handler(argv) {
-  // remove hashes from PR link
-  argv.identifier = argv.identifier.replace(/#.*$/, '');
-
-  const parsed = {};
+  let parsed = {};
   const prid = Number.parseInt(argv.identifier);
   if (!Number.isNaN(prid)) {
     parsed.prid = prid;
-  } else if (PR_RE.test(argv.identifier)) {
-    const match = argv.identifier.match(PR_RE);
-    parsed.owner = match[1];
-    parsed.repo = match[2];
-    parsed.prid = Number.parseInt(match[3]);
   } else {
-    return yargs.showHelp();
+    parsed = parsePRFromURL(argv.identifier);
+    if (!parsed) {
+      return yargs.showHelp();
+    }
   }
 
   if (!Number.isInteger(argv.maxCommits) || argv.maxCommits < 0) {

lib/ci.js

@@ -1,33 +1,66 @@
 'use strict';
 
-// (XXX) Do we need the protocol?
-const CI_RE = /https:\/\/ci\.nodejs\.org(\S+)/mg;
+const CI_URL_RE = /\/\/ci\.nodejs\.org(\S+)/mg;
 const CI_DOMAIN = 'ci.nodejs.org';
 
 // constants
 const CITGM = 'CITGM';
-const FULL = 'FULL';
+const PR = 'PR';
+const COMMIT = 'COMMIT';
 const BENCHMARK = 'BENCHMARK';
 const LIBUV = 'LIBUV';
 const NOINTL = 'NOINTL';
 const V8 = 'V8';
 const LINTER = 'LINTER';
-const LITE = 'LITE';
+const LITE_PR = 'LITE_PR';
+const LITE_COMMIT = 'LITE_COMMIT';
 
 const CI_TYPES = new Map([
-  [CITGM, { name: 'CITGM', re: /citgm/ }],
-  [FULL,
-    { name: 'Full',
-      re: /node-test-pull-request\/|node-test-commit\// }],
-  [BENCHMARK, { name: 'Benchmark', re: /benchmark/ }],
-  [LIBUV, { name: 'libuv', re: /libuv/ }],
-  [NOINTL, { name: 'No Intl', re: /nointl/ }],
-  [V8, { name: 'V8', re: /node-test-commit-v8/ }],
-  [LINTER, { name: 'Linter', re: /node-test-linter/ }],
-  [LITE, { name: 'Lite', re: /node-test-.+-lite/ }]
+  [CITGM, { name: 'CITGM', jobName: 'citgm-smoker' }],
+  [PR, { name: 'Full PR', jobName: 'node-test-pull-request' }],
+  [COMMIT, { name: 'Full Commit', jobName: 'node-test-commit' }],
+  [BENCHMARK, {
+    name: 'Benchmark',
+    jobName: 'benchmark-node-micro-benchmarks'
+  }],
+  [LIBUV, { name: 'libuv', jobName: 'libuv-test-commit' }],
+  [NOINTL, { name: 'No Intl', jobName: 'node-test-commit-nointl' }],
+  [V8, { name: 'V8', jobName: 'node-test-commit-v8-linux' }],
+  [LINTER, { name: 'Linter', jobName: 'node-test-linter' }],
+  [LITE_PR, {
+    name: 'Lite PR',
+    jobName: 'node-test-pull-request-lite'
+  }],
+  [LITE_COMMIT, {
+    name: 'Lite Commit',
+    jobName: 'node-test-commit-lite'
+  }]
 ]);
 
-class CIParser {
+function parseJobFromURL(url) {
+  if (typeof url !== 'string') {
+    return undefined;
+  }
+
+  for (let [ type, info ] of CI_TYPES) {
+    const re = new RegExp(`${CI_DOMAIN}/job/${info.jobName}/(\\d+)`);
+    const match = url.match(re);
+    if (match) {
+      return {
+        link: url,
+        jobid: parseInt(match[1]),
+        type: type
+      };
+    }
+  }
+
+  return undefined;
+}
+
+/**
+ * Parse links of CI Jobs posted in a GitHub thread
+ */
+class JobParser {
   /**
    * @param {{bodyText: string, publishedAt: string}[]} thread
    */
@@ -44,11 +77,15 @@ class CIParser {
     for (const c of thread) {
       const text = c.bodyText;
       if (!text.includes(CI_DOMAIN)) continue;
-      const cis = this.parseText(text);
-      for (const ci of cis) {
-        const entry = result.get(ci.type);
+      const jobs = this.parseText(text);
+      for (const job of jobs) {
+        const entry = result.get(job.type);
         if (!entry || entry.date < c.publishedAt) {
-          result.set(ci.type, {link: ci.link, date: c.publishedAt});
+          result.set(job.type, {
+            link: job.link,
+            date: c.publishedAt,
+            jobid: job.jobid
+          });
         }
       }
     }
@@ -57,30 +94,31 @@ class CIParser {
 
   /**
    * @param {string} text
+   * @returns {{link: string, jobid: number, type: string}}
    */
   parseText(text) {
-    const m = text.match(CI_RE);
-    if (!m) {
+    const links = text.match(CI_URL_RE);
+    if (!links) {
       return [];
     }
 
     const result = [];
-    for (const link of m) {
-      for (const [type, info] of CI_TYPES) {
-        if (info.re.test(link)) {
-          result.push({ type, link });
-          break;
-        }
+    for (const link of links) {
+      const parsed = parseJobFromURL(`https:${link}`);
+      if (parsed) {
+        result.push(parsed);
       }
     }
 
     return result;
   }
 }
 
-CIParser.TYPES = CI_TYPES;
-CIParser.constants = {
-  CITGM, FULL, BENCHMARK, LIBUV, V8, NOINTL, LINTER, LITE
+module.exports = {
+  JobParser,
+  CI_TYPES,
+  constants: {
+    CITGM, PR, COMMIT, BENCHMARK, LIBUV, V8, NOINTL,
+    LINTER, LITE_PR, LITE_COMMIT
+  }
 };
-
-module.exports = CIParser;

lib/links.js

@@ -75,4 +75,21 @@ class LinkParser {
   }
 };
 
+const GITHUB_PULL_REQUEST_URL = /github.com\/([^/]+)\/([^/]+)\/pull\/(\d+)/;
+
+LinkParser.parsePRFromURL = function(url) {
+  if (typeof url !== 'string') {
+    return undefined;
+  }
+  const match = url.match(GITHUB_PULL_REQUEST_URL);
+  if (match) {
+    return {
+      owner: match[1],
+      repo: match[2],
+      prid: parseInt(match[3])
+    };
+  }
+  return undefined;
+};
+
 module.exports = LinkParser;

lib/pr_checker.js

@@ -17,9 +17,8 @@ const {
   CONFLICTING
 } = require('./mergeable_state');
 
-const CIParser = require('./ci');
-const CI_TYPES = CIParser.TYPES;
-const { FULL, LITE } = CIParser.constants;
+const { JobParser, CI_TYPES, constants } = require('./ci');
+const { COMMIT, PR, LITE_PR, LITE_COMMIT } = constants;
 
 class PRChecker {
   /**
@@ -187,6 +186,11 @@ class PRChecker {
     return true;
   }
 
+  hasFullOrLiteCI(ciMap) {
+    return (ciMap.has(COMMIT) || ciMap.has(PR) ||
+            ciMap.has(LITE_PR) || ciMap.has(LITE_COMMIT));
+  }
+
   // TODO: we might want to check CI status when it's less flaky...
   // TODO: not all PR requires CI...labels?
   checkCI() {
@@ -197,13 +201,13 @@ class PRChecker {
     };
     const { maxCommits } = argv;
     const thread = comments.concat([prNode]).concat(reviews);
-    const ciMap = new CIParser(thread).parse();
+    const ciMap = new JobParser(thread).parse();
     let status = true;
     if (!ciMap.size) {
       cli.error('No CI runs detected');
       this.CIStatus = false;
       return false;
-    } else if (!ciMap.get(FULL) && !ciMap.get(LITE)) {
+    } else if (!this.hasFullOrLiteCI(ciMap)) {
       status = false;
       cli.error('No full CI runs or lite CI runs detected');
     }

test/fixtures/comments_with_ci.json

@@ -1,15 +1,15 @@
 [
   {
     "publishedAt": "2017-10-27T04:16:36.458Z",
-    "bodyText": "CI: https://ci.nodejs.org/job/node-test-pull-request/10984/\ncitgm: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1030/"
+    "bodyText": "CI: https://ci.nodejs.org/job/node-test-pull-request/10984/\ncitgm: https://ci.nodejs.org/job/citgm-smoker/1030/"
   },
   {
     "publishedAt": "2017-10-24T04:16:36.458Z",
-    "bodyText": "https://ci.nodejs.org/view/libuv/job/libuv-test-commit/537/"
+    "bodyText": "https://ci.nodejs.org/job/libuv-test-commit/537/"
   },
   {
     "publishedAt": "2017-10-23T04:16:36.458Z",
-    "bodyText": "https://ci.nodejs.org/job/node-test-commit-linux-nointl/7"
+    "bodyText": "https://ci.nodejs.org/job/node-test-commit-nointl/7/"
   },
   {
     "publishedAt": "2017-10-22T04:16:36.458Z",