labels: fix labels not being added

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,25 +43,27 @@ function updatePrWithLabels (options, labels) {
     return
   }
 
+  options.logger.debug('Trying to add labels: ' + labels)
+
   githubClient.issues.addLabels({
     user: options.owner,
     repo: options.repo,
     number: options.prId,
     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)
   })
 }
 
 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)
   })
 }
 

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/<PR-NUMBER>/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

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/<SHA>', (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)
-}

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)
+}

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'))
+  })
+})