diff --git a/.eslintrc b/.eslintrc index 2a1c0e9..5941541 100644 --- a/.eslintrc +++ b/.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 diff --git a/components/git/land.js b/components/git/land.js index 6633c0a..465ec7c 100644 --- a/components/git/land.js +++ b/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 ' + diff --git a/components/git/metadata.js b/components/git/metadata.js index 726fd40..e8d9a73 100644 --- a/components/git/metadata.js +++ b/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) { diff --git a/lib/ci.js b/lib/ci.js index 374a809..27717f4 100644 --- a/lib/ci.js +++ b/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,20 +94,19 @@ 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); } } @@ -78,9 +114,11 @@ class CIParser { } } -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; diff --git a/lib/links.js b/lib/links.js index af81eeb..610c3a0 100644 --- a/lib/links.js +++ b/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; diff --git a/lib/pr_checker.js b/lib/pr_checker.js index 19ced56..e055003 100644 --- a/lib/pr_checker.js +++ b/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'); } diff --git a/test/fixtures/comments_with_ci.json b/test/fixtures/comments_with_ci.json index e85ad53..8393a1d 100644 --- a/test/fixtures/comments_with_ci.json +++ b/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", diff --git a/test/unit/ci.test.js b/test/unit/ci.test.js index 478dc6b..4638ff7 100644 --- a/test/unit/ci.test.js +++ b/test/unit/ci.test.js @@ -1,49 +1,57 @@ 'use strict'; -const CIParser = require('../../lib/ci'); +const { JobParser } = require('../../lib/ci'); const assert = require('assert'); const { commentsWithCI } = require('../fixtures/data'); const expected = new Map([ - ['FULL', { + ['PR', { link: 'https://ci.nodejs.org/job/node-test-pull-request/10984/', - date: '2017-10-27T04:16:36.458Z' + date: '2017-10-27T04:16:36.458Z', + jobid: 10984 }], ['CITGM', { - link: 'https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1030/', - date: '2017-10-27T04:16:36.458Z' + link: 'https://ci.nodejs.org/job/citgm-smoker/1030/', + date: '2017-10-27T04:16:36.458Z', + jobid: 1030 }], ['LIBUV', { - link: 'https://ci.nodejs.org/view/libuv/job/libuv-test-commit/537/', - date: '2017-10-24T04:16:36.458Z' + link: 'https://ci.nodejs.org/job/libuv-test-commit/537/', + date: '2017-10-24T04:16:36.458Z', + jobid: 537 }], ['NOINTL', { - link: 'https://ci.nodejs.org/job/node-test-commit-linux-nointl/7', - date: '2017-10-23T04:16:36.458Z' + link: 'https://ci.nodejs.org/job/node-test-commit-nointl/7/', + date: '2017-10-23T04:16:36.458Z', + jobid: 7 }], ['V8', { link: 'https://ci.nodejs.org/job/node-test-commit-v8-linux/1018/', - date: '2017-10-22T04:16:36.458Z' + date: '2017-10-22T04:16:36.458Z', + jobid: 1018 }], ['BENCHMARK', { link: 'https://ci.nodejs.org/job/benchmark-node-micro-benchmarks/20/', - date: '2017-10-21T04:16:36.458Z' + date: '2017-10-21T04:16:36.458Z', + jobid: 20 }], ['LINTER', { link: 'https://ci.nodejs.org/job/node-test-linter/13127/', - date: '2017-10-22T04:16:36.458Z' + date: '2017-10-22T04:16:36.458Z', + jobid: 13127 }], - ['LITE', { + ['LITE_COMMIT', { link: 'https://ci.nodejs.org/job/node-test-commit-lite/246/', - date: '2018-02-09T21:38:30Z' + date: '2018-02-09T21:38:30Z', + jobid: 246 }] ]); -describe('CIparser', () => { +describe('JobParser', () => { it('should parse CI results', () => { - const results = new CIParser(commentsWithCI).parse(); - assert.deepStrictEqual(expected, results); + const results = new JobParser(commentsWithCI).parse(); + assert.deepStrictEqual([...expected.entries()], [...results.entries()]); }); }); diff --git a/test/unit/links.test.js b/test/unit/links.test.js index 95ad92d..83061f6 100644 --- a/test/unit/links.test.js +++ b/test/unit/links.test.js @@ -5,22 +5,22 @@ const fixtures = require('../fixtures'); const assert = require('assert'); const htmls = fixtures.readJSON('op_html.json'); -const expected = [{ - fixes: ['https://github.com/nodejs/node/issues/16437'], - refs: ['https://github.com/nodejs/node/pull/15148'] -}, { - fixes: [], - refs: ['https://github.com/nodejs/node/pull/16293'] -}, { - fixes: ['https://github.com/nodejs/node/issues/16504'], - refs: [] -}, { - fixes: [], - refs: ['https://en.wikipedia.org/w/index.php?title=IPv6_address&type=revision&diff=809494791&oldid=804196124'] -}]; - describe('LinkParser', () => { it('should parse fixes and refs', () => { + const expected = [{ + fixes: ['https://github.com/nodejs/node/issues/16437'], + refs: ['https://github.com/nodejs/node/pull/15148'] + }, { + fixes: [], + refs: ['https://github.com/nodejs/node/pull/16293'] + }, { + fixes: ['https://github.com/nodejs/node/issues/16504'], + refs: [] + }, { + fixes: [], + refs: ['https://en.wikipedia.org/w/index.php?title=IPv6_address&type=revision&diff=809494791&oldid=804196124'] + }]; + for (let i = 0; i < htmls.length; ++i) { const op = htmls[i]; const parser = new LinkParser('nodejs', 'node', op); @@ -31,4 +31,50 @@ describe('LinkParser', () => { assert.deepStrictEqual(actual, expected[i]); } }); + + it('should parse PR URL', () => { + const tests = [{ + input: 'https://github.com/nodejs/node/pull/15148', + output: { + owner: 'nodejs', + repo: 'node', + prid: 15148 + } + }, { + input: 'https://github.com/nodejs/node/pull/15148/files', + output: { + owner: 'nodejs', + repo: 'node', + prid: 15148 + } + }, { + input: 'https://github.com/nodejs/node/pull/15148#pullrequestreview-114058064', + output: { + owner: 'nodejs', + repo: 'node', + prid: 15148 + } + }, { + input: 'https://github.com/foo/bar/pull/1234', + output: { + owner: 'foo', + repo: 'bar', + prid: 1234 + } + }, { + input: 'https://github.com/foo/bar/issues/1234', + output: undefined + }, { + input: '15148', + output: undefined + }, { + input: 15148, + output: undefined + }]; + + for (let test of tests) { + const actual = LinkParser.parsePRFromURL(test.input); + assert.deepStrictEqual(actual, test.output); + } + }); }); diff --git a/test/unit/pr_checker.test.js b/test/unit/pr_checker.test.js index a8fe4c8..1913109 100644 --- a/test/unit/pr_checker.test.js +++ b/test/unit/pr_checker.test.js @@ -399,20 +399,20 @@ describe('PRChecker', () => { const expectedLogs = { info: [ [ - 'Last Full CI on 2017-10-27T04:16:36.458Z: ' + + 'Last Full PR CI on 2017-10-27T04:16:36.458Z: ' + 'https://ci.nodejs.org/job/node-test-pull-request/10984/' ], [ 'Last CITGM CI on 2017-10-27T04:16:36.458Z: ' + - 'https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1030/' + 'https://ci.nodejs.org/job/citgm-smoker/1030/' ], [ 'Last libuv CI on 2017-10-24T04:16:36.458Z: ' + - 'https://ci.nodejs.org/view/libuv/job/libuv-test-commit/537/' + 'https://ci.nodejs.org/job/libuv-test-commit/537/' ], [ 'Last No Intl CI on 2017-10-23T04:16:36.458Z: ' + - 'https://ci.nodejs.org/job/node-test-commit-linux-nointl/7' + 'https://ci.nodejs.org/job/node-test-commit-nointl/7/' ], [ 'Last V8 CI on 2017-10-22T04:16:36.458Z: ' + @@ -427,7 +427,7 @@ describe('PRChecker', () => { 'https://ci.nodejs.org/job/node-test-linter/13127/' ], [ - 'Last Lite CI on 2018-02-09T21:38:30Z: ' + + 'Last Lite Commit CI on 2018-02-09T21:38:30Z: ' + 'https://ci.nodejs.org/job/node-test-commit-lite/246/' ] ] @@ -455,13 +455,13 @@ describe('PRChecker', () => { const expectedLogs = { warn: [ - ['Commits were pushed after the last Full CI run:'], + ['Commits were pushed after the last Full PR CI run:'], ['- fixup: adjust spelling'], ['- doc: add api description README'], ['- feat: add something'] ], info: [ - ['Last Full CI on 2017-10-24T11:19:25Z: https://ci.nodejs.org/job/node-test-pull-request/10984/'] + ['Last Full PR CI on 2017-10-24T11:19:25Z: https://ci.nodejs.org/job/node-test-pull-request/10984/'] ] }; @@ -487,7 +487,7 @@ describe('PRChecker', () => { const expectedLogs = { warn: [ - [ 'Commits were pushed after the last Full CI run:' ], + [ 'Commits were pushed after the last Full PR CI run:' ], [ '- doc: add api description README' ], [ '- feat: add something' ], [ '- style: format code' ], @@ -495,7 +495,7 @@ describe('PRChecker', () => { ], info: [ [ - 'Last Full CI on 2017-08-24T11:19:25Z: ' + + 'Last Full PR CI on 2017-08-24T11:19:25Z: ' + 'https://ci.nodejs.org/job/node-test-pull-request/12984/' ] ], @@ -523,12 +523,12 @@ describe('PRChecker', () => { const expectedLogs = { warn: [ - [ 'Commits were pushed after the last Full CI run:' ], + [ 'Commits were pushed after the last Full PR CI run:' ], [ '...(use `--max-commits 4` to see the full list of commits)' ] ], info: [ [ - 'Last Full CI on 2017-08-24T11:19:25Z: ' + + 'Last Full PR CI on 2017-08-24T11:19:25Z: ' + 'https://ci.nodejs.org/job/node-test-pull-request/12984/' ] ], @@ -556,7 +556,7 @@ describe('PRChecker', () => { const expectedLogs = { info: [ [ - 'Last Lite CI on 2018-02-09T21:38:30Z: ' + + 'Last Lite Commit CI on 2018-02-09T21:38:30Z: ' + 'https://ci.nodejs.org/job/node-test-commit-lite/246/' ] ]