From 92e3fb1fe00cc07bd8e8a20a4f4b09ed26d27af8 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 6 Apr 2018 19:34:10 +0800 Subject: [PATCH] lib: refactor collaborator getters Put the readme request code into the helper so it can be reused by other utilities. --- lib/cli.js | 2 + lib/collaborators.js | 43 ++++++++++++++++---- lib/pr_data.js | 22 +---------- test/fixtures/test_cli.js | 14 ++++++- test/unit/collaborators.test.js | 69 ++++++++++++++++++++++++--------- test/unit/pr_data.test.js | 36 +---------------- 6 files changed, 104 insertions(+), 82 deletions(-) diff --git a/lib/cli.js b/lib/cli.js index a8dd185d..368d9e65 100644 --- a/lib/cli.js +++ b/lib/cli.js @@ -137,4 +137,6 @@ class CLI { } }; +CLI.SPINNER_STATUS = SPINNER_STATUS; + module.exports = CLI; diff --git a/lib/collaborators.js b/lib/collaborators.js index a2f5fa00..580c43ca 100644 --- a/lib/collaborators.js +++ b/lib/collaborators.js @@ -1,5 +1,7 @@ 'use strict'; +const fs = require('fs'); + const TSC_TITLE = '### TSC (Technical Steering Committee)'; const TSCE_TITLE = '### TSC Emeriti'; const CL_TITLE = '### Collaborators'; @@ -41,10 +43,35 @@ Collaborator.TYPES = { TSC, COLLABORATOR }; -function getCollaborators(readme, cli, owner, repo) { +async function getCollaborators(cli, request, argv) { + const { readme, owner, repo } = argv; + let readmeText; + if (readme) { + cli.updateSpinner(`Reading collaborator contacts from ${readme}`); + readmeText = fs.readFileSync(readme, 'utf8'); + } else { + cli.updateSpinner( + `Getting collaborator contacts from README of ${owner}/${repo}`); + const url = `https://raw.githubusercontent.com/${owner}/${repo}/master/README.md`; + readmeText = await request.text(url); + } + + let collaborators; + try { + collaborators = parseCollaborators(readmeText, cli); + } catch (err) { + const readmePath = readme || `${owner}/${repo}/README.md`; + cli.stopSpinner(`Failed to get collaborator info from ${readmePath}`, + cli.SPINNER_STATUS.FAILED); + throw err; + } + return collaborators; +} + +function parseCollaborators(readme, cli) { // This is more or less taken from // https://github.com/rvagg/iojs-tools/blob/master/pr-metadata/pr-metadata.js - const members = new Map(); + const collaborators = new Map(); let m; const tscIndex = readme.indexOf(TSC_TITLE); @@ -77,7 +104,8 @@ function getCollaborators(readme, cli, owner, repo) { // eslint-disable-next-line no-cond-assign while ((m = CONTACT_RE.exec(readme)) && CONTACT_RE.lastIndex < tsceIndex) { const login = m[1].toLowerCase(); - members.set(login, new Collaborator(m[1], m[2], m[3], TSC)); + const user = new Collaborator(m[1], m[2], m[3], TSC); + collaborators.set(login, user); } CONTACT_RE.lastIndex = clIndex; @@ -85,16 +113,17 @@ function getCollaborators(readme, cli, owner, repo) { while ((m = CONTACT_RE.exec(readme)) && CONTACT_RE.lastIndex < cleIndex) { const login = m[1].toLowerCase(); - if (!members.get(login)) { - members.set(login, new Collaborator(m[1], m[2], m[3], COLLABORATOR)); + if (!collaborators.get(login)) { + const user = new Collaborator(m[1], m[2], m[3], COLLABORATOR); + collaborators.set(login, user); } } - if (!members.size) { + if (!collaborators.size) { throw new Error('Could not find any collaborators'); } - return members; + return collaborators; } /** diff --git a/lib/pr_data.js b/lib/pr_data.js index 23dc1732..1ab20919 100644 --- a/lib/pr_data.js +++ b/lib/pr_data.js @@ -2,7 +2,6 @@ const { getCollaborators } = require('./collaborators'); const { ReviewAnalyzer } = require('./reviews'); -const fs = require('fs'); const { FIRST_TIME_CONTRIBUTOR, FIRST_TIMER @@ -59,25 +58,8 @@ class PRData { } async getCollaborators() { - const { owner, repo, cli, request, argv } = this; - let readme; - if (argv.readme) { - cli.updateSpinner(`Reading collaborator contacts from ${argv.readme}`); - readme = fs.readFileSync(argv.readme, 'utf8'); - } else { - cli.updateSpinner( - `Getting collaborator contacts from README of ${owner}/${repo}`); - const url = `https://raw.githubusercontent.com/${owner}/${repo}/master/README.md`; - readme = await request.text(url); - } - try { - this.collaborators = getCollaborators(readme, cli, owner, repo); - } catch (err) { - const readme = argv.readme || `${owner}/${repo}/README.md`; - cli.stopSpinner(`Failed to get collaborator info from ${readme}`, - cli.SPINNER_STATUS.FAILED); - throw err; - } + const { cli, request, argv } = this; + this.collaborators = await getCollaborators(cli, request, argv); } async getPR() { diff --git a/test/fixtures/test_cli.js b/test/fixtures/test_cli.js index e4dcf2cc..af2c52b8 100644 --- a/test/fixtures/test_cli.js +++ b/test/fixtures/test_cli.js @@ -17,15 +17,25 @@ class TestCLI { constructor() { this.spinner = {}; this._calls = newCalls(); + this.SPINNER_STATUS = CLI.SPINNER_STATUS; } clearCalls() { this._calls = newCalls(); } - assertCalledWith(calls, msg) { + assertCalledWith(calls, options = {}) { const expected = Object.assign(newCalls(), calls); - assert.deepStrictEqual(this._calls, expected); + const actual = {}; + const ignore = options.ignore || []; + for (const func of Object.keys(this._calls)) { + if (!ignore.includes(func)) { + actual[func] = this._calls[func]; + } else { + actual[func] = []; + } + } + assert.deepStrictEqual(actual, expected); } } diff --git a/test/unit/collaborators.test.js b/test/unit/collaborators.test.js index 40fb49ca..166fc98c 100644 --- a/test/unit/collaborators.test.js +++ b/test/unit/collaborators.test.js @@ -1,6 +1,7 @@ 'use strict'; const assert = require('assert'); +const path = require('path'); const { isCollaborator, @@ -20,11 +21,6 @@ const assertThrowsAsync = require('../fixtures/assert_throws_async'); describe('collaborators', function() { const collaborator = collaborators.get('bar'); - let cli = null; - - beforeEach(() => { - cli = new TestCLI(); - }); describe('Collaborator', () => { describe('isActor', () => { @@ -83,32 +79,65 @@ describe('collaborators', function() { }); describe('getCollaborators', () => { + let cli = null; + + beforeEach(() => { + cli = new TestCLI(); + }); + + function mockRequest(content, argv) { + const { owner, repo } = argv; + const expectedUrl = + `https://raw.githubusercontent.com/${owner}/${repo}/master/README.md`; + return { + async text(url) { + assert.strictEqual(url, expectedUrl); + return content; + } + }; + } + + it('should use specified readme', async function() { + const readmePath = path.resolve( + __dirname, '..', 'fixtures', 'README', 'README.md'); + const argv = { owner: 'nodejs', repo: 'node', readme: readmePath }; + const request = { async text() { assert.fail('should not call'); } }; + const parsed = await getCollaborators(cli, request, argv); + assert.deepStrictEqual(parsed, collaborators); + }); + it('should return all collaborators', async function() { - const parsed = getCollaborators(readme, cli, 'nodejs', 'node'); + const argv = { owner: 'nodejs', repo: 'node' }; + const request = mockRequest(readme, argv); + const parsed = await getCollaborators(cli, request, argv); assert.deepStrictEqual(parsed, collaborators); }); - it( - 'should throw error if there is no TSC section in the README', + it('should throw error if there is no TSC section in the README', async() => { + const argv = { owner: 'nodejs', repo: 'node' }; + const request = mockRequest(readmeNoTsc, argv); await assertThrowsAsync( - async() => getCollaborators(readmeNoTsc, cli, 'nodejs', 'node'), + async() => getCollaborators(cli, request, argv), Error); }); it( 'should throw error if there is no TSC Emeriti section in the README', async() => { + const argv = { owner: 'nodejs', repo: 'node' }; + const request = mockRequest(readmeNoTscE, argv); await assertThrowsAsync( - async() => getCollaborators(readmeNoTscE, cli, 'nodejs', 'node'), + async() => getCollaborators(cli, request, argv), /Error: Couldn't find ### TSC Emeriti in the README/); }); it('should throw error if there is no Collaborators section in the README', async() => { + const argv = { owner: 'nodejs', repo: 'node' }; + const request = mockRequest(readmeNoCollaborators, argv); await assertThrowsAsync( - async() => getCollaborators( - readmeNoCollaborators, cli, 'nodejs', 'node'), + async() => getCollaborators(cli, request, argv), /Error: Couldn't find ### Collaborators in the README/); }); @@ -116,9 +145,10 @@ describe('collaborators', function() { 'should throw error if there is no Collaborator' + 'Emeriti section in the README', async() => { + const argv = { owner: 'nodejs', repo: 'node' }; + const request = mockRequest(readmeNoCollaboratorE, argv); await assertThrowsAsync( - async() => getCollaborators( - readmeNoCollaboratorE, cli, 'nodejs', 'node'), + async() => getCollaborators(cli, request, argv), /Error: Couldn't find ### Collaborator Emeriti in the README/); }); @@ -126,21 +156,24 @@ describe('collaborators', function() { 'should WARN if the TSC and Collaborators' + 'section are not ordered in the README', async() => { + const argv = { owner: 'nodejs', repo: 'node' }; + const request = mockRequest(readmeUnordered, argv); await assertThrowsAsync( - async() => getCollaborators( - readmeUnordered, cli, 'nodejs', 'node'), + async() => getCollaborators(cli, request, argv), /Error/); cli.assertCalledWith({ warn: [[ 'Contacts in the README is out of order, analysis could go wrong.', { newline: true } ]] - }); + }, { ignore: ['updateSpinner', 'stopSpinner'] }); }); it('should throw error if there are no collaborators', async() => { + const argv = { owner: 'nodejs', repo: 'node' }; + const request = mockRequest(readmeUnordered, argv); await assertThrowsAsync( - async() => getCollaborators(readmeUnordered, cli, 'nodejs', 'node'), + async() => getCollaborators(cli, request, argv), /Error: Could not find any collaborators/); }); }); diff --git a/test/unit/pr_data.test.js b/test/unit/pr_data.test.js index cd9bd5a4..d9e755de 100644 --- a/test/unit/pr_data.test.js +++ b/test/unit/pr_data.test.js @@ -2,7 +2,7 @@ const assert = require('assert'); const sinon = require('sinon'); -const path = require('path'); + const { approvingReviews, allGreenReviewers, @@ -56,37 +56,3 @@ describe('PRData', function() { assert.deepStrictEqual(data.reviewers, allGreenReviewers, 'reviewers'); }); }); - -describe('PRData', function() { - const request = { - text: sinon.stub(), - gql: sinon.stub() - }; - request.text - .withArgs('https://raw.githubusercontent.com/nodejs/node/master/README.md') - .returns(new Error('Should not call')); - request.text.returns(new Error('unknown query')); - request.gql.withArgs('PR').returns(Promise.resolve(rawPR)); - request.gql.withArgs('Reviews').returns( - Promise.resolve(toRaw(approvingReviews))); - request.gql.withArgs('PRComments').returns( - Promise.resolve(toRaw(commentsWithLGTM))); - request.gql.withArgs('PRCommits').returns( - Promise.resolve(toRaw(oddCommits))); - request.gql.returns(new Error('unknown query')); - - it('getAll with specified readme', async() => { - const cli = new TestCLI(); - const readmePath = path.resolve( - __dirname, '..', 'fixtures', 'README', 'README.md'); - const argv2 = Object.assign({ readme: readmePath }); - const data = new PRData(argv2, cli, request); - await data.getAll(); - assert.deepStrictEqual(data.collaborators, collaborators, 'collaborators'); - assert.deepStrictEqual(data.pr, firstTimerPR, 'pr'); - assert.deepStrictEqual(data.reviews, approvingReviews, 'reviews'); - assert.deepStrictEqual(data.comments, commentsWithLGTM, 'comments'); - assert.deepStrictEqual(data.commits, oddCommits, 'commits'); - assert.deepStrictEqual(data.reviewers, allGreenReviewers, 'reviewers'); - }); -});