From 79f4389707f0d90e1a27358c31a37a7e8c2a8c44 Mon Sep 17 00:00:00 2001 From: Phillip Johnsen Date: Wed, 19 Oct 2016 20:09:44 +0200 Subject: [PATCH] labels: fix labels not being added. After PR https://github.com/nodejs/github-bot/pull/80 got merged, no labels were added to new PRs. That regression probably came from the fact that the existing repository labels was fetched and compared to the label names our labelling algorithm thought we should add. The problem is that we compared the whole label meta object (from api.github.com) against a label name (string). Example of the two different types of objects: ``` // complete label object { "url": "https://api.github.com/repos/nodejs/node/labels/buffer", "name": "buffer", "color": "f7c6c7" } // label name "buffer" ``` Comparing two such objects will obviuosly never match. These changes extracts the existing repo label *names* for comparison, and adds more logging for future debugging purposes. --- lib/node-repo.js | 24 ++++++++++++----- test/integration/node-labels-webhook.test.js | 9 ++----- test/integration/push-jenkins-update.test.js | 9 ++----- test/read-fixture.js | 9 +++++++ test/unit/node-repo.test.js | 28 ++++++++++++++++---- 5 files changed, 53 insertions(+), 26 deletions(-) create mode 100644 test/read-fixture.js 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')) + }) +})