From 2e6dd93aaa40e9f205a2e84920213effab81bea1 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. PR-URL: https://github.com/nodejs/node/pull/19464 Reviewed-By: James M Snell Reviewed-By: Matteo Collina --- 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 16b4cd2416f..e79041d91d5 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}`; @@ -768,10 +771,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 195d9bbc801..54972657e94 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 00000000000..7c5845bdaa2 --- /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 00000000000..e69de29bb2d