From 1de1af6b0c9c29657fc79a2ebaa8c92739650c56 Mon Sep 17 00:00:00 2001 From: Isaac Murchie Date: Tue, 20 Dec 2016 12:55:23 -0500 Subject: [PATCH] Remove logger, and start using one from appium-support --- README.md | 1 - index.js | 4 -- lib/basedriver/logger.js | 4 +- lib/express/logger.js | 4 +- lib/jsonwp-proxy/proxy.js | 5 +- lib/logger/README.md | 89 ----------------------------- lib/logger/logger.js | 80 -------------------------- lib/mjsonwp/mjsonwp.js | 6 +- package.json | 3 +- test/logger/helpers.js | 54 ------------------ test/logger/logger-force-specs.js | 37 ------------ test/logger/logger-normal-specs.js | 90 ------------------------------ test/logger/logger-test-specs.js | 38 ------------- 13 files changed, 10 insertions(+), 405 deletions(-) delete mode 100644 lib/logger/README.md delete mode 100644 lib/logger/logger.js delete mode 100644 test/logger/helpers.js delete mode 100644 test/logger/logger-force-specs.js delete mode 100644 test/logger/logger-normal-specs.js delete mode 100644 test/logger/logger-test-specs.js diff --git a/README.md b/README.md index 287716cdf..0faa0bfc9 100644 --- a/README.md +++ b/README.md @@ -15,7 +15,6 @@ This is the parent class that all [appium](appium.io) drivers inherit from, alon * [The Mobile JSON Wire Protocol Encapsulation](lib/mjsonwp) * [The JSONWP Proxy Library](lib/jsonwp-proxy) * [The JSONWP Status Library](lib/jsonwp-status) -* [The Appium Logger](lib/logger) *Note*: Issue tracking for this repo has been disabled. Please use the [main Appium issue tracker](https://github.com/appium/appium/issues) instead. diff --git a/index.js b/index.js index 0345bed9c..cd84a4b10 100644 --- a/index.js +++ b/index.js @@ -27,7 +27,3 @@ export { JWProxy }; import { codes, getSummaryByCode } from './lib/jsonwp-status/status'; const statusCodes = codes; export { statusCodes, getSummaryByCode }; - -// logger exports -import { getLogger, patchLogger } from './lib/logger/logger'; -export { getLogger, patchLogger }; diff --git a/lib/basedriver/logger.js b/lib/basedriver/logger.js index 9b41dd2ed..1e8b06765 100644 --- a/lib/basedriver/logger.js +++ b/lib/basedriver/logger.js @@ -1,4 +1,4 @@ -import { getLogger } from '../logger/logger'; +import { logger } from 'appium-support'; -const log = getLogger('BaseDriver'); +const log = logger.getLogger('BaseDriver'); export default log; diff --git a/lib/express/logger.js b/lib/express/logger.js index cfd47b159..39fdaa0fb 100644 --- a/lib/express/logger.js +++ b/lib/express/logger.js @@ -1,4 +1,4 @@ -import { getLogger } from '../logger/logger'; +import { logger } from 'appium-support'; -const log = getLogger('HTTP'); +const log = logger.getLogger('HTTP'); export default log; diff --git a/lib/jsonwp-proxy/proxy.js b/lib/jsonwp-proxy/proxy.js index 468404b25..5727a8024 100644 --- a/lib/jsonwp-proxy/proxy.js +++ b/lib/jsonwp-proxy/proxy.js @@ -1,13 +1,12 @@ // jshint ignore: start import _ from 'lodash'; -import { getLogger } from '../logger/logger'; -import { util } from 'appium-support'; +import { logger, util } from 'appium-support'; import request from 'request-promise'; import { getSummaryByCode } from '../jsonwp-status/status'; import { errors } from '../mjsonwp/errors'; -const log = getLogger('JSONWP Proxy'); +const log = logger.getLogger('JSONWP Proxy'); // TODO: Make this value configurable as a server side capability const LOG_OBJ_LENGTH = 1024; // MAX LENGTH Logged to file / console diff --git a/lib/logger/README.md b/lib/logger/README.md deleted file mode 100644 index 69829daa0..000000000 --- a/lib/logger/README.md +++ /dev/null @@ -1,89 +0,0 @@ -## logger - -Basic logger defaulting to [npmlog](https://github.com/npm/npmlog) with special consideration for running -tests (doesn't output logs when run with `_TESTING=1` in the env). - -### Logging levels - -There are a number of levels, exposed as methods on the log object, at which logging can be made. The built-in ones correspond to those of [npmlog](https://github.com/npm/npmlog#loglevelprefix-message-), and are: -`silly`, `verbose`, `info`, `http`, `warn`, and `error`. In addition there is a `debug` level. - -The default threshhold level is `verbose`. - -The logged output, by default, will be `level prefix message`. So - -```js -import { getLogger } from 'appium-base-driver'; -let log = getLogger('mymodule'); -log.warn('a warning');` -``` - -Will produce - -```shell -warn mymodule a warning -``` - - -### Environment variables - -There are two environment variable flags that affect the way `appium-base-driver` `logger` works. - -`_TESTING` - -- `_TESTING=1` stops output of logs when set to `1`. - -`_FORCE_LOGS` - -- This flag, when set to `1`, reverses the `_TESTING` - - -### Usage - -`log.level` - -- get and set the threshhold level at which to display the logs. Any logs at or above this level will be displayed. The special level silent will prevent anything from being displayed ever. See [npmlog#level](https://github.com/npm/npmlog#loglevel). - -`log[level](message)` - -- logs to `level` - ```js - import { getLogger } from 'appium-base-driver'; - let log = getLogger('mymodule'); - - log.info('hi!'); - // => info mymodule hi! - ``` - -`log.unwrap()` - -- retrieves the underlying [npmlog](https://github.com/npm/npmlog) object, in order to manage how logging is done at a low level (e.g., changing output streams, retrieving an array of messages, adding log levels, etc.). - - ```js - import { getLogger } from 'appium-base-driver'; - let log = getLogger('mymodule'); - - log.info('hi!'); - - let npmlogger = log.unwrap(); - - // any `npmlog` methods - let logs = npmlogger.record; - // logs === [ { id: 0, level: 'info', prefix: 'mymodule', message: 'hi!', messageRaw: [ 'hi!' ] }] - ``` - -`log.errorAndThrow(error)` - -- logs the error passed in, at `error` level, and then throws the error. If the error passed in is not an instance of [Error](https://nodejs.org/api/errors.html#errors_class_error) (either directly, or a subclass of `Error`) it will be wrapped in a generic `Error` object. - - ```js - import { getLogger } from 'appium-base-driver'; - let log = getLogger('mymodule'); - - // previously there would be two lines - log.error('This is an error'); - throw new Error('This is an error'); - - // now is compacted - log.errorAndThrow('This is an error'); - ``` diff --git a/lib/logger/logger.js b/lib/logger/logger.js deleted file mode 100644 index 8d6c4000a..000000000 --- a/lib/logger/logger.js +++ /dev/null @@ -1,80 +0,0 @@ -import npmlog from 'npmlog'; - - -// levels that are available from `npmlog` -const NPM_LEVELS = ['silly', 'verbose', 'debug', 'info', 'http', 'warn', 'error']; - -// mock log object used in testing mode -let mockLog = {}; -for (let level of NPM_LEVELS) { - mockLog[level] = () => {}; -} - -function patchLogger (logger) { - if (!logger.debug) { - logger.addLevel('debug', 1000, { fg: 'blue', bg: 'black' }, 'dbug'); - } -} - -function _getLogger () { - // check if the user set the `_TESTING` or `_FORCE_LOGS` flag - const testingMode = parseInt(process.env._TESTING, 10) === 1; - const forceLogMode = parseInt(process.env._FORCE_LOGS, 10) === 1; - - // if is possible that there is a logger instance that is already around, - // in which case we want t o use that - const usingGlobalLog = !!global._global_npmlog; - let logger; - if (testingMode && !forceLogMode) { - // in testing mode, use a mock logger object that we can query - logger = mockLog; - } else { - // otherwise, either use the global, or a new `npmlog` object - logger = global._global_npmlog || npmlog; - } - patchLogger(logger); - return [logger, usingGlobalLog]; -} - -function getLogger (prefix = null) { - let [logger, usingGlobalLog] = _getLogger(); - - // wrap the logger so that we can catch and modify any logging - let wrappedLogger = {unwrap: () => logger}; - - // allow access to the level of the underlying logger - Object.defineProperty(wrappedLogger, 'level', { - get: () => { return logger.level; }, - set: (newValue) => { logger.level = newValue; }, - enumerable: true, - configurable: true - }); - // add all the levels from `npmlog`, and map to the underlying logger - for (let level of NPM_LEVELS) { - wrappedLogger[level] = logger[level].bind(logger, prefix); - } - // add method to log an error, and throw it, for convenience - wrappedLogger.errorAndThrow = function (err) { - // make sure we have an `Error` object. Wrap if necessary - if (!(err instanceof Error)) { - err = new Error(err); - } - // log and throw - this.error(err); - throw err; - }; - if (!usingGlobalLog) { - // if we're not using a global log specified from some top-level package, - // set the log level to a default of verbose. Otherwise, let the top-level - // package set the log level - wrappedLogger.level = 'verbose'; - } - wrappedLogger.levels = NPM_LEVELS; - return wrappedLogger; -} - -// export a default logger with no prefix -const log = getLogger(); - -export { patchLogger, getLogger }; -export default log; diff --git a/lib/mjsonwp/mjsonwp.js b/lib/mjsonwp/mjsonwp.js index d8a4bddfe..f1a231a2c 100644 --- a/lib/mjsonwp/mjsonwp.js +++ b/lib/mjsonwp/mjsonwp.js @@ -1,13 +1,13 @@ // jshint ignore: start import _ from 'lodash'; -import { getLogger } from '../logger/logger'; +import { logger, util } from 'appium-support'; import { validators } from './validators'; import { errors, isErrorType, MJSONWPError, errorFromCode } from './errors'; import { METHOD_MAP, NO_SESSION_ID_COMMANDS } from './routes'; import B from 'bluebird'; -import { util } from 'appium-support'; -const log = getLogger('MJSONWP'); + +const log = logger.getLogger('MJSONWP'); const JSONWP_SUCCESS_STATUS_CODE = 0; // TODO: Make this value configurable as a server side capability const LOG_OBJ_LENGTH = 1024; // MAX LENGTH Logged to file / console diff --git a/package.json b/package.json index 0fed91063..530c44ca8 100644 --- a/package.json +++ b/package.json @@ -24,7 +24,7 @@ }, "dependencies": { "adm-zip": "^0.4.7", - "appium-support": "^2.0.9", + "appium-support": "^2.4.0", "asyncbox": "^2.3.1", "babel-runtime": "=5.8.24", "bluebird": "^2.10.2", @@ -35,7 +35,6 @@ "lodash": "^4.0.0", "method-override": "^2.3.5", "morgan": "^1.6.1", - "npmlog": "^4.0.1", "request": "^2.64.0", "request-promise": "^4.1.1", "serve-favicon": "^2.3.0", diff --git a/test/logger/helpers.js b/test/logger/helpers.js deleted file mode 100644 index a8b4af996..000000000 --- a/test/logger/helpers.js +++ /dev/null @@ -1,54 +0,0 @@ -import chai from 'chai'; -import sinon from 'sinon'; -import _ from 'lodash'; -import { getLogger } from '../..'; - - -chai.should(); - -function setupWriters () { - return {'stdout': sinon.spy(process.stdout, 'write'), - 'stderr': sinon.spy(process.stderr, 'write')}; -} - -function getDynamicLogger (testingMode, forceLogs, prefix=null) { - process.env._TESTING = testingMode ? '1' : '0'; - process.env._FORCE_LOGS = forceLogs ? '1' : '0'; - return getLogger(prefix); -} - -function restoreWriters (writers) { - for (let w of _.values(writers)) { - w.restore(); - } -} - -function someoneHadOutput (writers, output) { - let hadOutput = false; - let matchOutput = sinon.match(function (value) { - return value && value.indexOf(output) >= 0; - }, 'matchOutput'); - - for (let writer of _.values(writers)) { - if (writer.calledWith) { - hadOutput = writer.calledWithMatch(matchOutput); - if (hadOutput) break; - } - } - return hadOutput; -} - -function assertOutputContains (writers, output) { - if (!someoneHadOutput(writers, output)) { - throw new Error(`Expected something to have been called with: '${output}'`); - } -} - -function assertOutputDoesntContain (writers, output) { - if (someoneHadOutput(writers, output)) { - throw new Error(`Expected nothing to have been called with: '${output}'`); - } -} - -export { setupWriters, restoreWriters, assertOutputContains, - assertOutputDoesntContain, getDynamicLogger }; diff --git a/test/logger/logger-force-specs.js b/test/logger/logger-force-specs.js deleted file mode 100644 index e87f59e22..000000000 --- a/test/logger/logger-force-specs.js +++ /dev/null @@ -1,37 +0,0 @@ -// transpile:mocha - -import { getDynamicLogger, restoreWriters, setupWriters, - assertOutputContains } from './helpers'; - -describe('logger with force log', () => { - let writers, log; - before(() => { - writers = setupWriters(); - log = getDynamicLogger(true, true); - log.level = 'silly'; - }); - - after(() => { - restoreWriters(writers); - }); - - it('should not rewrite log levels even during testing', () => { - log.silly('silly'); - assertOutputContains(writers, 'silly'); - log.verbose('verbose'); - assertOutputContains(writers, 'verbose'); - log.verbose('debug'); - assertOutputContains(writers, 'debug'); - log.info('info'); - assertOutputContains(writers, 'info'); - log.http('http'); - assertOutputContains(writers, 'http'); - log.warn('warn'); - assertOutputContains(writers, 'warn'); - log.error('error'); - assertOutputContains(writers, 'error'); - (() => { log.errorAndThrow('msg'); }).should.throw('msg'); - assertOutputContains(writers, 'error'); - assertOutputContains(writers, 'msg'); - }); -}); diff --git a/test/logger/logger-normal-specs.js b/test/logger/logger-normal-specs.js deleted file mode 100644 index d4c149899..000000000 --- a/test/logger/logger-normal-specs.js +++ /dev/null @@ -1,90 +0,0 @@ -// transpile:mocha - -import { getDynamicLogger, restoreWriters, setupWriters, - assertOutputContains, assertOutputDoesntContain } from './helpers'; - -describe('normal logger', () => { - let writers, log; - beforeEach(() => { - writers = setupWriters(); - log = getDynamicLogger(false, false); - log.level = 'silly'; - }); - - afterEach(() => { - restoreWriters(writers); - }); - - it('should not rewrite log levels outside of testing', () => { - log.silly('silly'); - assertOutputContains(writers, 'silly'); - log.verbose('verbose'); - assertOutputContains(writers, 'verbose'); - log.verbose('debug'); - assertOutputContains(writers, 'debug'); - log.info('info'); - assertOutputContains(writers, 'info'); - log.http('http'); - assertOutputContains(writers, 'http'); - log.warn('warn'); - assertOutputContains(writers, 'warn'); - log.error('error'); - assertOutputContains(writers, 'error'); - }); - it('throw should not rewrite log levels outside of testing and throw error', () => { - (() => { log.errorAndThrow('msg1'); }).should.throw('msg1'); - (() => { log.errorAndThrow(new Error('msg2')); }).should.throw('msg2'); - assertOutputContains(writers, 'msg1'); - assertOutputContains(writers, 'msg2'); - }); - it('should get and set log levels', () => { - log.level = 'warn'; - log.level.should.equal('warn'); - log.info('information'); - log.warn('warning'); - assertOutputDoesntContain(writers, 'information'); - assertOutputContains(writers, 'warning'); - }); -}); - -describe('normal logger with prefix', () => { - let writers, log; - before(() => { - writers = setupWriters(); - log = getDynamicLogger(false, false, 'myprefix'); - log.level = 'silly'; - }); - - after(() => { - restoreWriters(writers); - }); - - it('should not rewrite log levels outside of testing', () => { - log.silly('silly'); - assertOutputContains(writers, 'silly'); - assertOutputContains(writers, 'myprefix'); - log.verbose('verbose'); - assertOutputContains(writers, 'verbose'); - assertOutputContains(writers, 'myprefix'); - log.verbose('debug'); - assertOutputContains(writers, 'debug'); - assertOutputContains(writers, 'myprefix'); - log.info('info'); - assertOutputContains(writers, 'info'); - assertOutputContains(writers, 'myprefix'); - log.http('http'); - assertOutputContains(writers, 'http'); - assertOutputContains(writers, 'myprefix'); - log.warn('warn'); - assertOutputContains(writers, 'warn'); - assertOutputContains(writers, 'myprefix'); - log.error('error'); - assertOutputContains(writers, 'error'); - assertOutputContains(writers, 'myprefix'); - }); - it('throw should not rewrite log levels outside of testing and throw error', () => { - (() => { log.errorAndThrow('msg'); }).should.throw('msg'); - assertOutputContains(writers, 'error'); - assertOutputContains(writers, 'myprefix'); - }); -}); diff --git a/test/logger/logger-test-specs.js b/test/logger/logger-test-specs.js deleted file mode 100644 index 81a17a634..000000000 --- a/test/logger/logger-test-specs.js +++ /dev/null @@ -1,38 +0,0 @@ -// transpile:mocha - -import { getDynamicLogger, restoreWriters, setupWriters, - assertOutputDoesntContain } from './helpers'; - -describe('test logger', () => { - let writers, log; - before(() => { - writers = setupWriters(); - log = getDynamicLogger(true); - }); - - after(() => { - restoreWriters(writers); - }); - - it('should contains levels', () => { - log.levels.should.have.length.above(3); - log.levels[2].should.equal('debug'); - }); - - it('should unwrap', () => { - log.unwrap.should.exist; - log.unwrap().should.exist; - }); - - it('should rewrite npmlog levels during testing', () => { - const text = 'hi'; - log.silly(text); - log.verbose(text); - log.info(text); - log.http(text); - log.warn(text); - log.error(text); - (() => { log.errorAndThrow(text); }).should.throw(text); - assertOutputDoesntContain(writers, text); - }); -});