From 7914a34d4d73d7ded86aa76641eb5ba3c5289bc9 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 20 Mar 2018 02:25:22 +0100 Subject: [PATCH] assert: fix diff color output The color output was broken at some point and that was not detected because it was not tested for properly so far. This makes sure the colors work again and it adds a regression test as well. --- lib/internal/errors.js | 66 +++++++++++++------------- test/parallel/test-assert.js | 49 +++++++++---------- test/pseudo-tty/test-assert-colors.js | 18 +++++++ test/pseudo-tty/test-assert-colors.out | 0 4 files changed, 74 insertions(+), 59 deletions(-) create mode 100644 test/pseudo-tty/test-assert-colors.js create mode 100644 test/pseudo-tty/test-assert-colors.out diff --git a/lib/internal/errors.js b/lib/internal/errors.js index a8c4bfb3c33b9e..1c7a69bbc4108a 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -15,9 +15,9 @@ const kInfo = Symbol('info'); const messages = new Map(); const codes = {}; -var green = ''; -var red = ''; -var white = ''; +let green = ''; +let red = ''; +let white = ''; const { errmap, @@ -29,16 +29,9 @@ const { kMaxLength } = process.binding('buffer'); const { defineProperty } = Object; // Lazily loaded -var util_ = null; +var util; var buffer; -function lazyUtil() { - if (!util_) { - util_ = require('util'); - } - return util_; -} - var internalUtil = null; function lazyInternalUtil() { if (!internalUtil) { @@ -47,6 +40,13 @@ function lazyInternalUtil() { return internalUtil; } +function inspectValue(val) { + return util.inspect( + val, + { compact: false, customInspect: false } + ).split('\n'); +} + function makeNodeError(Base) { return class NodeError extends Base { constructor(key, ...args) { @@ -210,11 +210,9 @@ function createErrDiff(actual, expected, operator) { var lastPos = 0; var end = ''; var skipped = false; - const util = lazyUtil(); - const actualLines = util - .inspect(actual, { compact: false, customInspect: false }).split('\n'); - const expectedLines = util - .inspect(expected, { compact: false, customInspect: false }).split('\n'); + if (util === undefined) util = require('util'); + const actualLines = inspectValue(actual); + const expectedLines = inspectValue(expected); const msg = `Input A expected to ${operator} input B:\n` + `${green}+ expected${white} ${red}- actual${white}`; const skippedMsg = ' ... Lines skipped'; @@ -333,14 +331,20 @@ class AssertionError extends Error { if (message != null) { super(message); } else { - if (util_ === null && - process.stdout.isTTY && - process.stdout.getColorDepth() !== 1) { - green = '\u001b[32m'; - white = '\u001b[39m'; - red = '\u001b[31m'; + if (process.stdout.isTTY) { + // Reset on each call to make sure we handle dynamically set environment + // variables correct. + if (process.stdout.getColorDepth() !== 1) { + green = '\u001b[32m'; + white = '\u001b[39m'; + red = '\u001b[31m'; + } else { + green = ''; + white = ''; + red = ''; + } } - const util = lazyUtil(); + if (util === undefined) util = require('util'); if (typeof actual === 'object' && actual !== null && 'stack' in actual && actual instanceof Error) { actual = `${actual.name}: ${actual.message}`; @@ -361,10 +365,7 @@ class AssertionError extends Error { } else if (errorDiff === 1) { // In case the objects are equal but the operator requires unequal, show // the first object and say A equals B - const res = util.inspect( - actual, - { compact: false, customInspect: false } - ).split('\n'); + const res = inspectValue(actual); if (res.length > 20) { res[19] = '...'; @@ -406,10 +407,10 @@ function message(key, args) { const msg = messages.get(key); internalAssert(msg, `An invalid error message key was used: ${key}.`); let fmt; + if (util === undefined) util = require('util'); if (typeof msg === 'function') { fmt = msg; } else { - const util = lazyUtil(); fmt = util.format; if (args === undefined || args.length === 0) return msg; @@ -479,7 +480,8 @@ function errnoException(err, syscall, original) { // getSystemErrorName(err) to guard against invalid arguments from users. // This can be replaced with [ code ] = errmap.get(err) when this method // is no longer exposed to user land. - const code = lazyUtil().getSystemErrorName(err); + if (util === undefined) util = require('util'); + const code = util.getSystemErrorName(err); const message = original ? `${syscall} ${code} ${original}` : `${syscall} ${code}`; @@ -508,7 +510,8 @@ function exceptionWithHostPort(err, syscall, address, port, additional) { // getSystemErrorName(err) to guard against invalid arguments from users. // This can be replaced with [ code ] = errmap.get(err) when this method // is no longer exposed to user land. - const code = lazyUtil().getSystemErrorName(err); + if (util === undefined) util = require('util'); + const code = util.getSystemErrorName(err); let details = ''; if (port && port > 0) { details = ` ${address}:${port}`; @@ -746,10 +749,9 @@ E('ERR_INSPECTOR_NOT_CONNECTED', 'Session is not connected', Error); E('ERR_INVALID_ADDRESS_FAMILY', 'Invalid address family: %s', RangeError); E('ERR_INVALID_ARG_TYPE', invalidArgType, TypeError); E('ERR_INVALID_ARG_VALUE', (name, value, reason = 'is invalid') => { - const util = lazyUtil(); let inspected = util.inspect(value); if (inspected.length > 128) { - inspected = inspected.slice(0, 128) + '...'; + inspected = `${inspected.slice(0, 128)}...`; } return `The argument '${name}' ${reason}. Received ${inspected}`; }, TypeError, RangeError); diff --git a/test/parallel/test-assert.js b/test/parallel/test-assert.js index 195d9bbc8013ca..54972657e94dd4 100644 --- a/test/parallel/test-assert.js +++ b/test/parallel/test-assert.js @@ -34,11 +34,8 @@ const { writeFileSync, unlinkSync } = require('fs'); const { inspect } = require('util'); const a = assert; -const colors = process.stdout.isTTY && process.stdout.getColorDepth() > 1; const start = 'Input A expected to deepStrictEqual input B:'; -const actExp = colors ? - '\u001b[32m+ expected\u001b[39m \u001b[31m- actual\u001b[39m' : - '+ expected - actual'; +const actExp = '+ expected - actual'; assert.ok(a.AssertionError.prototype instanceof Error, 'a.AssertionError instanceof Error'); @@ -442,8 +439,6 @@ common.expectsError( Error.stackTraceLimit = tmpLimit; // Test error diffs. - const plus = colors ? '\u001b[32m+\u001b[39m' : '+'; - const minus = colors ? '\u001b[31m-\u001b[39m' : '-'; let message = [ start, `${actExp} ... Lines skipped`, @@ -452,8 +447,8 @@ common.expectsError( ' [', '...', ' 2,', - `${minus} 3`, - `${plus} '3'`, + '- 3', + "+ '3'", ' ]', '...', ' 5', @@ -470,7 +465,7 @@ common.expectsError( ' 1,', '...', ' 0,', - `${plus} 1,`, + '+ 1,', ' 1,', '...', ' 1', @@ -490,7 +485,7 @@ common.expectsError( ' 1,', '...', ' 0,', - `${minus} 1,`, + '- 1,', ' 1,', '...', ' 1', @@ -508,12 +503,12 @@ common.expectsError( '', ' [', ' 1,', - `${minus} 2,`, - `${plus} 1,`, + '- 2,', + '+ 1,', ' 1,', ' 1,', ' 0,', - `${minus} 1,`, + '- 1,', ' 1', ' ]' ].join('\n'); @@ -527,12 +522,12 @@ common.expectsError( start, actExp, '', - `${minus} [`, - `${minus} 1,`, - `${minus} 2,`, - `${minus} 1`, - `${minus} ]`, - `${plus} undefined`, + '- [', + '- 1,', + '- 2,', + '- 1', + '- ]', + '+ undefined', ].join('\n'); assert.throws( () => assert.deepEqual([1, 2, 1]), @@ -543,7 +538,7 @@ common.expectsError( actExp, '', ' [', - `${minus} 1,`, + '- 1,', ' 2,', ' 1', ' ]' @@ -556,9 +551,9 @@ common.expectsError( `${actExp} ... Lines skipped\n` + '\n' + ' [\n' + - `${minus} 1,\n`.repeat(10) + + '- 1,\n'.repeat(10) + '...\n' + - `${plus} 2,\n`.repeat(10) + + '+ 2,\n'.repeat(10) + '...'; assert.throws( () => assert.deepEqual(Array(12).fill(1), Array(12).fill(2)), @@ -572,11 +567,11 @@ common.expectsError( message: `${start}\n` + `${actExp}\n` + '\n' + - `${minus} {}\n` + - `${plus} {\n` + - `${plus} loop: 'forever',\n` + - `${plus} [Symbol(util.inspect.custom)]: [Function]\n` + - `${plus} }` + '- {}\n' + + '+ {\n' + + "+ loop: 'forever',\n" + + '+ [Symbol(util.inspect.custom)]: [Function]\n' + + '+ }' }); // notDeepEqual tests diff --git a/test/pseudo-tty/test-assert-colors.js b/test/pseudo-tty/test-assert-colors.js new file mode 100644 index 00000000000000..7c5845bdaa271a --- /dev/null +++ b/test/pseudo-tty/test-assert-colors.js @@ -0,0 +1,18 @@ +'use strict'; +require('../common'); +const assert = require('assert').strict; + +try { + // Activate colors even if the tty does not support colors. + process.env.COLORTERM = '1'; + assert.deepStrictEqual([1, 2], [2, 2]); +} catch (err) { + const expected = 'Input A expected to deepStrictEqual input B:\n' + + '\u001b[32m+ expected\u001b[39m \u001b[31m- actual\u001b[39m\n\n' + + ' [\n' + + '\u001b[31m-\u001b[39m 1,\n' + + '\u001b[32m+\u001b[39m 2,\n' + + ' 2\n' + + ' ]'; + assert.strictEqual(err.message, expected); +} diff --git a/test/pseudo-tty/test-assert-colors.out b/test/pseudo-tty/test-assert-colors.out new file mode 100644 index 00000000000000..e69de29bb2d1d6