From 69dd29dbb82a5c91a785deece103a08df4556545 Mon Sep 17 00:00:00 2001 From: Jessica Lopatta Date: Wed, 26 Jul 2023 09:45:11 -0400 Subject: [PATCH] refactor: update native metrics to download, then build by default --- lib/gyp-utils.js | 16 +- lib/pre-build.js | 219 +++++-------- tests/integration/download-server.js | 2 +- tests/unit/gyp-utils.tap.js | 55 ++-- tests/unit/pre-build.tap.js | 451 +++++++++++++-------------- 5 files changed, 332 insertions(+), 411 deletions(-) diff --git a/lib/gyp-utils.js b/lib/gyp-utils.js index 8c4eb56..3f8b329 100644 --- a/lib/gyp-utils.js +++ b/lib/gyp-utils.js @@ -81,7 +81,7 @@ utils.gypVersion = function gypVersion() { return match && match[1] } -utils.execGyp = function execGyp(args, opts, cb) { +utils.execGyp = function execGyp(args, opts) { const cmd = utils.extractGypCmd(args) const spawnOpts = {} if (!opts.quiet) { @@ -89,13 +89,9 @@ utils.execGyp = function execGyp(args, opts, cb) { } console.log('> ' + cmd + ' ' + args.join(' ')) // eslint-disable-line no-console - const child = cp.spawn(cmd, args, spawnOpts) - child.on('error', cb) - child.on('close', function onGypClose(code) { - if (code !== 0) { - cb(new Error('Command exited with non-zero code: ' + code)) - } else { - cb(null) - } - }) + const child = cp.spawnSync(cmd, args, spawnOpts) + + if (child.status !== 0) { + throw new Error('Command exited with non-zero code: ' + child.status) + } } diff --git a/lib/pre-build.js b/lib/pre-build.js index e451b39..0b9c62e 100644 --- a/lib/pre-build.js +++ b/lib/pre-build.js @@ -13,7 +13,7 @@ // XXX This file must not have any deps. This file will run during the install // XXX step of the module and we are _not_ guaranteed that the dependencies have // XXX already installed. Core modules are okay. -const fs = require('fs') +const fs = require('fs/promises') const http = require('http') const https = require('https') const os = require('os') @@ -46,31 +46,25 @@ preBuild.load = function load(target) { return require(path.join(BUILD_PATH, getBinFileName(target))) } -preBuild.makePath = function makePath(pathToMake, cb) { +preBuild.makePath = async function makePath(pathToMake) { const accessRights = fs.constants.R_OK | fs.constants.W_OK // We only want to make the parts after the package directory. pathToMake = path.join(PACKAGE_ROOT, pathToMake) - fs.access(pathToMake, accessRights, function fsAccessCB(err) { - if (!err) { - return cb() - } else if (err?.code !== 'ENOENT') { + + try { + await fs.access(pathToMake, accessRights) + } catch (err) { + if (err?.code !== 'ENOENT') { // It exists but we don't have read+write access! This is a problem. - return cb(new Error(`Do not have access to '${pathToMake}': ${err}`)) + throw new Error(`Do not have access to '${pathToMake}': ${err}`) } - // It probably does not exist, so try to make it. - fs.mkdir(pathToMake, { recursive: true }, function fsMkDirDb(mkdirErr) { - if (mkdirErr) { - return cb(mkdirErr) - } - - cb() - }) - }) + await fs.mkdir(pathToMake, { recursive: true }) + } } -preBuild.build = function build(target, rebuild, cb) { +preBuild.build = function build(target, rebuild) { const HAS_OLD_NODE_GYP_ARGS_FOR_WINDOWS = semver.lt(gypVersion() || '0.0.0', '3.7.0') if (IS_WIN && HAS_OLD_NODE_GYP_ARGS_FOR_WINDOWS) { @@ -79,70 +73,16 @@ preBuild.build = function build(target, rebuild, cb) { const cmds = rebuild ? ['clean', 'configure'] : ['configure'] - execGyp(cmds, opts, function cleanCb(err) { - if (err) { - return cb(err) - } + execGyp(cmds, opts) - const jobs = Math.round(CPU_COUNT / 2) - execGyp(['build', '-j', jobs, target], opts, cb) - }) + const jobs = Math.round(CPU_COUNT / 2) + execGyp(['build', '-j', jobs, target], opts) } -preBuild.moveBuild = function moveBuild(target, cb) { +preBuild.moveBuild = async function moveBuild(target) { const filePath = path.join(BUILD_PATH, target + '.node') const destination = path.join(BUILD_PATH, getBinFileName(target)) - fs.rename(filePath, destination, cb) -} - -/** - * Pipes the response and gunzip and unzips the data - * - * @param {Object} params - * @param {http.ServerResponse} params.res response from download site - * @param {string} url download url - * @param {Function} cb callback when download is done - */ -function unzipFile(url, cb, res) { - if (res.statusCode === 404) { - return cb(new Error('No pre-built artifacts for your OS/architecture.')) - } else if (res.statusCode !== 200) { - return cb(new Error('Failed to download ' + url + ': code ' + res.statusCode)) - } - - let hasCalledBack = false - const unzip = zlib.createGunzip() - const buffers = [] - let size = 0 - - res.pipe(unzip).on('data', function onResData(data) { - buffers.push(data) - size += data.length - }) - - res.on('error', function onResError(err) { - if (!hasCalledBack) { - hasCalledBack = true - cb(new Error('Failed to download ' + url + ': ' + err.message)) - } - }) - - unzip.on('error', function onResError(err) { - if (!hasCalledBack) { - hasCalledBack = true - cb(new Error('Failed to unzip ' + url + ': ' + err.message)) - } - }) - - unzip.on('end', function onResEnd() { - if (hasCalledBack) { - return - } - hasCalledBack = true - cb(null, Buffer.concat(buffers, size)) - }) - - res.resume() + await fs.rename(filePath, destination) } function setupRequest(url, fileName) { @@ -170,91 +110,96 @@ function setupRequest(url, fileName) { return { client, options } } -preBuild.download = function download(target, cb) { +preBuild.download = async function download(target) { const fileName = getPackageFileName(target) const url = DOWNLOAD_HOST + REMOTE_PATH + fileName const { client, options } = setupRequest(url, fileName) - client.get(options, unzipFile.bind(null, url, cb)) -} - -preBuild.saveDownload = function saveDownload(target, data, cb) { - preBuild.makePath(BUILD_PATH, function makePathCB(err) { - if (err) { - return cb(err) - } + return new Promise((resolve, reject) => { + client.get(options, function handleResponse(res) { + if (res.statusCode === 404) { + reject(new Error('No pre-built artifacts for your OS/architecture.')) + } else if (res.statusCode !== 200) { + reject(new Error('Failed to download ' + url + ': code ' + res.statusCode)) + } - const filePath = path.join(BUILD_PATH, getBinFileName(target)) - fs.writeFile(filePath, data, cb) - }) -} + const unzip = zlib.createGunzip() + const buffers = [] + let size = 0 -preBuild.install = function install(target, cb) { - const errors = [] + res.on('error', function httpError(err) { + reject(new Error('Failed to download ' + url + ': ' + err.message)) + }) - const noBuild = opts['no-build'] || process.env.NR_NATIVE_METRICS_NO_BUILD - const noDownload = opts['no-download'] || process.env.NR_NATIVE_METRICS_NO_DOWNLOAD + unzip.on('error', function unzipError(err) { + reject(new Error('Failed to unzip ' + url + ': ' + err.message)) + }) - // If NR_NATIVE_METRICS_NO_BUILD env var is specified, jump straight to downloading - if (noBuild) { - return doDownload() - } + res.pipe(unzip).on('data', function onResData(data) { + buffers.push(data) + size += data.length + }) - // Otherwise, first attempt to build the package using the source. If that fails, try - // downloading the package. If that also fails, whoops! - preBuild.build(target, true, function buildCB(buildErr) { - if (!buildErr) { - return preBuild.moveBuild(target, function moveBuildCB(moveErr) { - if (moveErr) { - errors.push(moveErr) - doDownload() - } else { - doCallback() - } + unzip.on('end', function onResEnd() { + resolve(Buffer.concat(buffers, size)) }) - } - errors.push(buildErr) - // Building failed, try downloading. - doDownload() + res.resume() + }) }) +} - function doDownload() { - if (noDownload && !noBuild) { - return doCallback(new Error('Downloading is disabled.')) - } +preBuild.saveDownload = async function saveDownload(target, data) { + await preBuild.makePath(BUILD_PATH) - preBuild.download(target, function downloadCB(err, data) { - if (err) { - return doCallback(err) - } + const filePath = path.join(BUILD_PATH, getBinFileName(target)) + await fs.writeFile(filePath, data) +} - preBuild.saveDownload(target, data, doCallback) - }) +preBuild.install = async function install(target) { + const noBuild = opts['no-build'] || process.env.NR_NATIVE_METRICS_NO_BUILD + const noDownload = opts['no-download'] || process.env.NR_NATIVE_METRICS_NO_DOWNLOAD + + if (noDownload && !noBuild) { + // If NR_NATIVE_METRICS_NO_DOWNLOAD env var is specified, jump straight to building + preBuild.build(target, true) + return await preBuild.moveBuild(target) } - function doCallback(err) { - if (err) { - errors.push(err) - cb(err) - } else { - cb() + // Try the download path first, if that fails try building if config allows + try { + const data = await preBuild.download(target) + await preBuild.saveDownload(target, data) + } catch (err) { + // eslint-disable-next-line no-console + console.log(`Download error: ${err.message}, falling back to build`) + + if (noBuild) { + throw new Error('Building is disabled by configuration') } + + preBuild.build(target, true) + await preBuild.moveBuild(target) } } -preBuild.executeCli = function executeCli(cmd, target) { +preBuild.executeCli = async function executeCli(cmd, target) { logStart(cmd) if (cmd === 'build' || cmd === 'rebuild') { - preBuild.build(target, cmd === 'rebuild', function buildCb(err) { - if (err) { - logFinish(cmd, target, err) - } else { - preBuild.moveBuild(target, logFinish.bind(this, cmd, target)) - } - }) + try { + preBuild.build(target, cmd === 'rebuild') + await preBuild.moveBuild(target) + logFinish(cmd, target) + } catch (err) { + logFinish(cmd, target, err) + } } else if (cmd === 'install') { - preBuild.install(target, logFinish.bind(this, cmd, target)) + try { + await preBuild.install(target) + logFinish(cmd, target) + } catch (err) { + logFinish(cmd, target, err) + } } } diff --git a/tests/integration/download-server.js b/tests/integration/download-server.js index f9eb6f4..cb95f51 100644 --- a/tests/integration/download-server.js +++ b/tests/integration/download-server.js @@ -19,7 +19,7 @@ function findBinary() { // building module to serve in the server instead of grabbing from // download.newrelic.com -execSync(`node ./lib/pre-build install native_metrics`) +execSync(`node ./lib/pre-build rebuild native_metrics`) // moving module to avoid a passing test on download // even though the file existing in the build/Release folder execSync(`mv ./build/Release/*.node ${BINARY_TMP}`) diff --git a/tests/unit/gyp-utils.tap.js b/tests/unit/gyp-utils.tap.js index 2f02c36..a484605 100644 --- a/tests/unit/gyp-utils.tap.js +++ b/tests/unit/gyp-utils.tap.js @@ -10,7 +10,7 @@ const sinon = require('sinon') const cp = require('child_process') const common = require('../../lib/common') const gypUtils = require('../../lib/gyp-utils') -const EventEmitter = require('events') +const fs = require('fs') tap.test('gyp-utils tests', (t) => { t.autoend() @@ -30,14 +30,13 @@ tap.test('gyp-utils tests', (t) => { t.end() }) - // TODO: figure out how to stub require.resolve - /* t.test('should return node-gyp path', (t) => { - require.cache['node-gyp'] = process.cwd() + t.test('should return null if all lookups fail`', (t) => { sinon.stub(fs, 'accessSync') - fs.accessSync.returns(true) + fs.accessSync.throws(new Error('nope, could not access')) const gypPath = gypUtils.findNodeGyp() + t.equal(gypPath, null) t.end() - })*/ + }) }) t.test('extractGypCmd', (t) => { @@ -109,53 +108,43 @@ tap.test('gyp-utils tests', (t) => { t.beforeEach(() => { sandbox = sinon.createSandbox() - sandbox.stub(cp, 'spawn') + sandbox.stub(cp, 'spawnSync') }) t.afterEach(() => { sandbox.restore() }) - t.test('should return if code is 0', (t) => { - const emitter = new EventEmitter() - cp.spawn.returns(emitter) - gypUtils.execGyp([], {}, t.end) + t.test('should return if status is 0', (t) => { + cp.spawnSync.returns({ status: 0 }) + gypUtils.execGyp([], {}) t.same( - cp.spawn.args[0][2], + cp.spawnSync.args[0][2], { stdio: [0, 1, 2] }, - 'spawn opts should include stdio if not quiet' + 'spawnSync opts should include stdio if not quiet' ) - emitter.emit('close', 0) + t.end() }) t.test('should not set stdio on spawn if opts.quiet is true', (t) => { const opts = { quiet: true } - const emitter = new EventEmitter() - cp.spawn.returns(emitter) - gypUtils.execGyp([], opts, t.end) - t.same(cp.spawn.args[0][2], {}, 'spawn opts should include stdio if not quiet') - emitter.emit('close', 0) + cp.spawnSync.returns({ status: 0 }) + gypUtils.execGyp([], opts) + t.same(cp.spawnSync.args[0][2], {}, 'spawn opts should include stdio if not quiet') + t.end() }) t.test('should return with error if spawn fails', (t) => { - const emitter = new EventEmitter() - cp.spawn.returns(emitter) const expectedErr = new Error('failed to spawn gyp cmd') - gypUtils.execGyp([], {}, (err) => { - t.same(err, expectedErr) - t.end() - }) - emitter.emit('error', expectedErr) + cp.spawnSync.throws(expectedErr) + t.throws(() => gypUtils.execGyp([], {}), expectedErr) + t.end() }) t.test('should return with error if code is not 0', (t) => { - const emitter = new EventEmitter() - cp.spawn.returns(emitter) - gypUtils.execGyp([], {}, (err) => { - t.equal(err.message, 'Command exited with non-zero code: 1') - t.end() - }) - emitter.emit('close', 1) + cp.spawnSync.returns({ status: 1 }) + t.throws(() => gypUtils.execGyp([], {}), 'Command exited with non-zero code: 1') + t.end() }) }) }) diff --git a/tests/unit/pre-build.tap.js b/tests/unit/pre-build.tap.js index 1cef323..2ae5d50 100644 --- a/tests/unit/pre-build.tap.js +++ b/tests/unit/pre-build.tap.js @@ -8,10 +8,6 @@ const tap = require('tap') const sinon = require('sinon') const proxyquire = require('proxyquire') -const preBuild = require('../../lib/pre-build') -const { IS_WIN } = require('../../lib/common') -const { rm, mkdir, chmod } = require('fs/promises') -const fs = require('fs') const nock = require('nock') const zlib = require('zlib') @@ -20,61 +16,62 @@ tap.test('pre-build tests', (t) => { t.test('makePath', (t) => { t.autoend() + const fakePath = 'tests/unit/fake-path' - t.afterEach(async () => { - try { - await rm(`${process.cwd()}/${fakePath}`, { recursive: true }) - } catch { - // swallow removing folder - } - }) + let mockFsPromiseApi + let preBuild - t.test('should make a nested folder path accordingly if it does not exist', (t) => { - preBuild.makePath(fakePath, (err) => { - t.error(err) - t.ok(fs.statSync(fakePath), 'path should be made') - t.end() + t.beforeEach(() => { + mockFsPromiseApi = { + constants: { + R_OK: 4, + W_OK: 2 + }, + access: sinon.stub().resolves(), + mkdir: sinon.stub().resolves() + } + preBuild = proxyquire('../../lib/pre-build', { + 'fs/promises': mockFsPromiseApi }) }) - t.test('should return error if it cannot write nested path', async (t) => { - await mkdir(`${process.cwd()}/${fakePath}`, { recursive: true }) - t.ok(fs.statSync(fakePath), 'path exists') - return new Promise((resolve) => { - preBuild.makePath(fakePath, (err) => { - t.error(err) - t.ok(fs.statSync(fakePath), 'path should still exist') - resolve() - }) - }) + t.test('should make a nested folder path accordingly if it does not exist', async (t) => { + mockFsPromiseApi.access.rejects({ code: 'ENOENT' }) + await preBuild.makePath(fakePath) + + t.ok( + mockFsPromiseApi.mkdir.calledOnceWith(`${process.cwd()}/${fakePath}`, { recursive: true }), + 'should have called mkdir' + ) }) - t.test('should not make nested path if it already exists', { skip: IS_WIN }, async (t) => { + t.test('should throw if permissions to path are incorrect', async (t) => { const fullPath = `${process.cwd()}/${fakePath}` - await mkdir(fullPath, { recursive: true }) - t.ok(fs.statSync(fakePath), 'path exists') - // chmod does not work on windows, will skip - await chmod(fullPath, '00400') - return new Promise((resolve) => { - preBuild.makePath(fakePath, (err) => { - t.ok( - err.message.startsWith(`Do not have access to '${fullPath}'`), - 'should error with EACCESS' - ) - resolve() - }) - }) + mockFsPromiseApi.access.rejects({ code: 'EACCESS' }) + t.equal(mockFsPromiseApi.mkdir.callCount, 0, 'should not have called mkdir') + t.rejects( + preBuild.makePath(fakePath), + new Error(`Do not have access to '${fullPath}'`), + 'should error with EACCESS' + ) }) - t.test('should return error if it fails to make nested path', (t) => { - const expectedErr = new Error('Failed to create dir') - sinon.stub(fs, 'mkdir') - fs.mkdir.yields(expectedErr) - preBuild.makePath(fakePath, (err) => { - t.same(err, expectedErr, 'should error when it cannot mkdir') - t.end() - }) + t.test('should throw if creating the nested folder path fails', async (t) => { + mockFsPromiseApi.access.rejects({ code: 'ENOENT' }) + const expectedError = new Error('whoops') + mockFsPromiseApi.mkdir.rejects(expectedError) + + t.rejects( + preBuild.makePath(fakePath), + expectedError, + 'should have rejected with expectedError' + ) + }) + + t.test('should not create the nested folder path if it exists and is accessible', async (t) => { + await preBuild.makePath(fakePath) + t.equal(mockFsPromiseApi.mkdir.callCount, 0, 'should not have called mkdir') }) }) @@ -97,65 +94,65 @@ tap.test('pre-build tests', (t) => { }) t.test('should run clean configure if rebuild is true', (t) => { - gypStub.execGyp.yields(null) - build('target', true, (err) => { - t.error(err) - t.ok(gypStub.execGyp.callCount, 2, 'should call execGyp twice') - t.same(gypStub.execGyp.args[0][0], ['clean', 'configure']) - t.equal(gypStub.execGyp.args[1][0][3], 'target') - t.end() - }) + gypStub.execGyp.returns(null) + build('target', true) + + t.ok(gypStub.execGyp.callCount, 2, 'should call execGyp twice') + t.same(gypStub.execGyp.args[0][0], ['clean', 'configure']) + t.equal(gypStub.execGyp.args[1][0][3], 'target') + t.end() }) t.test('should run configure and build if rebuild is false', (t) => { - gypStub.execGyp.yields(null) - build('target', false, (err) => { - t.error(err) - t.ok(gypStub.execGyp.callCount, 2, 'should call execGyp twice') - t.same(gypStub.execGyp.args[0][0], ['configure']) - t.end() - }) + gypStub.execGyp.returns(null) + build('target', false) + + t.ok(gypStub.execGyp.callCount, 2, 'should call execGyp twice') + t.same(gypStub.execGyp.args[0][0], ['configure']) + t.end() }) t.test('should return error if configure fails', (t) => { const expectedErr = new Error('failed to execute cmd') - gypStub.execGyp.yields(expectedErr) - build('target', false, (err) => { - t.same(err, expectedErr) - t.end() - }) + gypStub.execGyp.throws(expectedErr) + t.throws(() => build('target', false), expectedErr) + t.end() }) t.test('should return error if build fails', (t) => { const expectedErr = new Error('failed to execute cmd') - gypStub.execGyp.onCall(0).yields(null) - gypStub.execGyp.onCall(1).yields(expectedErr) - build('target', false, (err) => { - t.same(err, expectedErr) - t.end() - }) + gypStub.execGyp.onCall(0).returns(null) + gypStub.execGyp.onCall(1).throws(expectedErr) + t.throws(() => build('target', false), expectedErr) + t.end() }) }) - t.test('should move build accordingly', (t) => { - sinon.stub(fs, 'rename') - t.teardown(() => { - fs.rename.restore() - }) + t.test('moveBuild', (t) => { + t.autoend() - preBuild.moveBuild('target', (err) => { - t.error(err) - t.equal(fs.rename.callCount, 1) - t.end() + let mockFsPromiseApi + let preBuild + + t.beforeEach(() => { + mockFsPromiseApi = { + rename: sinon.stub().resolves() + } + preBuild = proxyquire('../../lib/pre-build', { + 'fs/promises': mockFsPromiseApi + }) }) - // call cb manually to end test - fs.rename.args[0][2]() + t.test('should move build accordingly', async (t) => { + await preBuild.moveBuild('target') + t.equal(mockFsPromiseApi.rename.callCount, 1) + }) }) t.test('download', (t) => { t.autoend() let sandbox + let preBuild t.before(() => { nock.disableNetConnect() @@ -163,6 +160,9 @@ tap.test('pre-build tests', (t) => { t.beforeEach(() => { sandbox = sinon.createSandbox() + preBuild = proxyquire('../../lib/pre-build', { + 'fs/promises': {} + }) }) t.afterEach(() => { @@ -171,73 +171,67 @@ tap.test('pre-build tests', (t) => { delete process.env.NR_NATIVE_METRICS_DOWNLOAD_HOST }) - t.test('should download and unzip file accordingly', (t) => { + t.test('should download and unzip file accordingly', async (t) => { const expectedData = Buffer.from('testing', 'utf-8') const file = zlib.gzipSync(expectedData) nock('https://download.newrelic.com/').get(/.*/).reply(200, file) - preBuild.download('test', (err, data) => { - t.error(err) - t.same(data, expectedData) - t.end() - }) + const data = await preBuild.download('test') + t.same(data, expectedData) }) - t.test('should return error if 404 occurs', (t) => { + t.test('should return error if 404 occurs', async (t) => { nock('https://download.newrelic.com/').get(/.*/).reply(404) - preBuild.download('test', (err) => { - t.equal(err.message, 'No pre-built artifacts for your OS/architecture.') - t.end() - }) + t.rejects( + preBuild.download('test'), + new Error('No pre-built artifacts for your OS/architecture.'), + 'should reject with expected error' + ) }) - t.test('should return failed to download error if response is not 200 nor 404', (t) => { + t.test('should return failed to download error if response is not 200 nor 404', async (t) => { nock('https://download.newrelic.com/').get(/.*/).reply(500) - preBuild.download('test', (err) => { - t.match(err.message, /Failed to download.*code 500/) - t.end() - }) + t.rejects( + preBuild.download('test'), + new Error('Failed to download'), + 'should reject with expected error' + ) }) - t.test('should fail if it cannot unzip', (t) => { + t.test('should fail if it cannot unzip', async (t) => { const expectedData = Buffer.from('testing', 'utf-8') nock('https://download.newrelic.com/').get(/.*/).reply(200, expectedData) - preBuild.download('test', (err) => { - t.match(err.message, /Failed to unzip.*/) - t.end() - }) + t.rejects( + preBuild.download('test'), + new Error('Failed to unzip'), + 'should reject with expected error' + ) }) - t.test('should use https proxy host', (t) => { + t.test('should use https proxy host', async (t) => { process.env.NR_NATIVE_METRICS_PROXY_HOST = 'https://proxy-stuff.com' const expectedData = Buffer.from('testing', 'utf-8') const file = zlib.gzipSync(expectedData) nock('https://download.newrelic.com/').get(/.*/).reply(200, file) - preBuild.download('test', (err, data) => { - t.error(err) - t.same(data, expectedData) - t.end() - }) + const data = await preBuild.download('test') + t.same(data, expectedData) }) - t.test('should use http proxy host', (t) => { + t.test('should use http proxy host', async (t) => { process.env.NR_NATIVE_METRICS_PROXY_HOST = 'http://proxy-stuff.com' const expectedData = Buffer.from('testing', 'utf-8') const file = zlib.gzipSync(expectedData) nock('http://download.newrelic.com/').get(/.*/).reply(200, file) - preBuild.download('test', (err, data) => { - t.error(err) - t.same(data, expectedData) - t.end() - }) + const data = await preBuild.download('test') + t.same(data, expectedData) }) - t.test('should use http download host', (t) => { + t.test('should use http download host', async (t) => { process.env.NR_NATIVE_METRICS_DOWNLOAD_HOST = 'http://fake-stuff.com/' // busting cache to re-load the env var so it uses a diff host delete require.cache[require.resolve('../../lib/pre-build')] @@ -246,52 +240,68 @@ tap.test('pre-build tests', (t) => { const file = zlib.gzipSync(expectedData) nock('http://fake-stuff.com/').get(/.*/).reply(200, file) - localPreBuild.download('test', (err, data) => { - t.error(err) - t.same(data, expectedData) - t.end() - }) + const data = await localPreBuild.download('test') + t.same(data, expectedData) }) }) t.test('saveDownload', (t) => { t.autoend() let sandbox + let preBuild + let mockFsPromiseApi t.beforeEach(() => { sandbox = sinon.createSandbox() + + mockFsPromiseApi = { + writeFile: sinon.stub().resolves() + } + + preBuild = proxyquire('../../lib/pre-build', { + 'fs/promises': mockFsPromiseApi + }) + sandbox.stub(preBuild, 'makePath') - sandbox.stub(fs, 'writeFile') }) t.afterEach(() => { sandbox.restore() }) - t.test('should write download to appropriate path', (t) => { - preBuild.makePath.yields() - preBuild.saveDownload('target', 'data', t.end) - t.equal(fs.writeFile.callCount, 1, 'should save download') - // call the callback manually to end test - fs.writeFile.args[0][2]() + t.test('should write download to appropriate path', async (t) => { + preBuild.makePath.resolves() + await preBuild.saveDownload('target', 'data') + t.equal(mockFsPromiseApi.writeFile.callCount, 1, 'should save download') }) - t.test('should return error if creating directory fails', (t) => { + t.test('should return error if creating directory fails', async (t) => { const expectedErr = new Error('failed to write') - preBuild.makePath.yields(expectedErr) - preBuild.saveDownload('target', 'data', (err) => { - t.same(err, expectedErr) - t.end() - }) + preBuild.makePath.rejects(expectedErr) + + t.rejects( + preBuild.saveDownload('target', 'data'), + expectedErr, + 'should reject with expected error' + ) }) }) t.test('install', (t) => { t.autoend() let sandbox + let mockFsPromiseApi + let preBuild t.beforeEach(() => { sandbox = sinon.createSandbox() + + mockFsPromiseApi = { + rename: sinon.stub().resolves() + } + preBuild = proxyquire('../../lib/pre-build', { + 'fs/promises': mockFsPromiseApi + }) sandbox.stub(preBuild, 'build') sandbox.stub(preBuild, 'download') sandbox.stub(preBuild, 'saveDownload') @@ -304,106 +314,82 @@ tap.test('pre-build tests', (t) => { delete process.env.NR_NATIVE_METRICS_NO_DOWNLOAD }) - t.test('should download without building when no-build is specified', (t) => { + t.test('should download without building when no-build is specified', async (t) => { const data = 'foo' process.env.NR_NATIVE_METRICS_NO_BUILD = true - preBuild.download.yields(null, data) - preBuild.saveDownload.yields(null) - preBuild.install('target', (err) => { - t.error(err) - t.equal(preBuild.build.callCount, 0, 'should not build') - t.equal(preBuild.download.callCount, 1, 'should download only') - t.equal(preBuild.saveDownload.callCount, 1, 'should download only') - t.equal(preBuild.saveDownload.args[0][0], 'target') - t.equal(preBuild.saveDownload.args[0][1], data) - t.end() - }) + preBuild.download.resolves(data) + preBuild.saveDownload.resolves() + await preBuild.install('target') + t.equal(preBuild.build.callCount, 0, 'should not build') + t.equal(preBuild.download.callCount, 1, 'should download only') + t.equal(preBuild.saveDownload.callCount, 1, 'should download only') + t.equal(preBuild.saveDownload.args[0][0], 'target') + t.equal(preBuild.saveDownload.args[0][1], data) }) - t.test('should build and move', (t) => { - preBuild.build.yields(null) - preBuild.moveBuild.yields(null) - preBuild.install('target', (err) => { - t.error(err) - t.equal(preBuild.build.callCount, 1, 'should build') - t.equal(preBuild.moveBuild.callCount, 1, 'should move build') - t.equal(preBuild.download.callCount, 0, 'should not download') - t.end() - }) + t.test('should build without downloading when no-download is specified', async (t) => { + process.env.NR_NATIVE_METRICS_NO_DOWNLOAD = true + preBuild.build.resolves(null) + preBuild.moveBuild.resolves(null) + await preBuild.install('target') + t.equal(preBuild.build.callCount, 1, 'should build') + t.equal(preBuild.moveBuild.callCount, 1, 'should move build') + t.equal(preBuild.download.callCount, 0, 'should not download') }) - t.test('should download if build fails', (t) => { - const err = new Error('build failed, downloading') - preBuild.build.yields(err) + t.test('should only download if both env vars are set', async (t) => { + process.env.NR_NATIVE_METRICS_NO_DOWNLOAD = true + process.env.NR_NATIVE_METRICS_NO_BUILD = true const data = 'foo' - preBuild.download.yields(null, data) - preBuild.saveDownload.yields(null) - preBuild.install('target', (err) => { - t.error(err) - t.equal(preBuild.build.callCount, 1, 'should build') - t.equal(preBuild.download.callCount, 1, 'should not download') - t.equal(preBuild.saveDownload.callCount, 1, 'should not download') - t.equal(preBuild.moveBuild.callCount, 0, 'should not move build') - t.end() - }) + preBuild.download.resolves(data) + preBuild.saveDownload.resolves() + preBuild.build.resolves(null) + preBuild.moveBuild.resolves(null) + + await preBuild.install('target') + + t.equal(preBuild.build.callCount, 0, 'should not build') + t.equal(preBuild.moveBuild.callCount, 0, 'should not move build') + t.equal(preBuild.download.callCount, 1, 'should download') + t.equal(preBuild.saveDownload.callCount, 1, 'should save download') }) - t.test('should download if moving build fails', (t) => { - const err = new Error('move failed, downloading') - preBuild.build.yields(null) - preBuild.moveBuild.yields(err) + t.test('should try download then build by default', async (t) => { const data = 'foo' - preBuild.download.yields(null, data) - preBuild.saveDownload.yields(null) - preBuild.install('target', (err) => { - t.error(err) - t.equal(preBuild.build.callCount, 1, 'should build') - t.equal(preBuild.moveBuild.callCount, 1, 'should move build') - t.equal(preBuild.download.callCount, 1, 'should not download') - t.equal(preBuild.saveDownload.callCount, 1, 'should not download') - t.end() - }) - }) + preBuild.download.resolves(data) + preBuild.saveDownload.rejects(new Error('whoops')) + preBuild.build.resolves(null) + preBuild.moveBuild.resolves(null) - t.test('should fail if move build fails and download are true', (t) => { - process.env.NR_NATIVE_METRICS_NO_DOWNLOAD = true - preBuild.build.yields(null) - const err = new Error('move failed, downloading') - preBuild.build.yields(null) - preBuild.moveBuild.yields(err) - preBuild.install('target', (err) => { - t.equal(err.message, 'Downloading is disabled.') - t.equal(preBuild.download.callCount, 0, 'should not download') - t.end() - }) + await preBuild.install('target') + + t.equal(preBuild.build.callCount, 1, 'should build') + t.equal(preBuild.moveBuild.callCount, 1, 'should move build') + t.equal(preBuild.download.callCount, 1, 'should download') + t.equal(preBuild.saveDownload.callCount, 1, 'should save download') }) - t.test('should fail if download fails', (t) => { + t.test('should throw when download fails and noBuild is set', async (t) => { process.env.NR_NATIVE_METRICS_NO_BUILD = true - const expectedErr = new Error('download failed') - preBuild.download.yields(expectedErr) - preBuild.install('target', (err) => { - t.same(err, expectedErr) - t.equal(preBuild.build.callCount, 0, 'should not build') - t.equal(preBuild.download.callCount, 1, 'should download only') - t.equal(preBuild.saveDownload.callCount, 0, 'should not save download') - t.end() - }) + const data = 'foo' + preBuild.download.resolves(data) + preBuild.saveDownload.rejects(new Error('whoops')) + preBuild.build.resolves(null) + preBuild.moveBuild.resolves(null) + + t.rejects(preBuild.install('target'), new Error('Building is disabled by configuration')) }) - t.test('should fail if save download fails', (t) => { + t.test('should fail if save download fails and building is disabled', async (t) => { process.env.NR_NATIVE_METRICS_NO_BUILD = true const data = 'foo' - preBuild.download.yields(null, data) - const expectedErr = new Error('saving download failed') - preBuild.saveDownload.yields(expectedErr) - preBuild.install('target', (err) => { - t.same(err, expectedErr) - t.equal(preBuild.build.callCount, 0, 'should not build') - t.equal(preBuild.download.callCount, 1, 'should download only') - t.equal(preBuild.saveDownload.callCount, 1, 'should not save download') - t.end() - }) + preBuild.download.resolves(data) + preBuild.saveDownload.throws(new Error('saving download failed')) + + t.rejects(preBuild.install('target'), new Error('Building is disabled by configuration')) + + t.equal(preBuild.build.callCount, 0, 'should not build') + t.equal(preBuild.download.callCount, 1, 'should download only') }) }) @@ -431,31 +417,36 @@ tap.test('pre-build tests', (t) => { sandbox.restore() }) ;['build', 'rebuild'].forEach((cmd) => { - t.test(`should build and move when cmd is '${cmd}'`, (t) => { - localPreBuild.build.yields(null) - localPreBuild.moveBuild.yields(null) - localPreBuild.executeCli(cmd, 'target') + t.test(`should build and move when cmd is '${cmd}'`, async (t) => { + localPreBuild.build.resolves() + localPreBuild.moveBuild.resolves() + await localPreBuild.executeCli(cmd, 'target') t.equal(localPreBuild.build.callCount, 1, 'should build') t.equal(localPreBuild.moveBuild.callCount, 1, 'should move build') t.equal(commonStub.logFinish.callCount, 1, 'should log finish') - t.end() }) }) - t.test('sho8uld call install if cmd is install', (t) => { - localPreBuild.executeCli('install', 'target') + t.test('should call install if cmd is install', async (t) => { + await localPreBuild.executeCli('install', 'target') t.equal(localPreBuild.install.callCount, 1, 'should call install') - t.end() }) - t.test('should log finish and not move build if build fails', (t) => { + t.test('should log finish if install fails', async (t) => { + const err = new Error('install failed') + localPreBuild.install.rejects(err) + await localPreBuild.executeCli('install', 'target') + t.equal(localPreBuild.install.callCount, 1, 'should call install') + t.equal(commonStub.logFinish.callCount, 1, 'should log finish') + }) + + t.test('should log finish and not move build if build fails', async (t) => { const err = new Error('build failed') - localPreBuild.build.yields(err) + localPreBuild.build.throws(err) localPreBuild.executeCli('build', 'target') t.equal(localPreBuild.build.callCount, 1, 'should build') t.equal(localPreBuild.moveBuild.callCount, 0, 'should not move build') t.equal(commonStub.logFinish.callCount, 1, 'should log finish') - t.end() }) }) })