Skip to content

Commit 92e3fb1

Browse files
joyeecheungpriyank-p
joyeecheung
authored and
priyank-p
committed
lib: refactor collaborator getters
Put the readme request code into the helper so it can be reused by other utilities.
1 parent e983b41 commit 92e3fb1

File tree

6 files changed

+104
-82
lines changed

6 files changed

+104
-82
lines changed

‎lib/cli.js‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,4 +137,6 @@ class CLI {
137137
}
138138
};
139139

140+
CLI.SPINNER_STATUS = SPINNER_STATUS;
141+
140142
module.exports = CLI;

‎lib/collaborators.js‎

Lines changed: 36 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
'use strict';
22

3+
const fs = require('fs');
4+
35
const TSC_TITLE = '### TSC (Technical Steering Committee)';
46
const TSCE_TITLE = '### TSC Emeriti';
57
const CL_TITLE = '### Collaborators';
@@ -41,10 +43,35 @@ Collaborator.TYPES = {
4143
TSC, COLLABORATOR
4244
};
4345

44-
function getCollaborators(readme, cli, owner, repo) {
46+
async function getCollaborators(cli, request, argv) {
47+
const { readme, owner, repo } = argv;
48+
let readmeText;
49+
if (readme) {
50+
cli.updateSpinner(`Reading collaborator contacts from ${readme}`);
51+
readmeText = fs.readFileSync(readme, 'utf8');
52+
} else {
53+
cli.updateSpinner(
54+
`Getting collaborator contacts from README of ${owner}/${repo}`);
55+
const url = `https://raw.githubusercontent.com/${owner}/${repo}/master/README.md`;
56+
readmeText = await request.text(url);
57+
}
58+
59+
let collaborators;
60+
try {
61+
collaborators = parseCollaborators(readmeText, cli);
62+
} catch (err) {
63+
const readmePath = readme || `${owner}/${repo}/README.md`;
64+
cli.stopSpinner(`Failed to get collaborator info from ${readmePath}`,
65+
cli.SPINNER_STATUS.FAILED);
66+
throw err;
67+
}
68+
return collaborators;
69+
}
70+
71+
function parseCollaborators(readme, cli) {
4572
// This is more or less taken from
4673
// https://github.com/rvagg/iojs-tools/blob/master/pr-metadata/pr-metadata.js
47-
const members = new Map();
74+
const collaborators = new Map();
4875
let m;
4976

5077
const tscIndex = readme.indexOf(TSC_TITLE);
@@ -77,24 +104,26 @@ function getCollaborators(readme, cli, owner, repo) {
77104
// eslint-disable-next-line no-cond-assign
78105
while ((m = CONTACT_RE.exec(readme)) && CONTACT_RE.lastIndex < tsceIndex) {
79106
const login = m[1].toLowerCase();
80-
members.set(login, new Collaborator(m[1], m[2], m[3], TSC));
107+
const user = new Collaborator(m[1], m[2], m[3], TSC);
108+
collaborators.set(login, user);
81109
}
82110

83111
CONTACT_RE.lastIndex = clIndex;
84112
// eslint-disable-next-line no-cond-assign
85113
while ((m = CONTACT_RE.exec(readme)) &&
86114
CONTACT_RE.lastIndex < cleIndex) {
87115
const login = m[1].toLowerCase();
88-
if (!members.get(login)) {
89-
members.set(login, new Collaborator(m[1], m[2], m[3], COLLABORATOR));
116+
if (!collaborators.get(login)) {
117+
const user = new Collaborator(m[1], m[2], m[3], COLLABORATOR);
118+
collaborators.set(login, user);
90119
}
91120
}
92121

93-
if (!members.size) {
122+
if (!collaborators.size) {
94123
throw new Error('Could not find any collaborators');
95124
}
96125

97-
return members;
126+
return collaborators;
98127
}
99128

100129
/**

‎lib/pr_data.js‎

Lines changed: 2 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22

33
const { getCollaborators } = require('./collaborators');
44
const { ReviewAnalyzer } = require('./reviews');
5-
const fs = require('fs');
65

76
const {
87
FIRST_TIME_CONTRIBUTOR, FIRST_TIMER
@@ -59,25 +58,8 @@ class PRData {
5958
}
6059

6160
async getCollaborators() {
62-
const { owner, repo, cli, request, argv } = this;
63-
let readme;
64-
if (argv.readme) {
65-
cli.updateSpinner(`Reading collaborator contacts from ${argv.readme}`);
66-
readme = fs.readFileSync(argv.readme, 'utf8');
67-
} else {
68-
cli.updateSpinner(
69-
`Getting collaborator contacts from README of ${owner}/${repo}`);
70-
const url = `https://raw.githubusercontent.com/${owner}/${repo}/master/README.md`;
71-
readme = await request.text(url);
72-
}
73-
try {
74-
this.collaborators = getCollaborators(readme, cli, owner, repo);
75-
} catch (err) {
76-
const readme = argv.readme || `${owner}/${repo}/README.md`;
77-
cli.stopSpinner(`Failed to get collaborator info from ${readme}`,
78-
cli.SPINNER_STATUS.FAILED);
79-
throw err;
80-
}
61+
const { cli, request, argv } = this;
62+
this.collaborators = await getCollaborators(cli, request, argv);
8163
}
8264

8365
async getPR() {

‎test/fixtures/test_cli.js‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,15 +17,25 @@ class TestCLI {
1717
constructor() {
1818
this.spinner = {};
1919
this._calls = newCalls();
20+
this.SPINNER_STATUS = CLI.SPINNER_STATUS;
2021
}
2122

2223
clearCalls() {
2324
this._calls = newCalls();
2425
}
2526

26-
assertCalledWith(calls, msg) {
27+
assertCalledWith(calls, options = {}) {
2728
const expected = Object.assign(newCalls(), calls);
28-
assert.deepStrictEqual(this._calls, expected);
29+
const actual = {};
30+
const ignore = options.ignore || [];
31+
for (const func of Object.keys(this._calls)) {
32+
if (!ignore.includes(func)) {
33+
actual[func] = this._calls[func];
34+
} else {
35+
actual[func] = [];
36+
}
37+
}
38+
assert.deepStrictEqual(actual, expected);
2939
}
3040
}
3141

‎test/unit/collaborators.test.js‎

Lines changed: 51 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
'use strict';
22

33
const assert = require('assert');
4+
const path = require('path');
45

56
const {
67
isCollaborator,
@@ -20,11 +21,6 @@ const assertThrowsAsync = require('../fixtures/assert_throws_async');
2021

2122
describe('collaborators', function() {
2223
const collaborator = collaborators.get('bar');
23-
let cli = null;
24-
25-
beforeEach(() => {
26-
cli = new TestCLI();
27-
});
2824

2925
describe('Collaborator', () => {
3026
describe('isActor', () => {
@@ -83,64 +79,101 @@ describe('collaborators', function() {
8379
});
8480

8581
describe('getCollaborators', () => {
82+
let cli = null;
83+
84+
beforeEach(() => {
85+
cli = new TestCLI();
86+
});
87+
88+
function mockRequest(content, argv) {
89+
const { owner, repo } = argv;
90+
const expectedUrl =
91+
`https://raw.githubusercontent.com/${owner}/${repo}/master/README.md`;
92+
return {
93+
async text(url) {
94+
assert.strictEqual(url, expectedUrl);
95+
return content;
96+
}
97+
};
98+
}
99+
100+
it('should use specified readme', async function() {
101+
const readmePath = path.resolve(
102+
__dirname, '..', 'fixtures', 'README', 'README.md');
103+
const argv = { owner: 'nodejs', repo: 'node', readme: readmePath };
104+
const request = { async text() { assert.fail('should not call'); } };
105+
const parsed = await getCollaborators(cli, request, argv);
106+
assert.deepStrictEqual(parsed, collaborators);
107+
});
108+
86109
it('should return all collaborators', async function() {
87-
const parsed = getCollaborators(readme, cli, 'nodejs', 'node');
110+
const argv = { owner: 'nodejs', repo: 'node' };
111+
const request = mockRequest(readme, argv);
112+
const parsed = await getCollaborators(cli, request, argv);
88113
assert.deepStrictEqual(parsed, collaborators);
89114
});
90115

91-
it(
92-
'should throw error if there is no TSC section in the README',
116+
it('should throw error if there is no TSC section in the README',
93117
async() => {
118+
const argv = { owner: 'nodejs', repo: 'node' };
119+
const request = mockRequest(readmeNoTsc, argv);
94120
await assertThrowsAsync(
95-
async() => getCollaborators(readmeNoTsc, cli, 'nodejs', 'node'),
121+
async() => getCollaborators(cli, request, argv),
96122
Error);
97123
});
98124

99125
it(
100126
'should throw error if there is no TSC Emeriti section in the README',
101127
async() => {
128+
const argv = { owner: 'nodejs', repo: 'node' };
129+
const request = mockRequest(readmeNoTscE, argv);
102130
await assertThrowsAsync(
103-
async() => getCollaborators(readmeNoTscE, cli, 'nodejs', 'node'),
131+
async() => getCollaborators(cli, request, argv),
104132
/Error: Couldn't find ### TSC Emeriti in the README/);
105133
});
106134

107135
it('should throw error if there is no Collaborators section in the README',
108136
async() => {
137+
const argv = { owner: 'nodejs', repo: 'node' };
138+
const request = mockRequest(readmeNoCollaborators, argv);
109139
await assertThrowsAsync(
110-
async() => getCollaborators(
111-
readmeNoCollaborators, cli, 'nodejs', 'node'),
140+
async() => getCollaborators(cli, request, argv),
112141
/Error: Couldn't find ### Collaborators in the README/);
113142
});
114143

115144
it(
116145
'should throw error if there is no Collaborator' +
117146
'Emeriti section in the README',
118147
async() => {
148+
const argv = { owner: 'nodejs', repo: 'node' };
149+
const request = mockRequest(readmeNoCollaboratorE, argv);
119150
await assertThrowsAsync(
120-
async() => getCollaborators(
121-
readmeNoCollaboratorE, cli, 'nodejs', 'node'),
151+
async() => getCollaborators(cli, request, argv),
122152
/Error: Couldn't find ### Collaborator Emeriti in the README/);
123153
});
124154

125155
it(
126156
'should WARN if the TSC and Collaborators' +
127157
'section are not ordered in the README',
128158
async() => {
159+
const argv = { owner: 'nodejs', repo: 'node' };
160+
const request = mockRequest(readmeUnordered, argv);
129161
await assertThrowsAsync(
130-
async() => getCollaborators(
131-
readmeUnordered, cli, 'nodejs', 'node'),
162+
async() => getCollaborators(cli, request, argv),
132163
/Error/);
133164
cli.assertCalledWith({
134165
warn: [[
135166
'Contacts in the README is out of order, analysis could go wrong.',
136167
{ newline: true }
137168
]]
138-
});
169+
}, { ignore: ['updateSpinner', 'stopSpinner'] });
139170
});
140171

141172
it('should throw error if there are no collaborators', async() => {
173+
const argv = { owner: 'nodejs', repo: 'node' };
174+
const request = mockRequest(readmeUnordered, argv);
142175
await assertThrowsAsync(
143-
async() => getCollaborators(readmeUnordered, cli, 'nodejs', 'node'),
176+
async() => getCollaborators(cli, request, argv),
144177
/Error: Could not find any collaborators/);
145178
});
146179
});

‎test/unit/pr_data.test.js‎

Lines changed: 1 addition & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
const assert = require('assert');
44
const sinon = require('sinon');
5-
const path = require('path');
5+
66
const {
77
approvingReviews,
88
allGreenReviewers,
@@ -56,37 +56,3 @@ describe('PRData', function() {
5656
assert.deepStrictEqual(data.reviewers, allGreenReviewers, 'reviewers');
5757
});
5858
});
59-
60-
describe('PRData', function() {
61-
const request = {
62-
text: sinon.stub(),
63-
gql: sinon.stub()
64-
};
65-
request.text
66-
.withArgs('https://raw.githubusercontent.com/nodejs/node/master/README.md')
67-
.returns(new Error('Should not call'));
68-
request.text.returns(new Error('unknown query'));
69-
request.gql.withArgs('PR').returns(Promise.resolve(rawPR));
70-
request.gql.withArgs('Reviews').returns(
71-
Promise.resolve(toRaw(approvingReviews)));
72-
request.gql.withArgs('PRComments').returns(
73-
Promise.resolve(toRaw(commentsWithLGTM)));
74-
request.gql.withArgs('PRCommits').returns(
75-
Promise.resolve(toRaw(oddCommits)));
76-
request.gql.returns(new Error('unknown query'));
77-
78-
it('getAll with specified readme', async() => {
79-
const cli = new TestCLI();
80-
const readmePath = path.resolve(
81-
__dirname, '..', 'fixtures', 'README', 'README.md');
82-
const argv2 = Object.assign({ readme: readmePath });
83-
const data = new PRData(argv2, cli, request);
84-
await data.getAll();
85-
assert.deepStrictEqual(data.collaborators, collaborators, 'collaborators');
86-
assert.deepStrictEqual(data.pr, firstTimerPR, 'pr');
87-
assert.deepStrictEqual(data.reviews, approvingReviews, 'reviews');
88-
assert.deepStrictEqual(data.comments, commentsWithLGTM, 'comments');
89-
assert.deepStrictEqual(data.commits, oddCommits, 'commits');
90-
assert.deepStrictEqual(data.reviewers, allGreenReviewers, 'reviewers');
91-
});
92-
});

0 commit comments

Comments
 (0)