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: #19464 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
nodejs · Mar 27, 2018 · 2e6dd93 · 2e6dd93
1 parent d74919c
commit 2e6dd93
Show file tree

Hide file tree

Showing 4 changed files with 74 additions and 59 deletions.
diff --git 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
@@ -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
@@ -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