diff --git a/lib/node-repo.js b/lib/node-repo.js index 1d803bf2..f36f76b9 100644 --- a/lib/node-repo.js +++ b/lib/node-repo.js @@ -24,8 +24,13 @@ function resolveLabelsThenUpdatePr (options) { const filepathsChanged = res.map((fileMeta) => fileMeta.filename) const resolvedLabels = resolveLabels(filepathsChanged, options.baseBranch) - fetchExistingLabels(options, (existingLabels) => { + fetchExistingLabels(options, (err, existingLabels) => { + if (err) { + return options.logger.error(err, 'Error retrieving existing repo labels') + } + const labelsToAdd = itemsInCommon(existingLabels, resolvedLabels) + options.logger.debug('Resolved labels: ' + labelsToAdd) updatePrWithLabels(options, labelsToAdd) }) @@ -38,6 +43,8 @@ function updatePrWithLabels (options, labels) { return } + options.logger.debug('Trying to add labels: ' + labels) + githubClient.issues.addLabels({ user: options.owner, repo: options.repo, @@ -45,10 +52,10 @@ function updatePrWithLabels (options, labels) { body: labels }, (err) => { if (err) { - return options.logger.error(err, 'Error while editing issue to add labels') + return options.logger.error(err, 'Error while adding labels') } - options.logger.info(`Added labels: ${labels}`) + options.logger.info('Added labels: ' + labels) }) } @@ -56,7 +63,7 @@ function fetchExistingLabels (options, cb) { const cacheKey = `${options.owner}:${options.repo}` if (existingLabelsCache.has(cacheKey)) { - return cb(existingLabelsCache.get(cacheKey)) + return cb(null, existingLabelsCache.get(cacheKey)) } // the github client API is somewhat misleading, @@ -66,13 +73,16 @@ function fetchExistingLabels (options, cb) { repo: options.repo }, (err, existingLabels) => { if (err) { - return options.logger.error(err, 'Error retrieving existing repo labels from GitHub') + return cb(err) } + const existingLabelNames = existingLabels.map((label) => label.name) + // cache labels so we don't have to fetch these *all the time* - existingLabelsCache.set(cacheKey, existingLabels) + existingLabelsCache.set(cacheKey, existingLabelNames) + options.logger.debug('Filled existing repo labels cache: ' + existingLabelNames) - cb(existingLabels) + cb(null, existingLabelNames) }) } diff --git a/test/integration/node-labels-webhook.test.js b/test/integration/node-labels-webhook.test.js index 41788311..12643a3a 100644 --- a/test/integration/node-labels-webhook.test.js +++ b/test/integration/node-labels-webhook.test.js @@ -1,8 +1,6 @@ 'use strict' const tap = require('tap') -const fs = require('fs') -const path = require('path') const url = require('url') const nock = require('nock') const supertest = require('supertest') @@ -21,6 +19,8 @@ const testStubs = { const app = proxyquire('../../app', testStubs) +const readFixture = require('../read-fixture') + setupNoRequestMatchHandler() tap.test('Sends POST request to https://api.github.com/repos/nodejs/node/issues//labels', (t) => { @@ -124,11 +124,6 @@ function ignoreQueryParams (pathAndQuery) { return url.parse(pathAndQuery, true).pathname } -function readFixture (fixtureName) { - const content = fs.readFileSync(path.join(__dirname, '..', '_fixtures', fixtureName)).toString() - return JSON.parse(content) -} - // nock doesn't make the tests explode if an unexpected external request is made, // we therefore have to attach an explicit "no match" handler too make tests fail // if there's made outgoing request we didn't expect diff --git a/test/integration/push-jenkins-update.test.js b/test/integration/push-jenkins-update.test.js index 959592b8..39a49868 100644 --- a/test/integration/push-jenkins-update.test.js +++ b/test/integration/push-jenkins-update.test.js @@ -1,14 +1,14 @@ 'use strict' const tap = require('tap') -const fs = require('fs') -const path = require('path') const url = require('url') const nock = require('nock') const supertest = require('supertest') const app = require('../../app') +const readFixture = require('../read-fixture') + tap.test('Sends POST requests to https://api.github.com/repos/nodejs/node/statuses/', (t) => { const jenkinsPayload = readFixture('success-payload.json') @@ -85,8 +85,3 @@ function setupGetCommitsMock () { function ignoreQueryParams (pathAndQuery) { return url.parse(pathAndQuery, true).pathname } - -function readFixture (fixtureName) { - const content = fs.readFileSync(path.join(__dirname, '..', '_fixtures', fixtureName)).toString() - return JSON.parse(content) -} diff --git a/test/read-fixture.js b/test/read-fixture.js new file mode 100644 index 00000000..dcf9597c --- /dev/null +++ b/test/read-fixture.js @@ -0,0 +1,9 @@ +'use strict' + +const fs = require('fs') +const path = require('path') + +module.exports = function readFixture (fixtureName) { + const content = fs.readFileSync(path.join(__dirname, '_fixtures', fixtureName)).toString() + return JSON.parse(content) +} diff --git a/test/unit/node-repo.test.js b/test/unit/node-repo.test.js index 73b9a2e0..fb5351c1 100644 --- a/test/unit/node-repo.test.js +++ b/test/unit/node-repo.test.js @@ -5,7 +5,9 @@ const proxyquire = require('proxyquire') const sinon = require('sinon') const tap = require('tap') +const logger = require('../../lib/logger') const githubClient = require('../../lib/github-client') +const readFixture = require('../read-fixture') tap.test('fetchExistingLabels(): caches existing repository labels', (t) => { const fakeGithubClient = sinon.stub(githubClient.issues, 'getLabels').yields(null, []) @@ -16,8 +18,8 @@ tap.test('fetchExistingLabels(): caches existing repository labels', (t) => { t.plan(1) t.tearDown(() => githubClient.issues.getLabels.restore()) - nodeRepo._fetchExistingLabels({ owner: 'nodejs', repo: 'node' }, () => { - nodeRepo._fetchExistingLabels({ owner: 'nodejs', repo: 'node' }, () => { + nodeRepo._fetchExistingLabels({ owner: 'nodejs', repo: 'node', logger }, () => { + nodeRepo._fetchExistingLabels({ owner: 'nodejs', repo: 'node', logger }, () => { t.ok(fakeGithubClient.calledOnce) }) }) @@ -31,14 +33,30 @@ tap.test('fetchExistingLabels(): cache expires after one hour', (t) => { }) t.plan(1) - t.tearDown(() => clock.uninstall() && githubClient.issues.getLabels.restore()) + t.tearDown(() => githubClient.issues.getLabels.restore() && clock.uninstall()) - nodeRepo._fetchExistingLabels({ owner: 'nodejs', repo: 'node' }, () => { + nodeRepo._fetchExistingLabels({ owner: 'nodejs', repo: 'node', logger }, () => { // fetch labels again after 1 hour and 1 minute clock.tick(1000 * 60 * 61) - nodeRepo._fetchExistingLabels({ owner: 'nodejs', repo: 'node' }, () => { + nodeRepo._fetchExistingLabels({ owner: 'nodejs', repo: 'node', logger }, () => { t.equal(fakeGithubClient.callCount, 2) }) }) }) + +tap.test('fetchExistingLabels(): yields an array of existing label names', (t) => { + const labelsFixture = readFixture('repo-labels.json') + const fakeGithubClient = sinon.stub(githubClient.issues, 'getLabels').yields(null, labelsFixture) + const nodeRepo = proxyquire('../../lib/node-repo', { + './github-client': fakeGithubClient + }) + + t.plan(2) + t.tearDown(() => githubClient.issues.getLabels.restore()) + + nodeRepo._fetchExistingLabels({ owner: 'nodejs', repo: 'node', logger }, (err, existingLabels) => { + t.equal(err, null) + t.ok(existingLabels.includes('cluster')) + }) +})