From 574998fe3a6b6c4dbd8880144a3f718340c109f3 Mon Sep 17 00:00:00 2001 From: Honza Javorek Date: Mon, 14 Jan 2019 16:17:33 +0100 Subject: [PATCH 01/13] refactor: split reporter output from Dredd's internal logging A part of the effort to separate application logging from the reporters output. Enables #1089 and subsequently #765, supersedes #1099 BREAKING CHANGE: The --level option is no longer able to affect reporter output, it only affects application logging output. --- lib/addHooks.js | 3 +- lib/configuration.js | 14 ++++- lib/logger.js | 26 ++------- lib/reporters/ApiaryReporter.js | 3 +- lib/reporters/CLIReporter.js | 80 +++++++++++++-------------- lib/reporters/DotReporter.js | 15 ++--- lib/reporters/HTMLReporter.js | 7 ++- lib/reporters/MarkdownReporter.js | 7 ++- lib/reporters/NyanReporter.js | 15 ++--- lib/reporters/XUnitReporter.js | 9 +-- lib/reporters/reporterOutputLogger.js | 31 +++++++++++ 11 files changed, 121 insertions(+), 89 deletions(-) create mode 100644 lib/reporters/reporterOutputLogger.js diff --git a/lib/addHooks.js b/lib/addHooks.js index 1e3eec4e1..b5a5ac397 100644 --- a/lib/addHooks.js +++ b/lib/addHooks.js @@ -4,6 +4,7 @@ const proxyquire = require('proxyquire').noCallThru(); const Hooks = require('./Hooks'); const HooksWorkerClient = require('./HooksWorkerClient'); const logger = require('./logger'); +const reporterOutputLogger = require('./reporters/reporterOutputLogger'); const resolveHookfiles = require('./resolveHookfiles'); // Note: runner.configuration.options must be defined @@ -24,7 +25,7 @@ Stack: ${error.stack} } if (!runner.logs) { runner.logs = []; } - runner.hooks = new Hooks({ logs: runner.logs, logger }); + runner.hooks = new Hooks({ logs: runner.logs, logger: reporterOutputLogger }); if (!runner.hooks.transactions) { runner.hooks.transactions = {}; } diff --git a/lib/configuration.js b/lib/configuration.js index ef445fc2e..a839265b9 100644 --- a/lib/configuration.js +++ b/lib/configuration.js @@ -2,6 +2,8 @@ const clone = require('clone'); const { EventEmitter } = require('events'); const logger = require('./logger'); +const reporterOutputLogger = require('./reporters/reporterOutputLogger'); + function coerceToArray(value) { if (Array.isArray(value)) { @@ -14,6 +16,7 @@ function coerceToArray(value) { return value; } + function applyLoggingOptions(options) { // Color can be either specified as "stringified bool" or bool (nothing else // is expected valid value). Here we're coercing the value to boolean. @@ -24,13 +27,21 @@ function applyLoggingOptions(options) { } logger.transports.console.colorize = options.color; - logger.transports.console.silent = options.silent; + reporterOutputLogger.transports.console.colorize = options.color; + logger.transports.console.timestamp = options.timestamp; + reporterOutputLogger.transports.console.timestamp = options.timestamp; + + logger.transports.console.silent = options.silent; + reporterOutputLogger.transports.console.silent = options.silent; + logger.transports.console.level = options.level; + reporterOutputLogger.transports.console.level = 'test'; return options; } + function applyConfiguration(config) { const configuration = { server: null, @@ -117,6 +128,7 @@ function applyConfiguration(config) { return configuration; } + module.exports = { applyConfiguration, applyLoggingOptions, diff --git a/lib/logger.js b/lib/logger.js index 42d778a18..9417f1f17 100644 --- a/lib/logger.js +++ b/lib/logger.js @@ -5,19 +5,10 @@ module.exports = new (winston.Logger)({ new (winston.transports.Console)({ colorize: true }), ], levels: { - silly: 14, - debug: 13, - verbose: 12, - info: 11, - test: 10, - pass: 9, - fail: 8, - complete: 7, - actual: 6, - expected: 5, - hook: 4, - request: 3, - skip: 2, + silly: 5, + debug: 4, + verbose: 3, + info: 2, warn: 1, error: 0, }, @@ -26,15 +17,6 @@ module.exports = new (winston.Logger)({ debug: 'cyan', verbose: 'magenta', info: 'blue', - test: 'yellow', - pass: 'green', - fail: 'red', - complete: 'green', - actual: 'red', - expected: 'red', - hook: 'green', - request: 'green', - skip: 'yellow', warn: 'yellow', error: 'red', }, diff --git a/lib/reporters/ApiaryReporter.js b/lib/reporters/ApiaryReporter.js index e00bd772d..6d7c2e4c5 100644 --- a/lib/reporters/ApiaryReporter.js +++ b/lib/reporters/ApiaryReporter.js @@ -4,6 +4,7 @@ const os = require('os'); const request = require('request'); const logger = require('../logger'); +const reporterOutputLogger = require('./reporterOutputLogger'); const packageData = require('../../package.json'); @@ -186,7 +187,7 @@ ApiaryReporter.prototype.configureEmitter = function configureEmitter(emitter) { this._performRequestAsync(path, 'PATCH', data, (error) => { if (error) { return callback(error); } const reportUrl = this.reportUrl || `https://app.apiary.io/${this.configuration.apiSuite}/tests/run/${this.remoteId}`; - logger.complete(`See results in Apiary at: ${reportUrl}`); + reporterOutputLogger.complete(`See results in Apiary at: ${reportUrl}`); callback(); }); }); diff --git a/lib/reporters/CLIReporter.js b/lib/reporters/CLIReporter.js index 7339b2a16..517a90a61 100644 --- a/lib/reporters/CLIReporter.js +++ b/lib/reporters/CLIReporter.js @@ -1,6 +1,19 @@ const logger = require('../logger'); +const reporterOutputLogger = require('./reporterOutputLogger'); const prettifyResponse = require('../prettifyResponse'); + +const CONNECTION_ERRORS = [ + 'ECONNRESET', + 'ENOTFOUND', + 'ESOCKETTIMEDOUT', + 'ETIMEDOUT', + 'ECONNREFUSED', + 'EHOSTUNREACH', + 'EPIPE', +]; + + function CLIReporter(emitter, stats, tests, inlineErrors, details) { this.type = 'cli'; this.stats = stats; @@ -22,76 +35,63 @@ CLIReporter.prototype.configureEmitter = function configureEmitter(emitter) { emitter.on('end', (callback) => { if (!this.inlineErrors) { - if (this.errors.length !== 0) { logger.info('Displaying failed tests...'); } - for (const test of this.errors) { - logger.fail(`${test.title} duration: ${test.duration}ms`); - logger.fail(test.message); - if (test.request) { logger.request(`\n${prettifyResponse(test.request)}\n`); } - if (test.expected) { logger.expected(`\n${prettifyResponse(test.expected)}\n`); } - if (test.actual) { logger.actual(`\n${prettifyResponse(test.actual)}\n\n`); } + if (this.errors.length) { + logger.info('Displaying failed tests...'); } + this.errors.forEach((test) => { + reporterOutputLogger.fail(`${test.title} duration: ${test.duration}ms`); + reporterOutputLogger.fail(test.message); + if (test.request) reporterOutputLogger.request(`\n${prettifyResponse(test.request)}\n`); + if (test.expected) reporterOutputLogger.expected(`\n${prettifyResponse(test.expected)}\n`); + if (test.actual) reporterOutputLogger.actual(`\n${prettifyResponse(test.actual)}\n\n`); + }); } if (this.stats.tests > 0) { - logger.complete(`${this.stats.passes} passing, ` + reporterOutputLogger.complete(`${this.stats.passes} passing, ` + `${this.stats.failures} failing, ` + `${this.stats.errors} errors, ` + `${this.stats.skipped} skipped, ` + `${this.stats.tests} total`); } - logger.complete(`Tests took ${this.stats.duration}ms`); + reporterOutputLogger.complete(`Tests took ${this.stats.duration}ms`); callback(); }); emitter.on('test pass', (test) => { - logger.pass(`${test.title} duration: ${test.duration}ms`); + reporterOutputLogger.pass(`${test.title} duration: ${test.duration}ms`); if (this.details) { - logger.request(`\n${prettifyResponse(test.request)}\n`); - logger.expected(`\n${prettifyResponse(test.expected)}\n`); - logger.actual(`\n${prettifyResponse(test.actual)}\n\n`); + reporterOutputLogger.request(`\n${prettifyResponse(test.request)}\n`); + reporterOutputLogger.expected(`\n${prettifyResponse(test.expected)}\n`); + reporterOutputLogger.actual(`\n${prettifyResponse(test.actual)}\n\n`); } }); - emitter.on('test skip', test => logger.skip(test.title)); + emitter.on('test skip', test => reporterOutputLogger.skip(test.title)); emitter.on('test fail', (test) => { - logger.fail(`${test.title} duration: ${test.duration}ms`); + reporterOutputLogger.fail(`${test.title} duration: ${test.duration}ms`); if (this.inlineErrors) { - logger.fail(test.message); - if (test.request) { logger.request(`\n${prettifyResponse(test.request)}\n`); } - if (test.expected) { logger.expected(`\n${prettifyResponse(test.expected)}\n`); } - if (test.actual) { logger.actual(`\n${prettifyResponse(test.actual)}\n\n`); } + reporterOutputLogger.fail(test.message); + if (test.request) { reporterOutputLogger.request(`\n${prettifyResponse(test.request)}\n`); } + if (test.expected) { reporterOutputLogger.expected(`\n${prettifyResponse(test.expected)}\n`); } + if (test.actual) { reporterOutputLogger.actual(`\n${prettifyResponse(test.actual)}\n\n`); } } else { this.errors.push(test); } }); emitter.on('test error', (error, test) => { - const connectionErrors = [ - 'ECONNRESET', - 'ENOTFOUND', - 'ESOCKETTIMEDOUT', - 'ETIMEDOUT', - 'ECONNREFUSED', - 'EHOSTUNREACH', - 'EPIPE', - ]; - - if (connectionErrors.indexOf(error.code) > -1) { + if (CONNECTION_ERRORS.includes(error.code)) { test.message = 'Error connecting to server under test!'; + reporterOutputLogger.error(test.message); + } else { + reporterOutputLogger.error(error.stack); } - if (!this.inlineErrors) { - this.errors.push(test); - } - - logger.error(`${test.title} duration: ${test.duration}ms`); - - if (connectionErrors.indexOf(error.code) > -1) { - return logger.error(test.message); - } - logger.error(error.stack); + reporterOutputLogger.error(`${test.title} duration: ${test.duration}ms`); + if (!this.inlineErrors) { this.errors.push(test); } }); }; diff --git a/lib/reporters/DotReporter.js b/lib/reporters/DotReporter.js index b9bbf1ee4..9d00289ff 100644 --- a/lib/reporters/DotReporter.js +++ b/lib/reporters/DotReporter.js @@ -1,4 +1,5 @@ const logger = require('../logger'); +const reporterOutputLogger = require('./reporterOutputLogger'); const prettifyResponse = require('../prettifyResponse'); function DotReporter(emitter, stats, tests) { @@ -24,20 +25,20 @@ DotReporter.prototype.configureEmitter = function configureEmitter(emitter) { this.write('\n'); logger.info('Displaying failed tests...'); for (const test of this.errors) { - logger.fail(`${test.title} duration: ${test.duration}ms`); - logger.fail(test.message); - logger.request(`\n${prettifyResponse(test.request)}\n`); - logger.expected(`\n${prettifyResponse(test.expected)}\n`); - logger.actual(`\n${prettifyResponse(test.actual)}\n\n`); + reporterOutputLogger.fail(`${test.title} duration: ${test.duration}ms`); + reporterOutputLogger.fail(test.message); + reporterOutputLogger.request(`\n${prettifyResponse(test.request)}\n`); + reporterOutputLogger.expected(`\n${prettifyResponse(test.expected)}\n`); + reporterOutputLogger.actual(`\n${prettifyResponse(test.actual)}\n\n`); } } this.write('\n'); - logger.complete(`\ + reporterOutputLogger.complete(`\ ${this.stats.passes} passing, ${this.stats.failures} failing, \ ${this.stats.errors} errors, ${this.stats.skipped} skipped\ `); - logger.complete(`Tests took ${this.stats.duration}ms`); + reporterOutputLogger.complete(`Tests took ${this.stats.duration}ms`); callback(); } diff --git a/lib/reporters/HTMLReporter.js b/lib/reporters/HTMLReporter.js index 07427aded..9f80bd7ac 100644 --- a/lib/reporters/HTMLReporter.js +++ b/lib/reporters/HTMLReporter.js @@ -8,6 +8,7 @@ const md = require('markdown-it')(); const pathmodule = require('path'); const logger = require('../logger'); +const reporterOutputLogger = require('./reporterOutputLogger'); const prettifyResponse = require('../prettifyResponse'); function HTMLReporter(emitter, stats, tests, path, details) { @@ -29,7 +30,7 @@ function HTMLReporter(emitter, stats, tests, path, details) { HTMLReporter.prototype.sanitizedPath = function sanitizedPath(path = './report.html') { const filePath = pathmodule.resolve(untildify(path)); if (fs.existsSync(filePath)) { - logger.info(`File exists at ${filePath}, will be overwritten...`); + logger.warn(`File exists at ${filePath}, will be overwritten...`); } return filePath; }; @@ -48,12 +49,12 @@ HTMLReporter.prototype.configureEmitter = function configureEmitter(emitter) { makeDir(pathmodule.dirname(this.path)) .then(() => { fs.writeFile(this.path, html, (error) => { - if (error) { logger.error(error); } + if (error) { reporterOutputLogger.error(error); } callback(); }); }) .catch((err) => { - logger.error(err); + reporterOutputLogger.error(err); callback(); }); }); diff --git a/lib/reporters/MarkdownReporter.js b/lib/reporters/MarkdownReporter.js index 2c77c2b66..d7bab1514 100644 --- a/lib/reporters/MarkdownReporter.js +++ b/lib/reporters/MarkdownReporter.js @@ -7,6 +7,7 @@ const makeDir = require('make-dir'); const pathmodule = require('path'); const logger = require('../logger'); +const reporterOutputLogger = require('./reporterOutputLogger'); const prettifyResponse = require('../prettifyResponse'); function MarkdownReporter(emitter, stats, tests, path, details) { @@ -28,7 +29,7 @@ function MarkdownReporter(emitter, stats, tests, path, details) { MarkdownReporter.prototype.sanitizedPath = function sanitizedPath(path = './report.md') { const filePath = pathmodule.resolve(untildify(path)); if (fs.existsSync(filePath)) { - logger.info(`File exists at ${filePath}, will be overwritten...`); + logger.warn(`File exists at ${filePath}, will be overwritten...`); } return filePath; }; @@ -46,12 +47,12 @@ MarkdownReporter.prototype.configureEmitter = function configureEmitter(emitter) makeDir(pathmodule.dirname(this.path)) .then(() => { fs.writeFile(this.path, this.buf, (error) => { - if (error) { logger.error(error); } + if (error) { reporterOutputLogger.error(error); } callback(); }); }) .catch((err) => { - logger.error(err); + reporterOutputLogger.error(err); callback(); }); }); diff --git a/lib/reporters/NyanReporter.js b/lib/reporters/NyanReporter.js index f80eb8e58..5f0f099f6 100644 --- a/lib/reporters/NyanReporter.js +++ b/lib/reporters/NyanReporter.js @@ -2,6 +2,7 @@ const tty = require('tty'); const logger = require('../logger'); const prettifyResponse = require('../prettifyResponse'); +const reporterOutputLogger = require('./reporterOutputLogger'); function NyanCatReporter(emitter, stats, tests) { let windowWidth; @@ -56,16 +57,16 @@ NyanCatReporter.prototype.configureEmitter = function configureEmitter(emitter) this.write('\n'); logger.info('Displaying failed tests...'); for (const test of this.errors) { - logger.fail(`${test.title} duration: ${test.duration}ms`); - logger.fail(test.message); - logger.request(`\n${prettifyResponse(test.request)}\n`); - logger.expected(`\n${prettifyResponse(test.expected)}\n`); - logger.actual(`\n${prettifyResponse(test.actual)}\n\n`); + reporterOutputLogger.fail(`${test.title} duration: ${test.duration}ms`); + reporterOutputLogger.fail(test.message); + reporterOutputLogger.request(`\n${prettifyResponse(test.request)}\n`); + reporterOutputLogger.expected(`\n${prettifyResponse(test.expected)}\n`); + reporterOutputLogger.actual(`\n${prettifyResponse(test.actual)}\n\n`); } } - logger.complete(`${this.stats.passes} passing, ${this.stats.failures} failing, ${this.stats.errors} errors, ${this.stats.skipped} skipped`); - logger.complete(`Tests took ${this.stats.duration}ms`); + reporterOutputLogger.complete(`${this.stats.passes} passing, ${this.stats.failures} failing, ${this.stats.errors} errors, ${this.stats.skipped} skipped`); + reporterOutputLogger.complete(`Tests took ${this.stats.duration}ms`); callback(); }); diff --git a/lib/reporters/XUnitReporter.js b/lib/reporters/XUnitReporter.js index d31b6411a..b5db268fe 100644 --- a/lib/reporters/XUnitReporter.js +++ b/lib/reporters/XUnitReporter.js @@ -8,6 +8,7 @@ const makeDir = require('make-dir'); const pathmodule = require('path'); const logger = require('../logger'); +const reporterOutputLogger = require('./reporterOutputLogger'); const prettifyResponse = require('../prettifyResponse'); function XUnitReporter(emitter, stats, tests, path, details) { @@ -42,14 +43,14 @@ XUnitReporter.prototype.updateSuiteStats = function updateSuiteStats(path, stats }, false); const xmlHeader = ''; fs.writeFile(path, `${xmlHeader}\n${newStats}\n${restOfFile}`, (error) => { - if (error) { logger.error(error); } + if (error) { reporterOutputLogger.error(error); } callback(); }); } else { callback(); } } else { - logger.error(err); + reporterOutputLogger.error(err); callback(); } }); @@ -77,7 +78,7 @@ XUnitReporter.prototype.toTag = function toTag(name, attrs, close, content) { XUnitReporter.prototype.sanitizedPath = function sanitizedPath(path = './report.xml') { const filePath = pathmodule.resolve(untildify(path)); if (fs.existsSync(filePath)) { - logger.info(`File exists at ${filePath}, will be overwritten...`); + logger.warn(`File exists at ${filePath}, will be overwritten...`); fs.unlinkSync(filePath); } return filePath; @@ -99,7 +100,7 @@ XUnitReporter.prototype.configureEmitter = function configureEmitter(emitter) { callback(); }) .catch((err) => { - logger.error(err); + reporterOutputLogger.error(err); callback(); }); }); diff --git a/lib/reporters/reporterOutputLogger.js b/lib/reporters/reporterOutputLogger.js new file mode 100644 index 000000000..5722bb50f --- /dev/null +++ b/lib/reporters/reporterOutputLogger.js @@ -0,0 +1,31 @@ +const winston = require('winston'); + +module.exports = new (winston.Logger)({ + transports: [ + new (winston.transports.Console)({ colorize: true }), + ], + levels: { + test: 9, + pass: 8, + fail: 7, + complete: 6, + actual: 5, + expected: 4, + hook: 3, + request: 2, + skip: 1, + error: 0, + }, + colors: { + test: 'yellow', + pass: 'green', + fail: 'red', + complete: 'green', + actual: 'red', + expected: 'red', + hook: 'green', + request: 'green', + skip: 'yellow', + error: 'red', + }, +}); From 927046ce0d36c1dedfd6df2a7b8acaeb745941df Mon Sep 17 00:00:00 2001 From: Honza Javorek Date: Wed, 16 Jan 2019 19:02:37 +0100 Subject: [PATCH 02/13] test: update tests to reflect the logging split --- .../integration/cli/configuration-cli-test.js | 24 ++-- test/integration/cli/hookfiles-cli-test.js | 8 +- test/integration/dredd-test.js | 105 +++++++----------- test/integration/helpers.js | 21 +++- test/unit/CLI-test.js | 25 ++--- test/unit/hooksLog-test.js | 37 +++--- test/unit/reporters/ApiaryReporter-test.js | 22 ++-- test/unit/reporters/CLIReporter-test.js | 64 ++++++----- test/unit/reporters/DotReporter-test.js | 13 ++- test/unit/reporters/HTMLReporter-test.js | 24 ++-- test/unit/reporters/MarkdownReporter-test.js | 24 ++-- test/unit/reporters/NyanReporter-test.js | 24 ++-- test/unit/reporters/XUnitReporter-test.js | 26 +++-- 13 files changed, 209 insertions(+), 208 deletions(-) diff --git a/test/integration/cli/configuration-cli-test.js b/test/integration/cli/configuration-cli-test.js index 9c21d69f3..760028895 100644 --- a/test/integration/cli/configuration-cli-test.js +++ b/test/integration/cli/configuration-cli-test.js @@ -54,25 +54,21 @@ function execCommand(custom = {}, cb) { describe('CLI class Integration', () => { before(() => { ['warn', 'error'].forEach((method) => { - sinon.stub(loggerStub, method).callsFake((chunk) => { stderr += `\n${method}: ${chunk}`; }); + sinon + .stub(loggerStub, method) + .callsFake((chunk) => { stderr += `\n${method}: ${chunk}`; }); }); - [ - 'log', 'info', 'silly', 'verbose', 'test', - 'hook', 'complete', 'pass', 'skip', 'debug', - 'fail', 'request', 'expected', 'actual', - ].forEach((method) => { - sinon.stub(loggerStub, method).callsFake((chunk) => { stdout += `\n${method}: ${chunk}`; }); + ['log', 'info', 'silly', 'verbose', 'debug'].forEach((method) => { + sinon + .stub(loggerStub, method) + .callsFake((chunk) => { stdout += `\n${method}: ${chunk}`; }); }); }); after(() => { - ['warn', 'error'].forEach((method) => { loggerStub[method].restore(); }); - - [ - 'log', 'info', 'silly', 'verbose', 'test', - 'hook', 'complete', 'pass', 'skip', 'debug', - 'fail', 'request', 'expected', 'actual', - ].forEach((method) => { loggerStub[method].restore(); }); + ['warn', 'error', 'log', 'info', 'silly', 'verbose', 'debug'].forEach((method) => { + loggerStub[method].restore(); + }); }); describe('When using configuration file', () => { diff --git a/test/integration/cli/hookfiles-cli-test.js b/test/integration/cli/hookfiles-cli-test.js index 0ad4ace5c..ff6492570 100644 --- a/test/integration/cli/hookfiles-cli-test.js +++ b/test/integration/cli/hookfiles-cli-test.js @@ -533,7 +533,7 @@ describe('CLI', () => { }); }); - describe('when setting the log output level with -l', () => { + describe('when setting the log output level with --level', () => { let runtimeInfo; before((done) => { @@ -543,7 +543,8 @@ describe('CLI', () => { const args = [ './test/fixtures/single-get.apib', `http://127.0.0.1:${DEFAULT_SERVER_PORT}`, - '-l=error', + '--level=error', + '--color=false', ]; runCLIWithServer(args, app, (err, info) => { runtimeInfo = info; @@ -552,8 +553,7 @@ describe('CLI', () => { }); it('should not display anything', () => { - // At the "error" level, complete should not be shown - assert.isOk(runtimeInfo.dredd.stdout.indexOf('complete') === -1); + assert.notInclude(runtimeInfo.dredd.output, 'info:'); }); }); diff --git a/test/integration/dredd-test.js b/test/integration/dredd-test.js index 9f0a75522..0f3c0c1e7 100644 --- a/test/integration/dredd-test.js +++ b/test/integration/dredd-test.js @@ -2,36 +2,20 @@ const bodyParser = require('body-parser'); const clone = require('clone'); const express = require('express'); const fs = require('fs'); -const proxyquire = require('proxyquire').noCallThru(); -const sinon = require('sinon'); const { assert } = require('chai'); -const loggerStub = require('../../lib/logger'); +const logger = require('../../lib/logger'); +const reporterOutputLogger = require('../../lib/reporters/reporterOutputLogger'); +const Dredd = require('../../lib/Dredd'); const PORT = 9876; let exitStatus; -let stderr = ''; -let stdout = ''; - -const addHooksStub = proxyquire('../../lib/addHooks', { - './logger': loggerStub, -}); - -const transactionRunner = proxyquire('../../lib/TransactionRunner', { - './addHooks': addHooksStub, - './logger': loggerStub, -}); - -const Dredd = proxyquire('../../lib/Dredd', { - './TransactionRunner': transactionRunner, - './logger': loggerStub, -}); +let output = ''; function execCommand(options = {}, cb) { - stdout = ''; - stderr = ''; + output = ''; exitStatus = null; let finished = false; if (!options.server) { options.server = `http://127.0.0.1:${PORT}`; } @@ -40,41 +24,36 @@ function execCommand(options = {}, cb) { if (!finished) { finished = true; if (error ? error.message : undefined) { - stderr += error.message; + output += error.message; } exitStatus = (error || (((1 * stats.failures) + (1 * stats.errors)) > 0)) ? 1 : 0; - cb(null, stdout, stderr, exitStatus); + cb(); } }); } + +function record(transport, level, message) { + output += `\n${level}: ${message}`; +} + + describe('Dredd class Integration', () => { before(() => { - ['warn', 'error'].forEach((method) => { - sinon.stub(loggerStub, method).callsFake((chunk) => { stderr += `\n${method}: ${chunk}`; }); - }); - [ - 'log', 'info', 'silly', 'verbose', 'test', - 'hook', 'complete', 'pass', 'skip', 'debug', - 'fail', 'request', 'expected', 'actual', - ].forEach((method) => { - sinon.stub(loggerStub, method).callsFake((chunk) => { stdout += `\n${method}: ${chunk}`; }); - }); + logger.transports.console.silent = true; + logger.on('logging', record); + + reporterOutputLogger.transports.console.silent = true; + reporterOutputLogger.on('logging', record); }); after(() => { - ['warn', 'error'].forEach((method) => { - loggerStub[method].restore(); - }); - [ - 'log', 'info', 'silly', 'verbose', 'test', - 'hook', 'complete', 'pass', 'skip', 'debug', - 'fail', 'request', 'expected', 'actual', - ].forEach((method) => { - loggerStub[method].restore(); - }); - }); + logger.transports.console.silent = false; + logger.removeListener('logging', record); + reporterOutputLogger.transports.console.silent = false; + reporterOutputLogger.removeListener('logging', record); + }); describe('when creating Dredd instance with existing API description document and responding server', () => { describe('when the server is responding as specified in the API description', () => { @@ -181,7 +160,7 @@ describe('Dredd class Integration', () => { }); }); - it('should not print warning about missing Apiary API settings', () => assert.notInclude(stderr, 'Apiary API Key or API Project Subdomain were not provided.')); + it('should not print warning about missing Apiary API settings', () => assert.notInclude(output, 'Apiary API Key or API Project Subdomain were not provided.')); it('should contain Authentication header thanks to apiaryApiKey and apiaryApiName configuration', () => { assert.propertyVal(receivedHeaders, 'authentication', 'Token the-key'); @@ -193,7 +172,7 @@ describe('Dredd class Integration', () => { assert.propertyVal(receivedRequestTestRuns, 'public', false); }); - it('should print using the new reporter', () => assert.include(stdout, 'http://url.me/test/run/1234_id')); + it('should print using the new reporter', () => assert.include(output, 'http://url.me/test/run/1234_id')); it('should send results from Gavel', () => { assert.isObject(receivedRequest); @@ -276,7 +255,7 @@ describe('Dredd class Integration', () => { server2.on('close', done); }); - it('should print using the reporter', () => assert.include(stdout, 'http://url.me/test/run/1234_id')); + it('should print using the reporter', () => assert.include(output, 'http://url.me/test/run/1234_id')); it('should send results from gavel', () => { assert.isObject(receivedRequest); @@ -338,11 +317,11 @@ describe('Dredd class Integration', () => { server.on('close', done); }); - it('should print warning about missing Apiary API settings', () => assert.include(stderr, 'Apiary API Key or API Project Subdomain were not provided.')); + it('should print warning about missing Apiary API settings', () => assert.include(output, 'Apiary API Key or API Project Subdomain were not provided.')); - it('should print link to documentation', () => assert.include(stderr, 'https://dredd.org/en/latest/how-to-guides/#using-apiary-reporter-and-apiary-tests')); + it('should print link to documentation', () => assert.include(output, 'https://dredd.org/en/latest/how-to-guides/#using-apiary-reporter-and-apiary-tests')); - it('should print using the new reporter', () => assert.include(stdout, 'http://url.me/test/run/1234_id')); + it('should print using the new reporter', () => assert.include(output, 'http://url.me/test/run/1234_id')); it('should send results from Gavel', () => { assert.isObject(receivedRequest); @@ -426,10 +405,10 @@ describe('Dredd class Integration', () => { it('should exit with status 1', () => assert.equal(exitStatus, 1)); - it('should print error message to stderr', () => { - assert.include(stderr, 'Error when loading file from URL'); - assert.include(stderr, 'Is the provided URL correct?'); - assert.include(stderr, 'connection-error.apib'); + it('should print error message to the output', () => { + assert.include(output, 'Error when loading file from URL'); + assert.include(output, 'Is the provided URL correct?'); + assert.include(output, 'connection-error.apib'); }); }); @@ -444,10 +423,10 @@ describe('Dredd class Integration', () => { it('should exit with status 1', () => assert.equal(exitStatus, 1)); - it('should print error message to stderr', () => { - assert.include(stderr, 'Unable to load file from URL'); - assert.include(stderr, 'responded with status code 404'); - assert.include(stderr, 'not-found.apib'); + it('should print error message to the output', () => { + assert.include(output, 'Unable to load file from URL'); + assert.include(output, 'responded with status code 404'); + assert.include(output, 'not-found.apib'); }); }); @@ -463,7 +442,7 @@ describe('Dredd class Integration', () => { }); describe('when OpenAPI 2 document has multiple responses', () => { - const reTransaction = /(\w+): (\w+) \((\d+)\) \/honey/g; + const reTransaction = /(skip|fail): (\w+) \((\d+)\) \/honey/g; let actual; before(done => execCommand({ @@ -475,7 +454,7 @@ describe('Dredd class Integration', () => { let groups; const matches = []; // eslint-disable-next-line - while (groups = reTransaction.exec(stdout)) { matches.push(groups); } + while (groups = reTransaction.exec(output)) { matches.push(groups); } actual = matches.map((match) => { const keyMap = { 0: 'name', 1: 'action', 2: 'method', 3: 'statusCode', @@ -499,7 +478,7 @@ describe('Dredd class Integration', () => { }); describe('when OpenAPI 2 document has multiple responses and hooks unskip some of them', () => { - const reTransaction = /(\w+): (\w+) \((\d+)\) \/honey/g; + const reTransaction = /(skip|fail): (\w+) \((\d+)\) \/honey/g; let actual; before(done => execCommand({ @@ -512,7 +491,7 @@ describe('Dredd class Integration', () => { let groups; const matches = []; // eslint-disable-next-line - while (groups = reTransaction.exec(stdout)) { matches.push(groups); } + while (groups = reTransaction.exec(output)) { matches.push(groups); } actual = matches.map((match) => { const keyMap = { 0: 'name', 1: 'action', 2: 'method', 3: 'statusCode', @@ -552,7 +531,7 @@ describe('Dredd class Integration', () => { let groups; matches = []; // eslint-disable-next-line - while (groups = reTransactionName.exec(stdout)) { matches.push(groups[1]); } + while (groups = reTransactionName.exec(output)) { matches.push(groups[1]); } done(err); })); diff --git a/test/integration/helpers.js b/test/integration/helpers.js index a3f692468..11f1cd858 100644 --- a/test/integration/helpers.js +++ b/test/integration/helpers.js @@ -9,6 +9,7 @@ const ps = require('ps-node'); const spawn = require('cross-spawn'); const logger = require('../../lib/logger'); +const reporterOutputLogger = require('../../lib/reporters/reporterOutputLogger'); const DEFAULT_SERVER_PORT = 9876; const DREDD_BIN = require.resolve('../../bin/dredd'); @@ -22,16 +23,28 @@ const DREDD_BIN = require.resolve('../../bin/dredd'); // from the 'fn' function // - logging (string) - the recorded logging output function recordLogging(fn, callback) { - const silent = !!logger.transports.console.silent; - logger.transports.console.silent = true; // Supress Dredd's console output (remove if debugging) + const loggerSilent = !!logger.transports.console.silent; + const reporterOutputLoggerSilent = !!reporterOutputLogger.transports.console.silent; + + // Supress Dredd's console output (remove if debugging) + logger.transports.console.silent = true; + reporterOutputLogger.transports.console.silent = true; let logging = ''; - const record = (transport, level, message) => { logging += `${level}: ${message}\n`; }; + const record = (transport, level, message) => { + logging += `${level}: ${message}\n`; + }; logger.on('logging', record); + reporterOutputLogger.on('logging', record); + fn((...args) => { logger.removeListener('logging', record); - logger.transports.console.silent = silent; + logger.transports.console.silent = loggerSilent; + + reporterOutputLogger.removeListener('logging', record); + reporterOutputLogger.transports.console.silent = reporterOutputLoggerSilent; + callback(null, args, logging); }); } diff --git a/test/unit/CLI-test.js b/test/unit/CLI-test.js index 486ef7e79..1661c1eac 100644 --- a/test/unit/CLI-test.js +++ b/test/unit/CLI-test.js @@ -64,28 +64,19 @@ function execCommand(custom = {}, cb) { describe('CLI class', () => { before(() => { ['warn', 'error'].forEach((method) => { - sinon.stub(loggerStub, method).callsFake((chunk) => { stderr += `\n${method}: ${chunk}`; }); + sinon + .stub(loggerStub, method) + .callsFake((chunk) => { stderr += `\n${method}: ${chunk}`; }); }); - - [ - 'log', 'info', 'silly', 'verbose', 'test', - 'hook', 'complete', 'pass', 'skip', 'debug', - 'fail', 'request', 'expected', 'actual', - ].forEach((method) => { - sinon.stub(loggerStub, method).callsFake((chunk) => { stdout += `\n${method}: ${chunk}`; }); + ['log', 'info', 'silly', 'verbose', 'debug'].forEach((method) => { + sinon + .stub(loggerStub, method) + .callsFake((chunk) => { stdout += `\n${method}: ${chunk}`; }); }); }); after(() => { - ['warn', 'error'].forEach((method) => { - loggerStub[method].restore(); - }); - - [ - 'log', 'info', 'silly', 'verbose', 'test', - 'hook', 'complete', 'pass', 'skip', 'debug', - 'fail', 'request', 'expected', 'actual', - ].forEach((method) => { + ['warn', 'error', 'log', 'info', 'silly', 'verbose', 'debug'].forEach((method) => { loggerStub[method].restore(); }); }); diff --git a/test/unit/hooksLog-test.js b/test/unit/hooksLog-test.js index 502b85cdd..4d84fc933 100644 --- a/test/unit/hooksLog-test.js +++ b/test/unit/hooksLog-test.js @@ -5,7 +5,7 @@ const util = require('util'); const { assert } = require('chai'); const hooksLog = require('../../lib/hooksLog'); -const loggerStub = require('../../lib/logger'); +const reporterOutputLoggerStub = require('../../lib/reporters/reporterOutputLogger'); describe('hooksLog()', () => { const exampleLogs = [ @@ -13,21 +13,17 @@ describe('hooksLog()', () => { ]; before(() => { - sinon.stub(loggerStub, 'log').callsFake(() => { }); - sinon.stub(loggerStub, 'debug').callsFake(() => { }); - sinon.stub(loggerStub, 'hook').callsFake(() => { }); + sinon.stub(reporterOutputLoggerStub, 'hook').callsFake(() => { }); }); after(() => { - loggerStub.log.restore(); - loggerStub.debug.restore(); - loggerStub.hook.restore(); + reporterOutputLoggerStub.hook.restore(); }); it('should print using util.format only when content is an object type', () => { - const data = hooksLog(clone(exampleLogs), loggerStub, { hello: 'object world' }); - assert.equal(loggerStub.hook.callCount, 1); - assert.deepEqual(loggerStub.hook.getCall(0).args[0], { hello: 'object world' }); + const data = hooksLog(clone(exampleLogs), reporterOutputLoggerStub, { hello: 'object world' }); + assert.equal(reporterOutputLoggerStub.hook.callCount, 1); + assert.deepEqual(reporterOutputLoggerStub.hook.getCall(0).args[0], { hello: 'object world' }); assert.lengthOf(data, 2); assert.isObject(data[1]); assert.property(data[1], 'content'); @@ -38,14 +34,12 @@ describe('hooksLog()', () => { describe('functionality', () => { beforeEach(() => { - loggerStub.log.resetHistory(); - loggerStub.debug.resetHistory(); - loggerStub.hook.resetHistory(); + reporterOutputLoggerStub.hook.resetHistory(); }); it('should push message to the passed array and return the new array', () => { const originLogs = []; - const data = hooksLog(originLogs, loggerStub, 'one message'); + const data = hooksLog(originLogs, reporterOutputLoggerStub, 'one message'); assert.isArray(data); assert.lengthOf(data, 1); assert.strictEqual(data, originLogs); @@ -55,7 +49,7 @@ describe('hooksLog()', () => { it('should push message to undefined logs and return new array instead', () => { const originLogs = undefined; - const data = hooksLog(originLogs, loggerStub, 'another message'); + const data = hooksLog(originLogs, reporterOutputLoggerStub, 'another message'); assert.isArray(data); assert.lengthOf(data, 1); assert.isUndefined(originLogs); @@ -65,7 +59,7 @@ describe('hooksLog()', () => { it('should append message to an existing logs array', () => { const originLogs = clone(exampleLogs); - const data = hooksLog(originLogs, loggerStub, 'some other idea'); + const data = hooksLog(originLogs, reporterOutputLoggerStub, 'some other idea'); assert.isArray(data); assert.lengthOf(data, 2); assert.deepEqual(data, originLogs); @@ -74,15 +68,12 @@ describe('hooksLog()', () => { }); it('should use "hook" logger level', () => { - hooksLog([], loggerStub, 'there is a log'); + hooksLog([], reporterOutputLoggerStub, 'there is a log'); - assert.isTrue(loggerStub.hook.called); - assert.equal(loggerStub.hook.callCount, 1); + assert.isTrue(reporterOutputLoggerStub.hook.called); + assert.equal(reporterOutputLoggerStub.hook.callCount, 1); - assert.isFalse(loggerStub.log.called); - assert.isFalse(loggerStub.debug.called); - - assert.equal(loggerStub.hook.getCall(0).args[0], 'there is a log'); + assert.equal(reporterOutputLoggerStub.hook.getCall(0).args[0], 'there is a log'); }); }); }); diff --git a/test/unit/reporters/ApiaryReporter-test.js b/test/unit/reporters/ApiaryReporter-test.js index 582038dfb..e7204dab0 100644 --- a/test/unit/reporters/ApiaryReporter-test.js +++ b/test/unit/reporters/ApiaryReporter-test.js @@ -7,9 +7,11 @@ const { EventEmitter } = require('events'); const blueprintData = require('../../fixtures/blueprint-data'); const loggerStub = require('../../../lib/logger'); +const reporterOutputLoggerStub = require('../../../lib/reporters/reporterOutputLogger'); const ApiaryReporter = proxyquire('../../../lib/reporters/ApiaryReporter', { '../logger': loggerStub, + './reporterOutputLogger': reporterOutputLoggerStub, }); const PORT = 9876; @@ -18,21 +20,13 @@ nock.enableNetConnect(); describe('ApiaryReporter', () => { let env = {}; beforeEach(() => { - sinon.stub(loggerStub, 'info'); - sinon.stub(loggerStub, 'complete'); - sinon.stub(loggerStub, 'error'); - sinon.stub(loggerStub, 'warn'); - sinon.stub(loggerStub, 'log'); sinon.stub(loggerStub, 'verbose'); + sinon.stub(reporterOutputLoggerStub, 'complete'); }); afterEach(() => { - sinon.stub(loggerStub.info.restore()); - sinon.stub(loggerStub.complete.restore()); - sinon.stub(loggerStub.error.restore()); - sinon.stub(loggerStub.warn.restore()); - sinon.stub(loggerStub.log.restore()); sinon.stub(loggerStub.verbose.restore()); + sinon.stub(reporterOutputLoggerStub.complete.restore()); }); before(() => nock.disableNetConnect()); @@ -692,7 +686,7 @@ describe('ApiaryReporter', () => { const apiaryReporter = new ApiaryReporter(emitter, {}, {}, { custom: { apiaryReporterEnv: env } }); apiaryReporter.remoteId = runId; emitter.emit('end', () => { - assert.isOk(loggerStub.complete.calledWith('See results in Apiary at: https://app.apiary.io/public/tests/run/507f1f77bcf86cd799439011')); + assert.isOk(reporterOutputLoggerStub.complete.calledWith('See results in Apiary at: https://app.apiary.io/public/tests/run/507f1f77bcf86cd799439011')); done(); }); }); @@ -703,7 +697,7 @@ describe('ApiaryReporter', () => { apiaryReporter.remoteId = runId; apiaryReporter.reportUrl = 'https://absolutely.fancy.url/wich-can-change/some/id'; emitter.emit('end', () => { - assert.isOk(loggerStub.complete.calledWith('See results in Apiary at: https://absolutely.fancy.url/wich-can-change/some/id')); + assert.isOk(reporterOutputLoggerStub.complete.calledWith('See results in Apiary at: https://absolutely.fancy.url/wich-can-change/some/id')); done(); }); }); @@ -1002,7 +996,7 @@ describe('ApiaryReporter', () => { const apiaryReporter = new ApiaryReporter(emitter, {}, {}, { custom: { apiaryReporterEnv: env } }); apiaryReporter.remoteId = runId; emitter.emit('end', () => { - assert.isOk(loggerStub.complete.calledWith('See results in Apiary at: https://app.apiary.io/jakubtest/tests/run/507f1f77bcf86cd799439011')); + assert.isOk(reporterOutputLoggerStub.complete.calledWith('See results in Apiary at: https://app.apiary.io/jakubtest/tests/run/507f1f77bcf86cd799439011')); done(); }); }); @@ -1013,7 +1007,7 @@ describe('ApiaryReporter', () => { apiaryReporter.remoteId = runId; apiaryReporter.reportUrl = 'https://absolutely.fancy.url/wich-can-change/some/id'; emitter.emit('end', () => { - assert.isOk(loggerStub.complete.calledWith('See results in Apiary at: https://absolutely.fancy.url/wich-can-change/some/id')); + assert.isOk(reporterOutputLoggerStub.complete.calledWith('See results in Apiary at: https://absolutely.fancy.url/wich-can-change/some/id')); done(); }); }); diff --git a/test/unit/reporters/CLIReporter-test.js b/test/unit/reporters/CLIReporter-test.js index 31a45bd2e..fef1c8ddc 100644 --- a/test/unit/reporters/CLIReporter-test.js +++ b/test/unit/reporters/CLIReporter-test.js @@ -4,17 +4,25 @@ const { assert } = require('chai'); const { EventEmitter } = require('events'); const loggerStub = require('../../../lib/logger'); +const reporterOutputLoggerStub = require('../../../lib/reporters/reporterOutputLogger'); const CLIReporter = proxyquire('../../../lib/reporters/CLIReporter', { '../logger': loggerStub, + './reporterOutputLogger': reporterOutputLoggerStub, }); describe('CLIReporter', () => { let test = {}; - before(() => { loggerStub.transports.console.silent = true; }); + before(() => { + loggerStub.transports.console.silent = true; + reporterOutputLoggerStub.transports.console.silent = true; + }); - after(() => { loggerStub.transports.console.silent = false; }); + after(() => { + loggerStub.transports.console.silent = false; + reporterOutputLoggerStub.transports.console.silent = false; + }); describe('when starting', () => { beforeEach(() => sinon.spy(loggerStub, 'info')); @@ -39,27 +47,27 @@ describe('CLIReporter', () => { }; }); - beforeEach(() => sinon.spy(loggerStub, 'pass')); + beforeEach(() => sinon.spy(reporterOutputLoggerStub, 'pass')); - afterEach(() => loggerStub.pass.restore()); + afterEach(() => reporterOutputLoggerStub.pass.restore()); it('should write pass to the console', () => { const emitter = new EventEmitter(); (new CLIReporter(emitter, {}, {}, true)); emitter.emit('test pass', test); - assert.isOk(loggerStub.pass.calledOnce); + assert.isOk(reporterOutputLoggerStub.pass.calledOnce); }); describe('when details=true', () => { - beforeEach(() => sinon.spy(loggerStub, 'request')); + beforeEach(() => sinon.spy(reporterOutputLoggerStub, 'request')); - afterEach(() => loggerStub.request.restore()); + afterEach(() => reporterOutputLoggerStub.request.restore()); it('should write details for passing tests', () => { const emitter = new EventEmitter(); (new CLIReporter(emitter, {}, {}, true, true)); emitter.emit('test pass', test); - assert.isOk(loggerStub.request.calledOnce); + assert.isOk(reporterOutputLoggerStub.request.calledOnce); }); }); }); @@ -73,28 +81,28 @@ describe('CLIReporter', () => { }); describe('when errors are inline', () => { - beforeEach(() => sinon.spy(loggerStub, 'fail')); + beforeEach(() => sinon.spy(reporterOutputLoggerStub, 'fail')); - afterEach(() => loggerStub.fail.restore()); + afterEach(() => reporterOutputLoggerStub.fail.restore()); it('should write fail to the console', () => { const emitter = new EventEmitter(); (new CLIReporter(emitter, {}, {}, true)); emitter.emit('test fail', test); - assert.isOk(loggerStub.fail.calledTwice); + assert.isOk(reporterOutputLoggerStub.fail.calledTwice); }); }); describe('when errors are aggregated', () => { - beforeEach(() => sinon.spy(loggerStub, 'fail')); + beforeEach(() => sinon.spy(reporterOutputLoggerStub, 'fail')); - afterEach(() => loggerStub.fail.restore()); + afterEach(() => reporterOutputLoggerStub.fail.restore()); it('should not write full failure to the console at the time of failure', () => { const emitter = new EventEmitter(); (new CLIReporter(emitter, {}, {}, false)); emitter.emit('test fail', test); - assert.isOk(loggerStub.fail.calledOnce); + assert.isOk(reporterOutputLoggerStub.fail.calledOnce); }); it('should write full failure to the console after execution is complete', (done) => { @@ -102,7 +110,7 @@ describe('CLIReporter', () => { const cliReporter = new CLIReporter(emitter, {}, {}, false); cliReporter.errors = [test]; emitter.emit('end', () => { - assert.isOk(loggerStub.fail.calledTwice); + assert.isOk(reporterOutputLoggerStub.fail.calledTwice); done(); }); }); @@ -117,15 +125,15 @@ describe('CLIReporter', () => { }; }); - beforeEach(() => sinon.spy(loggerStub, 'error')); + beforeEach(() => sinon.spy(reporterOutputLoggerStub, 'error')); - afterEach(() => loggerStub.error.restore()); + afterEach(() => reporterOutputLoggerStub.error.restore()); it('should write error to the console', () => { const emitter = new EventEmitter(); (new CLIReporter(emitter, {}, {}, false)); emitter.emit('test error', new Error('Error'), test); - assert.isOk(loggerStub.error.calledTwice); + assert.isOk(reporterOutputLoggerStub.error.calledTwice); }); }); @@ -138,9 +146,9 @@ describe('CLIReporter', () => { }; }); - beforeEach(() => sinon.spy(loggerStub, 'error')); + beforeEach(() => sinon.spy(reporterOutputLoggerStub, 'error')); - afterEach(() => loggerStub.error.restore()); + afterEach(() => reporterOutputLoggerStub.error.restore()); const connectionErrors = ['ECONNRESET', 'ENOTFOUND', 'ESOCKETTIMEDOUT', 'ETIMEDOUT', 'ECONNREFUSED', 'EHOSTUNREACH', 'EPIPE']; @@ -151,7 +159,7 @@ describe('CLIReporter', () => { error.code = errType; emitter.emit('test error', error, test); - const messages = Object.keys(loggerStub.error.args).map((value, index) => loggerStub.error.args[index][0]); + const messages = Object.keys(reporterOutputLoggerStub.error.args).map((value, index) => reporterOutputLoggerStub.error.args[index][0]); assert.include(messages.join(), 'Error connecting'); }))); @@ -165,15 +173,15 @@ describe('CLIReporter', () => { }; }); - beforeEach(() => sinon.spy(loggerStub, 'skip')); + beforeEach(() => sinon.spy(reporterOutputLoggerStub, 'skip')); - afterEach(() => loggerStub.skip.restore()); + afterEach(() => reporterOutputLoggerStub.skip.restore()); it('should write skip to the console', () => { const emitter = new EventEmitter(); (new CLIReporter(emitter, {}, {}, false)); emitter.emit('test skip', test); - assert.isOk(loggerStub.skip.calledOnce); + assert.isOk(reporterOutputLoggerStub.skip.calledOnce); }); }); @@ -186,9 +194,9 @@ describe('CLIReporter', () => { }; }); - beforeEach(() => sinon.spy(loggerStub, 'complete')); + beforeEach(() => sinon.spy(reporterOutputLoggerStub, 'complete')); - afterEach(() => loggerStub.complete.restore()); + afterEach(() => reporterOutputLoggerStub.complete.restore()); describe('when there is at least one test', () => it('should write to the console', (done) => { const emitter = new EventEmitter(); @@ -196,7 +204,7 @@ describe('CLIReporter', () => { cliReporter.tests = [test]; cliReporter.stats.tests = 1; emitter.emit('end', () => { - assert.isOk(loggerStub.complete.calledTwice); + assert.isOk(reporterOutputLoggerStub.complete.calledTwice); done(); }); })); @@ -205,7 +213,7 @@ describe('CLIReporter', () => { const emitter = new EventEmitter(); (new CLIReporter(emitter, {}, {}, false)); emitter.emit('end', () => { - assert.isOk(loggerStub.complete.calledOnce); + assert.isOk(reporterOutputLoggerStub.complete.calledOnce); done(); }); })); diff --git a/test/unit/reporters/DotReporter-test.js b/test/unit/reporters/DotReporter-test.js index 052ae1fe1..d90acb418 100644 --- a/test/unit/reporters/DotReporter-test.js +++ b/test/unit/reporters/DotReporter-test.js @@ -5,6 +5,7 @@ const { assert } = require('chai'); const { EventEmitter } = require('events'); const loggerStub = require('../../../lib/logger'); +const reporterOutputLoggerStub = require('../../../lib/reporters/reporterOutputLogger'); const DotReporter = proxyquire('../../../lib/reporters/DotReporter', { '../logger': loggerStub, @@ -48,16 +49,16 @@ describe('DotReporter', () => { describe('when ending', () => { beforeEach(() => { stats.tests = 1; - sinon.spy(loggerStub, 'complete'); + sinon.spy(reporterOutputLoggerStub, 'complete'); sinon.stub(dotReporter, 'write'); }); afterEach(() => { - loggerStub.complete.restore(); + reporterOutputLoggerStub.complete.restore(); dotReporter.write.restore(); }); - it('should log that testing is complete', () => emitter.emit('end', () => assert.isOk(loggerStub.complete.calledTwice))); + it('should log that testing is complete', () => emitter.emit('end', () => assert.isOk(reporterOutputLoggerStub.complete.calledTwice))); describe('when there are failures', () => { before(() => { @@ -71,13 +72,13 @@ describe('DotReporter', () => { dotReporter.errors = [test]; dotReporter.stats.tests = 1; emitter.emit('test start', test); - sinon.spy(loggerStub, 'fail'); + sinon.spy(reporterOutputLoggerStub, 'fail'); }); - afterEach(() => loggerStub.fail.restore()); + afterEach(() => reporterOutputLoggerStub.fail.restore()); it('should log the failures at the end of testing', done => emitter.emit('end', () => { - assert.isOk(loggerStub.fail.called); + assert.isOk(reporterOutputLoggerStub.fail.called); done(); })); }); diff --git a/test/unit/reporters/HTMLReporter-test.js b/test/unit/reporters/HTMLReporter-test.js index a3aea431b..b9bfa78fb 100644 --- a/test/unit/reporters/HTMLReporter-test.js +++ b/test/unit/reporters/HTMLReporter-test.js @@ -6,6 +6,7 @@ const { assert } = require('chai'); const { EventEmitter } = require('events'); const loggerStub = require('../../../lib/logger'); +const reporterOutputLoggerStub = require('../../../lib/reporters/reporterOutputLogger'); const makeDirStub = (input, options) => makeDirStubImpl(input, options); let makeDirStubImpl = () => Promise.resolve(); @@ -13,6 +14,7 @@ const makeDirStubImplBackup = makeDirStubImpl; const HTMLReporter = proxyquire('../../../lib/reporters/HTMLReporter', { '../logger': loggerStub, + './reporterOutputLogger': reporterOutputLoggerStub, fs: fsStub, 'make-dir': makeDirStub, }); @@ -24,9 +26,15 @@ describe('HTMLReporter', () => { let test = {}; let tests; - before(() => { loggerStub.transports.console.silent = true; }); + before(() => { + loggerStub.transports.console.silent = true; + reporterOutputLoggerStub.transports.console.silent = true; + }); - after(() => { loggerStub.transports.console.silent = false; }); + after(() => { + loggerStub.transports.console.silent = false; + reporterOutputLoggerStub.transports.console.silent = false; + }); beforeEach(() => { emitter = new EventEmitter(); @@ -48,15 +56,15 @@ describe('HTMLReporter', () => { describe('when file exists', () => { before(() => { sinon.stub(fsStub, 'existsSync').callsFake(() => true); - sinon.stub(loggerStub, 'info'); + sinon.stub(loggerStub, 'warn'); }); after(() => { fsStub.existsSync.restore(); - loggerStub.info.restore(); + loggerStub.warn.restore(); }); - it('should inform about the existing file', () => assert.isOk(loggerStub.info.called)); + it('should warn about the existing file', () => assert.isOk(loggerStub.warn.called)); }); describe('when file does not exist', () => { @@ -102,13 +110,13 @@ describe('HTMLReporter', () => { describe('when cannot create output directory', () => { beforeEach(() => { - sinon.stub(loggerStub, 'error'); + sinon.stub(reporterOutputLoggerStub, 'error'); sinon.stub(fsStub, 'writeFile').callsFake((path, data, callback) => callback()); makeDirStubImpl = sinon.stub().callsFake(() => Promise.reject(new Error())); }); after(() => { - loggerStub.error.restore(); + reporterOutputLoggerStub.error.restore(); fsStub.writeFile.restore(); makeDirStubImpl = makeDirStubImplBackup; }); @@ -116,7 +124,7 @@ describe('HTMLReporter', () => { it('should write to log', done => emitter.emit('end', () => { assert.isOk(makeDirStubImpl.called); assert.isOk(fsStub.writeFile.notCalled); - assert.isOk(loggerStub.error.called); + assert.isOk(reporterOutputLoggerStub.error.called); done(); })); }); diff --git a/test/unit/reporters/MarkdownReporter-test.js b/test/unit/reporters/MarkdownReporter-test.js index 737f4b8e7..21f26cace 100644 --- a/test/unit/reporters/MarkdownReporter-test.js +++ b/test/unit/reporters/MarkdownReporter-test.js @@ -6,6 +6,7 @@ const { assert } = require('chai'); const { EventEmitter } = require('events'); const loggerStub = require('../../../lib/logger'); +const reporterOutputLoggerStub = require('../../../lib/reporters/reporterOutputLogger'); const makeDirStub = (input, options) => makeDirStubImpl(input, options); let makeDirStubImpl = () => Promise.resolve(); @@ -13,6 +14,7 @@ const makeDirStubImplBackup = makeDirStubImpl; const MarkdownReporter = proxyquire('../../../lib/reporters/MarkdownReporter', { '../logger': loggerStub, + './reporterOutputLogger': reporterOutputLoggerStub, fs: fsStub, 'make-dir': makeDirStub, }); @@ -24,9 +26,15 @@ describe('MarkdownReporter', () => { let test = {}; let tests; - before(() => { loggerStub.transports.console.silent = true; }); + before(() => { + loggerStub.transports.console.silent = true; + reporterOutputLoggerStub.transports.console.silent = true; + }); - after(() => { loggerStub.transports.console.silent = false; }); + after(() => { + loggerStub.transports.console.silent = false; + reporterOutputLoggerStub.transports.console.silent = false; + }); beforeEach(() => { emitter = new EventEmitter(); @@ -49,15 +57,15 @@ describe('MarkdownReporter', () => { describe('when file exists', () => { before(() => { sinon.stub(fsStub, 'existsSync').callsFake(() => true); - sinon.stub(loggerStub, 'info'); + sinon.stub(loggerStub, 'warn'); }); after(() => { fsStub.existsSync.restore(); - loggerStub.info.restore(); + loggerStub.warn.restore(); }); - it('should inform about the existing file', () => assert.isOk(loggerStub.info.called)); + it('should warn about the existing file', () => assert.isOk(loggerStub.warn.called)); }); describe('when file does not exist', () => { @@ -106,20 +114,20 @@ describe('MarkdownReporter', () => { describe('when cannot create output directory', () => { beforeEach(() => { sinon.stub(fsStub, 'writeFile').callsFake((path, data, callback) => callback()); - sinon.stub(loggerStub, 'error'); + sinon.stub(reporterOutputLoggerStub, 'error'); makeDirStubImpl = sinon.stub().callsFake(() => Promise.reject(new Error())); }); after(() => { fsStub.writeFile.restore(); - loggerStub.error.restore(); + reporterOutputLoggerStub.error.restore(); makeDirStubImpl = makeDirStubImplBackup; }); it('should write to log', done => emitter.emit('end', () => { assert.isOk(makeDirStubImpl.called); assert.isOk(fsStub.writeFile.notCalled); - assert.isOk(loggerStub.error.called); + assert.isOk(reporterOutputLoggerStub.error.called); done(); })); }); diff --git a/test/unit/reporters/NyanReporter-test.js b/test/unit/reporters/NyanReporter-test.js index 657ada07f..d1ab5b0e7 100644 --- a/test/unit/reporters/NyanReporter-test.js +++ b/test/unit/reporters/NyanReporter-test.js @@ -4,10 +4,10 @@ const sinon = require('sinon'); const { assert } = require('chai'); const { EventEmitter } = require('events'); -const loggerStub = require('../../../lib/logger'); +const reporterOutputLoggerStub = require('../../../lib/reporters/reporterOutputLogger'); const NyanCatReporter = proxyquire('../../../lib/reporters/NyanReporter', { - '../logger': loggerStub, + './reporterOutputLogger': reporterOutputLoggerStub, }); describe('NyanCatReporter', () => { @@ -16,9 +16,13 @@ describe('NyanCatReporter', () => { let tests; let nyanReporter; - before(() => { loggerStub.transports.console.silent = true; }); + before(() => { + reporterOutputLoggerStub.transports.console.silent = true; + }); - after(() => { loggerStub.transports.console.silent = false; }); + after(() => { + reporterOutputLoggerStub.transports.console.silent = false; + }); beforeEach(() => { emitter = new EventEmitter(); @@ -58,19 +62,19 @@ describe('NyanCatReporter', () => { describe('when ending', () => { beforeEach(() => { - sinon.spy(loggerStub, 'complete'); + sinon.spy(reporterOutputLoggerStub, 'complete'); sinon.spy(nyanReporter, 'draw'); sinon.stub(nyanReporter, 'write'); }); afterEach(() => { - loggerStub.complete.restore(); + reporterOutputLoggerStub.complete.restore(); nyanReporter.draw.restore(); nyanReporter.write.restore(); }); it('should log that testing is complete', done => emitter.emit('end', () => { - assert.isOk(loggerStub.complete.calledTwice); + assert.isOk(reporterOutputLoggerStub.complete.calledTwice); done(); })); @@ -82,13 +86,13 @@ describe('NyanCatReporter', () => { }; nyanReporter.errors = [test]; emitter.emit('test start', test); - sinon.spy(loggerStub, 'fail'); + sinon.spy(reporterOutputLoggerStub, 'fail'); }); - afterEach(() => loggerStub.fail.restore()); + afterEach(() => reporterOutputLoggerStub.fail.restore()); it('should log the failures at the end of testing', done => emitter.emit('end', () => { - assert.isOk(loggerStub.fail.calledTwice); + assert.isOk(reporterOutputLoggerStub.fail.calledTwice); done(); })); }); diff --git a/test/unit/reporters/XUnitReporter-test.js b/test/unit/reporters/XUnitReporter-test.js index 8551b0260..66f02ef0b 100644 --- a/test/unit/reporters/XUnitReporter-test.js +++ b/test/unit/reporters/XUnitReporter-test.js @@ -6,6 +6,7 @@ const { assert } = require('chai'); const { EventEmitter } = require('events'); const loggerStub = require('../../../lib/logger'); +const reporterOutputLoggerStub = require('../../../lib/reporters/reporterOutputLogger'); const makeDirStub = (input, options) => makeDirStubImpl(input, options); let makeDirStubImpl = () => Promise.resolve(); @@ -13,6 +14,7 @@ const makeDirStubImplBackup = makeDirStubImpl; const XUnitReporter = proxyquire('../../../lib/reporters/XUnitReporter', { '../logger': loggerStub, + './reporterOutputLogger': reporterOutputLoggerStub, fs: fsStub, 'make-dir': makeDirStub, }); @@ -20,28 +22,34 @@ const XUnitReporter = proxyquire('../../../lib/reporters/XUnitReporter', { describe('XUnitReporter', () => { let test = {}; - before(() => { loggerStub.transports.console.silent = true; }); + before(() => { + loggerStub.transports.console.silent = true; + reporterOutputLoggerStub.transports.console.silent = true; + }); - after(() => { loggerStub.transports.console.silent = false; }); + after(() => { + loggerStub.transports.console.silent = false; + reporterOutputLoggerStub.transports.console.silent = false; + }); describe('when creating', () => { describe('when file exists', () => { before(() => { sinon.stub(fsStub, 'existsSync').callsFake(() => true); sinon.stub(fsStub, 'unlinkSync').callsFake(() => true); - sinon.stub(loggerStub, 'info'); + sinon.stub(loggerStub, 'warn'); }); after(() => { fsStub.existsSync.restore(); fsStub.unlinkSync.restore(); - loggerStub.info.restore(); + loggerStub.warn.restore(); }); - it('should inform about the existing file', () => { + it('should warn about the existing file', () => { const emitter = new EventEmitter(); (new XUnitReporter(emitter, {}, {}, 'test.xml')); - assert.isOk(loggerStub.info.called); + assert.isOk(loggerStub.warn.called); }); }); @@ -90,13 +98,13 @@ describe('XUnitReporter', () => { describe('when cannot create output directory', () => { beforeEach(() => { sinon.stub(fsStub, 'appendFileSync'); - sinon.stub(loggerStub, 'error'); + sinon.stub(reporterOutputLoggerStub, 'error'); makeDirStubImpl = sinon.stub().callsFake(() => Promise.reject(new Error())); }); after(() => { fsStub.appendFileSync.restore(); - loggerStub.error.restore(); + reporterOutputLoggerStub.error.restore(); makeDirStubImpl = makeDirStubImplBackup; }); @@ -106,7 +114,7 @@ describe('XUnitReporter', () => { emitter.emit('start', '', () => { assert.isOk(makeDirStubImpl.called); assert.isOk(fsStub.appendFileSync.notCalled); - assert.isOk(loggerStub.error.called); + assert.isOk(reporterOutputLoggerStub.error.called); done(); }); }); From f749a0d3e396aa73a323477d9a0284fb8a422b33 Mon Sep 17 00:00:00 2001 From: Honza Javorek Date: Thu, 17 Jan 2019 17:30:51 +0100 Subject: [PATCH 03/13] refactor: replace --level with --loglevel, remove --silent A part of the effort to separate application logging from the reporters output. Addresses https://github.com/apiaryio/dredd/issues/1089, supersedes https://github.com/apiaryio/dredd/pull/1099, enables https://github.com/apiaryio/dredd/issues/765 BREAKING CHANGE: Instead of --level use --loglevel. The option is no longer able to affect reporter output, it only affects application logging output. Use --loglevel=silent instead of --silent, which is now removed. The --loglevel option now only accepts 'silent', 'error', 'warning', 'debug' as values. --- .github/ISSUE_TEMPLATE/bug_report.md | 2 +- docs/usage-cli.rst | 3 +-- docs/usage-js.rst | 3 +-- lib/configuration.js | 20 +++++++++++++++----- lib/configureReporters.js | 2 +- lib/logger.js | 6 +++--- lib/options.json | 11 +++-------- 7 files changed, 25 insertions(+), 22 deletions(-) diff --git a/.github/ISSUE_TEMPLATE/bug_report.md b/.github/ISSUE_TEMPLATE/bug_report.md index c5fa3f6ec..73b127d40 100644 --- a/.github/ISSUE_TEMPLATE/bug_report.md +++ b/.github/ISSUE_TEMPLATE/bug_report.md @@ -25,7 +25,7 @@ A clear and concise description of what you expected to happen. ( paste your output here ) ``` -**Does `dredd --level=debug` uncover something?** +**Does `dredd --loglevel=debug` uncover something?** If you run Dredd with debugging output, do you see any interesting information relevant to the bug? **Can you send us failing test in a Pull Request?** diff --git a/docs/usage-cli.rst b/docs/usage-cli.rst index c7041f3e7..849a41348 100644 --- a/docs/usage-cli.rst +++ b/docs/usage-cli.rst @@ -70,9 +70,8 @@ See below how sample configuration file could look like. The structure is the sa inline-errors: false details: false method: [] - level: info + loglevel: warning timestamp: false - silent: false path: [] blueprint: api-description.apib endpoint: "http://127.0.0.1:3000" diff --git a/docs/usage-js.rst b/docs/usage-js.rst index ead5826b2..06f6c1607 100644 --- a/docs/usage-js.rst +++ b/docs/usage-js.rst @@ -39,8 +39,7 @@ Let’s have a look at an example configuration first. (Please also see the :ref 'dry-run': false, // Boolean, do not run any real HTTP transaction 'names': false, // Boolean, Print Transaction names and finish, similar to dry-run - 'level': 'info', // String, log-level (info, silly, debug, verbose, ...) - 'silent': false, // Boolean, Silences all logging output + 'loglevel': 'warning', // String, logging level (debug, warning, error, silent) 'only': [], // Array of Strings, run only transaction that match these names diff --git a/lib/configuration.js b/lib/configuration.js index a839265b9..122fae420 100644 --- a/lib/configuration.js +++ b/lib/configuration.js @@ -32,10 +32,21 @@ function applyLoggingOptions(options) { logger.transports.console.timestamp = options.timestamp; reporterOutputLogger.transports.console.timestamp = options.timestamp; - logger.transports.console.silent = options.silent; - reporterOutputLogger.transports.console.silent = options.silent; + if (options.loglevel) { + const loglevel = options.loglevel.toLowerCase(); + if (loglevel === 'silent') { + logger.transports.console.silent = true; + } else if (loglevel === 'warning') { + logger.transports.console.level = 'warn'; + } else if (['debug', 'warn', 'error'].includes(loglevel)) { + logger.transports.console.level = loglevel; + } else { + throw new Error(`Unsupported logging level: '${loglevel}'`); + } + } else { + logger.transports.console.level = 'warn'; + } - logger.transports.console.level = options.level; reporterOutputLogger.transports.console.level = 'test'; return options; @@ -52,7 +63,6 @@ function applyConfiguration(config) { }, options: { 'dry-run': false, - silent: false, reporter: null, output: null, debug: false, @@ -63,7 +73,7 @@ function applyConfiguration(config) { method: [], only: [], color: true, - level: 'info', + loglevel: 'warning', timestamp: false, sorted: false, names: false, diff --git a/lib/configureReporters.js b/lib/configureReporters.js index 68ca16788..60e548056 100644 --- a/lib/configureReporters.js +++ b/lib/configureReporters.js @@ -67,7 +67,7 @@ function configureReporters(config, stats, tests, runner) { } } - if (!config.options.silent) { addCli(reporters); } + addCli(reporters); const usedFileReporters = intersection(reporters, fileReporters); diff --git a/lib/logger.js b/lib/logger.js index 9417f1f17..3792d6f65 100644 --- a/lib/logger.js +++ b/lib/logger.js @@ -5,16 +5,16 @@ module.exports = new (winston.Logger)({ new (winston.transports.Console)({ colorize: true }), ], levels: { - silly: 5, - debug: 4, + debug: 5, + silly: 4, verbose: 3, info: 2, warn: 1, error: 0, }, colors: { - silly: 'gray', debug: 'cyan', + silly: 'gray', verbose: 'magenta', info: 'blue', warn: 'yellow', diff --git a/lib/options.json b/lib/options.json index a310084f5..6ac1667d0 100644 --- a/lib/options.json +++ b/lib/options.json @@ -92,21 +92,16 @@ "description": "Determines whether console output should include colors.", "default": true }, - "level": { + "loglevel": { "alias": "l", - "description": "The level of logging to output. Options: silly, debug, verbose, info, warn, error.", - "default": "info" + "description": "The level of logging to output. Options: 'debug', 'warning', 'error', 'silent'.", + "default": "warning" }, "timestamp": { "alias": "t", "description": "Determines whether console output should include timestamps.", "default": false }, - "silent": { - "alias": "q", - "description": "Silences commandline output.", - "default": false - }, "path": { "alias": "p", "description": "Additional API description paths or URLs. Can be used multiple times with glob pattern for paths.", From 79ff1ba9dcf85830398eb410b515ad0fbf085f47 Mon Sep 17 00:00:00 2001 From: Honza Javorek Date: Thu, 17 Jan 2019 17:41:39 +0100 Subject: [PATCH 04/13] test: update tests to reflect the --level and --silent removal --- test/integration/cli/hookfiles-cli-test.js | 6 ++++-- test/integration/cli/reporters-cli-test.js | 1 - .../cli/server-process-cli-test.js | 13 +++++++----- test/integration/dredd-test.js | 8 +++---- test/integration/helpers.js | 2 +- test/integration/proxy-test.js | 5 +++-- test/unit/CLI-test.js | 3 +-- test/unit/Dredd-test.js | 21 +++++++------------ test/unit/configUtils-test.js | 9 +++----- test/unit/configuration-test.js | 8 +++---- test/unit/configureReporters-test.js | 9 ++++---- 11 files changed, 39 insertions(+), 46 deletions(-) diff --git a/test/integration/cli/hookfiles-cli-test.js b/test/integration/cli/hookfiles-cli-test.js index ff6492570..2e469c3a5 100644 --- a/test/integration/cli/hookfiles-cli-test.js +++ b/test/integration/cli/hookfiles-cli-test.js @@ -533,7 +533,7 @@ describe('CLI', () => { }); }); - describe('when setting the log output level with --level', () => { + describe('when setting the log output level with --loglevel', () => { let runtimeInfo; before((done) => { @@ -543,7 +543,7 @@ describe('CLI', () => { const args = [ './test/fixtures/single-get.apib', `http://127.0.0.1:${DEFAULT_SERVER_PORT}`, - '--level=error', + '--loglevel=error', '--color=false', ]; runCLIWithServer(args, app, (err, info) => { @@ -734,6 +734,7 @@ describe('CLI', () => { './test/fixtures/multifile/*.apib', `http://127.0.0.1:${DEFAULT_SERVER_PORT}`, '--names', + '--loglevel=debug', ]; runCLI(args, (err, info) => { cliInfo = info; @@ -800,6 +801,7 @@ describe('CLI', () => { './test/fixtures/multiple-examples.apib', `http://127.0.0.1:${DEFAULT_SERVER_PORT}`, '--path=./test/fixtures/multifile/*.apib', + '--loglevel=debug', '--names', ]; runCLI(args, (err, info) => { diff --git a/test/integration/cli/reporters-cli-test.js b/test/integration/cli/reporters-cli-test.js index 5aea23a11..c9631659c 100644 --- a/test/integration/cli/reporters-cli-test.js +++ b/test/integration/cli/reporters-cli-test.js @@ -134,7 +134,6 @@ describe('CLI - Reporters', () => { }); it('hooks.log should print also to console', () => { - // Because --level=info is lower than --level=hook assert.include(cliInfo.output, 'using hooks.log to debug'); }); it('hooks.log should use toString on objects', () => assert.include(cliInfo.output, 'Error object!')); diff --git a/test/integration/cli/server-process-cli-test.js b/test/integration/cli/server-process-cli-test.js index 1a1d83040..a26047993 100644 --- a/test/integration/cli/server-process-cli-test.js +++ b/test/integration/cli/server-process-cli-test.js @@ -69,6 +69,7 @@ describe('CLI - Server Process', () => { `http://127.0.0.1:${DEFAULT_SERVER_PORT}`, `--server=node ./test/fixtures/scripts/dummy-server.js ${DEFAULT_SERVER_PORT}`, '--server-wait=1', + '--loglevel=debug', ]; before(done => runCLI(args, (err, info) => { @@ -88,6 +89,7 @@ describe('CLI - Server Process', () => { `http://127.0.0.1:${DEFAULT_SERVER_PORT}`, '--server=/foo/bar/baz', '--server-wait=1', + '--loglevel=debug', ]; before(done => runCLI(args, (err, info) => { @@ -101,25 +103,25 @@ describe('CLI - Server Process', () => { }); for (const scenario of [{ - description: 'When crashes before requests', + description: 'when crashes before requests', apiDescriptionDocument: './test/fixtures/single-get.apib', server: 'node test/fixtures/scripts/exit-3.js', expectServerBoot: false, }, { - description: 'When crashes during requests', + description: 'when crashes during requests', apiDescriptionDocument: './test/fixtures/apiary.apib', server: `node test/fixtures/scripts/dummy-server-crash.js ${DEFAULT_SERVER_PORT}`, expectServerBoot: true, }, { - description: 'When killed before requests', + description: 'when killed before requests', apiDescriptionDocument: './test/fixtures/single-get.apib', server: 'node test/fixtures/scripts/kill-self.js', expectServerBoot: false, }, { - description: 'When killed during requests', + description: 'when killed during requests', apiDescriptionDocument: './test/fixtures/apiary.apib', server: `node test/fixtures/scripts/dummy-server-kill.js ${DEFAULT_SERVER_PORT}`, expectServerBoot: true, @@ -132,6 +134,7 @@ describe('CLI - Server Process', () => { `http://127.0.0.1:${DEFAULT_SERVER_PORT}`, `--server=${scenario.server}`, '--server-wait=1', + '--loglevel=debug', ]; before(done => runCLI(args, (err, info) => { @@ -159,7 +162,7 @@ describe('CLI - Server Process', () => { `http://127.0.0.1:${DEFAULT_SERVER_PORT}`, `--server=node test/fixtures/scripts/dummy-server-ignore-term.js ${DEFAULT_SERVER_PORT}`, '--server-wait=1', - '--level=verbose', + '--loglevel=debug', ]; before(done => runCLI(args, (err, info) => { diff --git a/test/integration/dredd-test.js b/test/integration/dredd-test.js index 0f3c0c1e7..eaf64dc29 100644 --- a/test/integration/dredd-test.js +++ b/test/integration/dredd-test.js @@ -19,7 +19,7 @@ function execCommand(options = {}, cb) { exitStatus = null; let finished = false; if (!options.server) { options.server = `http://127.0.0.1:${PORT}`; } - if (!options.level) { options.level = 'info'; } + if (!options.loglevel) { options.loglevel = 'warning'; } new Dredd(options).run((error, stats = {}) => { if (!finished) { finished = true; @@ -112,7 +112,7 @@ describe('Dredd class Integration', () => { options: { path: ['./test/fixtures/single-get.apib'], reporter: ['apiary'], - level: 'verbose', + loglevel: 'debug', }, custom: { apiaryApiUrl: `http://127.0.0.1:${PORT + 1}`, @@ -224,7 +224,7 @@ describe('Dredd class Integration', () => { options: { path: ['./test/fixtures/single-get.apib'], reporter: ['apiary'], - level: 'verbose', + loglevel: 'debug', }, custom: { apiaryReporterEnv: { @@ -281,7 +281,7 @@ describe('Dredd class Integration', () => { options: { path: ['./test/fixtures/single-get.apib'], reporter: ['apiary'], - level: 'verbose', + loglevel: 'debug', }, custom: { apiaryReporterEnv: { diff --git a/test/integration/helpers.js b/test/integration/helpers.js index 11f1cd858..c9abe976b 100644 --- a/test/integration/helpers.js +++ b/test/integration/helpers.js @@ -149,7 +149,7 @@ function runDredd(dredd, serverPort, callback) { if (dredd.configuration.server == null) { dredd.configuration.server = `http://127.0.0.1:${serverPort}`; } if (dredd.configuration.options == null) { dredd.configuration.options = {}; } - if (dredd.configuration.options.level == null) { dredd.configuration.options.level = 'silly'; } + if (dredd.configuration.options.loglevel == null) { dredd.configuration.options.loglevel = 'debug'; } let stats; diff --git a/test/integration/proxy-test.js b/test/integration/proxy-test.js index 48c8b9aec..a8010a24c 100644 --- a/test/integration/proxy-test.js +++ b/test/integration/proxy-test.js @@ -19,8 +19,7 @@ const REGULAR_HTTP_METHODS = ['GET', 'POST', 'PUT', 'DELETE']; function createAndRunDredd(configuration, done) { if (!configuration.options) { configuration.options = {}; } configuration.options.color = false; - configuration.options.silent = true; - configuration.options.level = 'silly'; + configuration.options.loglevel = 'debug'; let dredd; recordLogging((next) => { @@ -172,6 +171,7 @@ ${protocol}_proxy=${PROXY_URL}\ describe('Requesting Server Under Test', () => test({ protocol, configureDredd(configuration) { + configuration.loglevel = 'debug'; configuration.server = serverUrl; configuration.options.path = './test/fixtures/single-get.apib'; }, @@ -230,6 +230,7 @@ http_proxy=${PROXY_URL}, no_proxy=${SERVER_HOST}\ describe('Requesting Server Under Test', () => test({ protocol: 'http', configureDredd(configuration) { + configuration.loglevel = 'debug'; configuration.server = serverUrl; configuration.options.path = './test/fixtures/single-get.apib'; }, diff --git a/test/unit/CLI-test.js b/test/unit/CLI-test.js index 1661c1eac..773033432 100644 --- a/test/unit/CLI-test.js +++ b/test/unit/CLI-test.js @@ -326,9 +326,8 @@ describe('CLI class', () => { details: false, method: [], color: true, - level: 'info', + loglevel: 'warning', timestamp: false, - silent: false, path: [], $0: 'node ./bin/dredd', })); diff --git a/test/unit/Dredd-test.js b/test/unit/Dredd-test.js index 3e70a0d0f..c782958dc 100644 --- a/test/unit/Dredd-test.js +++ b/test/unit/Dredd-test.js @@ -29,7 +29,6 @@ describe('Dredd class', () => { configuration = { server: 'http://127.0.0.1:3000/', options: { - silent: true, method: 'get', header: 'Accept:application/json', user: 'bob:test', @@ -41,7 +40,7 @@ describe('Dredd class', () => { it('should copy configuration on creation', () => { dredd = new Dredd(configuration); - assert.isOk(dredd.configuration.options.silent); + assert.isOk(dredd.configuration.options.sorted); assert.notOk(dredd.configuration.options['dry-run']); }); @@ -91,7 +90,7 @@ describe('Dredd class', () => { configuration = { server: 'http://127.0.0.1:3000/', options: { - silent: true, + path: ['./test/fixtures/multifile/*.apib', './test/fixtures/multifile/*.apib'], }, }; @@ -141,7 +140,7 @@ describe('Dredd class', () => { configuration = { server: 'http://127.0.0.1:3000/', options: { - silent: true, + path: ['./test/fixtures/multifile/*.balony', './test/fixtures/multifile/*.apib'], }, }; @@ -164,7 +163,7 @@ describe('Dredd class', () => { configuration = { server: 'http://127.0.0.1:3000/', options: { - silent: true, + path: ['./test/fixtures/multifile/*.balony'], }, }; @@ -187,7 +186,7 @@ describe('Dredd class', () => { configuration = { server: 'http://127.0.0.1:3000/', options: { - silent: true, + }, data: { testingDirectObject: { @@ -279,7 +278,7 @@ GET /url configuration = { server: 'http://127.0.0.1:3000/', options: { - silent: true, + path: ['http://some.path.to/file.apib', 'https://another.path.to/apiary.apib', './test/fixtures/multifile/*.apib'], }, }; @@ -411,7 +410,6 @@ GET /url configuration = { url: 'http://127.0.0.1:3000/', options: { - silent: true, path: ['./test/fixtures/error-blueprint.apib'], }, }; @@ -438,7 +436,6 @@ GET /url configuration = { url: 'http://127.0.0.1:3000/', options: { - silent: true, path: ['./test/fixtures/warning-ambiguous.apib'], }, }; @@ -471,7 +468,6 @@ GET /url configuration = { url: 'http://127.0.0.1:3000/', options: { - silent: true, path: ['./balony/path.apib'], }, }; @@ -497,7 +493,6 @@ GET /url configuration = { server: 'http://127.0.0.1:3000/', options: { - silent: true, path: ['./test/fixtures/error-uri-template.apib'], }, }; @@ -524,7 +519,6 @@ GET /url configuration = { server: 'http://127.0.0.1:3000/', options: { - silent: true, path: ['./test/fixtures/warning-ambiguous.apib'], }, }; @@ -559,7 +553,6 @@ GET /url configuration = { server: 'http://127.0.0.1:3000/', options: { - silent: true, path: ['./test/fixtures/apiary.apib'], }, }; @@ -585,7 +578,7 @@ GET /url configuration = { server: 'http://127.0.0.1:3000/', options: { - silent: true, + reporter: ['apiary'], path: ['./test/fixtures/apiary.apib'], custom: { diff --git a/test/unit/configUtils-test.js b/test/unit/configUtils-test.js index 73f18e2f9..a04b490e5 100644 --- a/test/unit/configUtils-test.js +++ b/test/unit/configUtils-test.js @@ -49,12 +49,10 @@ const argvData = { m: [], color: true, c: true, - level: 'info', - l: 'info', + loglevel: 'warning', + l: 'warning', timestamp: false, t: false, - silent: false, - q: false, path: [], p: [], $0: 'node ./bin/dredd', @@ -163,9 +161,8 @@ inline-errors: false details: false method: [] color: true -level: info +loglevel: info timestamp: false -silent: false path: [] blueprint: blueprint endpoint: endpoint\ diff --git a/test/unit/configuration-test.js b/test/unit/configuration-test.js index 2deae424b..1d51b30ee 100644 --- a/test/unit/configuration-test.js +++ b/test/unit/configuration-test.js @@ -14,13 +14,13 @@ describe('configuration.applyLoggingOptions()', () => { it('applies logging options', () => { config = configuration.applyLoggingOptions({ color: 'true', - level: 'debug', + loglevel: 'debug', }); assert.propertyVal(config, 'color', true); assert.equal(logger.transports.console.colorize, true); - assert.propertyVal(config, 'level', 'debug'); + assert.propertyVal(config, 'loglevel', 'debug'); assert.equal(logger.transports.console.level, 'debug'); }); @@ -46,14 +46,14 @@ describe('configuration.applyConfiguration()', () => { config = configuration.applyConfiguration({ options: { color: 'true', - level: 'debug', + loglevel: 'debug', }, }); assert.nestedPropertyVal(config, 'options.color', true); assert.equal(logger.transports.console.colorize, true); - assert.nestedPropertyVal(config, 'options.level', 'debug'); + assert.nestedPropertyVal(config, 'options.loglevel', 'debug'); assert.equal(logger.transports.console.level, 'debug'); }); }); diff --git a/test/unit/configureReporters-test.js b/test/unit/configureReporters-test.js index fb4d9fb93..313926b70 100644 --- a/test/unit/configureReporters-test.js +++ b/test/unit/configureReporters-test.js @@ -52,7 +52,6 @@ describe('configureReporters(config, stats, tests, onSaveCallback)', () => { options: { reporter: [], output: [], - silent: false, 'inline-errors': false, }, }; @@ -71,15 +70,15 @@ describe('configureReporters(config, stats, tests, onSaveCallback)', () => { }); describe('when silent', () => { - before(() => configuration.options.silent = true); + before(() => configuration.options.loglevel = 'silent'); - after(() => configuration.options.silent = false); + after(() => configuration.options.loglevel = 'silent'); beforeEach(() => resetStubs()); - it('should not add any reporters', (done) => { + it('should still add reporters', (done) => { configureReporters(configuration, {}, {}, null); - assert.notOk(CliReporterStub.called); + assert.ok(CliReporterStub.called); return done(); }); }); From 691c0a56e12332eafdd7660fd8205f1fb76583ed Mon Sep 17 00:00:00 2001 From: Honza Javorek Date: Thu, 17 Jan 2019 18:29:52 +0100 Subject: [PATCH 05/13] refactor: log only as error, warning, and debug --- docs/hooks/index.rst | 2 - docs/how-to-guides.rst | 2 - lib/CLI.js | 48 +++++++++--------- lib/Dredd.js | 18 +++---- lib/HooksWorkerClient.js | 34 ++++++------- lib/TransactionRunner.js | 41 +++++++-------- lib/addHooks.js | 2 +- lib/configuration.js | 2 +- lib/configureReporters.js | 2 +- lib/logger.js | 8 +-- lib/reporters/ApiaryReporter.js | 6 +-- lib/reporters/BaseReporter.js | 2 +- lib/reporters/CLIReporter.js | 6 +-- lib/reporters/DotReporter.js | 6 +-- lib/reporters/HTMLReporter.js | 2 +- lib/reporters/MarkdownReporter.js | 2 +- lib/reporters/NyanReporter.js | 4 +- lib/reporters/XUnitReporter.js | 2 +- lib/reporters/reporterOutputLogger.js | 2 + .../integration/cli/configuration-cli-test.js | 31 +++++------- test/integration/cli/hookfiles-cli-test.js | 4 +- .../cli/server-process-cli-test.js | 12 ++--- test/integration/dredd-test.js | 2 +- test/unit/CLI-test.js | 30 +++++------ test/unit/Dredd-test.js | 14 +++--- test/unit/HooksWorkerClient-test.js | 2 +- test/unit/reporters/ApiaryReporter-test.js | 10 ++-- test/unit/reporters/CLIReporter-test.js | 7 +-- test/unit/reporters/DotReporter-test.js | 6 +-- test/unit/transactionRunner-test.js | 50 +++++++++---------- 30 files changed, 173 insertions(+), 186 deletions(-) diff --git a/docs/hooks/index.rst b/docs/hooks/index.rst index 821292b56..69e6e18e3 100644 --- a/docs/hooks/index.rst +++ b/docs/hooks/index.rst @@ -137,7 +137,6 @@ You can get a list of all transaction names available in your API description do :emphasize-lines: 3, 5 $ dredd ./blog.apib http://127.0.0.1 --names - info: Beginning Dredd testing... info: Articles > List articles skip: GET (200) /articles info: Articles > Publish an article @@ -156,7 +155,6 @@ You can get a list of all transaction names available in your API description do :emphasize-lines: 3, 5 $ dredd ./blog.yaml http://127.0.0.1 --names - info: Beginning Dredd testing... info: Articles > List articles > 200 > application/json skip: GET (200) /articles info: Articles > Publish an article > 201 > application/json diff --git a/docs/how-to-guides.rst b/docs/how-to-guides.rst index b153f5553..c4799b2ef 100644 --- a/docs/how-to-guides.rst +++ b/docs/how-to-guides.rst @@ -656,7 +656,6 @@ Dredd will detect two HTTP transaction examples and will compile following trans :: $ dredd api-description.apib http://127.0.0.1 --names - info: Beginning Dredd testing... info: Resource > Update Resource > Example 1 info: Resource > Update Resource > Example 2 @@ -687,7 +686,6 @@ Command-line output of complex HTTP responses and expectations can be hard to re $ dredd apiary.apib http://127.0.0.1 --reporter=apiary warn: Apiary API Key or API Project Subdomain were not provided. Configure Dredd to be able to save test reports alongside your Apiary API project: https://dredd.org/en/latest/how-to-guides/#using-apiary-reporter-and-apiary-tests - info: Beginning Dredd testing... pass: DELETE /honey duration: 884ms complete: 1 passing, 0 failing, 0 errors, 0 skipped, 1 total complete: Tests took 1631ms diff --git a/lib/CLI.js b/lib/CLI.js index 5c7b69255..3b65b3f88 100644 --- a/lib/CLI.js +++ b/lib/CLI.js @@ -58,14 +58,14 @@ Example: // Gracefully terminate server stopServer(callback) { if (!this.serverProcess || !this.serverProcess.spawned) { - logger.verbose('No backend server process to terminate.'); + logger.debug('No backend server process to terminate.'); return callback(); } if (this.serverProcess.terminated) { logger.debug('The backend server process has already terminated'); return callback(); } - logger.verbose('Terminating backend server process, PID', this.serverProcess.pid); + logger.debug('Terminating backend server process, PID', this.serverProcess.pid); this.serverProcess.terminate({ force: true }); this.serverProcess.on('exit', () => callback()); } @@ -80,7 +80,7 @@ Example: if (this.exit) { this._processExit = (exitStatus) => { - logger.verbose(`Exiting Dredd process with status '${exitStatus}'.`); + logger.debug(`Exiting Dredd process with status '${exitStatus}'.`); logger.debug('Using configured custom exit() method to terminate the Dredd process.'); this.finished = true; this.stopServer(() => { @@ -89,19 +89,19 @@ Example: }; } else { this._processExit = (exitStatus) => { - logger.verbose(`Exiting Dredd process with status '${exitStatus}'.`); + logger.debug(`Exiting Dredd process with status '${exitStatus}'.`); logger.debug('Using native process.exit() method to terminate the Dredd process.'); this.stopServer(() => process.exit(exitStatus)); }; } } else { this._processExit = (exitStatus) => { - logger.verbose(`Exiting Dredd process with status '${exitStatus}'.`); + logger.debug(`Exiting Dredd process with status '${exitStatus}'.`); logger.debug('Using configured custom callback to terminate the Dredd process.'); this.finished = true; if (this.sigIntEventAdded) { if (this.serverProcess && !this.serverProcess.terminated) { - logger.verbose('Killing backend server process before Dredd exits.'); + logger.debug('Killing backend server process before Dredd exits.'); this.serverProcess.signalKill(); } process.removeEventListener('SIGINT', this.commandSigInt); @@ -145,7 +145,7 @@ Example: runExitingActions() { // Run interactive config if (this.argv._[0] === 'init' || this.argv.init === true) { - logger.silly('Starting interactive configuration.'); + logger.debug('Starting interactive configuration.'); this.finished = true; interactiveConfig(this.argv, (config) => { configUtils.save(config); @@ -156,13 +156,13 @@ Example: // Show help } else if (this.argv.help === true) { - logger.silly('Printing help.'); + logger.debug('Printing help.'); this.optimist.showHelp(console.error); this._processExit(0); // Show version } else if (this.argv.version === true) { - logger.silly('Printing version.'); + logger.debug('Printing version.'); console.log(`\ ${packageData.name} v${packageData.version} \ (${os.type()} ${os.release()}; ${os.arch()})\ @@ -173,10 +173,10 @@ ${packageData.name} v${packageData.version} \ loadDreddFile() { const configPath = this.argv.config; - logger.verbose('Loading configuration file:', configPath); + logger.debug('Loading configuration file:', configPath); if (configPath && fs.existsSync(configPath)) { - logger.info(`Configuration '${configPath}' found, ignoring other arguments.`); + logger.debug(`Configuration '${configPath}' found, ignoring other arguments.`); this.argv = configUtils.load(configPath); } @@ -197,17 +197,17 @@ ${packageData.name} v${packageData.version} \ runServerAndThenDredd() { if (!this.argv.server) { - logger.verbose('No backend server process specified, starting testing at once'); + logger.debug('No backend server process specified, starting testing at once'); this.runDredd(this.dreddInstance); } else { - logger.verbose('Backend server process specified, starting backend server and then testing'); + logger.debug('Backend server process specified, starting backend server and then testing'); const parsedArgs = spawnArgs(this.argv.server); const command = parsedArgs.shift(); - logger.verbose(`Using '${command}' as a server command, ${JSON.stringify(parsedArgs)} as arguments`); + logger.debug(`Using '${command}' as a server command, ${JSON.stringify(parsedArgs)} as arguments`); this.serverProcess = spawn(command, parsedArgs); - logger.info(`Starting backend server process with command: ${this.argv.server}`); + logger.debug(`Starting backend server process with command: ${this.argv.server}`); this.serverProcess.stdout.setEncoding('utf8'); this.serverProcess.stdout.on('data', data => process.stdout.write(data.toString())); @@ -215,15 +215,15 @@ ${packageData.name} v${packageData.version} \ this.serverProcess.stderr.setEncoding('utf8'); this.serverProcess.stderr.on('data', data => process.stdout.write(data.toString())); - this.serverProcess.on('signalTerm', () => logger.verbose('Gracefully terminating the backend server process')); - this.serverProcess.on('signalKill', () => logger.verbose('Killing the backend server process')); + this.serverProcess.on('signalTerm', () => logger.debug('Gracefully terminating the backend server process')); + this.serverProcess.on('signalKill', () => logger.debug('Killing the backend server process')); this.serverProcess.on('crash', (exitStatus, killed) => { - if (killed) { logger.info('Backend server process was killed'); } + if (killed) { logger.debug('Backend server process was killed'); } }); this.serverProcess.on('exit', () => { - logger.info('Backend server process exited'); + logger.debug('Backend server process exited'); }); this.serverProcess.on('error', (err) => { @@ -234,7 +234,7 @@ ${packageData.name} v${packageData.version} \ // Ensure server is not running when dredd exits prematurely somewhere process.on('beforeExit', () => { if (this.serverProcess && !this.serverProcess.terminated) { - logger.verbose('Killing backend server process before Dredd exits'); + logger.debug('Killing backend server process before Dredd exits'); this.serverProcess.signalKill(); } }); @@ -242,14 +242,14 @@ ${packageData.name} v${packageData.version} \ // Ensure server is not running when dredd exits prematurely somewhere process.on('exit', () => { if (this.serverProcess && !this.serverProcess.terminated) { - logger.verbose('Killing backend server process on Dredd\'s exit'); + logger.debug('Killing backend server process on Dredd\'s exit'); this.serverProcess.signalKill(); } }); const waitSecs = parseInt(this.argv['server-wait'], 10); const waitMilis = waitSecs * 1000; - logger.info(`Waiting ${waitSecs} seconds for backend server process to start`); + logger.debug(`Waiting ${waitSecs} seconds for backend server process to start`); this.wait = setTimeout(() => { this.runDredd(this.dreddInstance); @@ -356,9 +356,9 @@ ${packageData.name} v${packageData.version} \ process.on('SIGINT', this.commandSigInt); } - logger.verbose('Running Dredd instance.'); + logger.debug('Running Dredd instance.'); dreddInstance.run((error, stats) => { - logger.verbose('Dredd instance run finished.'); + logger.debug('Dredd instance run finished.'); this.exitWithStatus(error, stats); }); diff --git a/lib/Dredd.js b/lib/Dredd.js index d7597afee..47b9847f3 100644 --- a/lib/Dredd.js +++ b/lib/Dredd.js @@ -69,7 +69,7 @@ ${proxySettings.join(', ')}. Please read documentation on how Dredd works with proxies: https://dredd.org/en/latest/how-it-works/#using-https-proxy `; - logger.verbose(message); + logger.debug(message); } } @@ -122,27 +122,27 @@ https://dredd.org/en/latest/how-it-works/#using-https-proxy } // Spin that merry-go-round - logger.verbose('Expanding glob patterns.'); + logger.debug('Expanding glob patterns.'); this.expandGlobs((globsErr) => { if (globsErr) { return callback(globsErr, this.stats); } - logger.verbose('Reading API description files.'); + logger.debug('Reading API description files.'); this.loadFiles((loadErr) => { if (loadErr) { return callback(loadErr, this.stats); } - logger.verbose('Parsing API description files and compiling a list of HTTP transactions to test.'); + logger.debug('Parsing API description files and compiling a list of HTTP transactions to test.'); this.compileTransactions((compileErr) => { if (compileErr) { return callback(compileErr, this.stats); } - logger.verbose('Starting reporters and waiting until all of them are ready.'); + logger.debug('Starting reporters and waiting until all of them are ready.'); this.emitStart((emitStartErr) => { if (emitStartErr) { return callback(emitStartErr, this.stats); } - logger.verbose('Starting transaction runner.'); + logger.debug('Starting transaction runner.'); this.startRunner((runnerErr) => { if (runnerErr) { return callback(runnerErr, this.stats); } - logger.verbose('Wrapping up testing.'); + logger.debug('Wrapping up testing.'); this.transactionsComplete(callback); }); }); @@ -196,7 +196,7 @@ API description document (or documents) not found on path: async.eachLimit(this.configuration.files, 6, (fileUrlOrPath, loadCallback) => { const { protocol, host } = url.parse(fileUrlOrPath); if (host && ['http:', 'https:'].includes(protocol)) { - logger.verbose('Downloading remote file:', fileUrlOrPath); + logger.debug('Downloading remote file:', fileUrlOrPath); this.downloadFile(fileUrlOrPath, loadCallback); } else { this.readLocalFile(fileUrlOrPath, loadCallback); @@ -255,7 +255,7 @@ Is the provided path correct? const fileData = this.configuration.data[filename]; if (!fileData.annotations) { fileData.annotations = []; } - logger.verbose('Compiling HTTP transactions from API description file:', filename); + logger.debug('Compiling HTTP transactions from API description file:', filename); dreddTransactions.compile(fileData.raw, filename, (compilationError, compilationResult) => { if (compilationError) { return next(compilationError); } diff --git a/lib/HooksWorkerClient.js b/lib/HooksWorkerClient.js index efedea836..693987a7c 100644 --- a/lib/HooksWorkerClient.js +++ b/lib/HooksWorkerClient.js @@ -29,19 +29,19 @@ class HooksWorkerClient { } start(callback) { - logger.verbose('Looking up hooks handler implementation:', this.language); + logger.debug('Looking up hooks handler implementation:', this.language); this.setCommandAndCheckForExecutables((executablesError) => { if (executablesError) { return callback(executablesError); } - logger.verbose('Starting hooks handler.'); + logger.debug('Starting hooks handler.'); this.spawnHandler((spawnHandlerError) => { if (spawnHandlerError) { return callback(spawnHandlerError); } - logger.verbose('Connecting to hooks handler.'); + logger.debug('Connecting to hooks handler.'); this.connectToHandler((connectHandlerError) => { if (connectHandlerError) { return callback(connectHandlerError); } - logger.verbose('Registering hooks.'); + logger.debug('Registering hooks.'); this.registerHooks((registerHooksError) => { if (registerHooksError) { return callback(registerHooksError); } callback(); @@ -57,7 +57,7 @@ class HooksWorkerClient { } terminateHandler(callback) { - logger.verbose('Terminating hooks handler process, PID', this.handler.pid); + logger.debug('Terminating hooks handler process, PID', this.handler.pid); if (this.handler.terminated) { logger.debug('The hooks handler process has already terminated'); return callback(); @@ -169,7 +169,7 @@ $ go get github.com/snikch/goodman/cmd/goodman this.handlerCommand = parsedArgs.shift(); this.handlerCommandArgs = parsedArgs; - logger.verbose(`Using '${this.handlerCommand}' as a hook handler command, '${this.handlerCommandArgs.join(' ')}' as arguments`); + logger.debug(`Using '${this.handlerCommand}' as a hook handler command, '${this.handlerCommandArgs.join(' ')}' as arguments`); if (!which.which(this.handlerCommand)) { msg = `Hooks handler command not found: ${this.handlerCommand}`; callback(new Error(msg)); @@ -183,14 +183,14 @@ $ go get github.com/snikch/goodman/cmd/goodman const pathGlobs = [].concat(this.runner.hooks.configuration.options.hookfiles); const handlerCommandArgs = this.handlerCommandArgs.concat(pathGlobs); - logger.info(`Spawning '${this.language}' hooks handler process.`); + logger.debug(`Spawning '${this.language}' hooks handler process.`); this.handler = spawn(this.handlerCommand, handlerCommandArgs); - this.handler.stdout.on('data', data => logger.info('Hooks handler stdout:', data.toString())); - this.handler.stderr.on('data', data => logger.info('Hooks handler stderr:', data.toString())); + this.handler.stdout.on('data', data => logger.debug('Hooks handler stdout:', data.toString())); + this.handler.stderr.on('data', data => logger.debug('Hooks handler stderr:', data.toString())); - this.handler.on('signalTerm', () => logger.verbose('Gracefully terminating the hooks handler process')); - this.handler.on('signalKill', () => logger.verbose('Killing the hooks handler process')); + this.handler.on('signalTerm', () => logger.debug('Gracefully terminating the hooks handler process')); + this.handler.on('signalKill', () => logger.debug('Killing the hooks handler process')); this.handler.on('crash', (exitStatus, killed) => { let msg; @@ -236,7 +236,7 @@ $ go get github.com/snikch/goodman/cmd/goodman }; const connectAndSetupClient = () => { - logger.verbose('Starting TCP connection with hooks handler process.'); + logger.debug('Starting TCP connection with hooks handler process.'); if (this.runner.hookHandlerError) { callback(this.runner.hookHandlerError); @@ -245,7 +245,7 @@ $ go get github.com/snikch/goodman/cmd/goodman this.handlerClient = net.connect({ port: this.handlerPort, host: this.handlerHost }); this.handlerClient.on('connect', () => { - logger.info(`Successfully connected to hooks handler. Waiting ${this.afterConnectWait / 1000}s to start testing.`); + logger.debug(`Successfully connected to hooks handler. Waiting ${this.afterConnectWait / 1000}s to start testing.`); this.clientConnected = true; clearTimeout(timeout); setTimeout(callback, this.afterConnectWait); @@ -278,10 +278,10 @@ $ go get github.com/snikch/goodman/cmd/goodman const result = []; for (const message of messages) { if (message.uuid) { - logger.verbose('Dredd received a valid message from hooks handler:', message.uuid); + logger.debug('Dredd received a valid message from hooks handler:', message.uuid); result.push(this.emitter.emit(message.uuid, message)); } else { - result.push(logger.verbose('UUID not present in hooks handler message, ignoring:', JSON.stringify(message, null, 2))); + result.push(logger.debug('UUID not present in hooks handler message, ignoring:', JSON.stringify(message, null, 2))); } } return result; @@ -312,14 +312,14 @@ $ go get github.com/snikch/goodman/cmd/goodman data, }; - logger.verbose('Sending HTTP transaction data to hooks handler:', uuid); + logger.debug('Sending HTTP transaction data to hooks handler:', uuid); this.handlerClient.write(JSON.stringify(message)); this.handlerClient.write(this.handlerMessageDelimiter); // Register event for the sent transaction function messageHandler(receivedMessage) { let value; - logger.verbose('Handling hook:', uuid); + logger.debug('Handling hook:', uuid); clearTimeout(timeout); // We are directly modifying the `data` argument here. Neither direct diff --git a/lib/TransactionRunner.js b/lib/TransactionRunner.js index fac47f174..eda5306b1 100644 --- a/lib/TransactionRunner.js +++ b/lib/TransactionRunner.js @@ -7,6 +7,7 @@ const url = require('url'); const addHooks = require('./addHooks'); const logger = require('./logger'); +const reporterOutputLogger = require('./reporters/reporterOutputLogger'); const packageData = require('../package.json'); const sortTransactions = require('./sortTransactions'); const performRequest = require('./performRequest'); @@ -41,18 +42,18 @@ class TransactionRunner { } run(transactions, callback) { - logger.verbose('Sorting HTTP transactions'); + logger.debug('Sorting HTTP transactions'); transactions = this.configuration.options.sorted ? sortTransactions(transactions) : transactions; - logger.verbose('Configuring HTTP transactions'); + logger.debug('Configuring HTTP transactions'); transactions = transactions.map(this.configureTransaction.bind(this)); // Remainings of functional approach, probs to be eradicated - logger.verbose('Reading hook files and registering hooks'); + logger.debug('Reading hook files and registering hooks'); addHooks(this, transactions, (addHooksError) => { if (addHooksError) { return callback(addHooksError); } - logger.verbose('Executing HTTP transactions'); + logger.debug('Executing HTTP transactions'); this.executeAllTransactions(transactions, this.hooks, callback); }); } @@ -74,7 +75,7 @@ class TransactionRunner { if (this.hookHandlerError) { return callback(this.hookHandlerError); } - logger.verbose('Running \'beforeAll\' hooks'); + logger.debug('Running \'beforeAll\' hooks'); this.runHooksForData(hooks.beforeAllHooks, transactions, () => { if (this.hookHandlerError) { return callback(this.hookHandlerError); } @@ -84,13 +85,13 @@ class TransactionRunner { // we need to work with indexes (keys) here, no other way of access. return async.timesSeries(transactions.length, (transactionIndex, iterationCallback) => { transaction = transactions[transactionIndex]; - logger.verbose(`Processing transaction #${transactionIndex + 1}:`, transaction.name); + logger.debug(`Processing transaction #${transactionIndex + 1}:`, transaction.name); - logger.verbose('Running \'beforeEach\' hooks'); + logger.debug('Running \'beforeEach\' hooks'); this.runHooksForData(hooks.beforeEachHooks, transaction, () => { if (this.hookHandlerError) { return iterationCallback(this.hookHandlerError); } - logger.verbose('Running \'before\' hooks'); + logger.debug('Running \'before\' hooks'); this.runHooksForData(hooks.beforeHooks[transaction.name], transaction, () => { if (this.hookHandlerError) { return iterationCallback(this.hookHandlerError); } @@ -104,11 +105,11 @@ class TransactionRunner { this.executeTransaction(transaction, hooks, () => { if (this.hookHandlerError) { return iterationCallback(this.hookHandlerError); } - logger.verbose('Running \'afterEach\' hooks'); + logger.debug('Running \'afterEach\' hooks'); this.runHooksForData(hooks.afterEachHooks, transaction, () => { if (this.hookHandlerError) { return iterationCallback(this.hookHandlerError); } - logger.verbose('Running \'after\' hooks'); + logger.debug('Running \'after\' hooks'); this.runHooksForData(hooks.afterHooks[transaction.name], transaction, () => { if (this.hookHandlerError) { return iterationCallback(this.hookHandlerError); } @@ -123,7 +124,7 @@ class TransactionRunner { (iterationError) => { if (iterationError) { return callback(iterationError); } - logger.verbose('Running \'afterAll\' hooks'); + logger.debug('Running \'afterAll\' hooks'); this.runHooksForData(hooks.afterAllHooks, transactions, () => { if (this.hookHandlerError) { return callback(this.hookHandlerError); } callback(); @@ -424,27 +425,27 @@ class TransactionRunner { this.ensureTransactionResultsGeneralSection(transaction); if (transaction.skip) { - logger.verbose('HTTP transaction was marked in hooks as to be skipped. Skipping'); + logger.debug('HTTP transaction was marked in hooks as to be skipped. Skipping'); transaction.test = test; this.skipTransaction(transaction, 'Skipped in before hook'); return callback(); } if (transaction.fail) { - logger.verbose('HTTP transaction was marked in hooks as to be failed. Reporting as failed'); + logger.debug('HTTP transaction was marked in hooks as to be failed. Reporting as failed'); transaction.test = test; this.failTransaction(transaction, `Failed in before hook: ${transaction.fail}`); return callback(); } if (this.configuration.options['dry-run']) { - logger.info('Dry run. Not performing HTTP request'); + reporterOutputLogger.info('Dry run. Not performing HTTP request'); transaction.test = test; this.skipTransaction(transaction); return callback(); } if (this.configuration.options.names) { - logger.info(transaction.name); + reporterOutputLogger.info(transaction.name); transaction.test = test; this.skipTransaction(transaction); return callback(); } if ((this.configuration.options.method.length > 0) && !(Array.from(this.configuration.options.method).includes(transaction.request.method))) { - logger.info(`\ + logger.debug(`\ Only ${(Array.from(this.configuration.options.method).map(m => m.toUpperCase())).join(', ')}\ requests are set to be executed. \ Not performing HTTP ${transaction.request.method.toUpperCase()} request.\ @@ -453,7 +454,7 @@ Not performing HTTP ${transaction.request.method.toUpperCase()} request.\ this.skipTransaction(transaction); return callback(); } if ((this.configuration.options.only.length > 0) && !(Array.from(this.configuration.options.only).includes(transaction.name))) { - logger.info(`\ + logger.debug(`\ Only '${this.configuration.options.only}' transaction is set to be executed. \ Not performing HTTP request for '${transaction.name}'.\ `); @@ -492,11 +493,11 @@ Not performing HTTP request for '${transaction.name}'.\ transaction.real.body = ''; } - logger.verbose('Running \'beforeEachValidation\' hooks'); + logger.debug('Running \'beforeEachValidation\' hooks'); this.runHooksForData(hooks && hooks.beforeEachValidationHooks, transaction, () => { if (this.hookHandlerError) { return callback(this.hookHandlerError); } - logger.verbose('Running \'beforeValidation\' hooks'); + logger.debug('Running \'beforeValidation\' hooks'); this.runHooksForData(hooks && hooks.beforeValidationHooks[transaction.name], transaction, () => { if (this.hookHandlerError) { return callback(this.hookHandlerError); } @@ -507,7 +508,7 @@ Not performing HTTP request for '${transaction.name}'.\ } validateTransaction(test, transaction, callback) { - logger.verbose('Validating HTTP transaction by Gavel.js'); + logger.debug('Validating HTTP transaction by Gavel.js'); logger.debug('Determining whether HTTP transaction is valid (getting boolean verdict)'); gavel.isValid(transaction.real, transaction.expected, 'response', (isValidError, isValid) => { if (isValidError) { diff --git a/lib/addHooks.js b/lib/addHooks.js index b5a5ac397..b379c26f4 100644 --- a/lib/addHooks.js +++ b/lib/addHooks.js @@ -42,7 +42,7 @@ Stack: ${error.stack} } catch (err) { return callback(err); } - logger.info('Found Hookfiles:', files); + logger.debug('Found Hookfiles:', files); // Clone the configuration object to hooks.configuration to make it // accessible in the node.js hooks API diff --git a/lib/configuration.js b/lib/configuration.js index 122fae420..3bf0f3e40 100644 --- a/lib/configuration.js +++ b/lib/configuration.js @@ -47,7 +47,7 @@ function applyLoggingOptions(options) { logger.transports.console.level = 'warn'; } - reporterOutputLogger.transports.console.level = 'test'; + reporterOutputLogger.transports.console.level = 'info'; return options; } diff --git a/lib/configureReporters.js b/lib/configureReporters.js index 60e548056..331284de3 100644 --- a/lib/configureReporters.js +++ b/lib/configureReporters.js @@ -29,7 +29,7 @@ function configureReporters(config, stats, tests, runner) { const reporters = config.options.reporter; const outputs = config.options.output; - logger.verbose('Configuring reporters:', reporters, outputs); + logger.debug('Configuring reporters:', reporters, outputs); function addCli(reportersArr) { if (reportersArr.length > 0) { diff --git a/lib/logger.js b/lib/logger.js index 3792d6f65..97e27005d 100644 --- a/lib/logger.js +++ b/lib/logger.js @@ -5,18 +5,12 @@ module.exports = new (winston.Logger)({ new (winston.transports.Console)({ colorize: true }), ], levels: { - debug: 5, - silly: 4, - verbose: 3, - info: 2, + debug: 2, warn: 1, error: 0, }, colors: { debug: 'cyan', - silly: 'gray', - verbose: 'magenta', - info: 'blue', warn: 'yellow', error: 'red', }, diff --git a/lib/reporters/ApiaryReporter.js b/lib/reporters/ApiaryReporter.js index 6d7c2e4c5..3803d9a0c 100644 --- a/lib/reporters/ApiaryReporter.js +++ b/lib/reporters/ApiaryReporter.js @@ -40,7 +40,7 @@ function ApiaryReporter(emitter, stats, tests, config, runner) { this.configureEmitter(emitter); - logger.verbose(`Using '${this.type}' reporter.`); + logger.debug(`Using '${this.type}' reporter.`); if (!this.configuration.apiToken && !this.configuration.apiSuite) { logger.warn(` @@ -218,7 +218,7 @@ ApiaryReporter.prototype._performRequestAsync = function _performRequestAsync(pa return callback(err); } - logger.verbose('Handling HTTP response from Apiary API'); + logger.debug('Handling HTTP response from Apiary API'); try { parsedBody = JSON.parse(resBody); @@ -257,7 +257,7 @@ ${error.message}\n${resBody} try { const protocol = options.uri.split(':')[0].toUpperCase(); - logger.verbose(` + logger.debug(` About to perform an ${protocol} request from Apiary reporter to Apiary API: ${options.method} ${options.uri} \ (${body ? 'with' : 'without'} body) diff --git a/lib/reporters/BaseReporter.js b/lib/reporters/BaseReporter.js index dca540eef..136c94a23 100644 --- a/lib/reporters/BaseReporter.js +++ b/lib/reporters/BaseReporter.js @@ -5,7 +5,7 @@ function BaseReporter(emitter, stats, tests) { this.stats = stats; this.tests = tests; this.configureEmitter(emitter); - logger.verbose(`Using '${this.type}' reporter.`); + logger.debug(`Using '${this.type}' reporter.`); } BaseReporter.prototype.configureEmitter = function configureEmitter(emitter) { diff --git a/lib/reporters/CLIReporter.js b/lib/reporters/CLIReporter.js index 517a90a61..039c7ddd7 100644 --- a/lib/reporters/CLIReporter.js +++ b/lib/reporters/CLIReporter.js @@ -24,19 +24,19 @@ function CLIReporter(emitter, stats, tests, inlineErrors, details) { this.configureEmitter(emitter); - logger.verbose(`Using '${this.type}' reporter.`); + logger.debug(`Using '${this.type}' reporter.`); } CLIReporter.prototype.configureEmitter = function configureEmitter(emitter) { emitter.on('start', (rawBlueprint, callback) => { - logger.info('Beginning Dredd testing...'); + logger.debug('Beginning Dredd testing...'); callback(); }); emitter.on('end', (callback) => { if (!this.inlineErrors) { if (this.errors.length) { - logger.info('Displaying failed tests...'); + reporterOutputLogger.info('Displaying failed tests...'); } this.errors.forEach((test) => { reporterOutputLogger.fail(`${test.title} duration: ${test.duration}ms`); diff --git a/lib/reporters/DotReporter.js b/lib/reporters/DotReporter.js index 9d00289ff..9949e3561 100644 --- a/lib/reporters/DotReporter.js +++ b/lib/reporters/DotReporter.js @@ -10,12 +10,12 @@ function DotReporter(emitter, stats, tests) { this.configureEmitter(emitter); - logger.verbose(`Using '${this.type}' reporter.`); + logger.debug(`Using '${this.type}' reporter.`); } DotReporter.prototype.configureEmitter = function configureEmitter(emitter) { emitter.on('start', (rawBlueprint, callback) => { - logger.info('Beginning Dredd testing...'); + logger.debug('Beginning Dredd testing...'); callback(); }); @@ -23,7 +23,7 @@ DotReporter.prototype.configureEmitter = function configureEmitter(emitter) { if (this.stats.tests > 0) { if (this.errors.length > 0) { this.write('\n'); - logger.info('Displaying failed tests...'); + reporterOutputLogger.info('Displaying failed tests...'); for (const test of this.errors) { reporterOutputLogger.fail(`${test.title} duration: ${test.duration}ms`); reporterOutputLogger.fail(test.message); diff --git a/lib/reporters/HTMLReporter.js b/lib/reporters/HTMLReporter.js index 9f80bd7ac..5d1cb4313 100644 --- a/lib/reporters/HTMLReporter.js +++ b/lib/reporters/HTMLReporter.js @@ -24,7 +24,7 @@ function HTMLReporter(emitter, stats, tests, path, details) { this.configureEmitter(emitter); - logger.verbose(`Using '${this.type}' reporter.`); + logger.debug(`Using '${this.type}' reporter.`); } HTMLReporter.prototype.sanitizedPath = function sanitizedPath(path = './report.html') { diff --git a/lib/reporters/MarkdownReporter.js b/lib/reporters/MarkdownReporter.js index d7bab1514..3b54d43c2 100644 --- a/lib/reporters/MarkdownReporter.js +++ b/lib/reporters/MarkdownReporter.js @@ -23,7 +23,7 @@ function MarkdownReporter(emitter, stats, tests, path, details) { this.configureEmitter(emitter); - logger.verbose(`Using '${this.type}' reporter.`); + logger.debug(`Using '${this.type}' reporter.`); } MarkdownReporter.prototype.sanitizedPath = function sanitizedPath(path = './report.md') { diff --git a/lib/reporters/NyanReporter.js b/lib/reporters/NyanReporter.js index 5f0f099f6..332e89604 100644 --- a/lib/reporters/NyanReporter.js +++ b/lib/reporters/NyanReporter.js @@ -34,7 +34,7 @@ function NyanCatReporter(emitter, stats, tests) { this.configureEmitter(emitter); - logger.verbose(`Using '${this.type}' reporter.`); + logger.debug(`Using '${this.type}' reporter.`); } NyanCatReporter.prototype.configureEmitter = function configureEmitter(emitter) { @@ -55,7 +55,7 @@ NyanCatReporter.prototype.configureEmitter = function configureEmitter(emitter) if (this.errors.length > 0) { this.write('\n'); - logger.info('Displaying failed tests...'); + reporterOutputLogger.info('Displaying failed tests...'); for (const test of this.errors) { reporterOutputLogger.fail(`${test.title} duration: ${test.duration}ms`); reporterOutputLogger.fail(test.message); diff --git a/lib/reporters/XUnitReporter.js b/lib/reporters/XUnitReporter.js index b5db268fe..5c262af58 100644 --- a/lib/reporters/XUnitReporter.js +++ b/lib/reporters/XUnitReporter.js @@ -22,7 +22,7 @@ function XUnitReporter(emitter, stats, tests, path, details) { this.configureEmitter(emitter); - logger.verbose(`Using '${this.type}' reporter.`); + logger.debug(`Using '${this.type}' reporter.`); } XUnitReporter.prototype.updateSuiteStats = function updateSuiteStats(path, stats, callback) { diff --git a/lib/reporters/reporterOutputLogger.js b/lib/reporters/reporterOutputLogger.js index 5722bb50f..0cd806d75 100644 --- a/lib/reporters/reporterOutputLogger.js +++ b/lib/reporters/reporterOutputLogger.js @@ -5,6 +5,7 @@ module.exports = new (winston.Logger)({ new (winston.transports.Console)({ colorize: true }), ], levels: { + info: 10, test: 9, pass: 8, fail: 7, @@ -17,6 +18,7 @@ module.exports = new (winston.Logger)({ error: 0, }, colors: { + info: 'blue', test: 'yellow', pass: 'green', fail: 'red', diff --git a/test/integration/cli/configuration-cli-test.js b/test/integration/cli/configuration-cli-test.js index 760028895..72022e01a 100644 --- a/test/integration/cli/configuration-cli-test.js +++ b/test/integration/cli/configuration-cli-test.js @@ -12,7 +12,6 @@ const PORT = 9876; let exitStatus; let stderr = ''; -let stdout = ''; const addHooksStub = proxyquire('../../../lib/addHooks', { './logger': loggerStub, @@ -36,7 +35,6 @@ const CLIStub = proxyquire('../../../lib/CLI', { }); function execCommand(custom = {}, cb) { - stdout = ''; stderr = ''; exitStatus = null; let finished = false; @@ -44,7 +42,7 @@ function execCommand(custom = {}, cb) { if (!finished) { finished = true; exitStatus = (exitStatusCode != null ? exitStatusCode : 0); - cb(null, stdout, stderr, (exitStatusCode != null ? exitStatusCode : 0)); + cb(); } })); @@ -53,20 +51,15 @@ function execCommand(custom = {}, cb) { describe('CLI class Integration', () => { before(() => { - ['warn', 'error'].forEach((method) => { + ['warn', 'error', 'debug'].forEach((method) => { sinon .stub(loggerStub, method) .callsFake((chunk) => { stderr += `\n${method}: ${chunk}`; }); }); - ['log', 'info', 'silly', 'verbose', 'debug'].forEach((method) => { - sinon - .stub(loggerStub, method) - .callsFake((chunk) => { stdout += `\n${method}: ${chunk}`; }); - }); }); after(() => { - ['warn', 'error', 'log', 'info', 'silly', 'verbose', 'debug'].forEach((method) => { + ['warn', 'error', 'debug'].forEach((method) => { loggerStub[method].restore(); }); }); @@ -74,7 +67,7 @@ describe('CLI class Integration', () => { describe('When using configuration file', () => { describe('When specifying custom configuration file by --config', () => { const configPath = '../../../custom-dredd-config-path.yaml'; - const cmd = { argv: ['--config', configPath] }; + const cmd = { argv: ['--config', configPath, '--loglevel=debug'] }; const options = { _: ['api-description.apib', 'http://127.0.0.1'] }; let fsExistsSync; @@ -92,12 +85,12 @@ describe('CLI class Integration', () => { it('should call fs.existsSync with given path', () => assert.isTrue(fsExistsSync.calledWith(configPath))); it('should call configUtils.load with given path', () => assert.isTrue(configUtilsLoad.calledWith(configPath))); - it('should print message about using given configuration file', () => assert.include(stdout, `info: Configuration '${configPath}' found`)); + it('should print message about using given configuration file', () => assert.include(stderr, `debug: Configuration '${configPath}' found`)); }); describe('When dredd.yml exists', () => { const configPath = './dredd.yml'; - const cmd = { argv: [] }; + const cmd = { argv: ['--loglevel=debug'] }; const options = { _: ['api-description.apib', 'http://127.0.0.1'] }; let fsExistsSync; @@ -115,12 +108,12 @@ describe('CLI class Integration', () => { it('should call fs.existsSync with dredd.yml', () => assert.isTrue(fsExistsSync.calledWith(configPath))); it('should call configUtils.load with dredd.yml', () => assert.isTrue(configUtilsLoad.calledWith(configPath))); - it('should print message about using dredd.yml', () => assert.include(stdout, `info: Configuration '${configPath}' found`)); + it('should print message about using dredd.yml', () => assert.include(stderr, `debug: Configuration '${configPath}' found`)); }); describe('When dredd.yml does not exist', () => { const configPath = './dredd.yml'; - const cmd = { argv: [] }; + const cmd = { argv: ['--loglevel=debug'] }; let fsExistsSync; let configUtilsLoad; @@ -137,7 +130,7 @@ describe('CLI class Integration', () => { it('should call fs.existsSync with dredd.yml', () => assert.isTrue(fsExistsSync.calledWith(configPath))); it('should never call configUtils.load', () => assert.isFalse(configUtilsLoad.called)); - it('should not print message about using configuration file', () => assert.notInclude(stdout, 'info: Configuration')); + it('should not print message about using configuration file', () => assert.notInclude(stderr, 'debug: Configuration')); }); }); @@ -187,7 +180,7 @@ describe('CLI class Integration', () => { })); describe('and I try to load a file from bad hostname at all', () => { - before(done => execCommand(errorCmd, () => done())); + before(done => execCommand(errorCmd, done)); it('should exit with status 1', () => assert.equal(exitStatus, 1)); @@ -199,7 +192,7 @@ describe('CLI class Integration', () => { }); describe('and I try to load a file that does not exist from an existing server', () => { - before(done => execCommand(wrongCmd, () => done())); + before(done => execCommand(wrongCmd, done)); it('should exit with status 1', () => assert.equal(exitStatus, 1)); @@ -211,7 +204,7 @@ describe('CLI class Integration', () => { }); describe('and I try to load a file that actually is there', () => { - before(done => execCommand(goodCmd, () => done())); + before(done => execCommand(goodCmd, done)); it('should exit with status 0', () => assert.equal(exitStatus, 0)); }); diff --git a/test/integration/cli/hookfiles-cli-test.js b/test/integration/cli/hookfiles-cli-test.js index 2e469c3a5..4126357e6 100644 --- a/test/integration/cli/hookfiles-cli-test.js +++ b/test/integration/cli/hookfiles-cli-test.js @@ -552,8 +552,8 @@ describe('CLI', () => { }); }); - it('should not display anything', () => { - assert.notInclude(runtimeInfo.dredd.output, 'info:'); + it('should not display any debug logging', () => { + assert.notInclude(runtimeInfo.dredd.output, 'debug:'); }); }); diff --git a/test/integration/cli/server-process-cli-test.js b/test/integration/cli/server-process-cli-test.js index a26047993..b3286f72b 100644 --- a/test/integration/cli/server-process-cli-test.js +++ b/test/integration/cli/server-process-cli-test.js @@ -77,7 +77,7 @@ describe('CLI - Server Process', () => { done(err); })); - it('should inform about starting server with custom command', () => assert.include(cliInfo.stdout, 'Starting backend server process with command')); + it('should inform about starting server with custom command', () => assert.include(cliInfo.stderr, 'Starting backend server process with command')); it('should redirect server\'s welcome message', () => assert.include(cliInfo.stdout, `Dummy server listening on port ${DEFAULT_SERVER_PORT}`)); it('should exit with status 0', () => assert.equal(cliInfo.exitStatus, 0)); }); @@ -97,7 +97,7 @@ describe('CLI - Server Process', () => { done(err); })); - it('should inform about starting server with custom command', () => assert.include(cliInfo.stdout, 'Starting backend server process with command')); + it('should inform about starting server with custom command', () => assert.include(cliInfo.stderr, 'Starting backend server process with command')); it('should report problem with server process spawn', () => assert.include(cliInfo.stderr, 'Command to start backend server process failed, exiting Dredd')); it('should exit with status 1', () => assert.equal(cliInfo.exitStatus, 1)); }); @@ -142,7 +142,7 @@ describe('CLI - Server Process', () => { done(err); })); - it('should inform about starting server with custom command', () => assert.include(cliInfo.stdout, 'Starting backend server process with command')); + it('should inform about starting server with custom command', () => assert.include(cliInfo.stderr, 'Starting backend server process with command')); if (scenario.expectServerBoot) { it('should redirect server\'s boot message', () => assert.include(cliInfo.stdout, `Dummy server listening on port ${DEFAULT_SERVER_PORT}`)); } @@ -170,10 +170,10 @@ describe('CLI - Server Process', () => { done(err); })); - it('should inform about starting server with custom command', () => assert.include(cliInfo.stdout, 'Starting backend server process with command')); - it('should inform about gracefully terminating the server', () => assert.include(cliInfo.stdout, 'Gracefully terminating the backend server process')); + it('should inform about starting server with custom command', () => assert.include(cliInfo.stderr, 'Starting backend server process with command')); + it('should inform about gracefully terminating the server', () => assert.include(cliInfo.stderr, 'Gracefully terminating the backend server process')); it('should redirect server\'s message about ignoring termination', () => assert.include(cliInfo.stdout, 'ignoring termination')); - it('should inform about forcefully killing the server', () => assert.include(cliInfo.stdout, 'Killing the backend server process')); + it('should inform about forcefully killing the server', () => assert.include(cliInfo.stderr, 'Killing the backend server process')); it('the server should not be running', done => isProcessRunning('test/fixtures/scripts/', (err, isRunning) => { if (!err) { assert.isFalse(isRunning); } done(err); diff --git a/test/integration/dredd-test.js b/test/integration/dredd-test.js index eaf64dc29..f5b6e702c 100644 --- a/test/integration/dredd-test.js +++ b/test/integration/dredd-test.js @@ -98,7 +98,7 @@ describe('Dredd class Integration', () => { }); - describe("when using reporter -r apiary with 'verbose' logging with custom apiaryApiKey and apiaryApiName", () => { + describe("when using reporter -r apiary with 'debug' logging with custom apiaryApiKey and apiaryApiName", () => { let server; let server2; let receivedRequest; diff --git a/test/unit/CLI-test.js b/test/unit/CLI-test.js index 773033432..ba687fe3c 100644 --- a/test/unit/CLI-test.js +++ b/test/unit/CLI-test.js @@ -56,27 +56,25 @@ function execCommand(custom = {}, cb) { if (!finished) { finished = true; exitStatus = code || 0; - return cb(null, stdout, stderr, (code != null ? code : 0)); + return cb(); } })).run()); } describe('CLI class', () => { before(() => { - ['warn', 'error'].forEach((method) => { + ['warn', 'error', 'debug'].forEach((method) => { sinon .stub(loggerStub, method) .callsFake((chunk) => { stderr += `\n${method}: ${chunk}`; }); }); - ['log', 'info', 'silly', 'verbose', 'debug'].forEach((method) => { - sinon - .stub(loggerStub, method) - .callsFake((chunk) => { stdout += `\n${method}: ${chunk}`; }); - }); + sinon + .stub(loggerStub, 'log') + .callsFake((chunk) => { stdout += chunk; }); }); after(() => { - ['warn', 'error', 'log', 'info', 'silly', 'verbose', 'debug'].forEach((method) => { + ['warn', 'error', 'debug', 'log'].forEach((method) => { loggerStub[method].restore(); }); }); @@ -247,7 +245,7 @@ describe('CLI class', () => { describe('when called w/ OR wo/ exiting arguments', () => { describe('--help', () => { - before(done => execCommand({ argv: ['--help'] }, () => done())); + before(done => execCommand({ argv: ['--help'] }, done)); it('prints out some really nice help text with all options descriptions', () => { assert.include(stderr, 'Usage:'); @@ -258,15 +256,17 @@ describe('CLI class', () => { }); describe('--version', () => { - before(done => execCommand({ argv: ['--version'] }, () => done())); + before(done => execCommand({ argv: ['--version'] }, done)); - it('prints out version', () => assert.include(stdout, `${packageData.name} v${packageData.version}`)); + it('prints out version', () => { + assert.include(stdout, `${packageData.name} v${packageData.version}`); + }); }); describe('init', () => { before((done) => { sinon.stub(configUtilsStub, 'save'); - execCommand({ argv: ['init'] }, () => done()); + execCommand({ argv: ['init'] }, done); }); after(() => { @@ -278,7 +278,7 @@ describe('CLI class', () => { }); describe('without argv', () => { - before(done => execCommand({ argv: [] }, () => done())); + before(done => execCommand({ argv: [] }, done)); it('prints out an error message', () => assert.include(stderr, 'Error: Must specify')); }); @@ -332,7 +332,7 @@ describe('CLI class', () => { $0: 'node ./bin/dredd', })); - execCommand({ argv: ['--names'] }, () => done()); + execCommand({ argv: ['--names'] }, done); }); after(() => { @@ -367,7 +367,7 @@ describe('CLI class', () => { '--server', 'foo/bar', ], - }, () => done()); + }, done); }); after(() => crossSpawnStub.spawn.restore()); diff --git a/test/unit/Dredd-test.js b/test/unit/Dredd-test.js index c782958dc..bb063d3be 100644 --- a/test/unit/Dredd-test.js +++ b/test/unit/Dredd-test.js @@ -664,10 +664,10 @@ GET /url }); describe('#logProxySettings', () => { - let verboseLogger; + let debugLogger; - beforeEach(() => { verboseLogger = sinon.spy(loggerStub, 'verbose'); }); - afterEach(() => loggerStub.verbose.restore()); + beforeEach(() => { debugLogger = sinon.spy(loggerStub, 'debug'); }); + afterEach(() => loggerStub.debug.restore()); describe('when the proxy is set by lowercase environment variable', () => { beforeEach(() => { @@ -676,7 +676,7 @@ GET /url }); afterEach(() => delete process.env.http_proxy); - it('logs about the setting', () => assert.include(verboseLogger.lastCall.args[0], + it('logs about the setting', () => assert.include(debugLogger.lastCall.args[0], 'HTTP(S) proxy specified by environment variables: http_proxy=http://proxy.example.com')); }); @@ -687,7 +687,7 @@ GET /url }); afterEach(() => delete process.env.HTTPS_PROXY); - it('logs about the setting', () => assert.include(verboseLogger.lastCall.args[0], + it('logs about the setting', () => assert.include(debugLogger.lastCall.args[0], 'HTTP(S) proxy specified by environment variables: ' + 'HTTPS_PROXY=http://proxy.example.com')); }); @@ -703,7 +703,7 @@ GET /url delete process.env.NO_PROXY; }); - it('logs about the setting', () => assert.include(verboseLogger.lastCall.args[0], + it('logs about the setting', () => assert.include(debugLogger.lastCall.args[0], 'HTTP(S) proxy specified by environment variables: ' + 'HTTPS_PROXY=http://proxy.example.com, ' + 'NO_PROXY=whitelisted.example.com')); @@ -720,7 +720,7 @@ GET /url delete process.env.NO_PROXY; }); - it('is ignored', () => assert.include(verboseLogger.lastCall.args[0], + it('is ignored', () => assert.include(debugLogger.lastCall.args[0], 'HTTP(S) proxy specified by environment variables: ' + 'NO_PROXY=whitelisted.example.com')); }); diff --git a/test/unit/HooksWorkerClient-test.js b/test/unit/HooksWorkerClient-test.js index fafc8ad98..f56057d6b 100644 --- a/test/unit/HooksWorkerClient-test.js +++ b/test/unit/HooksWorkerClient-test.js @@ -25,7 +25,7 @@ const MIN_COMMAND_EXECUTION_DURATION_MS = 2 * measureExecutionDurationMs(() => c const PORT = 61321; let runner; -const logLevels = ['error', 'log', 'info', 'warn']; +const logLevels = ['error', 'warn', 'debug']; const HooksWorkerClient = proxyquire('../../lib/HooksWorkerClient', { 'cross-spawn': crossSpawnStub, diff --git a/test/unit/reporters/ApiaryReporter-test.js b/test/unit/reporters/ApiaryReporter-test.js index e7204dab0..78a10eec3 100644 --- a/test/unit/reporters/ApiaryReporter-test.js +++ b/test/unit/reporters/ApiaryReporter-test.js @@ -20,12 +20,12 @@ nock.enableNetConnect(); describe('ApiaryReporter', () => { let env = {}; beforeEach(() => { - sinon.stub(loggerStub, 'verbose'); + sinon.stub(loggerStub, 'debug'); sinon.stub(reporterOutputLoggerStub, 'complete'); }); afterEach(() => { - sinon.stub(loggerStub.verbose.restore()); + sinon.stub(loggerStub.debug.restore()); sinon.stub(reporterOutputLoggerStub.complete.restore()); }); @@ -199,7 +199,7 @@ describe('ApiaryReporter', () => { emitter = new EventEmitter(); const apiaryReporter = new ApiaryReporter(emitter, {}, {}, { custom }); apiaryReporter._performRequestAsync('/', 'POST', '', () => { - assert.isOk(loggerStub.verbose.calledWithMatch('POST https://api.example.com:1234/ (without body)')); + assert.isOk(loggerStub.debug.calledWithMatch('POST https://api.example.com:1234/ (without body)')); done(); }); }); @@ -215,7 +215,7 @@ describe('ApiaryReporter', () => { emitter = new EventEmitter(); const apiaryReporter = new ApiaryReporter(emitter, {}, {}, { custom }); apiaryReporter._performRequestAsync('/', 'POST', '', () => { - assert.isOk(loggerStub.verbose.calledWithMatch('POST https://api.example.com:1234/ (without body)')); + assert.isOk(loggerStub.debug.calledWithMatch('POST https://api.example.com:1234/ (without body)')); done(); }); })); @@ -224,7 +224,7 @@ describe('ApiaryReporter', () => { emitter = new EventEmitter(); const apiaryReporter = new ApiaryReporter(emitter, {}, {}, { custom }); apiaryReporter._performRequestAsync('/hello?q=1', 'POST', '', () => { - assert.isOk(loggerStub.verbose.calledWithMatch('POST https://api.example.com:1234/hello?q=1 (without body)')); + assert.isOk(loggerStub.debug.calledWithMatch('POST https://api.example.com:1234/hello?q=1 (without body)')); done(); }); })); diff --git a/test/unit/reporters/CLIReporter-test.js b/test/unit/reporters/CLIReporter-test.js index fef1c8ddc..8c253c747 100644 --- a/test/unit/reporters/CLIReporter-test.js +++ b/test/unit/reporters/CLIReporter-test.js @@ -25,15 +25,16 @@ describe('CLIReporter', () => { }); describe('when starting', () => { - beforeEach(() => sinon.spy(loggerStub, 'info')); + beforeEach(() => sinon.spy(loggerStub, 'debug')); - afterEach(() => loggerStub.info.restore()); + afterEach(() => loggerStub.debug.restore()); it('should write starting to the console', (done) => { const emitter = new EventEmitter(); (new CLIReporter(emitter, {}, {}, true)); + loggerStub.debug.resetHistory(); emitter.emit('start', '', () => { - assert.isOk(loggerStub.info.calledOnce); + assert.isOk(loggerStub.debug.calledOnce); done(); }); }); diff --git a/test/unit/reporters/DotReporter-test.js b/test/unit/reporters/DotReporter-test.js index d90acb418..0a46ce458 100644 --- a/test/unit/reporters/DotReporter-test.js +++ b/test/unit/reporters/DotReporter-test.js @@ -39,11 +39,11 @@ describe('DotReporter', () => { }); describe('when starting', () => { - beforeEach(() => sinon.spy(loggerStub, 'info')); + beforeEach(() => sinon.spy(loggerStub, 'debug')); - afterEach(() => loggerStub.info.restore()); + afterEach(() => loggerStub.debug.restore()); - it('should log that testing has begun', () => emitter.emit('start', '', () => assert.isOk(loggerStub.info.called))); + it('should log that testing has begun', () => emitter.emit('start', '', () => assert.isOk(loggerStub.debug.called))); }); describe('when ending', () => { diff --git a/test/unit/transactionRunner-test.js b/test/unit/transactionRunner-test.js index 928162590..02f975198 100644 --- a/test/unit/transactionRunner-test.js +++ b/test/unit/transactionRunner-test.js @@ -409,18 +409,18 @@ describe('TransactionRunner', () => { describe('when printing the names', () => { beforeEach(() => { - sinon.spy(loggerStub, 'info'); + sinon.spy(loggerStub, 'debug'); configuration.options.names = true; runner = new Runner(configuration); }); afterEach(() => { - loggerStub.info.restore(); + loggerStub.debug.restore(); configuration.options.names = false; }); it('should print the names and return', done => runner.executeTransaction(transaction, () => { - assert.isOk(loggerStub.info.called); + assert.isOk(loggerStub.debug.called); done(); })); }); @@ -1268,75 +1268,75 @@ describe('TransactionRunner', () => { describe('with hooks', () => { beforeEach(() => { - sinon.spy(loggerStub, 'info'); + sinon.spy(loggerStub, 'debug'); runner.hooks.beforeHooks = { 'Group Machine > Machine > Delete Message > Bogus example name': [ // eslint-disable-next-line - transaction => loggerStub.info('before') + transaction => loggerStub.debug('before') ], }; runner.hooks.beforeValidationHooks = { 'Group Machine > Machine > Delete Message > Bogus example name': [ // eslint-disable-next-line - transaction => loggerStub.info('beforeValidation') + transaction => loggerStub.debug('beforeValidation') ], }; runner.hooks.afterHooks = { 'Group Machine > Machine > Delete Message > Bogus example name': [ // eslint-disable-next-line function (transaction, done) { - loggerStub.info('after'); + loggerStub.debug('after'); done(); }, ], }; }); - afterEach(() => loggerStub.info.restore()); + afterEach(() => loggerStub.debug.restore()); it('should run the hooks', done => runner.executeAllTransactions([transaction], runner.hooks, () => { - assert.isOk(loggerStub.info.calledWith('before')); - assert.isOk(loggerStub.info.calledWith('beforeValidation')); - assert.isOk(loggerStub.info.calledWith('after')); + assert.isOk(loggerStub.debug.calledWith('before')); + assert.isOk(loggerStub.debug.calledWith('beforeValidation')); + assert.isOk(loggerStub.debug.calledWith('after')); done(); })); }); describe('with hooks, but without hooks.transactions set', () => { beforeEach(() => { - sinon.spy(loggerStub, 'info'); + sinon.spy(loggerStub, 'debug'); runner.hooks.transactions = null; runner.hooks.beforeHooks = { 'Group Machine > Machine > Delete Message > Bogus example name': [ // eslint-disable-next-line - transaction => loggerStub.info('before') + transaction => loggerStub.debug('before') ], }; runner.hooks.beforeValidationHooks = { 'Group Machine > Machine > Delete Message > Bogus example name': [ // eslint-disable-next-line - transaction => loggerStub.info('beforeValidation') + transaction => loggerStub.debug('beforeValidation') ], }; runner.hooks.afterHooks = { 'Group Machine > Machine > Delete Message > Bogus example name': [ // eslint-disable-next-line function (transaction, done) { - loggerStub.info('after'); + loggerStub.debug('after'); done(); }, ], }; }); - afterEach(() => loggerStub.info.restore()); + afterEach(() => loggerStub.debug.restore()); it('should run the hooks', (done) => { runner.hooks.transactions = null; runner.executeAllTransactions([transaction], runner.hooks, () => { - assert.isOk(loggerStub.info.calledWith('before')); - assert.isOk(loggerStub.info.calledWith('beforeValidation')); - assert.isOk(loggerStub.info.calledWith('after')); + assert.isOk(loggerStub.debug.calledWith('before')); + assert.isOk(loggerStub.debug.calledWith('beforeValidation')); + assert.isOk(loggerStub.debug.calledWith('after')); done(); }); }); @@ -1344,25 +1344,25 @@ describe('TransactionRunner', () => { describe('with multiple hooks for the same transaction', () => { beforeEach(() => { - sinon.spy(loggerStub, 'info'); + sinon.spy(loggerStub, 'debug'); runner.hooks.beforeHooks = { 'Group Machine > Machine > Delete Message > Bogus example name': [ // eslint-disable-next-line - transaction => loggerStub.info('first'), + transaction => loggerStub.debug('first'), // eslint-disable-next-line function (transaction, cb) { - loggerStub.info('second'); + loggerStub.debug('second'); cb(); }, ], }; }); - afterEach(() => loggerStub.info.restore()); + afterEach(() => loggerStub.debug.restore()); it('should run all hooks', done => runner.executeAllTransactions([transaction], runner.hooks, () => { - assert.isOk(loggerStub.info.calledWith('first')); - assert.isOk(loggerStub.info.calledWith('second')); + assert.isOk(loggerStub.debug.calledWith('first')); + assert.isOk(loggerStub.debug.calledWith('second')); done(); })); }); From 2f8f6b6d0f8ed0b0c0dff3a2ba93eab34f6c2b45 Mon Sep 17 00:00:00 2001 From: Honza Javorek Date: Thu, 24 Jan 2019 17:36:19 +0100 Subject: [PATCH 06/13] test: unit test processing of the --loglevel option --- lib/configuration.js | 10 +- test/unit/configuration-test.js | 234 +++++++++++++++++++++++++++++--- 2 files changed, 222 insertions(+), 22 deletions(-) diff --git a/lib/configuration.js b/lib/configuration.js index 3bf0f3e40..610441e4b 100644 --- a/lib/configuration.js +++ b/lib/configuration.js @@ -18,20 +18,21 @@ function coerceToArray(value) { function applyLoggingOptions(options) { - // Color can be either specified as "stringified bool" or bool (nothing else - // is expected valid value). Here we're coercing the value to boolean. + // The CLI layer should handle 'color' and 'timestamp' and they should be + // implemented as --color/--no-color and --timestamp/--no-timestamp boolean + // options with no values if (options.color === 'false') { options.color = false; } else if (options.color === 'true') { options.color = true; } - logger.transports.console.colorize = options.color; reporterOutputLogger.transports.console.colorize = options.color; logger.transports.console.timestamp = options.timestamp; reporterOutputLogger.transports.console.timestamp = options.timestamp; + // Handling the 'loglevel' value if (options.loglevel) { const loglevel = options.loglevel.toLowerCase(); if (loglevel === 'silent') { @@ -47,6 +48,9 @@ function applyLoggingOptions(options) { logger.transports.console.level = 'warn'; } + // Once the reporters' output is handled in a different way than with + // a logger, this part will not be necessary anymore + reporterOutputLogger.transports.console.silent = false; reporterOutputLogger.transports.console.level = 'info'; return options; diff --git a/test/unit/configuration-test.js b/test/unit/configuration-test.js index 1d51b30ee..031f80d2a 100644 --- a/test/unit/configuration-test.js +++ b/test/unit/configuration-test.js @@ -3,36 +3,232 @@ const { assert } = require('chai'); const configuration = require('../../lib/configuration'); const logger = require('../../lib/logger'); +const reporterOutputLogger = require('../../lib/reporters/reporterOutputLogger'); + describe('configuration.applyLoggingOptions()', () => { let loggerSettings; - let config; + let reporterOutputLoggerSettings; - beforeEach(() => { loggerSettings = clone(logger.transports.console); }); - afterEach(() => { logger.transports.console = loggerSettings; }); + beforeEach(() => { + loggerSettings = clone(logger.transports.console); + reporterOutputLoggerSettings = clone(reporterOutputLogger.transports.console); + }); + afterEach(() => { + logger.transports.console = loggerSettings; + reporterOutputLogger.transports.console = reporterOutputLoggerSettings; + }); - it('applies logging options', () => { - config = configuration.applyLoggingOptions({ - color: 'true', - loglevel: 'debug', + describe('with color set to true', () => { + let options; + + beforeEach(() => { + options = configuration.applyLoggingOptions({ color: true }); }); - assert.propertyVal(config, 'color', true); - assert.equal(logger.transports.console.colorize, true); + it('resulting configuration should contain \'color\' set to boolean true', () => { + assert.isTrue(options.color); + }); - assert.propertyVal(config, 'loglevel', 'debug'); - assert.equal(logger.transports.console.level, 'debug'); + it('the application logger should be set to colorize', () => { + assert.isTrue(logger.transports.console.colorize); + }); + + it('the application output should be set to colorize', () => { + assert.isTrue(reporterOutputLogger.transports.console.colorize); + }); }); - describe('with color set to legacy \'true\' string value', () => it('resulting configuration should contain \'color\' set to boolean true', () => { - const options = configuration.applyLoggingOptions({ color: 'true' }); - assert.propertyVal(options, 'color', true); - })); + describe('with color set to \'true\' string value', () => { + let options; - describe('with color option set to legacy \'false\' string value', () => it('resulting configuration should contain \'color\' set to boolean false', () => { - const options = configuration.applyLoggingOptions({ color: 'false' }); - assert.propertyVal(options, 'color', false); - })); + beforeEach(() => { + options = configuration.applyLoggingOptions({ color: 'true' }); + }); + + it('resulting configuration should contain \'color\' set to boolean true', () => { + assert.isTrue(options.color); + }); + + it('the application logger should be set to colorize', () => { + assert.isTrue(logger.transports.console.colorize); + }); + + it('the application output should be set to colorize', () => { + assert.isTrue(reporterOutputLogger.transports.console.colorize); + }); + }); + + describe('with color set to false', () => { + let options; + + beforeEach(() => { + options = configuration.applyLoggingOptions({ color: false }); + }); + + it('resulting configuration should contain \'color\' set to boolean false', () => { + assert.isFalse(options.color); + }); + + it('the application logger should be set not to colorize', () => { + assert.isFalse(logger.transports.console.colorize); + }); + + it('the application output should be set not to colorize', () => { + assert.isFalse(reporterOutputLogger.transports.console.colorize); + }); + }); + + describe('with color set to \'false\' string value', () => { + let options; + + beforeEach(() => { + options = configuration.applyLoggingOptions({ color: 'false' }); + }); + + it('resulting configuration should contain \'color\' set to boolean false', () => { + assert.isFalse(options.color); + }); + + it('the application logger should be set not to colorize', () => { + assert.isFalse(logger.transports.console.colorize); + }); + + it('the application output should be set not to colorize', () => { + assert.isFalse(reporterOutputLogger.transports.console.colorize); + }); + }); + + describe('with timestamp set to true', () => { + beforeEach(() => { + configuration.applyLoggingOptions({ timestamp: true }); + }); + + it('the application logger should be set to add timestamps', () => { + assert.isTrue(logger.transports.console.timestamp); + }); + + it('the application output should be set to add timestamps', () => { + assert.isTrue(reporterOutputLogger.transports.console.timestamp); + }); + }); + + describe('with timestamp set to false', () => { + beforeEach(() => { + configuration.applyLoggingOptions({ timestamp: false }); + }); + + it('the application logger should be set not to add timestamps', () => { + assert.isFalse(logger.transports.console.timestamp); + }); + + it('the application output should be set not to add timestamps', () => { + assert.isFalse(reporterOutputLogger.transports.console.timestamp); + }); + }); + + describe('with loglevel not set', () => { + beforeEach(() => { + configuration.applyLoggingOptions({}); + }); + + it('the application logger level is set to warn', () => { + assert.equal(logger.transports.console.level, 'warn'); + }); + + it('the application output logger is not influenced', () => { + assert.isFalse(reporterOutputLogger.transports.console.silent); + assert.equal(reporterOutputLogger.transports.console.level, 'info'); + }); + }); + + describe('with loglevel set to a valid value', () => { + beforeEach(() => { + configuration.applyLoggingOptions({ loglevel: 'error' }); + }); + + it('the application logger level is set', () => { + assert.equal(logger.transports.console.level, 'error'); + }); + + it('the application output logger is not influenced', () => { + assert.isFalse(reporterOutputLogger.transports.console.silent); + assert.equal(reporterOutputLogger.transports.console.level, 'info'); + }); + }); + + describe('with loglevel set to a valid value using uppercase', () => { + beforeEach(() => { + configuration.applyLoggingOptions({ loglevel: 'ERROR' }); + }); + + it('the application logger level is set', () => { + assert.equal(logger.transports.console.level, 'error'); + }); + }); + + describe('with loglevel set to an invalid value', () => { + it('throws an exception', () => { + assert.throws(() => { + configuration.applyLoggingOptions({ loglevel: 'verbose' }); + }, /unsupported.+verbose/i); + }); + }); + + describe('with loglevel set to silent', () => { + beforeEach(() => { + configuration.applyLoggingOptions({ loglevel: 'silent' }); + }); + + it('the application logger gets silenced', () => { + assert.isTrue(logger.transports.console.silent); + }); + + it('the application output logger is not influenced', () => { + assert.isFalse(reporterOutputLogger.transports.console.silent); + assert.equal(reporterOutputLogger.transports.console.level, 'info'); + }); + }); + + describe('with loglevel set to warn', () => { + beforeEach(() => { + configuration.applyLoggingOptions({ loglevel: 'warn' }); + }); + + it('the application logger level is set to warn', () => { + assert.equal(logger.transports.console.level, 'warn'); + }); + }); + + describe('with loglevel set to warning', () => { + beforeEach(() => { + configuration.applyLoggingOptions({ loglevel: 'warning' }); + }); + + it('the application logger level is set to warn', () => { + assert.equal(logger.transports.console.level, 'warn'); + }); + }); + + describe('with loglevel set to error', () => { + beforeEach(() => { + configuration.applyLoggingOptions({ loglevel: 'error' }); + }); + + it('the application logger level is set to error', () => { + assert.equal(logger.transports.console.level, 'error'); + }); + }); + + describe('with loglevel set to debug', () => { + beforeEach(() => { + configuration.applyLoggingOptions({ loglevel: 'debug' }); + }); + + it('the application logger level is set to debug', () => { + assert.equal(logger.transports.console.level, 'debug'); + }); + }); }); describe('configuration.applyConfiguration()', () => { From 8eb265b280c35aeadb065a51373ba8f8fd5e7d20 Mon Sep 17 00:00:00 2001 From: Honza Javorek Date: Thu, 24 Jan 2019 17:59:17 +0100 Subject: [PATCH 07/13] refactor: remove the --timestamp option BREAKING CHANGE: The --timestamp/-t option is no longer available. Instead, the --loglevel=debug automatically displays timestamps as well for the application logging entries. --- docs/usage-cli.rst | 1 - docs/usage-js.rst | 3 +- lib/configuration.js | 19 +++----- lib/options.json | 7 +-- lib/reporters/reporterOutputLogger.js | 2 +- test/integration/cli/hookfiles-cli-test.js | 6 +-- test/unit/CLI-test.js | 1 - test/unit/configUtils-test.js | 3 -- test/unit/configuration-test.js | 52 ++++++++-------------- 9 files changed, 30 insertions(+), 64 deletions(-) diff --git a/docs/usage-cli.rst b/docs/usage-cli.rst index 849a41348..641b53530 100644 --- a/docs/usage-cli.rst +++ b/docs/usage-cli.rst @@ -71,7 +71,6 @@ See below how sample configuration file could look like. The structure is the sa details: false method: [] loglevel: warning - timestamp: false path: [] blueprint: api-description.apib endpoint: "http://127.0.0.1:3000" diff --git a/docs/usage-js.rst b/docs/usage-js.rst index 06f6c1607..b294d0d0a 100644 --- a/docs/usage-js.rst +++ b/docs/usage-js.rst @@ -56,8 +56,7 @@ Let’s have a look at an example configuration first. (Please also see the :ref 'require': null, // String, When using nodejs hooks, require the given module before executing hooks - 'color': true, - 'timestamp': false + 'no-color': false, }, 'emitter': EventEmitterInstance, // optional - listen to test progress, your own instance of EventEmitter diff --git a/lib/configuration.js b/lib/configuration.js index 610441e4b..8c12de9e5 100644 --- a/lib/configuration.js +++ b/lib/configuration.js @@ -18,9 +18,8 @@ function coerceToArray(value) { function applyLoggingOptions(options) { - // The CLI layer should handle 'color' and 'timestamp' and they should be - // implemented as --color/--no-color and --timestamp/--no-timestamp boolean - // options with no values + // The CLI layer should handle 'color' and it should be implemented + // as a --color/--no-color boolean option with no values if (options.color === 'false') { options.color = false; } else if (options.color === 'true') { @@ -29,9 +28,6 @@ function applyLoggingOptions(options) { logger.transports.console.colorize = options.color; reporterOutputLogger.transports.console.colorize = options.color; - logger.transports.console.timestamp = options.timestamp; - reporterOutputLogger.transports.console.timestamp = options.timestamp; - // Handling the 'loglevel' value if (options.loglevel) { const loglevel = options.loglevel.toLowerCase(); @@ -39,7 +35,10 @@ function applyLoggingOptions(options) { logger.transports.console.silent = true; } else if (loglevel === 'warning') { logger.transports.console.level = 'warn'; - } else if (['debug', 'warn', 'error'].includes(loglevel)) { + } else if (loglevel === 'debug') { + logger.transports.console.level = 'debug'; + logger.transports.console.timestamp = true; + } else if (['warn', 'error'].includes(loglevel)) { logger.transports.console.level = loglevel; } else { throw new Error(`Unsupported logging level: '${loglevel}'`); @@ -48,11 +47,6 @@ function applyLoggingOptions(options) { logger.transports.console.level = 'warn'; } - // Once the reporters' output is handled in a different way than with - // a logger, this part will not be necessary anymore - reporterOutputLogger.transports.console.silent = false; - reporterOutputLogger.transports.console.level = 'info'; - return options; } @@ -78,7 +72,6 @@ function applyConfiguration(config) { only: [], color: true, loglevel: 'warning', - timestamp: false, sorted: false, names: false, hookfiles: null, diff --git a/lib/options.json b/lib/options.json index 6ac1667d0..4d3009c82 100644 --- a/lib/options.json +++ b/lib/options.json @@ -94,14 +94,9 @@ }, "loglevel": { "alias": "l", - "description": "The level of logging to output. Options: 'debug', 'warning', 'error', 'silent'.", + "description": "Application logging level. Supported levels: 'debug', 'warning', 'error', 'silent'. The value 'debug' also displays timestamps.", "default": "warning" }, - "timestamp": { - "alias": "t", - "description": "Determines whether console output should include timestamps.", - "default": false - }, "path": { "alias": "p", "description": "Additional API description paths or URLs. Can be used multiple times with glob pattern for paths.", diff --git a/lib/reporters/reporterOutputLogger.js b/lib/reporters/reporterOutputLogger.js index 0cd806d75..93b8cf03c 100644 --- a/lib/reporters/reporterOutputLogger.js +++ b/lib/reporters/reporterOutputLogger.js @@ -2,7 +2,7 @@ const winston = require('winston'); module.exports = new (winston.Logger)({ transports: [ - new (winston.transports.Console)({ colorize: true }), + new (winston.transports.Console)({ colorize: true, level: 'info' }), ], levels: { info: 10, diff --git a/test/integration/cli/hookfiles-cli-test.js b/test/integration/cli/hookfiles-cli-test.js index 4126357e6..b65f6f95c 100644 --- a/test/integration/cli/hookfiles-cli-test.js +++ b/test/integration/cli/hookfiles-cli-test.js @@ -557,7 +557,7 @@ describe('CLI', () => { }); }); - describe('when showing timestamps with -t', () => { + describe('when showing timestamps with --loglevel=debug', () => { let runtimeInfo; before((done) => { @@ -567,7 +567,7 @@ describe('CLI', () => { const args = [ './test/fixtures/single-get.apib', `http://127.0.0.1:${DEFAULT_SERVER_PORT}`, - '-t', + '--loglevel=debug', ]; runCLIWithServer(args, app, (err, info) => { runtimeInfo = info; @@ -577,7 +577,7 @@ describe('CLI', () => { it('should display timestamps', () => { // Look for the prefix for cli output with timestamps - assert.notEqual(runtimeInfo.dredd.stdout.indexOf('Z -'), -1); + assert.include(runtimeInfo.dredd.stderr, 'Z -'); }); }); }); diff --git a/test/unit/CLI-test.js b/test/unit/CLI-test.js index ba687fe3c..cb3834b11 100644 --- a/test/unit/CLI-test.js +++ b/test/unit/CLI-test.js @@ -327,7 +327,6 @@ describe('CLI class', () => { method: [], color: true, loglevel: 'warning', - timestamp: false, path: [], $0: 'node ./bin/dredd', })); diff --git a/test/unit/configUtils-test.js b/test/unit/configUtils-test.js index a04b490e5..96182bc70 100644 --- a/test/unit/configUtils-test.js +++ b/test/unit/configUtils-test.js @@ -51,8 +51,6 @@ const argvData = { c: true, loglevel: 'warning', l: 'warning', - timestamp: false, - t: false, path: [], p: [], $0: 'node ./bin/dredd', @@ -162,7 +160,6 @@ details: false method: [] color: true loglevel: info -timestamp: false path: [] blueprint: blueprint endpoint: endpoint\ diff --git a/test/unit/configuration-test.js b/test/unit/configuration-test.js index 031f80d2a..78480dbbc 100644 --- a/test/unit/configuration-test.js +++ b/test/unit/configuration-test.js @@ -99,34 +99,6 @@ describe('configuration.applyLoggingOptions()', () => { }); }); - describe('with timestamp set to true', () => { - beforeEach(() => { - configuration.applyLoggingOptions({ timestamp: true }); - }); - - it('the application logger should be set to add timestamps', () => { - assert.isTrue(logger.transports.console.timestamp); - }); - - it('the application output should be set to add timestamps', () => { - assert.isTrue(reporterOutputLogger.transports.console.timestamp); - }); - }); - - describe('with timestamp set to false', () => { - beforeEach(() => { - configuration.applyLoggingOptions({ timestamp: false }); - }); - - it('the application logger should be set not to add timestamps', () => { - assert.isFalse(logger.transports.console.timestamp); - }); - - it('the application output should be set not to add timestamps', () => { - assert.isFalse(reporterOutputLogger.transports.console.timestamp); - }); - }); - describe('with loglevel not set', () => { beforeEach(() => { configuration.applyLoggingOptions({}); @@ -162,7 +134,7 @@ describe('configuration.applyLoggingOptions()', () => { configuration.applyLoggingOptions({ loglevel: 'ERROR' }); }); - it('the application logger level is set', () => { + it('the value is understood', () => { assert.equal(logger.transports.console.level, 'error'); }); }); @@ -190,24 +162,28 @@ describe('configuration.applyLoggingOptions()', () => { }); }); - describe('with loglevel set to warn', () => { + describe('with loglevel set to warning', () => { beforeEach(() => { - configuration.applyLoggingOptions({ loglevel: 'warn' }); + configuration.applyLoggingOptions({ loglevel: 'warning' }); }); - it('the application logger level is set to warn', () => { + it('the value is understood as warn', () => { assert.equal(logger.transports.console.level, 'warn'); }); }); - describe('with loglevel set to warning', () => { + describe('with loglevel set to warn', () => { beforeEach(() => { - configuration.applyLoggingOptions({ loglevel: 'warning' }); + configuration.applyLoggingOptions({ loglevel: 'warn' }); }); it('the application logger level is set to warn', () => { assert.equal(logger.transports.console.level, 'warn'); }); + + it('the application logger is not set to add timestamps', () => { + assert.isFalse(logger.transports.console.timestamp); + }); }); describe('with loglevel set to error', () => { @@ -218,6 +194,10 @@ describe('configuration.applyLoggingOptions()', () => { it('the application logger level is set to error', () => { assert.equal(logger.transports.console.level, 'error'); }); + + it('the application logger is not set to add timestamps', () => { + assert.isFalse(logger.transports.console.timestamp); + }); }); describe('with loglevel set to debug', () => { @@ -228,6 +208,10 @@ describe('configuration.applyLoggingOptions()', () => { it('the application logger level is set to debug', () => { assert.equal(logger.transports.console.level, 'debug'); }); + + it('the application logger is set to add timestamps', () => { + assert.isTrue(logger.transports.console.timestamp); + }); }); }); From a5fe81bbd4d57aa0d9e1f0cee5d881cc8fd614aa Mon Sep 17 00:00:00 2001 From: Honza Javorek Date: Thu, 24 Jan 2019 18:31:43 +0100 Subject: [PATCH 08/13] refactor: change behavior of --color BREAKING CHANGE: The --color option doesn't support string values ('false', 'true') anymore. To disable coloring, use --no-color instead. The -c short option has been removed. --- docs/how-to-guides.rst | 2 +- docs/usage-js.rst | 2 +- lib/CLI.js | 9 ++- lib/configuration.js | 15 ++-- lib/options.json | 4 +- test/integration/cli/hookfiles-cli-test.js | 27 +------- test/unit/configuration-test.js | 81 +--------------------- 7 files changed, 16 insertions(+), 124 deletions(-) diff --git a/docs/how-to-guides.rst b/docs/how-to-guides.rst index c4799b2ef..d21048b0b 100644 --- a/docs/how-to-guides.rst +++ b/docs/how-to-guides.rst @@ -712,7 +712,7 @@ As you can see, the parameters go like this: :: - $ dredd -c apiaryApiKey: -c apiaryApiName: + $ dredd -j apiaryApiKey: -j apiaryApiName: In addition to using parameters and ``dredd.yml``, you can also use environment variables: diff --git a/docs/usage-js.rst b/docs/usage-js.rst index b294d0d0a..f1e36d464 100644 --- a/docs/usage-js.rst +++ b/docs/usage-js.rst @@ -56,7 +56,7 @@ Let’s have a look at an example configuration first. (Please also see the :ref 'require': null, // String, When using nodejs hooks, require the given module before executing hooks - 'no-color': false, + 'color': true, }, 'emitter': EventEmitterInstance, // optional - listen to test progress, your own instance of EventEmitter diff --git a/lib/CLI.js b/lib/CLI.js index 3b65b3f88..a62e170a6 100644 --- a/lib/CLI.js +++ b/lib/CLI.js @@ -19,12 +19,11 @@ class CLI { constructor(options = {}, cb) { this.cb = cb; this.finished = false; - ({ exit: this.exit, custom: this.custom } = options); + this.exit = options.exit; + this.custom = options.custom || {}; this.setExitOrCallback(); - if (!this.custom) { this.custom = {}; } - if (!this.custom.cwd || typeof this.custom.cwd !== 'string') { this.custom.cwd = process.cwd(); } @@ -52,7 +51,7 @@ Example: .wrap(80); this.argv = this.optimist.argv; - this.argv = applyLoggingOptions(this.argv); + applyLoggingOptions(this.argv); } // Gracefully terminate server @@ -188,7 +187,7 @@ ${packageData.name} v${packageData.version} \ } }); - this.argv = applyLoggingOptions(this.argv); + applyLoggingOptions(this.argv); } parseCustomConfig() { diff --git a/lib/configuration.js b/lib/configuration.js index 8c12de9e5..66d5686e9 100644 --- a/lib/configuration.js +++ b/lib/configuration.js @@ -18,15 +18,10 @@ function coerceToArray(value) { function applyLoggingOptions(options) { - // The CLI layer should handle 'color' and it should be implemented - // as a --color/--no-color boolean option with no values - if (options.color === 'false') { - options.color = false; - } else if (options.color === 'true') { - options.color = true; + if (options.color === false) { + logger.transports.console.colorize = false; + reporterOutputLogger.transports.console.colorize = false; } - logger.transports.console.colorize = options.color; - reporterOutputLogger.transports.console.colorize = options.color; // Handling the 'loglevel' value if (options.loglevel) { @@ -46,8 +41,6 @@ function applyLoggingOptions(options) { } else { logger.transports.console.level = 'warn'; } - - return options; } @@ -118,7 +111,7 @@ function applyConfiguration(config) { configuration.options.header.push(authHeader); } - configuration.options = applyLoggingOptions(configuration.options); + applyLoggingOptions(configuration.options); if (config) { if (config.hooksData diff --git a/lib/options.json b/lib/options.json index 4d3009c82..931baa757 100644 --- a/lib/options.json +++ b/lib/options.json @@ -88,8 +88,8 @@ "default": [] }, "color": { - "alias": "c", - "description": "Determines whether console output should include colors.", + "description": "Use --no-color to suppress colors on the output", + "boolean": true, "default": true }, "loglevel": { diff --git a/test/integration/cli/hookfiles-cli-test.js b/test/integration/cli/hookfiles-cli-test.js index b65f6f95c..07ff4598b 100644 --- a/test/integration/cli/hookfiles-cli-test.js +++ b/test/integration/cli/hookfiles-cli-test.js @@ -508,31 +508,6 @@ describe('CLI', () => { }); }); - describe('when suppressing color with --color=false', () => { - let runtimeInfo; - - before((done) => { - const app = createServer(); - app.get('/machines', (req, res) => res.json([{ type: 'bulldozer', name: 'willy' }])); - - const args = [ - './test/fixtures/single-get.apib', - `http://127.0.0.1:${DEFAULT_SERVER_PORT}`, - '--color=false', - ]; - runCLIWithServer(args, app, (err, info) => { - runtimeInfo = info; - done(err); - }); - }); - - it('should print without colors', () => { - // If colors are not on, there is no closing color code between - // the "pass" and the ":" - assert.include(runtimeInfo.dredd.stdout, 'pass:'); - }); - }); - describe('when setting the log output level with --loglevel', () => { let runtimeInfo; @@ -544,7 +519,7 @@ describe('CLI', () => { './test/fixtures/single-get.apib', `http://127.0.0.1:${DEFAULT_SERVER_PORT}`, '--loglevel=error', - '--color=false', + '--no-color', ]; runCLIWithServer(args, app, (err, info) => { runtimeInfo = info; diff --git a/test/unit/configuration-test.js b/test/unit/configuration-test.js index 78480dbbc..50dca652d 100644 --- a/test/unit/configuration-test.js +++ b/test/unit/configuration-test.js @@ -19,35 +19,9 @@ describe('configuration.applyLoggingOptions()', () => { reporterOutputLogger.transports.console = reporterOutputLoggerSettings; }); - describe('with color set to true', () => { - let options; - + describe('with color not set', () => { beforeEach(() => { - options = configuration.applyLoggingOptions({ color: true }); - }); - - it('resulting configuration should contain \'color\' set to boolean true', () => { - assert.isTrue(options.color); - }); - - it('the application logger should be set to colorize', () => { - assert.isTrue(logger.transports.console.colorize); - }); - - it('the application output should be set to colorize', () => { - assert.isTrue(reporterOutputLogger.transports.console.colorize); - }); - }); - - describe('with color set to \'true\' string value', () => { - let options; - - beforeEach(() => { - options = configuration.applyLoggingOptions({ color: 'true' }); - }); - - it('resulting configuration should contain \'color\' set to boolean true', () => { - assert.isTrue(options.color); + configuration.applyLoggingOptions({}); }); it('the application logger should be set to colorize', () => { @@ -60,34 +34,8 @@ describe('configuration.applyLoggingOptions()', () => { }); describe('with color set to false', () => { - let options; - - beforeEach(() => { - options = configuration.applyLoggingOptions({ color: false }); - }); - - it('resulting configuration should contain \'color\' set to boolean false', () => { - assert.isFalse(options.color); - }); - - it('the application logger should be set not to colorize', () => { - assert.isFalse(logger.transports.console.colorize); - }); - - it('the application output should be set not to colorize', () => { - assert.isFalse(reporterOutputLogger.transports.console.colorize); - }); - }); - - describe('with color set to \'false\' string value', () => { - let options; - beforeEach(() => { - options = configuration.applyLoggingOptions({ color: 'false' }); - }); - - it('resulting configuration should contain \'color\' set to boolean false', () => { - assert.isFalse(options.color); + configuration.applyLoggingOptions({ color: false }); }); it('the application logger should be set not to colorize', () => { @@ -214,26 +162,3 @@ describe('configuration.applyLoggingOptions()', () => { }); }); }); - -describe('configuration.applyConfiguration()', () => { - let loggerSettings; - let config; - - beforeEach(() => { loggerSettings = clone(logger.transports.console); }); - afterEach(() => { logger.transports.console = loggerSettings; }); - - it('applies logging options', () => { - config = configuration.applyConfiguration({ - options: { - color: 'true', - loglevel: 'debug', - }, - }); - - assert.nestedPropertyVal(config, 'options.color', true); - assert.equal(logger.transports.console.colorize, true); - - assert.nestedPropertyVal(config, 'options.loglevel', 'debug'); - assert.equal(logger.transports.console.level, 'debug'); - }); -}); From 7eb256b6d073c1e7fd4215b81d6fb9711371753a Mon Sep 17 00:00:00 2001 From: Honza Javorek Date: Thu, 24 Jan 2019 20:00:04 +0100 Subject: [PATCH 09/13] test: make 'applyLoggingOptions()' tests more proof against side effects This is a work around. The configuration and loggers setup should be refactored to work without side effects like this. --- test/unit/configuration-test.js | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/test/unit/configuration-test.js b/test/unit/configuration-test.js index 50dca652d..ce18ebfbb 100644 --- a/test/unit/configuration-test.js +++ b/test/unit/configuration-test.js @@ -6,18 +6,18 @@ const logger = require('../../lib/logger'); const reporterOutputLogger = require('../../lib/reporters/reporterOutputLogger'); -describe('configuration.applyLoggingOptions()', () => { - let loggerSettings; - let reporterOutputLoggerSettings; +const defaultLoggerConsole = clone(logger.transports.console); +const defaultReporterOutputLoggerConsole = clone(reporterOutputLogger.transports.console); - beforeEach(() => { - loggerSettings = clone(logger.transports.console); - reporterOutputLoggerSettings = clone(reporterOutputLogger.transports.console); - }); - afterEach(() => { - logger.transports.console = loggerSettings; - reporterOutputLogger.transports.console = reporterOutputLoggerSettings; - }); +function resetLoggerConsoles() { + logger.transports.console = defaultLoggerConsole; + reporterOutputLogger.transports.console = defaultReporterOutputLoggerConsole; +} + + +describe('configuration.applyLoggingOptions()', () => { + beforeEach(resetLoggerConsoles); + afterEach(resetLoggerConsoles); describe('with color not set', () => { beforeEach(() => { From aea95c78e2f97eff2b11df9127abca1d06e13a96 Mon Sep 17 00:00:00 2001 From: Honza Javorek Date: Fri, 1 Feb 2019 22:38:50 +0100 Subject: [PATCH 10/13] refactor: remove unnecessary logging --- lib/CLI.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/CLI.js b/lib/CLI.js index a62e170a6..b086b25b0 100644 --- a/lib/CLI.js +++ b/lib/CLI.js @@ -155,13 +155,11 @@ Example: // Show help } else if (this.argv.help === true) { - logger.debug('Printing help.'); this.optimist.showHelp(console.error); this._processExit(0); // Show version } else if (this.argv.version === true) { - logger.debug('Printing version.'); console.log(`\ ${packageData.name} v${packageData.version} \ (${os.type()} ${os.release()}; ${os.arch()})\ From 67b6b2e46ae4e272ef6011fbf0daa9032b42d8a3 Mon Sep 17 00:00:00 2001 From: Honza Javorek Date: Fri, 1 Feb 2019 22:44:42 +0100 Subject: [PATCH 11/13] refactor: simplify 'setExitOrCallback()' logging --- lib/CLI.js | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/lib/CLI.js b/lib/CLI.js index b086b25b0..5df6ecd83 100644 --- a/lib/CLI.js +++ b/lib/CLI.js @@ -79,8 +79,7 @@ Example: if (this.exit) { this._processExit = (exitStatus) => { - logger.debug(`Exiting Dredd process with status '${exitStatus}'.`); - logger.debug('Using configured custom exit() method to terminate the Dredd process.'); + logger.debug(`Using configured custom exit() method to terminate the Dredd process with status '${exitStatus}'.`); this.finished = true; this.stopServer(() => { this.exit(exitStatus); @@ -88,15 +87,13 @@ Example: }; } else { this._processExit = (exitStatus) => { - logger.debug(`Exiting Dredd process with status '${exitStatus}'.`); - logger.debug('Using native process.exit() method to terminate the Dredd process.'); + logger.debug(`Using native process.exit() method to terminate the Dredd process with status '${exitStatus}'.`); this.stopServer(() => process.exit(exitStatus)); }; } } else { this._processExit = (exitStatus) => { - logger.debug(`Exiting Dredd process with status '${exitStatus}'.`); - logger.debug('Using configured custom callback to terminate the Dredd process.'); + logger.debug(`Using configured custom callback to terminate the Dredd process with status '${exitStatus}'.`); this.finished = true; if (this.sigIntEventAdded) { if (this.serverProcess && !this.serverProcess.terminated) { From ff55694103072f0c2a19829731cfb3db044ab5aa Mon Sep 17 00:00:00 2001 From: Honza Javorek Date: Tue, 5 Feb 2019 14:54:17 +0100 Subject: [PATCH 12/13] refactor: properly deprecate options --- lib/CLI.js | 28 +-- lib/configuration.js | 200 +++++++++++---- lib/options.json | 2 +- test/unit/configuration-test.js | 422 +++++++++++++++++++++++++++++++- 4 files changed, 579 insertions(+), 73 deletions(-) diff --git a/lib/CLI.js b/lib/CLI.js index 5df6ecd83..2372d2ecc 100644 --- a/lib/CLI.js +++ b/lib/CLI.js @@ -269,22 +269,22 @@ ${packageData.name} v${packageData.version} \ } run() { - for (const task of [ - this.setOptimistArgv, - this.parseCustomConfig, - this.runExitingActions, - this.loadDreddFile, - this.checkRequiredArgs, - this.moveBlueprintArgToPath, - ]) { - task.call(this); - if (this.finished) { return; } - } + try { + for (const task of [ + this.setOptimistArgv, + this.parseCustomConfig, + this.runExitingActions, + this.loadDreddFile, + this.checkRequiredArgs, + this.moveBlueprintArgToPath, + ]) { + task.call(this); + if (this.finished) { return; } + } - const configurationForDredd = this.initConfig(); - this.logDebuggingInfo(configurationForDredd); + const configurationForDredd = this.initConfig(); + this.logDebuggingInfo(configurationForDredd); - try { this.dreddInstance = this.initDredd(configurationForDredd); } catch (e) { this.exitWithStatus(e); diff --git a/lib/configuration.js b/lib/configuration.js index 66d5686e9..67d81e1bb 100644 --- a/lib/configuration.js +++ b/lib/configuration.js @@ -6,17 +6,29 @@ const reporterOutputLogger = require('./reporters/reporterOutputLogger'); function coerceToArray(value) { - if (Array.isArray(value)) { - return value; - } if (typeof value === 'string') { - return [value]; - } if ((value == null)) { - return []; - } + if (Array.isArray(value)) return value; + if (typeof value === 'string') return [value]; + if (value == null) return []; return value; } +function coerceToBoolean(value) { + if (value === 'true') return true; + if (value === 'false') return false; + if (value) return true; + return false; +} + + +function cloneConfig(config) { + const emitter = config.emitter; + const clonedConfig = clone(config); + if (emitter) clonedConfig.emitter = emitter; + return clonedConfig; +} + + function applyLoggingOptions(options) { if (options.color === false) { logger.transports.console.colorize = false; @@ -36,7 +48,9 @@ function applyLoggingOptions(options) { } else if (['warn', 'error'].includes(loglevel)) { logger.transports.console.level = loglevel; } else { - throw new Error(`Unsupported logging level: '${loglevel}'`); + logger.transports.console.level = 'warn'; + throw new Error(`The logging level '${loglevel}' is unsupported, ` + + 'supported are: silent, error, warning, debug'); } } else { logger.transports.console.level = 'warn'; @@ -44,8 +58,92 @@ function applyLoggingOptions(options) { } -function applyConfiguration(config) { - const configuration = { +function coerceRemovedOptions(config = {}) { + const coercedConfig = cloneConfig(config); + + const errors = []; + const warnings = []; + + if (config.options) { + if (config.options.c != null) { + warnings.push('DEPRECATED: Dredd does not support ' + + '-c anymore, use --color/--no-color instead'); + coercedConfig.options.color = coerceToBoolean(config.options.c); + delete coercedConfig.options.c; + } + if (typeof config.options.color === 'string') { + warnings.push('DEPRECATED: Dredd does not support ' + + `--color=${config.options.color} anymore, use --color/--no-color instead`); + coercedConfig.options.color = coerceToBoolean(config.options.color); + } + if (config.options.level) { + warnings.push('DEPRECATED: Dredd does not support ' + + '--level anymore, use --loglevel instead'); + } + if (config.options.level || config.options.l) { + const level = config.options.level || config.options.l; + if (!['silent', 'error', 'warning', 'debug'].includes(level)) { + warnings.push('DEPRECATED: Dredd does not support ' + + `'${level}' log level anymore, use 'silent', 'error', ` + + "'warning', or 'debug' instead"); + } + let loglevel; + if (['silly', 'debug', 'verbose'].includes(level)) { + loglevel = 'debug'; + } else if (level === 'error') { + loglevel = 'error'; + } else if (level === 'silent') { + loglevel = 'silent'; + } else { + loglevel = 'warn'; + } + coercedConfig.options.loglevel = loglevel; + coercedConfig.options.l = loglevel; + delete coercedConfig.options.level; + } + if (config.options.timestamp || config.options.t) { + warnings.push('DEPRECATED: Dredd does not support ' + + '--timestamp anymore, use --loglevel=debug instead'); + coercedConfig.options.loglevel = 'debug'; + coercedConfig.options.l = 'debug'; + delete coercedConfig.options.timestamp; + delete coercedConfig.options.t; + } + if (config.options.silent || config.options.q) { + warnings.push('DEPRECATED: Dredd does not support ' + + '-q/--silent anymore, use --loglevel=silent instead'); + coercedConfig.options.loglevel = 'silent'; + coercedConfig.options.l = 'silent'; + delete coercedConfig.options.silent; + delete coercedConfig.options.q; + } + if (config.options.sandbox || config.options.b) { + errors.push('REMOVED: Dredd does not support ' + + 'sandboxed JS hooks anymore, use standard JS hooks instead'); + delete coercedConfig.options.sandbox; + delete coercedConfig.options.b; + } + } + if (config.hooksData) { + errors.push('REMOVED: Dredd does not support ' + + 'sandboxed JS hooks anymore, use standard JS hooks instead'); + delete coercedConfig.hooksData; + } + if (config.blueprintPath) { + warnings.push('DEPRECATED: Dredd does not support ' + + "the 'blueprintPath' option anymore, use 'path' instead"); + coercedConfig.options = config.options || {}; + coercedConfig.options.path = coerceToArray(coercedConfig.options.path); + coercedConfig.options.path.push(config.blueprintPath); + delete coercedConfig.blueprintPath; + } + + return { config: coercedConfig, errors, warnings }; +} + + +function applyConfiguration(inConfig) { + const outConfig = { server: null, emitter: new EventEmitter(), custom: { // Used for custom settings of various APIs or reporters @@ -56,7 +154,6 @@ function applyConfiguration(config) { 'dry-run': false, reporter: null, output: null, - debug: false, header: null, user: null, 'inline-errors': false, @@ -80,56 +177,59 @@ function applyConfiguration(config) { }, }; - // Normalize options and config - for (const key of Object.keys(config || {})) { - // Copy anything except "custom" hash - const value = config[key]; - if (key !== 'custom') { - configuration[key] = value; - } else { - if (!configuration.custom) { configuration.custom = {}; } - const object = config.custom || {}; - for (const customKey of Object.keys(object || {})) { - const customVal = object[customKey]; - configuration.custom[customKey] = clone(customVal, false); - } - } - } + // Gracefully deal with the removed options + const coerceResult = coerceRemovedOptions(inConfig); + const coercedInConfig = coerceResult.config; + + // Apply the values from the coerced config over the default ones + const custom = coercedInConfig.custom || {}; + Object.keys(custom) + .forEach((key) => { outConfig.custom[key] = clone(custom[key], false); }); + + const options = coercedInConfig.options || {}; + Object.keys(options) + .forEach((key) => { outConfig.options[key] = options[key]; }); + + Object.keys(coercedInConfig) + .filter(key => !['custom', 'options'].includes(key)) + .forEach((key) => { outConfig[key] = coercedInConfig[key]; }); // Coerce single/multiple options into an array - configuration.options.reporter = coerceToArray(configuration.options.reporter); - configuration.options.output = coerceToArray(configuration.options.output); - configuration.options.header = coerceToArray(configuration.options.header); - configuration.options.method = coerceToArray(configuration.options.method); - configuration.options.only = coerceToArray(configuration.options.only); - configuration.options.path = coerceToArray(configuration.options.path); - - configuration.options.method = configuration.options.method.map(method => method.toUpperCase()); - - if (configuration.options.user) { - const authHeader = `Authorization:Basic ${Buffer.from(configuration.options.user).toString('base64')}`; - configuration.options.header.push(authHeader); + outConfig.options.reporter = coerceToArray(outConfig.options.reporter); + outConfig.options.output = coerceToArray(outConfig.options.output); + outConfig.options.header = coerceToArray(outConfig.options.header); + outConfig.options.method = coerceToArray(outConfig.options.method); + outConfig.options.only = coerceToArray(outConfig.options.only); + outConfig.options.path = coerceToArray(outConfig.options.path); + + outConfig.options.method = outConfig.options.method + .map(method => method.toUpperCase()); + + if (outConfig.options.user) { + const token = Buffer.from(outConfig.options.user).toString('base64'); + outConfig.options.header.push(`Authorization: Basic ${token}`); + delete outConfig.options.user; } - applyLoggingOptions(configuration.options); + applyLoggingOptions(outConfig.options); - if (config) { - if (config.hooksData - || (config.options && (config.options.b || config.options.sandbox))) { - throw new Error('DEPRECATED: Dredd does not support ' - + 'sandboxed JS hooks anymore. Use standard JS hooks instead.'); - } - if (config.blueprintPath) { - throw new Error('DEPRECATED: Dredd does not support ' - + "the 'blueprintPath' option anymore. Use 'path' instead."); - } + // Log accumulated errors and warnings + coerceResult.errors.forEach(message => logger.error(message)); + coerceResult.warnings.forEach(message => logger.warn(message)); + + // Abort Dredd if there has been any errors + if (coerceResult.errors.length) { + throw new Error('Could not configure Dredd'); } - return configuration; + return outConfig; } module.exports = { applyConfiguration, applyLoggingOptions, + + // only for the purpose of unit tests + _coerceRemovedOptions: coerceRemovedOptions, }; diff --git a/lib/options.json b/lib/options.json index 931baa757..104112074 100644 --- a/lib/options.json +++ b/lib/options.json @@ -88,7 +88,7 @@ "default": [] }, "color": { - "description": "Use --no-color to suppress colors on the output", + "description": "Use --color/--no-color to enable/disable colored output", "boolean": true, "default": true }, diff --git a/test/unit/configuration-test.js b/test/unit/configuration-test.js index ce18ebfbb..c4790f071 100644 --- a/test/unit/configuration-test.js +++ b/test/unit/configuration-test.js @@ -27,7 +27,19 @@ describe('configuration.applyLoggingOptions()', () => { it('the application logger should be set to colorize', () => { assert.isTrue(logger.transports.console.colorize); }); + it('the application output should be set to colorize', () => { + assert.isTrue(reporterOutputLogger.transports.console.colorize); + }); + }); + describe('with color set to true', () => { + beforeEach(() => { + configuration.applyLoggingOptions({ color: true }); + }); + + it('the application logger should be set to colorize', () => { + assert.isTrue(logger.transports.console.colorize); + }); it('the application output should be set to colorize', () => { assert.isTrue(reporterOutputLogger.transports.console.colorize); }); @@ -41,7 +53,6 @@ describe('configuration.applyLoggingOptions()', () => { it('the application logger should be set not to colorize', () => { assert.isFalse(logger.transports.console.colorize); }); - it('the application output should be set not to colorize', () => { assert.isFalse(reporterOutputLogger.transports.console.colorize); }); @@ -55,7 +66,6 @@ describe('configuration.applyLoggingOptions()', () => { it('the application logger level is set to warn', () => { assert.equal(logger.transports.console.level, 'warn'); }); - it('the application output logger is not influenced', () => { assert.isFalse(reporterOutputLogger.transports.console.silent); assert.equal(reporterOutputLogger.transports.console.level, 'info'); @@ -70,7 +80,6 @@ describe('configuration.applyLoggingOptions()', () => { it('the application logger level is set', () => { assert.equal(logger.transports.console.level, 'error'); }); - it('the application output logger is not influenced', () => { assert.isFalse(reporterOutputLogger.transports.console.silent); assert.equal(reporterOutputLogger.transports.console.level, 'info'); @@ -91,7 +100,7 @@ describe('configuration.applyLoggingOptions()', () => { it('throws an exception', () => { assert.throws(() => { configuration.applyLoggingOptions({ loglevel: 'verbose' }); - }, /unsupported.+verbose/i); + }, /verbose.+unsupported/i); }); }); @@ -103,7 +112,6 @@ describe('configuration.applyLoggingOptions()', () => { it('the application logger gets silenced', () => { assert.isTrue(logger.transports.console.silent); }); - it('the application output logger is not influenced', () => { assert.isFalse(reporterOutputLogger.transports.console.silent); assert.equal(reporterOutputLogger.transports.console.level, 'info'); @@ -128,7 +136,6 @@ describe('configuration.applyLoggingOptions()', () => { it('the application logger level is set to warn', () => { assert.equal(logger.transports.console.level, 'warn'); }); - it('the application logger is not set to add timestamps', () => { assert.isFalse(logger.transports.console.timestamp); }); @@ -142,7 +149,6 @@ describe('configuration.applyLoggingOptions()', () => { it('the application logger level is set to error', () => { assert.equal(logger.transports.console.level, 'error'); }); - it('the application logger is not set to add timestamps', () => { assert.isFalse(logger.transports.console.timestamp); }); @@ -156,9 +162,409 @@ describe('configuration.applyLoggingOptions()', () => { it('the application logger level is set to debug', () => { assert.equal(logger.transports.console.level, 'debug'); }); - it('the application logger is set to add timestamps', () => { assert.isTrue(logger.transports.console.timestamp); }); }); }); + + +describe('configuration._coerceRemovedOptions()', () => { + describe("with -c set to string 'true'", () => { + let coerceResult; + + before(() => { + coerceResult = configuration._coerceRemovedOptions({ + options: { c: 'true' }, + }); + }); + + it('gets coerced to color set to boolean true', () => { + assert.deepEqual(coerceResult.config, { options: { color: true } }); + }); + it('produces no errors', () => { + assert.deepEqual(coerceResult.errors, []); + }); + it('produces one warning', () => { + assert.lengthOf(coerceResult.warnings, 1); + }); + }); + + describe("with -c set to string 'false'", () => { + let coerceResult; + + before(() => { + coerceResult = configuration._coerceRemovedOptions({ + options: { c: 'false' }, + }); + }); + + it('gets coerced to color set to boolean false', () => { + assert.deepEqual(coerceResult.config, { options: { color: false } }); + }); + it('produces no errors', () => { + assert.deepEqual(coerceResult.errors, []); + }); + it('produces one warning', () => { + assert.lengthOf(coerceResult.warnings, 1); + }); + }); + + describe('with -c set to true', () => { + let coerceResult; + + before(() => { + coerceResult = configuration._coerceRemovedOptions({ + options: { c: true }, + }); + }); + + it('gets coerced to color set to boolean true', () => { + assert.deepEqual(coerceResult.config, { options: { color: true } }); + }); + it('produces no errors', () => { + assert.deepEqual(coerceResult.errors, []); + }); + it('produces one warning', () => { + assert.lengthOf(coerceResult.warnings, 1); + }); + }); + + describe('with -c set to false', () => { + let coerceResult; + + before(() => { + coerceResult = configuration._coerceRemovedOptions({ + options: { c: false }, + }); + }); + + it('gets coerced to color set to boolean false', () => { + assert.deepEqual(coerceResult.config, { options: { color: false } }); + }); + it('produces no errors', () => { + assert.deepEqual(coerceResult.errors, []); + }); + it('produces one warning', () => { + assert.lengthOf(coerceResult.warnings, 1); + }); + }); + + describe("with --color set to string 'true'", () => { + let coerceResult; + + before(() => { + coerceResult = configuration._coerceRemovedOptions({ + options: { color: 'true' }, + }); + }); + + it('gets coerced to color set to boolean true', () => { + assert.deepEqual(coerceResult.config, { options: { color: true } }); + }); + it('produces no errors', () => { + assert.deepEqual(coerceResult.errors, []); + }); + it('produces one warning', () => { + assert.lengthOf(coerceResult.warnings, 1); + }); + }); + + describe("with --color set to string 'false'", () => { + let coerceResult; + + before(() => { + coerceResult = configuration._coerceRemovedOptions({ + options: { color: 'false' }, + }); + }); + + it('gets coerced to color set to boolean false', () => { + assert.deepEqual(coerceResult.config, { options: { color: false } }); + }); + it('produces no errors', () => { + assert.deepEqual(coerceResult.errors, []); + }); + it('produces one warning', () => { + assert.lengthOf(coerceResult.warnings, 1); + }); + }); + + describe('with --color set to true', () => { + let coerceResult; + + before(() => { + coerceResult = configuration._coerceRemovedOptions({ + options: { color: true }, + }); + }); + + it('gets coerced to color set to boolean true', () => { + assert.deepEqual(coerceResult.config, { options: { color: true } }); + }); + it('produces no errors', () => { + assert.deepEqual(coerceResult.errors, []); + }); + it('produces no warnings', () => { + assert.deepEqual(coerceResult.warnings, []); + }); + }); + + describe('with --color set to false', () => { + let coerceResult; + + before(() => { + coerceResult = configuration._coerceRemovedOptions({ + options: { color: false }, + }); + }); + + it('gets coerced to color set to boolean false', () => { + assert.deepEqual(coerceResult.config, { options: { color: false } }); + }); + it('produces no errors', () => { + assert.deepEqual(coerceResult.errors, []); + }); + it('produces no warnings', () => { + assert.deepEqual(coerceResult.warnings, []); + }); + }); + + describe('with --level/-l set to a supported value', () => { + let coerceResult; + + before(() => { + coerceResult = configuration._coerceRemovedOptions({ + options: { l: 'debug', level: 'debug' }, + }); + }); + + it('gets coerced to loglevel set to the value', () => { + assert.deepEqual(coerceResult.config, { + options: { l: 'debug', loglevel: 'debug' }, + }); + }); + it('produces no errors', () => { + assert.deepEqual(coerceResult.errors, []); + }); + it('produces one warning', () => { + assert.lengthOf(coerceResult.warnings, 1); + }); + }); + + describe('with --level/-l set to a consolidated value', () => { + let coerceResult; + + before(() => { + coerceResult = configuration._coerceRemovedOptions({ + options: { l: 'verbose', level: 'verbose' }, + }); + }); + + it('gets coerced to loglevel set to a corresponding value', () => { + assert.deepEqual(coerceResult.config, { + options: { l: 'debug', loglevel: 'debug' }, + }); + }); + it('produces no errors', () => { + assert.deepEqual(coerceResult.errors, []); + }); + it('produces two warnings', () => { + assert.lengthOf(coerceResult.warnings, 2); + }); + }); + + describe('with --level/-l set to a removed value', () => { + let coerceResult; + + before(() => { + coerceResult = configuration._coerceRemovedOptions({ + options: { l: 'complete', level: 'complete' }, + }); + }); + + it('gets coerced to loglevel set to the default value', () => { + assert.deepEqual(coerceResult.config, { + options: { l: 'warn', loglevel: 'warn' }, + }); + }); + it('produces no errors', () => { + assert.deepEqual(coerceResult.errors, []); + }); + it('produces two warnings', () => { + assert.lengthOf(coerceResult.warnings, 2); + }); + }); + + describe("with -l set to 'silent'", () => { + let coerceResult; + + before(() => { + coerceResult = configuration._coerceRemovedOptions({ + options: { l: 'silent' }, + }); + }); + + it('gets coerced to loglevel set to silent', () => { + assert.deepEqual(coerceResult.config, { + options: { l: 'silent', loglevel: 'silent' }, + }); + }); + it('produces no errors', () => { + assert.deepEqual(coerceResult.errors, []); + }); + it('produces no warnings', () => { + assert.deepEqual(coerceResult.warnings, []); + }); + }); + + describe('with --timestamp/-t set', () => { + let coerceResult; + + before(() => { + coerceResult = configuration._coerceRemovedOptions({ + options: { timestamp: true, t: true }, + }); + }); + + it('gets coerced to loglevel set to debug', () => { + assert.deepEqual(coerceResult.config, { + options: { l: 'debug', loglevel: 'debug' }, + }); + }); + it('produces no errors', () => { + assert.deepEqual(coerceResult.errors, []); + }); + it('produces one warning', () => { + assert.lengthOf(coerceResult.warnings, 1); + }); + }); + + describe('with --silent/-q set', () => { + let coerceResult; + + before(() => { + coerceResult = configuration._coerceRemovedOptions({ + options: { silent: true, q: true }, + }); + }); + + it('gets coerced to loglevel set to silent', () => { + assert.deepEqual(coerceResult.config, { + options: { l: 'silent', loglevel: 'silent' }, + }); + }); + it('produces no errors', () => { + assert.deepEqual(coerceResult.errors, []); + }); + it('produces one warning', () => { + assert.lengthOf(coerceResult.warnings, 1); + }); + }); + + describe('with --sandbox/-b set', () => { + let coerceResult; + + before(() => { + coerceResult = configuration._coerceRemovedOptions({ + options: { sandbox: true, b: true }, + }); + }); + + it('gets removed', () => { + assert.deepEqual(coerceResult.config, { options: {} }); + }); + it('produces one error', () => { + assert.lengthOf(coerceResult.errors, 1); + }); + it('produces no warnings', () => { + assert.deepEqual(coerceResult.warnings, []); + }); + }); + + describe('with hooksData set', () => { + let coerceResult; + + before(() => { + coerceResult = configuration._coerceRemovedOptions({ + hooksData: {}, + }); + }); + + it('gets removed', () => { + assert.deepEqual(coerceResult.config, {}); + }); + it('produces one error', () => { + assert.lengthOf(coerceResult.errors, 1); + }); + it('produces no warnings', () => { + assert.deepEqual(coerceResult.warnings, []); + }); + }); + + describe('with blueprintPath set and empty path', () => { + let coerceResult; + + before(() => { + coerceResult = configuration._coerceRemovedOptions({ + blueprintPath: 'foo/bar', + }); + }); + + it('gets reassigned as path', () => { + assert.deepEqual(coerceResult.config, { options: { path: ['foo/bar'] } }); + }); + it('produces no errors', () => { + assert.deepEqual(coerceResult.errors, []); + }); + it('produces no warnings', () => { + assert.lengthOf(coerceResult.warnings, 1); + }); + }); + + describe('with blueprintPath set and path set to a string', () => { + let coerceResult; + + before(() => { + coerceResult = configuration._coerceRemovedOptions({ + blueprintPath: 'foo/bar', + options: { path: 'moo.js' }, + }); + }); + + it('gets reassigned as path', () => { + assert.deepEqual(coerceResult.config, { + options: { path: ['moo.js', 'foo/bar'] }, + }); + }); + it('produces no errors', () => { + assert.deepEqual(coerceResult.errors, []); + }); + it('produces no warnings', () => { + assert.lengthOf(coerceResult.warnings, 1); + }); + }); + + describe('with blueprintPath set and path set to an array', () => { + let coerceResult; + + before(() => { + coerceResult = configuration._coerceRemovedOptions({ + blueprintPath: 'foo/bar', + options: { path: ['moo.js'] }, + }); + }); + + it('gets reassigned as path', () => { + assert.deepEqual(coerceResult.config, { + options: { path: ['moo.js', 'foo/bar'] }, + }); + }); + it('produces no errors', () => { + assert.deepEqual(coerceResult.errors, []); + }); + it('produces no warnings', () => { + assert.lengthOf(coerceResult.warnings, 1); + }); + }); +}); From c8cf96f3d7ca60e96fcafa31dae7a1ff83ce5402 Mon Sep 17 00:00:00 2001 From: Honza Javorek Date: Thu, 7 Feb 2019 15:17:44 +0100 Subject: [PATCH 13/13] refactor: modify the options in place --- lib/configuration.js | 62 ++++++-------- test/unit/configuration-test.js | 139 ++++++++++++++------------------ 2 files changed, 84 insertions(+), 117 deletions(-) diff --git a/lib/configuration.js b/lib/configuration.js index 67d81e1bb..52fc5cac4 100644 --- a/lib/configuration.js +++ b/lib/configuration.js @@ -21,14 +21,6 @@ function coerceToBoolean(value) { } -function cloneConfig(config) { - const emitter = config.emitter; - const clonedConfig = clone(config); - if (emitter) clonedConfig.emitter = emitter; - return clonedConfig; -} - - function applyLoggingOptions(options) { if (options.color === false) { logger.transports.console.colorize = false; @@ -59,8 +51,6 @@ function applyLoggingOptions(options) { function coerceRemovedOptions(config = {}) { - const coercedConfig = cloneConfig(config); - const errors = []; const warnings = []; @@ -68,13 +58,13 @@ function coerceRemovedOptions(config = {}) { if (config.options.c != null) { warnings.push('DEPRECATED: Dredd does not support ' + '-c anymore, use --color/--no-color instead'); - coercedConfig.options.color = coerceToBoolean(config.options.c); - delete coercedConfig.options.c; + config.options.color = coerceToBoolean(config.options.c); + delete config.options.c; } if (typeof config.options.color === 'string') { warnings.push('DEPRECATED: Dredd does not support ' + `--color=${config.options.color} anymore, use --color/--no-color instead`); - coercedConfig.options.color = coerceToBoolean(config.options.color); + config.options.color = coerceToBoolean(config.options.color); } if (config.options.level) { warnings.push('DEPRECATED: Dredd does not support ' @@ -97,48 +87,47 @@ function coerceRemovedOptions(config = {}) { } else { loglevel = 'warn'; } - coercedConfig.options.loglevel = loglevel; - coercedConfig.options.l = loglevel; - delete coercedConfig.options.level; + config.options.loglevel = loglevel; + config.options.l = loglevel; + delete config.options.level; } if (config.options.timestamp || config.options.t) { warnings.push('DEPRECATED: Dredd does not support ' + '--timestamp anymore, use --loglevel=debug instead'); - coercedConfig.options.loglevel = 'debug'; - coercedConfig.options.l = 'debug'; - delete coercedConfig.options.timestamp; - delete coercedConfig.options.t; + config.options.loglevel = 'debug'; + config.options.l = 'debug'; + delete config.options.timestamp; + delete config.options.t; } if (config.options.silent || config.options.q) { warnings.push('DEPRECATED: Dredd does not support ' + '-q/--silent anymore, use --loglevel=silent instead'); - coercedConfig.options.loglevel = 'silent'; - coercedConfig.options.l = 'silent'; - delete coercedConfig.options.silent; - delete coercedConfig.options.q; + config.options.loglevel = 'silent'; + config.options.l = 'silent'; + delete config.options.silent; + delete config.options.q; } if (config.options.sandbox || config.options.b) { errors.push('REMOVED: Dredd does not support ' + 'sandboxed JS hooks anymore, use standard JS hooks instead'); - delete coercedConfig.options.sandbox; - delete coercedConfig.options.b; + delete config.options.sandbox; + delete config.options.b; } } if (config.hooksData) { errors.push('REMOVED: Dredd does not support ' + 'sandboxed JS hooks anymore, use standard JS hooks instead'); - delete coercedConfig.hooksData; + delete config.hooksData; } if (config.blueprintPath) { warnings.push('DEPRECATED: Dredd does not support ' + "the 'blueprintPath' option anymore, use 'path' instead"); - coercedConfig.options = config.options || {}; - coercedConfig.options.path = coerceToArray(coercedConfig.options.path); - coercedConfig.options.path.push(config.blueprintPath); - delete coercedConfig.blueprintPath; + config.options = config.options || {}; + config.options.path = [].concat([config.blueprintPath], coerceToArray(config.options.path)); + delete config.blueprintPath; } - return { config: coercedConfig, errors, warnings }; + return { errors, warnings }; } @@ -179,20 +168,19 @@ function applyConfiguration(inConfig) { // Gracefully deal with the removed options const coerceResult = coerceRemovedOptions(inConfig); - const coercedInConfig = coerceResult.config; // Apply the values from the coerced config over the default ones - const custom = coercedInConfig.custom || {}; + const custom = inConfig.custom || {}; Object.keys(custom) .forEach((key) => { outConfig.custom[key] = clone(custom[key], false); }); - const options = coercedInConfig.options || {}; + const options = inConfig.options || {}; Object.keys(options) .forEach((key) => { outConfig.options[key] = options[key]; }); - Object.keys(coercedInConfig) + Object.keys(inConfig) .filter(key => !['custom', 'options'].includes(key)) - .forEach((key) => { outConfig[key] = coercedInConfig[key]; }); + .forEach((key) => { outConfig[key] = inConfig[key]; }); // Coerce single/multiple options into an array outConfig.options.reporter = coerceToArray(outConfig.options.reporter); diff --git a/test/unit/configuration-test.js b/test/unit/configuration-test.js index c4790f071..a6cd2b586 100644 --- a/test/unit/configuration-test.js +++ b/test/unit/configuration-test.js @@ -171,16 +171,15 @@ describe('configuration.applyLoggingOptions()', () => { describe('configuration._coerceRemovedOptions()', () => { describe("with -c set to string 'true'", () => { + const config = { options: { c: 'true' } }; let coerceResult; before(() => { - coerceResult = configuration._coerceRemovedOptions({ - options: { c: 'true' }, - }); + coerceResult = configuration._coerceRemovedOptions(config); }); it('gets coerced to color set to boolean true', () => { - assert.deepEqual(coerceResult.config, { options: { color: true } }); + assert.deepEqual(config, { options: { color: true } }); }); it('produces no errors', () => { assert.deepEqual(coerceResult.errors, []); @@ -191,16 +190,15 @@ describe('configuration._coerceRemovedOptions()', () => { }); describe("with -c set to string 'false'", () => { + const config = { options: { c: 'false' } }; let coerceResult; before(() => { - coerceResult = configuration._coerceRemovedOptions({ - options: { c: 'false' }, - }); + coerceResult = configuration._coerceRemovedOptions(config); }); it('gets coerced to color set to boolean false', () => { - assert.deepEqual(coerceResult.config, { options: { color: false } }); + assert.deepEqual(config, { options: { color: false } }); }); it('produces no errors', () => { assert.deepEqual(coerceResult.errors, []); @@ -211,16 +209,15 @@ describe('configuration._coerceRemovedOptions()', () => { }); describe('with -c set to true', () => { + const config = { options: { c: true } }; let coerceResult; before(() => { - coerceResult = configuration._coerceRemovedOptions({ - options: { c: true }, - }); + coerceResult = configuration._coerceRemovedOptions(config); }); it('gets coerced to color set to boolean true', () => { - assert.deepEqual(coerceResult.config, { options: { color: true } }); + assert.deepEqual(config, { options: { color: true } }); }); it('produces no errors', () => { assert.deepEqual(coerceResult.errors, []); @@ -231,16 +228,15 @@ describe('configuration._coerceRemovedOptions()', () => { }); describe('with -c set to false', () => { + const config = { options: { c: false } }; let coerceResult; before(() => { - coerceResult = configuration._coerceRemovedOptions({ - options: { c: false }, - }); + coerceResult = configuration._coerceRemovedOptions(config); }); it('gets coerced to color set to boolean false', () => { - assert.deepEqual(coerceResult.config, { options: { color: false } }); + assert.deepEqual(config, { options: { color: false } }); }); it('produces no errors', () => { assert.deepEqual(coerceResult.errors, []); @@ -251,16 +247,15 @@ describe('configuration._coerceRemovedOptions()', () => { }); describe("with --color set to string 'true'", () => { + const config = { options: { color: 'true' } }; let coerceResult; before(() => { - coerceResult = configuration._coerceRemovedOptions({ - options: { color: 'true' }, - }); + coerceResult = configuration._coerceRemovedOptions(config); }); it('gets coerced to color set to boolean true', () => { - assert.deepEqual(coerceResult.config, { options: { color: true } }); + assert.deepEqual(config, { options: { color: true } }); }); it('produces no errors', () => { assert.deepEqual(coerceResult.errors, []); @@ -271,16 +266,15 @@ describe('configuration._coerceRemovedOptions()', () => { }); describe("with --color set to string 'false'", () => { + const config = { options: { color: 'false' } }; let coerceResult; before(() => { - coerceResult = configuration._coerceRemovedOptions({ - options: { color: 'false' }, - }); + coerceResult = configuration._coerceRemovedOptions(config); }); it('gets coerced to color set to boolean false', () => { - assert.deepEqual(coerceResult.config, { options: { color: false } }); + assert.deepEqual(config, { options: { color: false } }); }); it('produces no errors', () => { assert.deepEqual(coerceResult.errors, []); @@ -291,16 +285,15 @@ describe('configuration._coerceRemovedOptions()', () => { }); describe('with --color set to true', () => { + const config = { options: { color: true } }; let coerceResult; before(() => { - coerceResult = configuration._coerceRemovedOptions({ - options: { color: true }, - }); + coerceResult = configuration._coerceRemovedOptions(config); }); it('gets coerced to color set to boolean true', () => { - assert.deepEqual(coerceResult.config, { options: { color: true } }); + assert.deepEqual(config, { options: { color: true } }); }); it('produces no errors', () => { assert.deepEqual(coerceResult.errors, []); @@ -311,16 +304,15 @@ describe('configuration._coerceRemovedOptions()', () => { }); describe('with --color set to false', () => { + const config = { options: { color: false } }; let coerceResult; before(() => { - coerceResult = configuration._coerceRemovedOptions({ - options: { color: false }, - }); + coerceResult = configuration._coerceRemovedOptions(config); }); it('gets coerced to color set to boolean false', () => { - assert.deepEqual(coerceResult.config, { options: { color: false } }); + assert.deepEqual(config, { options: { color: false } }); }); it('produces no errors', () => { assert.deepEqual(coerceResult.errors, []); @@ -331,16 +323,15 @@ describe('configuration._coerceRemovedOptions()', () => { }); describe('with --level/-l set to a supported value', () => { + const config = { options: { l: 'debug', level: 'debug' } }; let coerceResult; before(() => { - coerceResult = configuration._coerceRemovedOptions({ - options: { l: 'debug', level: 'debug' }, - }); + coerceResult = configuration._coerceRemovedOptions(config); }); it('gets coerced to loglevel set to the value', () => { - assert.deepEqual(coerceResult.config, { + assert.deepEqual(config, { options: { l: 'debug', loglevel: 'debug' }, }); }); @@ -353,16 +344,15 @@ describe('configuration._coerceRemovedOptions()', () => { }); describe('with --level/-l set to a consolidated value', () => { + const config = { options: { l: 'verbose', level: 'verbose' } }; let coerceResult; before(() => { - coerceResult = configuration._coerceRemovedOptions({ - options: { l: 'verbose', level: 'verbose' }, - }); + coerceResult = configuration._coerceRemovedOptions(config); }); it('gets coerced to loglevel set to a corresponding value', () => { - assert.deepEqual(coerceResult.config, { + assert.deepEqual(config, { options: { l: 'debug', loglevel: 'debug' }, }); }); @@ -375,16 +365,15 @@ describe('configuration._coerceRemovedOptions()', () => { }); describe('with --level/-l set to a removed value', () => { + const config = { options: { l: 'complete', level: 'complete' } }; let coerceResult; before(() => { - coerceResult = configuration._coerceRemovedOptions({ - options: { l: 'complete', level: 'complete' }, - }); + coerceResult = configuration._coerceRemovedOptions(config); }); it('gets coerced to loglevel set to the default value', () => { - assert.deepEqual(coerceResult.config, { + assert.deepEqual(config, { options: { l: 'warn', loglevel: 'warn' }, }); }); @@ -397,16 +386,15 @@ describe('configuration._coerceRemovedOptions()', () => { }); describe("with -l set to 'silent'", () => { + const config = { options: { l: 'silent' } }; let coerceResult; before(() => { - coerceResult = configuration._coerceRemovedOptions({ - options: { l: 'silent' }, - }); + coerceResult = configuration._coerceRemovedOptions(config); }); it('gets coerced to loglevel set to silent', () => { - assert.deepEqual(coerceResult.config, { + assert.deepEqual(config, { options: { l: 'silent', loglevel: 'silent' }, }); }); @@ -419,16 +407,15 @@ describe('configuration._coerceRemovedOptions()', () => { }); describe('with --timestamp/-t set', () => { + const config = { options: { timestamp: true, t: true } }; let coerceResult; before(() => { - coerceResult = configuration._coerceRemovedOptions({ - options: { timestamp: true, t: true }, - }); + coerceResult = configuration._coerceRemovedOptions(config); }); it('gets coerced to loglevel set to debug', () => { - assert.deepEqual(coerceResult.config, { + assert.deepEqual(config, { options: { l: 'debug', loglevel: 'debug' }, }); }); @@ -441,16 +428,15 @@ describe('configuration._coerceRemovedOptions()', () => { }); describe('with --silent/-q set', () => { + const config = { options: { silent: true, q: true } }; let coerceResult; before(() => { - coerceResult = configuration._coerceRemovedOptions({ - options: { silent: true, q: true }, - }); + coerceResult = configuration._coerceRemovedOptions(config); }); it('gets coerced to loglevel set to silent', () => { - assert.deepEqual(coerceResult.config, { + assert.deepEqual(config, { options: { l: 'silent', loglevel: 'silent' }, }); }); @@ -463,16 +449,15 @@ describe('configuration._coerceRemovedOptions()', () => { }); describe('with --sandbox/-b set', () => { + const config = { options: { sandbox: true, b: true } }; let coerceResult; before(() => { - coerceResult = configuration._coerceRemovedOptions({ - options: { sandbox: true, b: true }, - }); + coerceResult = configuration._coerceRemovedOptions(config); }); it('gets removed', () => { - assert.deepEqual(coerceResult.config, { options: {} }); + assert.deepEqual(config, { options: {} }); }); it('produces one error', () => { assert.lengthOf(coerceResult.errors, 1); @@ -483,16 +468,15 @@ describe('configuration._coerceRemovedOptions()', () => { }); describe('with hooksData set', () => { + const config = { hooksData: {} }; let coerceResult; before(() => { - coerceResult = configuration._coerceRemovedOptions({ - hooksData: {}, - }); + coerceResult = configuration._coerceRemovedOptions(config); }); it('gets removed', () => { - assert.deepEqual(coerceResult.config, {}); + assert.deepEqual(config, {}); }); it('produces one error', () => { assert.lengthOf(coerceResult.errors, 1); @@ -503,16 +487,15 @@ describe('configuration._coerceRemovedOptions()', () => { }); describe('with blueprintPath set and empty path', () => { + const config = { blueprintPath: 'foo/bar' }; let coerceResult; before(() => { - coerceResult = configuration._coerceRemovedOptions({ - blueprintPath: 'foo/bar', - }); + coerceResult = configuration._coerceRemovedOptions(config); }); it('gets reassigned as path', () => { - assert.deepEqual(coerceResult.config, { options: { path: ['foo/bar'] } }); + assert.deepEqual(config, { options: { path: ['foo/bar'] } }); }); it('produces no errors', () => { assert.deepEqual(coerceResult.errors, []); @@ -523,18 +506,16 @@ describe('configuration._coerceRemovedOptions()', () => { }); describe('with blueprintPath set and path set to a string', () => { + const config = { blueprintPath: 'foo/bar', options: { path: 'moo.js' } }; let coerceResult; before(() => { - coerceResult = configuration._coerceRemovedOptions({ - blueprintPath: 'foo/bar', - options: { path: 'moo.js' }, - }); + coerceResult = configuration._coerceRemovedOptions(config); }); it('gets reassigned as path', () => { - assert.deepEqual(coerceResult.config, { - options: { path: ['moo.js', 'foo/bar'] }, + assert.deepEqual(config, { + options: { path: ['foo/bar', 'moo.js'] }, }); }); it('produces no errors', () => { @@ -546,18 +527,16 @@ describe('configuration._coerceRemovedOptions()', () => { }); describe('with blueprintPath set and path set to an array', () => { + const config = { blueprintPath: 'foo/bar', options: { path: ['moo.js'] } }; let coerceResult; before(() => { - coerceResult = configuration._coerceRemovedOptions({ - blueprintPath: 'foo/bar', - options: { path: ['moo.js'] }, - }); + coerceResult = configuration._coerceRemovedOptions(config); }); it('gets reassigned as path', () => { - assert.deepEqual(coerceResult.config, { - options: { path: ['moo.js', 'foo/bar'] }, + assert.deepEqual(config, { + options: { path: ['foo/bar', 'moo.js'] }, }); }); it('produces no errors', () => {