diff --git a/app.js b/app.js index 82684c49..b0b2308d 100644 --- a/app.js +++ b/app.js @@ -1,9 +1,9 @@ 'use strict' -const glob = require('glob') const express = require('express') const bodyParser = require('body-parser') const bunyanMiddleware = require('bunyan-middleware') +const AsyncEventEmitter = require('events-async') const logger = require('./lib/logger') const authMiddleware = require('./lib/auth-middleware') @@ -11,8 +11,8 @@ const authMiddleware = require('./lib/auth-middleware') const captureRaw = (req, res, buffer) => { req.raw = buffer } const app = express() +const events = new AsyncEventEmitter() -const scriptsToLoad = process.env.SCRIPTS || './scripts/**/*.js' const logsDir = process.env.LOGS_DIR || '' app.use(bodyParser.json({ verify: captureRaw })) @@ -29,17 +29,11 @@ app.use(bunyanMiddleware({ obscureHeaders: ['x-hub-signature'] })) -require('./lib/github-events')(app) - -// load all the files in the scripts folder -glob.sync(scriptsToLoad).forEach((file) => { - logger.info('Loading:', file) - require(file)(app) -}) +require('./lib/github-events')(app, events) app.use(function logUnhandledErrors (err, req, res, next) { logger.error(err, 'Unhandled error while responding to incoming HTTP request') res.status(500).end() }) -module.exports = app +module.exports = { app, events } diff --git a/lib/github-comment.js b/lib/github-comment.js index 221bc195..90ea2e15 100644 --- a/lib/github-comment.js +++ b/lib/github-comment.js @@ -2,15 +2,16 @@ const githubClient = require('./github-client') -exports.createPrComment = function createPrComment ({ owner, repo, number, logger }, body) { - githubClient.issues.createComment({ - owner, - repo, - number, - body - }, (err) => { - if (err) { - logger.error(err, 'Error while creating comment on GitHub') - } - }) +exports.createPrComment = async function createPrComment ({ owner, repo, number, logger }, body) { + try { + await githubClient.issues.createComment({ + owner, + repo, + number, + body + }) + } catch (err) { + logger.error(err, 'Error while creating comment on GitHub') + throw err + } } diff --git a/lib/github-events.js b/lib/github-events.js index b4bd6b9b..00523af1 100644 --- a/lib/github-events.js +++ b/lib/github-events.js @@ -2,8 +2,8 @@ const debug = require('debug')('github-events') const githubSecret = require('./github-secret') -module.exports = (app) => { - app.post('/hooks/github', (req, res) => { +module.exports = (app, events) => { + app.post('/hooks/github', async (req, res) => { const event = req.headers['x-github-event'] if (!event) { res.writeHead(400, 'Event Header Missing') @@ -19,9 +19,15 @@ module.exports = (app) => { const data = req.body data.action = data.action ? event + '.' + data.action : event - res.end() + try { + await app.emitGhEvent(data, req.log) + res.status(200) + } catch (err) { + req.log.error(err, 'Error while emitting GitHub event') + res.status(500) + } - app.emitGhEvent(data, req.log) + res.end() }) app.emitGhEvent = function emitGhEvent (data, logger) { @@ -45,6 +51,6 @@ module.exports = (app) => { data.logger.info('Emitting GitHub event') debug(data) - app.emit(data.action, data, org, repo, data.sender.login) + return events.emit(data.action, data, org, repo, data.sender.login) } } diff --git a/lib/node-repo.js b/lib/node-repo.js index 5b39f84b..1c3ee860 100644 --- a/lib/node-repo.js +++ b/lib/node-repo.js @@ -9,9 +9,12 @@ const existingLabelsCache = new LRU({ max: 1, maxAge: 1000 * 60 * 60 }) const fiveSeconds = 5 * 1000 -function deferredResolveLabelsThenUpdatePr (options) { +const sleep = (ms) => new Promise(resolve => setTimeout(resolve, ms)) + +async function deferredResolveLabelsThenUpdatePr (options) { const timeoutMillis = (options.timeoutInSec || 0) * 1000 - setTimeout(resolveLabelsThenUpdatePr, timeoutMillis, options) + await sleep(timeoutMillis) + return resolveLabelsThenUpdatePr(options) } function resolveLabelsThenUpdatePr (options) { @@ -25,35 +28,36 @@ function resolveLabelsThenUpdatePr (options) { }, cb) } - retry({ times: 5, interval: fiveSeconds }, getFiles, (err, res) => { - if (err) { - return options.logger.error(err, 'Error retrieving files from GitHub') - } + return new Promise((resolve, reject) => { + retry({ times: 5, interval: fiveSeconds }, getFiles, (err, res) => { + if (err) { + options.logger.error(err, 'Error retrieving files from GitHub') + return reject(err) + } - const filepathsChanged = res.data.map((fileMeta) => fileMeta.filename) - const resolvedLabels = resolveLabels(filepathsChanged, options.baseBranch) + const filepathsChanged = res.data.map((fileMeta) => fileMeta.filename) + const resolvedLabels = resolveLabels(filepathsChanged, options.baseBranch) - fetchExistingThenUpdatePr(options, resolvedLabels) + resolve(fetchExistingThenUpdatePr(options, resolvedLabels)) + }) }) } -function fetchExistingThenUpdatePr (options, labels) { - fetchExistingLabels(options, (err, existingLabels) => { - if (err) { - options.logger.error(err, 'Error retrieving existing repo labels') - - updatePrWithLabels(options, labels) - return - } - +async function fetchExistingThenUpdatePr (options, labels) { + try { + const existingLabels = await fetchExistingLabels(options) const labelsToAdd = stringsInCommon(existingLabels, labels) - options.logger.debug('Resolved labels: ' + labelsToAdd) + options.logger.debug('Resolved labels: ' + labelsToAdd, labels, existingLabels) - updatePrWithLabels(options, labelsToAdd) - }) + return updatePrWithLabels(options, labelsToAdd) + } catch (err) { + options.logger.error(err, 'Error retrieving existing repo labels') + + return updatePrWithLabels(options, labels) + } } -function updatePrWithLabels (options, labels) { +async function updatePrWithLabels (options, labels) { // no need to request github if we didn't resolve any labels if (!labels.length) { return @@ -61,18 +65,18 @@ function updatePrWithLabels (options, labels) { options.logger.debug('Trying to add labels: ' + labels) - githubClient.issues.addLabels({ - owner: options.owner, - repo: options.repo, - number: options.prId, - labels: labels - }, (err) => { - if (err) { - return options.logger.error(err, 'Error while adding labels') - } + try { + await githubClient.issues.addLabels({ + owner: options.owner, + repo: options.repo, + number: options.prId, + labels: labels + }) options.logger.info('Added labels: ' + labels) - }) + } catch (err) { + options.logger.error(err, 'Error while adding labels') + } } function removeLabelFromPR (options, label) { @@ -99,51 +103,44 @@ function removeLabelFromPR (options, label) { }) } -function fetchExistingLabels (options, cb) { +async function fetchExistingLabels (options) { const cacheKey = `${options.owner}:${options.repo}` if (existingLabelsCache.has(cacheKey)) { - return cb(null, existingLabelsCache.get(cacheKey)) + return existingLabelsCache.get(cacheKey) } - fetchLabelPages(options, 1, (err, existingLabels) => { - if (err) { - return cb(err) - } - - existingLabels = existingLabels.data || existingLabels || [] - const existingLabelNames = existingLabels.map((label) => label.name) + const labelsResult = await fetchLabelPages(options, 1) + const existingLabels = labelsResult.data || labelsResult || [] + const existingLabelNames = existingLabels.map((label) => label.name) - // cache labels so we don't have to fetch these *all the time* - existingLabelsCache.set(cacheKey, existingLabelNames) - options.logger.debug('Filled existing repo labels cache: ' + existingLabelNames) + // cache labels so we don't have to fetch these *all the time* + existingLabelsCache.set(cacheKey, existingLabelNames) + options.logger.debug('Filled existing repo labels cache: ' + existingLabelNames) - cb(null, existingLabelNames) - }) + return existingLabelNames } -function fetchLabelPages (options, startPageNum, cb) { +async function fetchLabelPages (options, startPageNum, cb) { // the github client API is somewhat misleading, // this fetches *all* repo labels not just for an issue - githubClient.issues.getLabels({ + const result = await githubClient.issues.getLabels({ owner: options.owner, repo: options.repo, page: startPageNum, per_page: 100 - }, (err, res) => { - const existingLabels = res.data || [] - if (err) { - return cb(err) - } - if (!githubClient.hasNextPage(res)) { - return cb(null, existingLabels) - } - fetchLabelPages( - options, - startPageNum + 1, - (err, remainingLabels) => err ? cb(err) : cb(null, existingLabels.concat(remainingLabels)) - ) }) + + const existingLabels = result.data || [] + if (!githubClient.hasNextPage(result)) { + return existingLabels + } + + const remainingLabels = await fetchLabelPages( + options, + startPageNum + 1) + + return existingLabels.concat(remainingLabels) } function getBotPrLabels (options, cb) { diff --git a/package-lock.json b/package-lock.json index 484ff03b..b7669c9e 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1463,6 +1463,11 @@ "resolved": "https://registry.npmjs.org/etag/-/etag-1.8.1.tgz", "integrity": "sha1-Qa4u62XvpiJorr/qg6x9eSmbCIc=" }, + "events-async": { + "version": "1.2.1", + "resolved": "https://registry.npmjs.org/events-async/-/events-async-1.2.1.tgz", + "integrity": "sha512-bI2+3DF7SoORyw9xooadSyKHEFCZmkNx+fwe1mWCNUdSkkcsNQQeSvNw/ludXqWEI0zS3b93r07Uslnk304tXg==" + }, "events-to-array": { "version": "1.1.2", "resolved": "https://registry.npmjs.org/events-to-array/-/events-to-array-1.1.2.tgz", diff --git a/package.json b/package.json index 4ddc0983..1ab10604 100644 --- a/package.json +++ b/package.json @@ -20,6 +20,7 @@ "bunyan-middleware": "0.8.0", "debug": "^2.2.0", "dotenv": "^2.0.0", + "events-async": "^1.2.1", "express": "^4.13.4", "github": "^13.1.1", "glob": "^7.0.3", diff --git a/scripts/node-subsystem-label.js b/scripts/node-subsystem-label.js index 409e882b..ce3bd0b9 100644 --- a/scripts/node-subsystem-label.js +++ b/scripts/node-subsystem-label.js @@ -4,8 +4,10 @@ const debug = require('debug')('node_subsystem_label') const nodeRepo = require('../lib/node-repo') -module.exports = function (app) { - app.on('pull_request.opened', handlePrCreated) +const timeoutInSec = process.env.WAIT_SECONDS_BEFORE_RESOLVING_LABELS || 2 + +module.exports = function (app, events) { + events.on('pull_request.opened', handlePrCreated) } function handlePrCreated (event, owner, repo) { @@ -20,12 +22,12 @@ function handlePrCreated (event, owner, repo) { // by not hard coding the owner repo to nodejs/node here, // we can test these this script in a different repo than // *actual* node core as long as the repo is named "node" - nodeRepo.resolveLabelsThenUpdatePr({ + return nodeRepo.resolveLabelsThenUpdatePr({ owner, repo, prId, logger, baseBranch, - timeoutInSec: 2 + timeoutInSec }) } diff --git a/scripts/trigger-jenkins-build.js b/scripts/trigger-jenkins-build.js index 3392d6fb..44a5b53b 100644 --- a/scripts/trigger-jenkins-build.js +++ b/scripts/trigger-jenkins-build.js @@ -58,62 +58,61 @@ function triggerBuild (options, cb) { options.logger.debug('Triggering Jenkins build') - request.post({ - uri: `https://ci.nodejs.org/blue/rest/organizations/jenkins/pipelines/${jobName}/runs/`, - headers: { authorization }, - qs: { token: buildAuthToken }, - json: { parameters: buildParametersForRepo(options, repo) } - }, (err, response, body) => { - if (err) { - return cb(err) - } else if (response.statusCode !== 200) { - return cb(new Error(`Expected 200 from Jenkins, got ${response.statusCode}: ${body}`)) - } - - cb(null, { jobName, jobId: response.body.id }) + return new Promise((resolve, reject) => { + request.post({ + uri: `https://ci.nodejs.org/blue/rest/organizations/jenkins/pipelines/${jobName}/runs/`, + headers: { authorization }, + qs: { token: buildAuthToken }, + json: { parameters: buildParametersForRepo(options, repo) } + }, (err, response, body) => { + if (err) { + return reject(err) + } else if (response.statusCode !== 200) { + return reject(new Error(`Expected 200 from Jenkins, got ${response.statusCode}: ${body}`)) + } + + resolve({ jobName, jobId: response.body.id }) + }) }) } -function triggerBuildIfValid (options) { +async function triggerBuildIfValid (options) { const { owner, repo, author, logger } = options - githubClient.repos.checkCollaborator({ owner, repo, username: author }, function onResponse (err) { - if (err) { - return logger.debug(`Ignoring comment to me by @${options.author} because they are not a repo collaborator`) - } + try { + await githubClient.repos.checkCollaborator({ owner, repo, username: author }) + } catch (_err) { + logger.debug(`Ignoring comment to me by @${options.author} because they are not a repo collaborator`) + return + } - triggerBuild(options, function onBuildTriggered (err, result) { - if (err) { - return logger.error(err, 'Error while triggering Jenkins build') - } + try { + const result = await triggerBuild(options) + const jobUrl = `https://ci.nodejs.org/job/${result.jobName}/${result.jobId}` + logger.info({ jobUrl }, 'Jenkins build started') - const jobUrl = `https://ci.nodejs.org/job/${result.jobName}/${result.jobId}` - logger.info({ jobUrl }, 'Jenkins build started') - createPrComment(options, `Lite-CI: ${jobUrl}`) - }) - }) + return createPrComment(options, `Lite-CI: ${jobUrl}`) + } catch (err) { + logger.error(err, 'Error while triggering Jenkins build') + throw err + } } -module.exports = (app) => { - app.on('issue_comment.created', handleCommentCreated) - - app.on('pull_request.opened', handlePullCreated) +module.exports = (app, events) => { + events.on('issue_comment.created', handleCommentCreated) + events.on('pull_request.opened', handlePullCreated) } function handleCommentCreated (event, owner, repo) { const { number, logger, comment: { body, user: { login: author } } } = event const options = { owner, repo, number, logger, author } - if (wasBotMentionedInCiComment(body)) { - triggerBuildIfValid(options) - } + return wasBotMentionedInCiComment(body) ? triggerBuildIfValid(options) : Promise.resolve() } function handlePullCreated (event, owner, repo) { const { number, logger, pull_request: { user: { login: author } } } = event const options = { owner, repo, number, logger, author } - if (repo === 'node') { - triggerBuildIfValid(options) - } + return repo === 'node' ? triggerBuildIfValid(options) : Promise.resolve() } diff --git a/server.js b/server.js index e08b9f08..41b63ea1 100644 --- a/server.js +++ b/server.js @@ -2,6 +2,7 @@ require('dotenv').load({ silent: true }) +const glob = require('glob') const logger = require('./lib/logger') const { spawnSync } = require('child_process') @@ -19,7 +20,14 @@ if (process.env.NODE_REPO_DIR) { } const port = process.env.PORT || 3000 -const app = require('./app') +const scriptsToLoad = process.env.SCRIPTS || './scripts/**/*.js' +const { app, events } = require('./app') + +// load all the files in the scripts folder +glob.sync(scriptsToLoad).forEach((file) => { + logger.info('Loading:', file) + require(file)(app, events) +}) app.listen(port, () => { logger.info('Listening on port', port) diff --git a/test/_fixtures/repo-labels-page-2.json b/test/_fixtures/repo-labels-page-2.json index db72898d..ad0ccb5c 100644 --- a/test/_fixtures/repo-labels-page-2.json +++ b/test/_fixtures/repo-labels-page-2.json @@ -14,13 +14,6 @@ "color": "eb6420", "default": false }, - { - "id": 364835500, - "url": "https://api.github.com/repos/nodejs/node/labels/v6.x", - "name": "v6.x", - "color": "c2e0c6", - "default": false - }, { "id": 441404503, "url": "https://api.github.com/repos/nodejs/node/labels/v7.x", diff --git a/test/_fixtures/repo-labels.json b/test/_fixtures/repo-labels.json index 46a461f1..b3d5e759 100644 --- a/test/_fixtures/repo-labels.json +++ b/test/_fixtures/repo-labels.json @@ -154,6 +154,16 @@ "url": "https://api.github.com/repos/nodejs/node/labels/V8 Engine", "name": "V8 Engine", "color": "0052cc" + }, + { + "url": "https://api.github.com/repos/nodejs/node/labels/timers", + "name": "timers", + "color": "f7c6c7" + }, + { + "url": "https://api.github.com/repos/nodejs/node/labels/v6.x", + "name": "v6.x", + "color": "c2e0c6" } ], "meta": { diff --git a/test/integration/node-labels-webhook.test.js b/test/integration/node-labels-webhook.test.js index 273bd99f..2f5a26f5 100644 --- a/test/integration/node-labels-webhook.test.js +++ b/test/integration/node-labels-webhook.test.js @@ -5,7 +5,6 @@ const url = require('url') const nock = require('nock') const supertest = require('supertest') const proxyquire = require('proxyquire') -const lolex = require('lolex') const testStubs = { './github-secret': { @@ -17,14 +16,31 @@ const testStubs = { } } -const app = proxyquire('../../app', testStubs) +process.env.WAIT_SECONDS_BEFORE_RESOLVING_LABELS = 0 const readFixture = require('../read-fixture') +// Clearing the require cache is needed due to labels being cached into a singleton variable. +// To ensure every test can run on its own without relying on other tests having run already +// resulted in the cache being filled up, we enforce all tests to run without any "cache warming", +// hence labels has to be fetched every time +function clearRequireCache () { + for (const modulePath of Object.keys(require.cache)) { + delete require.cache[modulePath] + } +} + +function initializeApp () { + const { app, events } = proxyquire('../../app', testStubs) + clearRequireCache() + require('../../scripts/node-subsystem-label')(app, events) + return app +} + setupNoRequestMatchHandler() tap.test('Sends POST request to https://api.github.com/repos/nodejs/node/issues//labels', (t) => { - const clock = lolex.install() + const app = initializeApp() const expectedLabels = ['timers'] const webhookPayload = readFixture('pull-request-opened.json') @@ -44,7 +60,11 @@ tap.test('Sends POST request to https://api.github.com/repos/nodejs/node/issues/ .reply(200) t.plan(1) - t.tearDown(() => filesScope.done() && existingRepoLabelsScope.done() && newLabelsScope.done() && clock.uninstall()) + t.tearDown(() => { + filesScope.done() + existingRepoLabelsScope.done() + newLabelsScope.done() + }) supertest(app) .post('/hooks/github') @@ -52,13 +72,12 @@ tap.test('Sends POST request to https://api.github.com/repos/nodejs/node/issues/ .send(webhookPayload) .expect(200) .end((err, res) => { - clock.runAll() t.equal(err, null) }) }) tap.test('Adds v6.x label when PR is targeting the v6.x-staging branch', (t) => { - const clock = lolex.install() + const app = initializeApp() const expectedLabels = ['timers', 'v6.x'] const webhookPayload = readFixture('pull-request-opened-v6.x.json') @@ -78,7 +97,11 @@ tap.test('Adds v6.x label when PR is targeting the v6.x-staging branch', (t) => .reply(200) t.plan(1) - t.tearDown(() => filesScope.done() && existingRepoLabelsScope.done() && newLabelsScope.done() && clock.uninstall()) + t.tearDown(() => { + filesScope.done() + existingRepoLabelsScope.done() + newLabelsScope.done() + }) supertest(app) .post('/hooks/github') @@ -86,14 +109,13 @@ tap.test('Adds v6.x label when PR is targeting the v6.x-staging branch', (t) => .send(webhookPayload) .expect(200) .end((err, res) => { - clock.runAll() t.equal(err, null) }) }) // reported bug: https://github.com/nodejs/github-bot/issues/58 tap.test('Does not create labels which does not already exist', (t) => { - const clock = lolex.install() + const app = initializeApp() const webhookPayload = readFixture('pull-request-opened-mapproxy.json') const filesScope = nock('https://api.github.com') @@ -107,7 +129,10 @@ tap.test('Does not create labels which does not already exist', (t) => { .reply(200, readFixture('repo-labels.json')) t.plan(1) - t.tearDown(() => filesScope.done() && existingRepoLabelsScope.done() && clock.uninstall()) + t.tearDown(() => { + filesScope.done() + existingRepoLabelsScope.done() + }) supertest(app) .post('/hooks/github') @@ -115,14 +140,13 @@ tap.test('Does not create labels which does not already exist', (t) => { .send(webhookPayload) .expect(200) .end((err, res) => { - clock.runAll() t.equal(err, null) }) }) // reported bug: https://github.com/nodejs/github-bot/issues/92 tap.test('Adds V8 Engine label when PR has deps/v8 file changes', (t) => { - const clock = lolex.install() + const app = initializeApp() const expectedLabels = ['V8 Engine'] const webhookPayload = readFixture('pull-request-opened-v8.json') @@ -142,7 +166,11 @@ tap.test('Adds V8 Engine label when PR has deps/v8 file changes', (t) => { .reply(200) t.plan(1) - t.tearDown(() => filesScope.done() && existingRepoLabelsScope.done() && newLabelsScope.done() && clock.uninstall()) + t.tearDown(() => { + filesScope.done() + existingRepoLabelsScope.done() + newLabelsScope.done() + }) supertest(app) .post('/hooks/github') @@ -150,7 +178,6 @@ tap.test('Adds V8 Engine label when PR has deps/v8 file changes', (t) => { .send(webhookPayload) .expect(200) .end((err, res) => { - clock.runAll() t.equal(err, null) }) }) diff --git a/test/integration/ping.test.js b/test/integration/ping.test.js index 104095a7..3f9c0207 100644 --- a/test/integration/ping.test.js +++ b/test/integration/ping.test.js @@ -3,7 +3,9 @@ const tap = require('tap') const request = require('request') -const app = require('../../app') +const { app, events } = require('../../app') + +require('../../scripts/ping')(app, events) tap.test('GET /ping responds with status 200 / "pong"', (t) => { const server = app.listen() diff --git a/test/integration/push-jenkins-update.test.js b/test/integration/push-jenkins-update.test.js index 8f79d6b2..ee3a66bf 100644 --- a/test/integration/push-jenkins-update.test.js +++ b/test/integration/push-jenkins-update.test.js @@ -5,10 +5,12 @@ const url = require('url') const nock = require('nock') const supertest = require('supertest') -const app = require('../../app') +const { app, events } = require('../../app') const readFixture = require('../read-fixture') +require('../../scripts/jenkins-status')(app, events) + tap.test('Sends POST requests to https://api.github.com/repos/nodejs/node/statuses/', (t) => { const jenkinsPayload = readFixture('success-payload.json') diff --git a/test/integration/trigger-jenkins-build.test.js b/test/integration/trigger-jenkins-build.test.js index 090cbbe8..2548c4c5 100644 --- a/test/integration/trigger-jenkins-build.test.js +++ b/test/integration/trigger-jenkins-build.test.js @@ -5,10 +5,9 @@ const url = require('url') const nock = require('nock') const supertest = require('supertest') const proxyquire = require('proxyquire') -const lolex = require('lolex') const readFixture = require('../read-fixture') -const app = proxyquire('../../app', { +const { app, events } = proxyquire('../../app', { './github-secret': { isValid: () => true, @@ -18,16 +17,16 @@ const app = proxyquire('../../app', { } }) -tap.test('Sends POST request to https://ci.nodejs.org', (t) => { - const clock = lolex.install() +require('../../scripts/trigger-jenkins-build')(app, events) +tap.test('Sends POST request to https://ci.nodejs.org', (t) => { const originalJobUrlValue = process.env.JENKINS_JOB_URL_NODE const originalTokenValue = process.env.JENKINS_BUILD_TOKEN_NODE process.env.JENKINS_JOB_NODE = 'node-test-pull-request-lite-pipeline' process.env.JENKINS_BUILD_TOKEN_NODE = 'myToken' const webhookPayload = readFixture('pull-request-opened.json') - const pipelineUrl = 'https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1/pipeline' + const pipelineUrl = 'https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/1' const collaboratorsScope = nock('https://api.github.com') .filteringPath(ignoreQueryParams) @@ -40,11 +39,15 @@ tap.test('Sends POST request to https://ci.nodejs.org', (t) => { const commentScope = nock('https://api.github.com') .filteringPath(ignoreQueryParams) - .post('/repos/nodejs/node/issues/19/comments', { body: `@phillipj build started: ${pipelineUrl}` }) + .post('/repos/nodejs/node/issues/19/comments', { body: `Lite-CI: ${pipelineUrl}` }) .reply(200) t.plan(1) - t.tearDown(() => collaboratorsScope.done() && ciJobScope.done() && commentScope.done() && clock.uninstall()) + t.tearDown(() => { + collaboratorsScope.done() + ciJobScope.done() + commentScope.done() + }) supertest(app) .post('/hooks/github') @@ -54,7 +57,6 @@ tap.test('Sends POST request to https://ci.nodejs.org', (t) => { .end((err, res) => { process.env.JENKINS_JOB_URL_NODE = originalJobUrlValue process.env.JENKINS_BUILD_TOKEN_NODE = originalTokenValue - clock.runAll() t.equal(err, null) }) }) diff --git a/test/unit/node-repo.test.js b/test/unit/node-repo.test.js index 396ee7c1..e92c7216 100644 --- a/test/unit/node-repo.test.js +++ b/test/unit/node-repo.test.js @@ -1,16 +1,16 @@ 'use strict' -const lolex = require('lolex') const proxyquire = require('proxyquire') const sinon = require('sinon') const tap = require('tap') +const lolex = require('lolex') const logger = require('../../lib/logger') const githubClient = require('../../lib/github-client') const readFixture = require('../read-fixture') -tap.test('fetchExistingLabels(): caches existing repository labels', (t) => { - sinon.stub(githubClient.issues, 'getLabels').yields(null, []) +tap.test('fetchExistingLabels(): caches existing repository labels', async (t) => { + sinon.stub(githubClient.issues, 'getLabels', () => Promise.resolve([])) sinon.stub(githubClient, 'hasNextPage', () => false) const nodeRepo = proxyquire('../../lib/node-repo', { './github-client': githubClient @@ -22,16 +22,14 @@ tap.test('fetchExistingLabels(): caches existing repository labels', (t) => { githubClient.hasNextPage.restore() }) - nodeRepo._fetchExistingLabels({ owner: 'nodejs', repo: 'node', logger }, () => { - nodeRepo._fetchExistingLabels({ owner: 'nodejs', repo: 'node', logger }, () => { - t.ok(githubClient.issues.getLabels.calledOnce) - }) - }) + await nodeRepo._fetchExistingLabels({ owner: 'nodejs', repo: 'node', logger }) + await nodeRepo._fetchExistingLabels({ owner: 'nodejs', repo: 'node', logger }) + t.ok(githubClient.issues.getLabels.calledOnce) }) -tap.test('fetchExistingLabels(): cache expires after one hour', (t) => { +tap.test('fetchExistingLabels(): cache expires after one hour', async (t) => { const clock = lolex.install() - sinon.stub(githubClient.issues, 'getLabels').yields(null, []) + sinon.stub(githubClient.issues, 'getLabels', () => Promise.resolve([])) sinon.stub(githubClient, 'hasNextPage', () => false) const nodeRepo = proxyquire('../../lib/node-repo', { './github-client': githubClient @@ -44,53 +42,49 @@ tap.test('fetchExistingLabels(): cache expires after one hour', (t) => { clock.uninstall() }) - nodeRepo._fetchExistingLabels({ owner: 'nodejs', repo: 'node', logger }, () => { - // fetch labels again after 1 hour and 1 minute - clock.tick(1000 * 60 * 61) + await nodeRepo._fetchExistingLabels({ owner: 'nodejs', repo: 'node', logger }) - nodeRepo._fetchExistingLabels({ owner: 'nodejs', repo: 'node', logger }, () => { - t.equal(githubClient.issues.getLabels.callCount, 2) - }) - }) + // fetch labels again after 1 hour and 1 minute + clock.tick(1000 * 60 * 61) + + await nodeRepo._fetchExistingLabels({ owner: 'nodejs', repo: 'node', logger }) + t.equal(githubClient.issues.getLabels.callCount, 2) }) -tap.test('fetchExistingLabels(): yields an array of existing label names', (t) => { +tap.test('fetchExistingLabels(): yields an array of existing label names', async (t) => { const labelsFixture = readFixture('repo-labels.json') - sinon.stub(githubClient.issues, 'getLabels').yields(null, labelsFixture) + sinon.stub(githubClient.issues, 'getLabels', () => Promise.resolve(labelsFixture)) sinon.stub(githubClient, 'hasNextPage', () => false) const nodeRepo = proxyquire('../../lib/node-repo', { './github-client': githubClient }) - t.plan(2) + t.plan(1) t.tearDown(() => { githubClient.issues.getLabels.restore() githubClient.hasNextPage.restore() }) - nodeRepo._fetchExistingLabels({ owner: 'nodejs', repo: 'node', logger }, (err, existingLabels) => { - t.equal(err, null) - t.ok(existingLabels.includes('cluster')) - }) + const existingLabels = await nodeRepo._fetchExistingLabels({ owner: 'nodejs', repo: 'node', logger }) + t.ok(existingLabels.includes('cluster')) }) -tap.test('fetchExistingLabels(): can retrieve more than 100 labels', (t) => { +tap.test('fetchExistingLabels(): can retrieve more than 100 labels', async (t) => { const labelsFixturePage1 = readFixture('repo-labels.json') const labelsFixturePage2 = readFixture('repo-labels-page-2.json') - sinon.stub(githubClient.issues, 'getLabels', (options, cb) => cb(null, options.page === 1 ? labelsFixturePage1 : labelsFixturePage2)) + sinon.stub(githubClient.issues, 'getLabels', (options) => Promise.resolve(options.page === 1 ? labelsFixturePage1 : labelsFixturePage2)) sinon.stub(githubClient, 'hasNextPage', (listing) => listing === labelsFixturePage1) const nodeRepo = proxyquire('../../lib/node-repo', { './github-client': githubClient }) - t.plan(3) + t.plan(2) t.tearDown(() => { githubClient.issues.getLabels.restore() githubClient.hasNextPage.restore() }) - nodeRepo._fetchExistingLabels({ owner: 'nodejs', repo: 'node', logger }, (err, existingLabels) => { - t.equal(err, null) - t.ok(existingLabels.includes('cluster')) - t.ok(existingLabels.includes('windows')) - }) + + const existingLabels = await nodeRepo._fetchExistingLabels({ owner: 'nodejs', repo: 'node', logger }) + t.ok(existingLabels.includes('cluster')) + t.ok(existingLabels.includes('windows')) }) tap.test('getBotPrLabels(): returns labels added by nodejs-github-bot', (t) => {