From baabb295a397c42f604aaed645b8efd40d0b7078 Mon Sep 17 00:00:00 2001 From: acatl pacheco Date: Wed, 31 Jan 2018 10:06:54 -0500 Subject: [PATCH 01/11] feat(bench-trial): new package to benchmark performance (#192) --- packages/bench-trial/README.md | 94 +++++ .../bench-trial/examples/array-iteration.js | 43 +++ .../bench-trial/examples/async-example.js | 30 ++ packages/bench-trial/examples/manual-tests.js | 33 ++ packages/bench-trial/lib/index.js | 54 +++ packages/bench-trial/package.json | 22 ++ packages/bench-trial/runner.js | 333 ++++++++++++++++++ yarn.lock | 4 +- 8 files changed, 611 insertions(+), 2 deletions(-) create mode 100644 packages/bench-trial/README.md create mode 100644 packages/bench-trial/examples/array-iteration.js create mode 100644 packages/bench-trial/examples/async-example.js create mode 100644 packages/bench-trial/examples/manual-tests.js create mode 100644 packages/bench-trial/lib/index.js create mode 100644 packages/bench-trial/package.json create mode 100755 packages/bench-trial/runner.js diff --git a/packages/bench-trial/README.md b/packages/bench-trial/README.md new file mode 100644 index 00000000..d4ac74d6 --- /dev/null +++ b/packages/bench-trial/README.md @@ -0,0 +1,94 @@ +# Bench trial + +> Runs one or multiple benchmark tests + +## Install + +```bash +npm install -g bench-trial +``` + +## Usage + +```bash +bench-trial benchmarks/map-helper-vs-entity.js -i 5 +``` + +## TL;DR + +Runs one (or more) BenchmarkJs test multiple times enough to get less ambiguous results, includes basic testing to make sure tests are reliable. + +## Why? + +While running [benchmarkjs](https://benchmarkjs.com) to compare different versions of code I found out a couple of things: + +- **Ambiguous results**: I noticed that the same benchmark tests were returning different results every time they executed. If they were re-run consecutively, I would get more operations per second on each benchmark. I believe the reason may be related to the v8 engine warming up and optimizing the code the more it ran, since if I let some time to "cool off" the operations per second for each test would decrease. These ambiguous results meant having to repeat tests to ensure some consistency. +- **Unreliable execution**: Occasionally I made changes to the benchmarked code and would overlook that it was not executing correctly, further compounding the issue of making the results unreliable. + +## Solution + +- **Consistency**: By running benchmark tests more than once, we can get median and average results and get a bigger picture with less fluctuation. Because the tests will run multiple times in succession, the code will get optimized by the engine, and we can use the median time as a more consistent and stable metric. + +- **Reliable execution**: By running simple assertion tests on each suite before the actual benchmark runs, we can be sure our tests are executing correctly. + +## API + +```bash +bench-trial [-i ] [-s] +``` + +- `-i --iterations ` iterations default to 10 iterations if not provided. +- `-s --skip-tests` if provided, it will skip the assertion tests. + +### Writing your benchmark suites + +The file provided to **bench-trial** should export an `array` of test suites, each test suite is an object in the form of: + +``` +{ + name: string, + test: function, + benchmark: function +} +``` + +| Property | Type | Description | +|:---|:---|:---| +| *name* | `String` | Name that describes the test you are running | +| *test* | `function` | function to run assertion test against the result of the code you want to benchmark | +| *benchmark* | `function` | function to pass to benchmarkjs Suite that actually runs the benchmark | + +#### Sync vs Async + +- Synchronous methods are simple methods that expect a return value. +- Asynchronous methods are a bit different to benchmarkjs async methods, bench-trial expects async methods to follow the [error-first callbacks](https://nodejs.org/api/errors.html#errors_error_first_callbacks). + +#### Testing + +bench-trial provides a convenience method that accepts the function to execute and a value to check against the result of the code you are testing. It takes care of async vs async depending on how you set the `async` flag. + +```js +test(test:function, value:*) +``` + +To write your manual test see the manual test example below + +## Examples + +- Test synchronous code [example](examples/array-iteration.js) +- Test asynchronous code [example](examples/async-example.js) +- Write manual test sync/asynchronous code [example](examples/manual-tests.js) + +## Acknowledgements + +This tool is a wrapper of [benchmarkjs](https://benchmarkjs.com), so all credit related to benchmarking itself really goes to them. + +Thanks to [Paul Molluzzo](https://github.com/paulmolluzzo) for coming up with the name **bench-trial**! + +## Contributing + +Please read [CONTRIBUTING.md](https://github.com/ViacomInc/data-point/blob/master/CONTRIBUTING.md) for details on our code of conduct, and the process for submitting pull requests to us. + +## License + +This project is licensed under the Apache License Version 2.0 - see the [LICENSE](LICENSE) file for details diff --git a/packages/bench-trial/examples/array-iteration.js b/packages/bench-trial/examples/array-iteration.js new file mode 100644 index 00000000..8453546d --- /dev/null +++ b/packages/bench-trial/examples/array-iteration.js @@ -0,0 +1,43 @@ +const { test } = require('bench-trial') + +const array = Array(100).fill('foo') +const expected = array.join('').length + +function forLoop () { + let result = '' + for (let index = 0; index < array.length; index++) { + result = result + array[index] + } + + const length = result.length + result = '' + return length +} + +function whileLoop () { + let result = '' + let index = 0 + while (index !== array.length) { + result = result + array[index] + index++ + } + + const length = result.length + result = '' + return length +} + +module.exports = [ + { + async: false, + name: 'while-loop', + test: test(whileLoop, expected), + benchmark: whileLoop + }, + { + async: false, + name: 'for-loop', + test: test(forLoop, expected), + benchmark: forLoop + } +] diff --git a/packages/bench-trial/examples/async-example.js b/packages/bench-trial/examples/async-example.js new file mode 100644 index 00000000..70d7dfa1 --- /dev/null +++ b/packages/bench-trial/examples/async-example.js @@ -0,0 +1,30 @@ +const { test } = require('bench-trial') + +const expected = true + +function testPromise (done) { + Promise.resolve(true).then(() => { + done(null, true) + }) +} + +function testSetTimeOut (done) { + setTimeout(() => { + done(null, true) + }, 0) +} + +module.exports = [ + { + async: true, + name: 'promise', + test: test(testPromise, expected), + benchmark: testPromise + }, + { + async: true, + name: 'timeout', + test: test(testSetTimeOut, expected), + benchmark: testSetTimeOut + } +] diff --git a/packages/bench-trial/examples/manual-tests.js b/packages/bench-trial/examples/manual-tests.js new file mode 100644 index 00000000..7e76f211 --- /dev/null +++ b/packages/bench-trial/examples/manual-tests.js @@ -0,0 +1,33 @@ +const assert = require('assert') + +const expected = 2 + +function addsNumbersSync () { + return 1 + 1 +} + +function addsNumbersAsync (done) { + Promise.resolve().then(() => { + done(null, 1 + 1) + }) +} + +module.exports = [ + { + async: false, + name: 'addsNumbersSync', + test: () => assert.deepEqual(addsNumbersSync(), expected), + benchmark: addsNumbersSync + }, + { + async: true, + name: 'addsNumbersAsync', + test: done => { + addsNumbersAsync((e, val) => { + assert.deepEqual(val, expected) + done(null) + }) + }, + benchmark: addsNumbersAsync + } +] diff --git a/packages/bench-trial/lib/index.js b/packages/bench-trial/lib/index.js new file mode 100644 index 00000000..7e17dedf --- /dev/null +++ b/packages/bench-trial/lib/index.js @@ -0,0 +1,54 @@ +const assert = require('assert') + +function testSync (method, expected) { + return () => assert.deepEqual(method(), expected) +} + +function testAsync (method, expected) { + return done => { + return method((err, value) => { + if (err) { + return done(err) + } + try { + assert.deepEqual(value, expected) + done(null, value) + } catch (e) { + done(e) + } + }) + } +} + +function test (method, expected) { + return { + test: method, + expected + } +} + +test.sync = testSync +test.async = testAsync + +function benchmarkSync (method) { + return method +} + +function benchmarkAsync (method) { + return deferred => { + return method((err, value) => { + if (err) { + throw err + } + deferred.resolve() + }) + } +} + +module.exports = { + test, + benchmark: { + sync: benchmarkSync, + async: benchmarkAsync + } +} diff --git a/packages/bench-trial/package.json b/packages/bench-trial/package.json new file mode 100644 index 00000000..1f2a4f1d --- /dev/null +++ b/packages/bench-trial/package.json @@ -0,0 +1,22 @@ +{ + "name": "bench-trial", + "version": "2.0.0", + "description": "Runs one or multiple benchmark tests", + "main": "./lib/index.js", + "license": "Apache-2.0", + "engines": { + "node": ">=6" + }, + "bin": { + "bench-trial": "runner.js" + }, + "author": { + "name": "Acatl Pacheco", + "email": "acatl.pacheco@viacom.com" + }, + "dependencies": { + "benchmark": "^2.1.4", + "commander": "^2.13.0", + "chalk": "^2.3.0" + } +} diff --git a/packages/bench-trial/runner.js b/packages/bench-trial/runner.js new file mode 100755 index 00000000..0c5f2289 --- /dev/null +++ b/packages/bench-trial/runner.js @@ -0,0 +1,333 @@ +#!/usr/bin/env node --expose-gc + +const path = require('path') +const program = require('commander') +const Benchmark = require('benchmark') +const Promise = require('bluebird') +const chalk = require('chalk') + +const pkg = require('./package.json') +const lib = require('./lib') + +program + .version(pkg.version) + .usage('') + .option( + '-i, --iterations [count]', + 'Number of iterations, defaults to 10', + parseInt + ) + .option('-s, --skip-tests', 'skip tests') + +program.parse(process.argv) + +const filePath = program.args[0] + +const testSuite = require(path.resolve(filePath)) + +const suites = Array.isArray(testSuite) ? testSuite : [testSuite] + +const iterations = program.iterations || 10 + +function start (suites) { + console.log( + 'Running %s suite(s) with %s iterations each\n', + chalk.yellow(suites.length), + chalk.yellow(iterations) + ) + return runTests(suites) + .then(runBenchmarks) + .then(reportFinal) + .catch(err => { + console.error(chalk.red('\nFailed to run benchmark\n')) + console.log(err.stack) + process.exit(1) + }) +} + +function runTest (suite) { + const isAsync = !!suite.async + + if (typeof suite.test === 'function') { + if (isAsync) { + return Promise.fromCallback(suite.test) + } + + return suite.test() + } + + if (typeof suite.test.test === 'function') { + const { test, expected } = suite.test + + if (isAsync) { + return Promise.fromCallback(lib.test.async(test, expected)) + } + + return lib.test.sync(test, expected)() + } + + throw new Error('Test was not provided or has invalid form') +} + +function runTests (suites) { + if (program.skipTests) { + console.warn('%s Tests are skiped\n', chalk.yellow('WARNING:')) + return Promise.resolve(suites) + } + + console.log(chalk.white.bold('Test suite(s):')) + return Promise.each(suites, suite => { + return Promise.resolve(suite) + .then(runTest) + .then(() => { + console.log(' %s %s', chalk.green(' ✔ '), suite.name) + }) + .catch(err => { + console.error( + '%s %s Error: %s', + chalk.red(' ✕ '), + suite.name, + chalk.red(err.toString()) + ) + throw err + }) + }).return(suites) +} + +function runBenchmarks (suites) { + return Promise.reduce( + suites, + (acc, suite) => { + return runGC(suite) + .then(suite => { + suite.memoryBefore = process.memoryUsage().heapUsed + return runBenchmark(suite) + }) + .then(suite => { + suite.memoryAfter = process.memoryUsage().heapUsed + suite.memoryEfficiency = suite.memoryAfter - suite.memoryBefore + return suite + }) + .then(suite => { + acc.push(suite) + return acc + }) + }, + [] + ) +} + +function reportFinal (suites) { + console.log(chalk.white.bold('\nReport:')) + + if (suites.length === 2) { + reportFasterOpsperSec(suites) + } + + if (suites.length > 2) { + listBySpeed(suites) + } + + const hzSet = suites.map(suite => suite.median) + console.log( + '\n Total number of operations per second: %s', + chalk.yellow(fnumber(sum(hzSet)) + 'Hz') + ) + + return suites +} + +function listBySpeed (suites) { + console.log(chalk.white.bold('\n Fastest (median ops/sec):')) + const sorted = suites.sort((a, b) => b.median - a.median) + sorted.forEach((suite, index) => { + const name = index === 0 ? chalk.yellow(suite.name) : chalk.bold(suite.name) + + console.log(' %s: %s', name, chalk.yellow(fnumber(suite.median) + 'Hz')) + }) +} + +function reportFasterOpsperSec (suites) { + const sorted = suites.sort((a, b) => b.median - a.median) + const first = sorted[0] + const second = sorted[1] + + const diffMedian = (first.median - second.median) / first.median * 100 + + console.log( + ` Speed: %s was faster by %s (%s vs %s)`, + chalk.yellow(first.name), + chalk.white.bold(diffMedian.toFixed(2) + '%'), + chalk.yellow(fnumber(first.median) + 'Hz'), + chalk.yellow(fnumber(second.median) + 'Hz') + ) +} + +function runBenchmark (suiteBenchmark) { + return new Promise((resolve, reject) => { + const suite = new Benchmark.Suite() + + const asyncLabel = suiteBenchmark.async ? 'ASYNC' : 'SYNC' + console.log( + '\n%s %s [%s]\n', + chalk.white.bold('Benchmarking:'), + chalk.bold(suiteBenchmark.name), + asyncLabel + ) + + const isAsync = !!suiteBenchmark.async + const benchmarkMethod = + isAsync === true + ? lib.benchmark.async(suiteBenchmark.benchmark) + : suiteBenchmark.benchmark + + for (let index = 0; index < iterations; index++) { + suite.add(`${index + 1} ${suiteBenchmark.name}`, { + defer: isAsync, + fn: benchmarkMethod + }) + } + + // add listeners + suite + .on('cycle', function (event) { + console.log('', String(event.target)) + }) + .on('error', reject) + .on('complete', function () { + const benchmarks = Array.from(this) + const hzSet = benchmarks + .map(benchmark => benchmark.hz) + .sort((a, b) => a - b) + const hzSum = sum(hzSet) + + const average = hzSum / hzSet.length + const median = middle(hzSet) + + console.log( + '\n Ran %s (%s times) with an average of %s ops/sec', + chalk.yellow(suiteBenchmark.name), + chalk.yellow(iterations), + chalk.yellow(fnumber(average)) + ) + console.log(' Fastest: %s ops/sec', fnumber(Math.max(...hzSet))) + console.log(' Average: %s ops/sec', chalk.bold(fnumber(average))) + console.log(' Median : %s ops/sec', chalk.white.bold(fnumber(median))) + console.log(' Slowest: %s ops/sec', fnumber(Math.min(...hzSet))) + + resolve( + Object.assign({}, suiteBenchmark, { + average, + median + }) + ) + }) + // run async + .run({ async: true }) + }) +} + +function fnumber (x) { + return Math.floor(x) + .toString() + .replace(/\B(?=(\d{3})+(?!\d))/g, ',') +} + +function sum (values) { + return values.reduce((acc, val) => acc + val) +} + +function middle (values) { + const len = values.length + const half = Math.floor(len / 2) + + if (len % 2) { + return (values[half - 1] + values[half]) / 2.0 + } else { + return values[half] + } +} + +function bytesToKb (bytes) { + return Math.round(bytes / 1024 * 100) / 100 +} + +function runGC (val) { + return Promise.resolve(val).then(r => { + global.gc() + return r + }) +} + +function listByMemoryEfficiency (suites) { + console.log(chalk.white.bold('\nMore memory efficient:')) + const sorted = suites.sort((a, b) => a.memoryEfficiency - b.memoryEfficiency) + sorted.forEach((suite, index) => { + const name = index === 0 ? chalk.yellow(suite.name) : chalk.bold(suite.name) + + console.log( + ' %s: %s', + name, + chalk.yellow(fnumber(bytesToKb(suite.memoryEfficiency)) + 'Kb') + ) + }) +} + +function reportSuiteMemory (suite) { + const { memoryBefore, memoryAfter } = suite + console.log( + ' Memory: not freed %s (before %s after %s)', + chalk.red.bold(fnumber(bytesToKb(memoryAfter - memoryBefore)) + 'Kb'), + chalk.white.bold(fnumber(bytesToKb(memoryBefore)) + 'Kb'), + chalk.white.bold(fnumber(bytesToKb(memoryAfter)) + 'Kb') + ) + return suite +} + +function reportMemoryEfficincy (suites) { + const sortedSuites = suites.sort( + (a, b) => a.memoryEfficiency - b.memoryEfficiency + ) + const first = sortedSuites[0] + const second = sortedSuites[1] + + const diffMemory = + (second.memoryEfficiency - first.memoryEfficiency) / + second.memoryEfficiency * + 100 + + console.log( + ` Memory: %s was more memory efficient by %s (%s vs %s)`, + chalk.yellow(first.name), + chalk.white.bold(diffMemory.toFixed(2) + '%'), + chalk.yellow(fnumber(bytesToKb(first.memoryEfficiency)) + 'Kb'), + chalk.yellow(fnumber(bytesToKb(second.memoryEfficiency)) + 'Kb') + ) +} + +function unhandledError (err) { + console.log('Failed Tests: ' + err.stack) +} + +process.on('unhandledRejection', unhandledError) +process.on('uncaughtException', unhandledError) + +start(suites) + +module.exports = { + start, + runTests, + runBenchmarks, + reportFinal, + listBySpeed, + reportFasterOpsperSec, + runBenchmark, + fnumber, + sum, + middle, + bytesToKb, + runGC, + listByMemoryEfficiency, + reportSuiteMemory, + reportMemoryEfficincy +} diff --git a/yarn.lock b/yarn.lock index 739b992c..e1b714d8 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1120,7 +1120,7 @@ bcrypt-pbkdf@^1.0.0: dependencies: tweetnacl "^0.14.3" -benchmark@2.1.4: +benchmark@2.1.4, benchmark@^2.1.4: version "2.1.4" resolved "https://registry.yarnpkg.com/benchmark/-/benchmark-2.1.4.tgz#09f3de31c916425d498cc2ee565a0ebf3c2a5629" dependencies: @@ -1441,7 +1441,7 @@ commander@^2.11.0, commander@^2.9.0: version "2.12.2" resolved "https://registry.yarnpkg.com/commander/-/commander-2.12.2.tgz#0f5946c427ed9ec0d91a46bb9def53e54650e555" -commander@^2.5.0: +commander@^2.13.0, commander@^2.5.0: version "2.13.0" resolved "https://registry.yarnpkg.com/commander/-/commander-2.13.0.tgz#6964bca67685df7c1f1430c584f07d7597885b9c" From 51696a2fad2f4d1c2ba6f8843c7744cf88c4fa1f Mon Sep 17 00:00:00 2001 From: Matthew Armstrong Date: Wed, 31 Jan 2018 11:47:32 -0500 Subject: [PATCH 02/11] feat(data-point/reducer-path): Add a custom name to ReducerPath#body functions (#193) closes #182 --- .../lib/reducer-types/reducer-path/factory.js | 11 +++++---- .../reducer-path/factory.test.js | 23 +++++++++++++------ 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/packages/data-point/lib/reducer-types/reducer-path/factory.js b/packages/data-point/lib/reducer-types/reducer-path/factory.js index 13d75c43..54157344 100644 --- a/packages/data-point/lib/reducer-types/reducer-path/factory.js +++ b/packages/data-point/lib/reducer-types/reducer-path/factory.js @@ -82,7 +82,7 @@ module.exports.mapFromAccumulatorValue = mapFromAccumulatorValue * @returns {Function} */ function getPathReducerFunction (jsonPath, asCollection) { - if (jsonPath === '$' || _.isEmpty(jsonPath)) { + if (jsonPath === '$') { return getAccumulatorValue } @@ -101,15 +101,16 @@ module.exports.getPathReducerFunction = getPathReducerFunction /** * @param {Function} createReducer - * @param {string} source + * @param {string} source - must begin with $ * @return {reducer} */ function create (createReducer, source) { const reducer = new ReducerPath() - - reducer.asCollection = source.slice(-2) === '[]' - reducer.name = (source.substr(1) || '$').replace(/\[]$/, '') + const value = source.replace(/\[]$/, '') + reducer.name = value.substr(1) || '$' + reducer.asCollection = source.endsWith('[]') reducer.body = getPathReducerFunction(reducer.name, reducer.asCollection) + Object.defineProperty(reducer.body, 'name', { value }) return reducer } diff --git a/packages/data-point/lib/reducer-types/reducer-path/factory.test.js b/packages/data-point/lib/reducer-types/reducer-path/factory.test.js index 23ac7901..072f58a1 100644 --- a/packages/data-point/lib/reducer-types/reducer-path/factory.test.js +++ b/packages/data-point/lib/reducer-types/reducer-path/factory.test.js @@ -53,7 +53,6 @@ describe('ReducerPath getters', () => { describe('reducer/reducer-path#getPathReducerFunction', () => { it('should always return a function', () => { - expect(factory.getPathReducerFunction()).toBeInstanceOf(Function) expect(factory.getPathReducerFunction('')).toBeInstanceOf(Function) expect(factory.getPathReducerFunction('.')).toBeInstanceOf(Function) expect(factory.getPathReducerFunction('..')).toBeInstanceOf(Function) @@ -63,36 +62,46 @@ describe('reducer/reducer-path#getPathReducerFunction', () => { }) describe('reducer/reducer-path#create', () => { - it('basic path', () => { - const reducer = factory.create(createReducer, '$a') + it('empty path', () => { + const reducer = factory.create(createReducer, '$') expect(reducer.type).toBe('ReducerPath') - expect(reducer.name).toBe('a') + expect(reducer.name).toBe('$') expect(reducer.asCollection).toBe(false) + expect(reducer.body).toBeInstanceOf(Function) + expect(reducer.body.name).toBe('$') }) - it('empty path', () => { - const reducer = factory.create(createReducer, '$') + it('basic path', () => { + const reducer = factory.create(createReducer, '$a') expect(reducer.type).toBe('ReducerPath') - expect(reducer.name).toBe('$') + expect(reducer.name).toBe('a') expect(reducer.asCollection).toBe(false) + expect(reducer.body).toBeInstanceOf(Function) + expect(reducer.body.name).toBe('$a') }) it('compound path', () => { const reducer = factory.create(createReducer, '$foo.bar') expect(reducer.name).toBe('foo.bar') expect(reducer.asCollection).toBe(false) + expect(reducer.body).toBeInstanceOf(Function) + expect(reducer.body.name).toBe('$foo.bar') }) it('compound path', () => { const reducer = factory.create(createReducer, '$foo.bar[0]') expect(reducer.name).toBe('foo.bar[0]') expect(reducer.asCollection).toBe(false) + expect(reducer.body).toBeInstanceOf(Function) + expect(reducer.body.name).toBe('$foo.bar[0]') }) it('path with asCollection', () => { const reducer = factory.create(createReducer, '$foo.bar[]') expect(reducer.name).toBe('foo.bar') expect(reducer.asCollection).toBe(true) + expect(reducer.body).toBeInstanceOf(Function) + expect(reducer.body.name).toBe('$foo.bar') }) }) From 75e328fab75d5248b64bd976cae97d992dc254ac Mon Sep 17 00:00:00 2001 From: Matthew Armstrong Date: Wed, 31 Jan 2018 12:10:20 -0500 Subject: [PATCH 03/11] feat(data-point/reducer-default: Add the ReducerDefault type (#194) closes #169 --- packages/data-point/README.md | 45 ++++++++++++++ .../lib/__snapshots__/index.test.js.snap | 1 + .../__snapshots__/helpers.test.js.snap | 1 + packages/data-point/lib/helpers/helpers.js | 1 + .../data-point/lib/reducer-types/factory.js | 17 ++++-- .../reducer-types/reducer-helpers/index.js | 3 + .../reducer-default/factory.js | 22 +++++++ .../reducer-default/factory.test.js | 21 +++++++ .../reducer-helpers/reducer-default/index.js | 9 +++ .../reducer-default/resolve.js | 30 ++++++++++ .../reducer-default/resolve.test.js | 24 ++++++++ .../lib/reducer-types/reducer-symbol.js | 1 - .../lib/reducer-types/reducer-symbols.js | 3 + .../data-point/lib/reducer-types/resolve.js | 44 +++++++++++--- .../lib/reducer-types/resolve.test.js | 60 +++++++++++++++++-- 15 files changed, 261 insertions(+), 21 deletions(-) create mode 100644 packages/data-point/lib/reducer-types/reducer-helpers/reducer-default/factory.js create mode 100644 packages/data-point/lib/reducer-types/reducer-helpers/reducer-default/factory.test.js create mode 100644 packages/data-point/lib/reducer-types/reducer-helpers/reducer-default/index.js create mode 100644 packages/data-point/lib/reducer-types/reducer-helpers/reducer-default/resolve.js create mode 100644 packages/data-point/lib/reducer-types/reducer-helpers/reducer-default/resolve.test.js delete mode 100644 packages/data-point/lib/reducer-types/reducer-symbol.js create mode 100644 packages/data-point/lib/reducer-types/reducer-symbols.js diff --git a/packages/data-point/README.md b/packages/data-point/README.md index 19833bee..ac58e7f0 100644 --- a/packages/data-point/README.md +++ b/packages/data-point/README.md @@ -38,6 +38,7 @@ npm install --save data-point - [find](#reducer-find) - [constant](#reducer-constant) - [parallel](#reducer-parallel) + - [withDefault](#reducer-default) - [Entities](#entities) - [dataPoint.addEntities](#api-data-point-add-entities) - [Built-in entities](#built-in-entities) @@ -1228,6 +1229,50 @@ parallel(reducers:Array):Array ``` +### withDefault + +The **withDefault** reducer adds a default value to any reducer type. If the reducer resolves to `null`, `undefined`, `NaN`, or `''`, +the default is returned instead. + +**SYNOPSIS** + +```js +withDefault(source:*, value:*):* +``` + +**Reducer's arguments** + +| Argument | Type | Description | +|:---|:---|:---| +| *source* | * | Source data for creating a [reducer](#reducers) | +| *value* | * | The default value to use (or a function that returns the default value) | + +The default value is not cloned before it's returned, so it's good practice to wrap any Objects in a function. + +**EXAMPLE:** + +```js +const { withDefault } = DataPoint.helpers + +const input = { + a: undefined +} + +// adds a default to a PathReducer +const r1 = withDefault('$a', 50) + +dataPoint.resolve(r1, input) // => 50 + +// passing a function is useful when the default value is +// an object, because it returns a new object every time +const r2 = withDefault('$a', () => { + return { b: 1 } +}) + +dataPoint.resolve(r2, input) // => { b: 1 } + +``` + ## Entities Entities are artifacts that transform data. An entity is represented by a data structure (spec) that defines how the entity behaves. diff --git a/packages/data-point/lib/__snapshots__/index.test.js.snap b/packages/data-point/lib/__snapshots__/index.test.js.snap index bc081610..ac64b38b 100644 --- a/packages/data-point/lib/__snapshots__/index.test.js.snap +++ b/packages/data-point/lib/__snapshots__/index.test.js.snap @@ -21,6 +21,7 @@ Object { "omit": [Function], "parallel": [Function], "pick": [Function], + "withDefault": [Function], }, "reducify": [Function], "reducifyAll": [Function], diff --git a/packages/data-point/lib/helpers/__snapshots__/helpers.test.js.snap b/packages/data-point/lib/helpers/__snapshots__/helpers.test.js.snap index 22a99f0d..434f9170 100644 --- a/packages/data-point/lib/helpers/__snapshots__/helpers.test.js.snap +++ b/packages/data-point/lib/helpers/__snapshots__/helpers.test.js.snap @@ -22,6 +22,7 @@ Object { "omit": [Function], "parallel": [Function], "pick": [Function], + "withDefault": [Function], }, "isReducer": [Function], "mockReducer": [Function], diff --git a/packages/data-point/lib/helpers/helpers.js b/packages/data-point/lib/helpers/helpers.js index 4ae35d0e..153736ec 100644 --- a/packages/data-point/lib/helpers/helpers.js +++ b/packages/data-point/lib/helpers/helpers.js @@ -15,6 +15,7 @@ module.exports.helpers = { omit: stubFactories.omit, parallel: stubFactories.parallel, pick: stubFactories.pick, + withDefault: stubFactories.withDefault, isString: typeCheckFunctionReducers.isString, isNumber: typeCheckFunctionReducers.isNumber, isBoolean: typeCheckFunctionReducers.isBoolean, diff --git a/packages/data-point/lib/reducer-types/factory.js b/packages/data-point/lib/reducer-types/factory.js index 940080a3..097c6734 100644 --- a/packages/data-point/lib/reducer-types/factory.js +++ b/packages/data-point/lib/reducer-types/factory.js @@ -1,7 +1,7 @@ const _ = require('lodash') const util = require('util') -const REDUCER_SYMBOL = require('./reducer-symbol') +const { IS_REDUCER, DEFAULT_VALUE } = require('./reducer-symbols') const ReducerEntity = require('./reducer-entity') const ReducerFunction = require('./reducer-function') @@ -24,7 +24,7 @@ const reducerTypes = [ * @returns {boolean} */ function isReducer (item) { - return !!(item && item[REDUCER_SYMBOL]) + return !!(item && item[IS_REDUCER]) } module.exports.isReducer = isReducer @@ -34,7 +34,7 @@ module.exports.isReducer = isReducer * @param {*} source * @returns {Array|reducer} */ -function dealWithPipeOperators (source) { +function normalizeInput (source) { let result = ReducerList.parse(source) if (result.length === 1) { // do not create a ReducerList that only contains a single reducer @@ -47,11 +47,12 @@ function dealWithPipeOperators (source) { /** * parse reducer * @param {*} source + * @param {Object} options * @throws if source is not a valid type for creating a reducer * @return {reducer} */ -function createReducer (source) { - source = dealWithPipeOperators(source) +function createReducer (source, options = {}) { + source = normalizeInput(source) const reducerType = reducerTypes.find(r => r.isType(source)) if (_.isUndefined(reducerType)) { @@ -68,7 +69,11 @@ function createReducer (source) { // NOTE: recursive call const reducer = reducerType.create(createReducer, source) - reducer[REDUCER_SYMBOL] = true + reducer[IS_REDUCER] = true + if (_.has(options, 'default')) { + reducer[DEFAULT_VALUE] = { value: options.default } + } + return Object.freeze(reducer) } diff --git a/packages/data-point/lib/reducer-types/reducer-helpers/index.js b/packages/data-point/lib/reducer-types/reducer-helpers/index.js index 64587e99..824872e8 100644 --- a/packages/data-point/lib/reducer-types/reducer-helpers/index.js +++ b/packages/data-point/lib/reducer-types/reducer-helpers/index.js @@ -2,6 +2,7 @@ const createStub = require('./reducer-stub').create const reducerAssign = require('./reducer-assign') const reducerConstant = require('./reducer-constant') +const reducerDefault = require('./reducer-default') const reducerFilter = require('./reducer-filter') const reducerFind = require('./reducer-find') const reducerMap = require('./reducer-map') @@ -33,6 +34,7 @@ module.exports.isType = require('./reducer-stub').isType const reducers = { [reducerAssign.type]: reducerAssign, [reducerConstant.type]: reducerConstant, + [reducerDefault.type]: reducerDefault, [reducerFilter.type]: reducerFilter, [reducerFind.type]: reducerFind, [reducerMap.type]: reducerMap, @@ -50,6 +52,7 @@ function bindStubFunction (reducerType) { const stubFactories = { [reducerAssign.name]: bindStubFunction(reducerAssign.type), [reducerConstant.name]: bindStubFunction(reducerConstant.type), + [reducerDefault.name]: bindStubFunction(reducerDefault.type), [reducerFilter.name]: bindStubFunction(reducerFilter.type), [reducerFind.name]: bindStubFunction(reducerFind.type), [reducerMap.name]: bindStubFunction(reducerMap.type), diff --git a/packages/data-point/lib/reducer-types/reducer-helpers/reducer-default/factory.js b/packages/data-point/lib/reducer-types/reducer-helpers/reducer-default/factory.js new file mode 100644 index 00000000..b9510c90 --- /dev/null +++ b/packages/data-point/lib/reducer-types/reducer-helpers/reducer-default/factory.js @@ -0,0 +1,22 @@ +const REDUCER_DEFAULT = 'ReducerDefault' + +module.exports.type = REDUCER_DEFAULT + +const HELPER_NAME = 'withDefault' + +module.exports.name = HELPER_NAME + +/** + * this is used as a decorator + * it attaches a default value + * to an existing reducer type + * @param {Function} createReducer + * @param {*} source - raw source for a reducer + * @param {*} value + * @return {reducer} + */ +function create (createReducer, source, value) { + return createReducer(source, { default: value }) +} + +module.exports.create = create diff --git a/packages/data-point/lib/reducer-types/reducer-helpers/reducer-default/factory.test.js b/packages/data-point/lib/reducer-types/reducer-helpers/reducer-default/factory.test.js new file mode 100644 index 00000000..9fa5127b --- /dev/null +++ b/packages/data-point/lib/reducer-types/reducer-helpers/reducer-default/factory.test.js @@ -0,0 +1,21 @@ +/* eslint-env jest */ + +const Factory = require('./factory') +const createReducer = require('../../index').create +const { IS_REDUCER, DEFAULT_VALUE } = require('../../reducer-symbols') + +describe('ReducerDefault#factory', () => { + test('create with ReducerPath and string as default', () => { + const reducer = Factory.create(createReducer, '$a', 'DEFAULT_VALUE') + expect(reducer[IS_REDUCER]).toBe(true) + expect(reducer[DEFAULT_VALUE]).toEqual({ value: 'DEFAULT_VALUE' }) + expect(reducer.type).toBe('ReducerPath') + }) + test('create with ReducerPath and function as default', () => { + const _default = () => 'DEFAULT_VALUE' + const reducer = Factory.create(createReducer, '$a', _default) + expect(reducer[IS_REDUCER]).toBe(true) + expect(reducer[DEFAULT_VALUE]).toEqual({ value: _default }) + expect(reducer.type).toBe('ReducerPath') + }) +}) diff --git a/packages/data-point/lib/reducer-types/reducer-helpers/reducer-default/index.js b/packages/data-point/lib/reducer-types/reducer-helpers/reducer-default/index.js new file mode 100644 index 00000000..fafee9cc --- /dev/null +++ b/packages/data-point/lib/reducer-types/reducer-helpers/reducer-default/index.js @@ -0,0 +1,9 @@ +const factory = require('./factory') +const resolve = require('./resolve').resolve + +module.exports = { + create: factory.create, + type: factory.type, + name: factory.name, + resolve: resolve +} diff --git a/packages/data-point/lib/reducer-types/reducer-helpers/reducer-default/resolve.js b/packages/data-point/lib/reducer-types/reducer-helpers/reducer-default/resolve.js new file mode 100644 index 00000000..2b4990e0 --- /dev/null +++ b/packages/data-point/lib/reducer-types/reducer-helpers/reducer-default/resolve.js @@ -0,0 +1,30 @@ +const utils = require('../../../utils') + +/** + * @param {*} value + * @return {boolean} + */ +function isFalsy (value) { + return ( + value === null || + typeof value === 'undefined' || + Number.isNaN(value) || + value === '' + ) +} + +/** + * @param {Accumulator} accumulator + * @param {*} _default + * @returns {Accumulator} + */ +function resolve (accumulator, _default) { + let value = accumulator.value + if (isFalsy(value)) { + value = typeof _default === 'function' ? _default() : _default + } + + return utils.assign(accumulator, { value }) +} + +module.exports.resolve = resolve diff --git a/packages/data-point/lib/reducer-types/reducer-helpers/reducer-default/resolve.test.js b/packages/data-point/lib/reducer-types/reducer-helpers/reducer-default/resolve.test.js new file mode 100644 index 00000000..ddc3bc2e --- /dev/null +++ b/packages/data-point/lib/reducer-types/reducer-helpers/reducer-default/resolve.test.js @@ -0,0 +1,24 @@ +/* eslint-env jest */ + +const { resolve } = require('./resolve') + +describe('ReducerDefault#resolve', () => { + test('resolve with input value', () => { + expect(resolve({ value: 0 }, 5)).toEqual({ value: 0 }) + expect(resolve({ value: false }, 5)).toEqual({ value: false }) + expect(resolve({ value: 'value' }, 5)).toEqual({ value: 'value' }) + expect(resolve({ value: { a: 1 } }, 5)).toEqual({ value: { a: 1 } }) + }) + test('resolve with default value', () => { + expect(resolve({ value: '' }, 5)).toEqual({ value: 5 }) + expect(resolve({ value: undefined }, 5)).toEqual({ value: 5 }) + expect(resolve({ value: null }, 5)).toEqual({ value: 5 }) + expect(resolve({ value: NaN }, 5)).toEqual({ value: 5 }) + expect(resolve({ value: undefined }, undefined)).toEqual({ + value: undefined + }) + }) + test('resolve with default value function', () => { + expect(resolve({ value: undefined }, () => 5)).toEqual({ value: 5 }) + }) +}) diff --git a/packages/data-point/lib/reducer-types/reducer-symbol.js b/packages/data-point/lib/reducer-types/reducer-symbol.js deleted file mode 100644 index 1d459f89..00000000 --- a/packages/data-point/lib/reducer-types/reducer-symbol.js +++ /dev/null @@ -1 +0,0 @@ -module.exports = Symbol('reducer') diff --git a/packages/data-point/lib/reducer-types/reducer-symbols.js b/packages/data-point/lib/reducer-types/reducer-symbols.js new file mode 100644 index 00000000..a6682de3 --- /dev/null +++ b/packages/data-point/lib/reducer-types/reducer-symbols.js @@ -0,0 +1,3 @@ +module.exports.IS_REDUCER = Symbol('reducer') + +module.exports.DEFAULT_VALUE = Symbol('default') diff --git a/packages/data-point/lib/reducer-types/resolve.js b/packages/data-point/lib/reducer-types/resolve.js index dc2deca8..777b0a92 100644 --- a/packages/data-point/lib/reducer-types/resolve.js +++ b/packages/data-point/lib/reducer-types/resolve.js @@ -1,11 +1,12 @@ const Promise = require('bluebird') + const ReducerEntity = require('./reducer-entity') const ReducerFunction = require('./reducer-function') const ReducerList = require('./reducer-list') const ReducerObject = require('./reducer-object') const ReducerPath = require('./reducer-path') - const ReducerHelpers = require('./reducer-helpers').reducers +const { DEFAULT_VALUE } = require('./reducer-symbols') const reducers = Object.assign({}, ReducerHelpers, { [ReducerEntity.type]: ReducerEntity, @@ -15,6 +16,30 @@ const reducers = Object.assign({}, ReducerHelpers, { [ReducerPath.type]: ReducerPath }) +/** + * @param {Reducer} reducer + * @return {boolean} + */ +function hasDefault (reducer) { + return !!reducer[DEFAULT_VALUE] +} + +module.exports.hasDefault = hasDefault + +/** + * @param {Reducer} reducer + * @throws if reducer is not valid + * @return {Function} + */ +function getResolveFunction (reducer) { + const reducerType = reducers[reducer.type] + if (reducerType) { + return reducerType.resolve + } + + throw new Error(`Reducer type '${reducer.type}' was not recognized`) +} + /** * apply a Reducer to an accumulator * @param {Object} manager @@ -22,7 +47,7 @@ const reducers = Object.assign({}, ReducerHelpers, { * @param {Reducer} reducer * @returns {Promise} */ -function resolve (manager, accumulator, reducer) { +function resolveReducer (manager, accumulator, reducer) { // this conditional is here because BaseEntity#resolve // does not check that lifecycle methods are defined // before trying to resolve them @@ -30,13 +55,16 @@ function resolve (manager, accumulator, reducer) { return Promise.resolve(accumulator) } - const reducerType = reducers[reducer.type] - if (!reducerType) { - throw new Error(`Reducer type '${reducer.type}' was not recognized`) + const resolve = getResolveFunction(reducer) + // NOTE: recursive call + const result = resolve(manager, resolveReducer, accumulator, reducer) + if (hasDefault(reducer)) { + const _default = reducer[DEFAULT_VALUE].value + const resolveDefault = reducers.ReducerDefault.resolve + return result.then(acc => resolveDefault(acc, _default)) } - // NOTE: recursive call - return reducerType.resolve(manager, resolve, accumulator, reducer) + return result } -module.exports.resolve = resolve +module.exports.resolve = resolveReducer diff --git a/packages/data-point/lib/reducer-types/resolve.test.js b/packages/data-point/lib/reducer-types/resolve.test.js index 4a342828..eacff4d9 100644 --- a/packages/data-point/lib/reducer-types/resolve.test.js +++ b/packages/data-point/lib/reducer-types/resolve.test.js @@ -16,14 +16,18 @@ beforeAll(() => { manager = fixtureStore.create() }) +function resolve (source, options, value) { + const reducer = Factory.create(source, options) + const accumulator = AccumulatorFactory.create({ value }) + return Resolve.resolve(manager, accumulator, reducer).catch(e => e) +} + describe('reducer#resolve', () => { test('It should work for valid input', () => { - const accumulator = AccumulatorFactory.create({ - value: testData.a.b.c - }) - - const reducer = Factory.create(reducers.addCollectionValues()) - return Resolve.resolve(manager, accumulator, reducer).then(result => { + const source = reducers.addCollectionValues() + const value = testData.a.b.c + const options = {} + return resolve(source, options, value).then(result => { expect(result.value).toEqual(6) }) }) @@ -38,4 +42,48 @@ describe('reducer#resolve', () => { Resolve.resolve(manager, accumulator, reducer) }).toThrowErrorMatchingSnapshot() }) + + test('It should return undefined when no default is provided', () => { + const source = '$a' + const value = { a: undefined } + const options = {} + return resolve(source, options, value).then(result => { + expect(result.value).toBeUndefined() + }) + }) +}) + +describe('reducer#resolve with default value', () => { + test('do not overwrite false', () => { + const source = '$a' + const value = { a: false } + const options = { default: 500 } + return resolve(source, options, value).then(result => { + expect(result.value).toBe(false) + }) + }) + test('do not overwrite true', () => { + const source = '$a' + const value = { a: true } + const options = { default: 500 } + return resolve(source, options, value).then(result => { + expect(result.value).toBe(true) + }) + }) + test('overwrite undefined', () => { + const source = '$a' + const value = { a: undefined } + const options = { default: 500 } + return resolve(source, options, value).then(result => { + expect(result.value).toBe(500) + }) + }) + test('overwrite undefined with function as default', () => { + const source = '$a' + const value = { a: undefined } + const options = { default: () => 500 } + return resolve(source, options, value).then(result => { + expect(result.value).toBe(500) + }) + }) }) From e0a9f59731df5c811653697660607aa2e7e9d802 Mon Sep 17 00:00:00 2001 From: Victor Singh Date: Wed, 31 Jan 2018 12:31:33 -0500 Subject: [PATCH 04/11] fix(scripts/preinstall-script.js): Removes yarn vs npm warning (#197) Removes default warning that recommends yarn when user runs npm install --- scripts/preinstall-script.js | 7 ------- 1 file changed, 7 deletions(-) diff --git a/scripts/preinstall-script.js b/scripts/preinstall-script.js index a53f1250..111f320b 100755 --- a/scripts/preinstall-script.js +++ b/scripts/preinstall-script.js @@ -15,10 +15,3 @@ if (process.env.npm_execpath.indexOf('yarn') === -1) { ) process.exit(1) } - -console.warn( - '\x1b[36m%s\x1b[0m', - '\nWe recommend using yarn for this project instead of npm.\n' + - '\nTo learn more about why we use yarn visit:\n' + - setup -) From 75dab4a4487814b709c13da7ea69af3f2ccf6165 Mon Sep 17 00:00:00 2001 From: Victor Singh Date: Wed, 31 Jan 2018 12:36:54 -0500 Subject: [PATCH 05/11] style(*.js) formatted *.js files that did not use the prettify standard (#199) --- packages/data-point-cache/lib/cache.js | 1 - packages/data-point-cache/lib/in-memory.js | 1 - packages/data-point-cache/lib/index.js | 1 - packages/data-point-cache/lib/redis-client.js | 1 - packages/data-point/lib/entity-types/spec/factory.js | 12 +++--------- 5 files changed, 3 insertions(+), 13 deletions(-) diff --git a/packages/data-point-cache/lib/cache.js b/packages/data-point-cache/lib/cache.js index 88f67471..4e1a90f7 100644 --- a/packages/data-point-cache/lib/cache.js +++ b/packages/data-point-cache/lib/cache.js @@ -1,4 +1,3 @@ - const RedisClient = require('./redis-client') const InMemory = require('./in-memory') const Promise = require('bluebird') diff --git a/packages/data-point-cache/lib/in-memory.js b/packages/data-point-cache/lib/in-memory.js index c69dc571..54c9ea40 100644 --- a/packages/data-point-cache/lib/in-memory.js +++ b/packages/data-point-cache/lib/in-memory.js @@ -1,4 +1,3 @@ - const logger = require('./logger') function set (cache, key, value, ttl) { diff --git a/packages/data-point-cache/lib/index.js b/packages/data-point-cache/lib/index.js index 78eb14a6..7e77318c 100644 --- a/packages/data-point-cache/lib/index.js +++ b/packages/data-point-cache/lib/index.js @@ -1,2 +1 @@ - module.exports = require('./cache') diff --git a/packages/data-point-cache/lib/redis-client.js b/packages/data-point-cache/lib/redis-client.js index 82f24e40..3afe9cff 100644 --- a/packages/data-point-cache/lib/redis-client.js +++ b/packages/data-point-cache/lib/redis-client.js @@ -1,4 +1,3 @@ - const IORedis = require('ioredis') const Promise = require('bluebird') const logger = require('./logger') diff --git a/packages/data-point/lib/entity-types/spec/factory.js b/packages/data-point/lib/entity-types/spec/factory.js index 0d3bb286..96b5fcf0 100644 --- a/packages/data-point/lib/entity-types/spec/factory.js +++ b/packages/data-point/lib/entity-types/spec/factory.js @@ -9,9 +9,7 @@ const utils = require('../../utils') function create (entitySpec, id) { if (!_.isFunction(entitySpec.create)) { throw new Error( - `Entity Module '${ - id - }' should expose a 'create' method, instead got: ${Object.keys( + `Entity Module '${id}' should expose a 'create' method, instead got: ${Object.keys( entitySpec )}` ) @@ -19,9 +17,7 @@ function create (entitySpec, id) { if (!_.isFunction(entitySpec.resolve)) { throw new Error( - `Entity Module '${ - id - }' should expose a 'resolve' method, instead got: ${Object.keys( + `Entity Module '${id}' should expose a 'resolve' method, instead got: ${Object.keys( entitySpec )}` ) @@ -29,9 +25,7 @@ function create (entitySpec, id) { if (entitySpec.resolve.length !== 2) { throw new Error( - `Entity Module '${ - id - }' 'resolve' method should have an arity of 2, instead got: ${ + `Entity Module '${id}' 'resolve' method should have an arity of 2, instead got: ${ entitySpec.resolve.length }` ) From 3d1c52c4b735c90a8475cd188e0bd41376e1d53f Mon Sep 17 00:00:00 2001 From: Victor Singh Date: Thu, 1 Feb 2018 15:55:25 -0500 Subject: [PATCH 06/11] ci(.travis.yml): check for unstyled files Added is-it-pretty.sh script for travis-ci to check if there are files not passing prettier-standard closes #142 --- .travis.yml | 1 + scripts/is-it-pretty.sh | 14 ++++++++++++++ 2 files changed, 15 insertions(+) create mode 100755 scripts/is-it-pretty.sh diff --git a/.travis.yml b/.travis.yml index bb80c0d2..5b7d83b0 100644 --- a/.travis.yml +++ b/.travis.yml @@ -11,5 +11,6 @@ script: # Run test script - "yarn run lint" - "yarn test -- --maxWorkers=4" + - "scripts/is-it-pretty.sh" after_script: - "test -e ./coverage/lcov.info && npm install coveralls@2 && cat ./coverage/lcov.info | coveralls" diff --git a/scripts/is-it-pretty.sh b/scripts/is-it-pretty.sh new file mode 100755 index 00000000..0069e580 --- /dev/null +++ b/scripts/is-it-pretty.sh @@ -0,0 +1,14 @@ +#!/bin/bash + +node_modules/.bin/prettier-standard '**/*.js' +if [ -z "$(git status --porcelain)" ]; then + # Working directory clean + exit 0 +else + # Uncommitted changes + echo 'There are javascript files in this project that do not pass prettier-standard.' + echo 'To avoid this error make commits by using the "yarn run commit" command.' + echo 'For more info: https://github.com/ViacomInc/data-point/blob/master/CONTRIBUTING.md\n' + echo "$(git diff --name-only)" + exit 1 +fi From b8fe51db9c6a389053beb680de80f39ff2070f4b Mon Sep 17 00:00:00 2001 From: Victor Date: Tue, 13 Feb 2018 13:01:44 -0500 Subject: [PATCH 07/11] feat(packages/data-point/lib/entity-type/entity-request/resolve.js): Added a parameter to resolveReq #195 --- .../data-point/lib/entity-types/entity-request/resolve.js | 5 +++-- .../lib/entity-types/entity-request/resolve.test.js | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/data-point/lib/entity-types/entity-request/resolve.js b/packages/data-point/lib/entity-types/entity-request/resolve.js index e02f4016..668ac08b 100644 --- a/packages/data-point/lib/entity-types/entity-request/resolve.js +++ b/packages/data-point/lib/entity-types/entity-request/resolve.js @@ -98,9 +98,10 @@ module.exports.inspect = inspect /** * @param {Accumulator} acc * @param {Function} resolveReducer + * @param {boolean} showErr * @return {Promise} */ -function resolveRequest (acc, resolveReducer) { +function resolveRequest (acc, resolveReducer, showErr) { inspect(acc) return rp(acc.options) .then(result => utils.set(acc, 'value', result)) @@ -129,7 +130,7 @@ function resolveRequest (acc, resolveReducer) { // this is useful in the case the error itself is not logged by the // implementation - console.info(redactedError.toString(), message) + if (showErr) console.info(redactedError.toString(), message) // attaching to error so it can be exposed by a handler outside datapoint error.message = `${error.message}\n\n${message}` diff --git a/packages/data-point/lib/entity-types/entity-request/resolve.test.js b/packages/data-point/lib/entity-types/entity-request/resolve.test.js index de7d11f9..69867fee 100644 --- a/packages/data-point/lib/entity-types/entity-request/resolve.test.js +++ b/packages/data-point/lib/entity-types/entity-request/resolve.test.js @@ -261,7 +261,7 @@ describe('resolveRequest', () => { } _.set(acc, 'reducer.spec.id', 'test:test') console.info = jest.fn() - return Resolve.resolveRequest(acc) + return Resolve.resolveRequest(acc, null, true) .catch(e => e) .then(result => { expect(console.info).toBeCalled() From aa29f56baea50f89db2134789a53eb97dc1c48fd Mon Sep 17 00:00:00 2001 From: Victor Date: Tue, 13 Feb 2018 13:39:25 -0500 Subject: [PATCH 08/11] fix(packages/data-point/lib/entity-types/entity-request/resolve.js): Adjusted toggle for console.inf #195 --- .../data-point/lib/entity-types/entity-request/resolve.js | 6 +++--- .../lib/entity-types/entity-request/resolve.test.js | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/data-point/lib/entity-types/entity-request/resolve.js b/packages/data-point/lib/entity-types/entity-request/resolve.js index 668ac08b..e4eb1655 100644 --- a/packages/data-point/lib/entity-types/entity-request/resolve.js +++ b/packages/data-point/lib/entity-types/entity-request/resolve.js @@ -98,10 +98,10 @@ module.exports.inspect = inspect /** * @param {Accumulator} acc * @param {Function} resolveReducer - * @param {boolean} showErr + * @param {boolean} omitLog * @return {Promise} */ -function resolveRequest (acc, resolveReducer, showErr) { +function resolveRequest (acc, resolveReducer, omitLog) { inspect(acc) return rp(acc.options) .then(result => utils.set(acc, 'value', result)) @@ -130,7 +130,7 @@ function resolveRequest (acc, resolveReducer, showErr) { // this is useful in the case the error itself is not logged by the // implementation - if (showErr) console.info(redactedError.toString(), message) + if (!omitLog) console.info(redactedError.toString(), message) // attaching to error so it can be exposed by a handler outside datapoint error.message = `${error.message}\n\n${message}` diff --git a/packages/data-point/lib/entity-types/entity-request/resolve.test.js b/packages/data-point/lib/entity-types/entity-request/resolve.test.js index db57ac58..d531b76c 100644 --- a/packages/data-point/lib/entity-types/entity-request/resolve.test.js +++ b/packages/data-point/lib/entity-types/entity-request/resolve.test.js @@ -261,7 +261,7 @@ describe('resolveRequest', () => { } _.set(acc, 'reducer.spec.id', 'test:test') console.info = jest.fn() - return Resolve.resolveRequest(acc, null, true) + return Resolve.resolveRequest(acc) .catch(e => e) .then(result => { expect(console.info).toBeCalled() From 002d46f47ae61831108439c59712e83c06d65a26 Mon Sep 17 00:00:00 2001 From: Victor Date: Tue, 13 Feb 2018 14:38:14 -0500 Subject: [PATCH 09/11] refactor(packages/data-point/lib/entity-types/entity-request): Removed console.info log from resolve #195 --- .../data-point/lib/entity-types/entity-request/resolve.js | 7 +------ .../lib/entity-types/entity-request/resolve.test.js | 2 -- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/packages/data-point/lib/entity-types/entity-request/resolve.js b/packages/data-point/lib/entity-types/entity-request/resolve.js index e4eb1655..99391599 100644 --- a/packages/data-point/lib/entity-types/entity-request/resolve.js +++ b/packages/data-point/lib/entity-types/entity-request/resolve.js @@ -98,10 +98,9 @@ module.exports.inspect = inspect /** * @param {Accumulator} acc * @param {Function} resolveReducer - * @param {boolean} omitLog * @return {Promise} */ -function resolveRequest (acc, resolveReducer, omitLog) { +function resolveRequest (acc, resolveReducer) { inspect(acc) return rp(acc.options) .then(result => utils.set(acc, 'value', result)) @@ -128,10 +127,6 @@ function resolveRequest (acc, resolveReducer, omitLog) { ) ].join('') - // this is useful in the case the error itself is not logged by the - // implementation - if (!omitLog) console.info(redactedError.toString(), message) - // attaching to error so it can be exposed by a handler outside datapoint error.message = `${error.message}\n\n${message}` throw error diff --git a/packages/data-point/lib/entity-types/entity-request/resolve.test.js b/packages/data-point/lib/entity-types/entity-request/resolve.test.js index d531b76c..0214ca2d 100644 --- a/packages/data-point/lib/entity-types/entity-request/resolve.test.js +++ b/packages/data-point/lib/entity-types/entity-request/resolve.test.js @@ -264,7 +264,6 @@ describe('resolveRequest', () => { return Resolve.resolveRequest(acc) .catch(e => e) .then(result => { - expect(console.info).toBeCalled() expect(result.message).toMatchSnapshot() }) }) @@ -401,7 +400,6 @@ describe('resolve', () => { return transform('request:a9', {}).catch(err => { expect(err.statusCode).toEqual(404) expect(err.message).toMatchSnapshot() - expect(console.info).toBeCalled() // credentials are still available in the raw error.options expect(err.options.auth).toEqual({ From d29e205c7bdb802047ed853538db04b0bad5635a Mon Sep 17 00:00:00 2001 From: Victor Date: Tue, 13 Feb 2018 15:21:39 -0500 Subject: [PATCH 10/11] test(packages/data-point/lib/entity-types/entity-request/resolve.test.js): Removed console.info mock --- .../entity-request/resolve.test.js | 44 ------------------- 1 file changed, 44 deletions(-) diff --git a/packages/data-point/lib/entity-types/entity-request/resolve.test.js b/packages/data-point/lib/entity-types/entity-request/resolve.test.js index 0214ca2d..5a8b9981 100644 --- a/packages/data-point/lib/entity-types/entity-request/resolve.test.js +++ b/packages/data-point/lib/entity-types/entity-request/resolve.test.js @@ -219,13 +219,6 @@ describe('getRequestOptions', () => { }) describe('resolveRequest', () => { - let consoleInfo - beforeAll(() => { - consoleInfo = console.info - }) - afterEach(() => { - console.info = consoleInfo - }) test('resolve reducer locals', () => { nock('http://remote.test') .get('/source1') @@ -260,7 +253,6 @@ describe('resolveRequest', () => { value: 'foo' } _.set(acc, 'reducer.spec.id', 'test:test') - console.info = jest.fn() return Resolve.resolveRequest(acc) .catch(e => e) .then(result => { @@ -269,41 +261,6 @@ describe('resolveRequest', () => { }) }) -describe('inspect', () => { - let consoleInfo - function getAcc () { - const acc = {} - _.set(acc, 'reducer.spec.id', 'test:test') - _.set(acc, 'params.inspect', true) - return acc - } - beforeAll(() => { - consoleInfo = console.info - }) - afterEach(() => { - console.info = consoleInfo - }) - test('It should not execute utils.inspect', () => { - console.info = jest.fn() - const acc = getAcc() - acc.params.inspect = undefined - Resolve.inspect(acc) - expect(console.info).not.toBeCalled() - }) - test('It should not execute utils.inspect', () => { - console.info = jest.fn() - Resolve.inspect(getAcc()) - expect(console.info.mock.calls[0]).toContain('test:test') - }) - test('It should output options', () => { - console.info = jest.fn() - const acc = getAcc() - _.set(acc, 'options', { options: 1 }) - Resolve.inspect(acc) - expect(console.info.mock.calls[0]).toContain('\noptions:') - }) -}) - describe('resolve', () => { test('simplest json call', () => { nock('http://remote.test') @@ -392,7 +349,6 @@ describe('resolve', () => { }) test('it should omit options.auth when encountering an error', () => { - console.info = jest.fn() nock('http://remote.test') .get('/source1') .reply(404) From 429ac30812f52a2768074ce91e0f6c82ec2656c4 Mon Sep 17 00:00:00 2001 From: Victor Date: Tue, 13 Feb 2018 15:48:53 -0500 Subject: [PATCH 11/11] test(packages/data-point/entity-types/entity-request/resolve.test.js): Brought back test for inspect --- .../entity-request/resolve.test.js | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/packages/data-point/lib/entity-types/entity-request/resolve.test.js b/packages/data-point/lib/entity-types/entity-request/resolve.test.js index 5a8b9981..bc35ec0c 100644 --- a/packages/data-point/lib/entity-types/entity-request/resolve.test.js +++ b/packages/data-point/lib/entity-types/entity-request/resolve.test.js @@ -261,6 +261,41 @@ describe('resolveRequest', () => { }) }) +describe('inspect', () => { + let consoleInfo + function getAcc () { + const acc = {} + _.set(acc, 'reducer.spec.id', 'test:test') + _.set(acc, 'params.inspect', true) + return acc + } + beforeAll(() => { + consoleInfo = console.info + }) + afterEach(() => { + console.info = consoleInfo + }) + test('It should not execute utils.inspect', () => { + console.info = jest.fn() + const acc = getAcc() + acc.params.inspect = undefined + Resolve.inspect(acc) + expect(console.info).not.toBeCalled() + }) + test('It should not execute utils.inspect', () => { + console.info = jest.fn() + Resolve.inspect(getAcc()) + expect(console.info.mock.calls[0]).toContain('test:test') + }) + test('It should output options', () => { + console.info = jest.fn() + const acc = getAcc() + _.set(acc, 'options', { options: 1 }) + Resolve.inspect(acc) + expect(console.info.mock.calls[0]).toContain('\noptions:') + }) +}) + describe('resolve', () => { test('simplest json call', () => { nock('http://remote.test')