Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test_runner: display failed test stack trace with dot reporter #52655

Merged
merged 1 commit into from
May 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions lib/internal/test_runner/reporter/dot.js
Original file line number Diff line number Diff line change
@@ -1,18 +1,22 @@
'use strict';

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change seems irrelevant

const {
ArrayPrototypePush,
MathMax,
} = primordials;
const colors = require('internal/util/colors');
const { formatTestReport } = require('internal/test_runner/reporter/utils');

module.exports = async function* dot(source) {
let count = 0;
let columns = getLineLength();
for await (const { type } of source) {
const failedTests = [];
for await (const { type, data } of source) {
if (type === 'test:pass') {
yield '.';
}
if (type === 'test:fail') {
yield 'X';
ArrayPrototypePush(failedTests, data);
}
if ((type === 'test:fail' || type === 'test:pass') && ++count === columns) {
yield '\n';
Expand All @@ -23,6 +27,12 @@ module.exports = async function* dot(source) {
}
}
yield '\n';
if (failedTests.length > 0) {
yield `\n${colors.red}Failed tests:${colors.white}\n\n`;
for (const test of failedTests) {
yield formatTestReport('test:fail', test);
}
}
};

function getLineLength() {
Expand Down
89 changes: 14 additions & 75 deletions lib/internal/test_runner/reporter/spec.js
Original file line number Diff line number Diff line change
@@ -1,97 +1,35 @@
'use strict';

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

const {
ArrayPrototypeJoin,
ArrayPrototypePop,
ArrayPrototypePush,
ArrayPrototypeShift,
ArrayPrototypeUnshift,
RegExpPrototypeSymbolSplit,
SafeMap,
StringPrototypeRepeat,
hardenRegExp,
} = primordials;
const assert = require('assert');
const Transform = require('internal/streams/transform');
const { inspectWithNoCustomRetry } = require('internal/errors');
const colors = require('internal/util/colors');
const { kSubtestsFailed } = require('internal/test_runner/test');
const { getCoverageReport } = require('internal/test_runner/utils');
const { relative } = require('path');
const {
formatTestReport,
indent,
reporterColorMap,
reporterUnicodeSymbolMap,
} = require('internal/test_runner/reporter/utils');

const symbols = {
'__proto__': null,
'test:fail': '\u2716 ',
'test:pass': '\u2714 ',
'test:diagnostic': '\u2139 ',
'test:coverage': '\u2139 ',
'arrow:right': '\u25B6 ',
'hyphen:minus': '\uFE63 ',
};
class SpecReporter extends Transform {
#stack = [];
#reported = [];
#indentMemo = new SafeMap();
#failedTests = [];
#cwd = process.cwd();
#inspectOptions;
#colors;

constructor() {
super({ __proto__: null, writableObjectMode: true });
colors.refresh();
this.#inspectOptions = { __proto__: null, colors: colors.shouldColorize(process.stdout), breakLength: Infinity };
this.#colors = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moving colors to utils breaks when there is no TTY, see https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/35939/console:

colors cannot be deconstructed too early.

11:01:44         not ok 41 - test-runner/output/arbitrary-output-colored.js
11:01:44           ---
11:01:44           duration_ms: 5585.15492
11:01:44           location: '/home/iojs/build/workspace/node-test-commit-custom-suites-freestyle/test/parallel/test-runner-output.mjs:155:5'
11:01:44           failureType: 'testCodeFailure'
11:01:44           error: |-
11:01:44             Expected values to be strictly equal:
11:01:44             + actual - expected ... Lines skipped
11:01:44             
11:01:44               "{ foo: [32m'bar'[39m }\n" +
11:01:44                 '[33m1[39m\n' +
11:01:44             +   '✔ passing test [90m(*ms)[39m[39m\n' +
11:01:44             +   'ℹ tests 1[39m\n' +
11:01:44             +   'ℹ suites 0[39m\n' +
11:01:44             +   'ℹ pass 1[39m\n' +
11:01:44             +   'ℹ fail 0[39m\n' +
11:01:44             +   'ℹ cancelled 0[39m\n' +
11:01:44             +   'ℹ skipped 0[39m\n' +
11:01:44             +   'ℹ todo 0[39m\n' +
11:01:44             +   'ℹ duration_ms *[39m\n' +
11:01:44             -   '[32m✔ passing test [90m(*ms)[39m[39m\n' +
11:01:44             -   '[34mℹ tests 1[39m\n' +
11:01:44             -   '[34mℹ suites 0[39m\n' +
11:01:44             -   '[34mℹ pass 1[39m\n' +
11:01:44             -   '[34mℹ fail 0[39m\n' +
11:01:44             -   '[34mℹ cancelled 0[39m\n' +
11:01:44             -   '[34mℹ skipped 0[39m\n' +
11:01:44             -   '[34mℹ todo 0[39m\n' +
11:01:44             -   '[34mℹ duration_ms *[39m\n' +
11:01:44                 'TAP version 13\n' +
11:01:44             ...
11:01:44                 '# skipped 0\n' +
11:01:44                 '# todo 0\n' +
11:01:44                 '# duration_ms *\n'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changing reporterColorMap to a function might be the easiest fix

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

arbitrary-output-colored test passes on my machine. How can I reproduce the error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changing reporterColorMap to a function might be the easiest fix

are you suggesting to replace the map with a function in utils.js or modify the constructor in spec.js?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test passes on my machine. How can I reproduce the error?

with FORCE_COLOR=0 NODE_REGENERATE_SNAPSHOTS=1 ./node test/parallel/test-runner-output.mjs

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you suggesting to replace the map with a function in utils.js or modify the constructor in spec.js?

either will work I think

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this might be an easier & cleaner fix:

diff --git a/lib/internal/test_runner/reporter/utils.js b/lib/internal/test_runner/reporter/utils.js
index caa35959ee..42c4ffa3cd 100644
--- a/lib/internal/test_runner/reporter/utils.js
+++ b/lib/internal/test_runner/reporter/utils.js
@@ -28,9 +28,15 @@ const reporterUnicodeSymbolMap = {
 
 const reporterColorMap = {
   '__proto__': null,
-  'test:fail': colors.red,
-  'test:pass': colors.green,
-  'test:diagnostic': colors.blue,
+  get 'test:fail'() {
+    return colors.red;
+  },
+  get 'test:pass'() {
+    return colors.green;
+  },
+  get 'test:diagnostic'() {
+    return colors.blue;
+  },
 };

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test passes on my machine. How can I reproduce the error?

with FORCE_COLOR=0 NODE_REGENERATE_SNAPSHOTS=1 ./node test/parallel/test-runner-output.mjs

Still passes all tests :(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have made the suggested changes, please let me know if I need to add anything else.

'__proto__': null,
'test:fail': colors.red,
'test:pass': colors.green,
'test:diagnostic': colors.blue,
};
}

#indent(nesting) {
let value = this.#indentMemo.get(nesting);
if (value === undefined) {
value = StringPrototypeRepeat(' ', nesting);
this.#indentMemo.set(nesting, value);
}

return value;
}
#formatError(error, indent) {
if (!error) return '';
const err = error.code === 'ERR_TEST_FAILURE' ? error.cause : error;
const message = ArrayPrototypeJoin(
RegExpPrototypeSymbolSplit(
hardenRegExp(/\r?\n/),
inspectWithNoCustomRetry(err, this.#inspectOptions),
), `\n${indent} `);
return `\n${indent} ${message}\n`;
}
#formatTestReport(type, data, prefix = '', indent = '', hasChildren = false) {
let color = this.#colors[type] ?? colors.white;
let symbol = symbols[type] ?? ' ';
const { skip, todo } = data;
const duration_ms = data.details?.duration_ms ? ` ${colors.gray}(${data.details.duration_ms}ms)${colors.white}` : '';
let title = `${data.name}${duration_ms}`;

if (skip !== undefined) {
title += ` # ${typeof skip === 'string' && skip.length ? skip : 'SKIP'}`;
} else if (todo !== undefined) {
title += ` # ${typeof todo === 'string' && todo.length ? todo : 'TODO'}`;
}
const error = this.#formatError(data.details?.error, indent);
if (hasChildren) {
// If this test has had children - it was already reported, so slightly modify the output
const err = !error || data.details?.error?.failureType === 'subtestsFailed' ? '' : `\n${error}`;
return `${prefix}${indent}${color}${symbols['arrow:right']}${colors.white}${title}${err}`;
}
if (skip !== undefined) {
color = colors.gray;
symbol = symbols['hyphen:minus'];
}
return `${prefix}${indent}${color}${symbol}${title}${colors.white}${error}`;
}
#handleTestReportEvent(type, data) {
const subtest = ArrayPrototypeShift(this.#stack); // This is the matching `test:start` event
if (subtest) {
Expand All @@ -106,15 +44,15 @@ class SpecReporter extends Transform {
assert(parent.type === 'test:start');
const msg = parent.data;
ArrayPrototypeUnshift(this.#reported, msg);
prefix += `${this.#indent(msg.nesting)}${symbols['arrow:right']}${msg.name}\n`;
prefix += `${indent(msg.nesting)}${reporterUnicodeSymbolMap['arrow:right']}${msg.name}\n`;
}
let hasChildren = false;
if (this.#reported[0] && this.#reported[0].nesting === data.nesting && this.#reported[0].name === data.name) {
ArrayPrototypeShift(this.#reported);
hasChildren = true;
}
const indent = this.#indent(data.nesting);
return `${this.#formatTestReport(type, data, prefix, indent, hasChildren)}\n`;
const indentation = indent(data.nesting);
return `${formatTestReport(type, data, prefix, indentation, hasChildren)}\n`;
}
#handleEvent({ type, data }) {
switch (type) {
Expand All @@ -132,9 +70,10 @@ class SpecReporter extends Transform {
case 'test:stdout':
return data.message;
case 'test:diagnostic':
return `${this.#colors[type]}${this.#indent(data.nesting)}${symbols[type]}${data.message}${colors.white}\n`;
return `${reporterColorMap[type]}${indent(data.nesting)}${reporterUnicodeSymbolMap[type]}${data.message}${colors.white}\n`;
case 'test:coverage':
return getCoverageReport(this.#indent(data.nesting), data.summary, symbols['test:coverage'], colors.blue, true);
return getCoverageReport(indent(data.nesting), data.summary,
reporterUnicodeSymbolMap['test:coverage'], colors.blue, true);
}
}
_transform({ type, data }, encoding, callback) {
Expand All @@ -145,10 +84,10 @@ class SpecReporter extends Transform {
callback(null, '');
return;
}
const results = [`\n${this.#colors['test:fail']}${symbols['test:fail']}failing tests:${colors.white}\n`];
const results = [`\n${reporterColorMap['test:fail']}${reporterUnicodeSymbolMap['test:fail']}failing tests:${colors.white}\n`];
for (let i = 0; i < this.#failedTests.length; i++) {
const test = this.#failedTests[i];
const formattedErr = this.#formatTestReport('test:fail', test);
const formattedErr = formatTestReport('test:fail', test);

if (test.file) {
const relPath = relative(this.#cwd, test.file);
Expand Down
93 changes: 93 additions & 0 deletions lib/internal/test_runner/reporter/utils.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
'use strict';
const {
ArrayPrototypeJoin,
RegExpPrototypeSymbolSplit,
SafeMap,
StringPrototypeRepeat,
hardenRegExp,
} = primordials;
const colors = require('internal/util/colors');
const { inspectWithNoCustomRetry } = require('internal/errors');
const indentMemo = new SafeMap();

const inspectOptions = {
__proto__: null,
colors: colors.shouldColorize(process.stdout),
breakLength: Infinity,
};

const reporterUnicodeSymbolMap = {
'__proto__': null,
MoLow marked this conversation as resolved.
Show resolved Hide resolved
'test:fail': '\u2716 ',
'test:pass': '\u2714 ',
'test:diagnostic': '\u2139 ',
'test:coverage': '\u2139 ',
'arrow:right': '\u25B6 ',
'hyphen:minus': '\uFE63 ',
};

const reporterColorMap = {
'__proto__': null,
get 'test:fail'() {
return colors.red;
},
get 'test:pass'() {
return colors.green;
},
get 'test:diagnostic'() {
return colors.blue;
},
};

function indent(nesting) {
let value = indentMemo.get(nesting);
if (value === undefined) {
value = StringPrototypeRepeat(' ', nesting);
indentMemo.set(nesting, value);
}
return value;
}

function formatError(error, indent) {
if (!error) return '';
const err = error.code === 'ERR_TEST_FAILURE' ? error.cause : error;
const message = ArrayPrototypeJoin(
RegExpPrototypeSymbolSplit(
hardenRegExp(/\r?\n/),
inspectWithNoCustomRetry(err, inspectOptions),
), `\n${indent} `);
return `\n${indent} ${message}\n`;
}

function formatTestReport(type, data, prefix = '', indent = '', hasChildren = false) {
let color = reporterColorMap[type] ?? colors.white;
let symbol = reporterUnicodeSymbolMap[type] ?? ' ';
const { skip, todo } = data;
const duration_ms = data.details?.duration_ms ? ` ${colors.gray}(${data.details.duration_ms}ms)${colors.white}` : '';
let title = `${data.name}${duration_ms}`;

if (skip !== undefined) {
title += ` # ${typeof skip === 'string' && skip.length ? skip : 'SKIP'}`;
} else if (todo !== undefined) {
title += ` # ${typeof todo === 'string' && todo.length ? todo : 'TODO'}`;
}
const error = formatError(data.details?.error, indent);
if (hasChildren) {
// If this test has had children - it was already reported, so slightly modify the output
const err = !error || data.details?.error?.failureType === 'subtestsFailed' ? '' : `\n${error}`;
return `${prefix}${indent}${color}${reporterUnicodeSymbolMap['arrow:right']}${colors.white}${title}${err}`;
}
if (skip !== undefined) {
color = colors.gray;
symbol = reporterUnicodeSymbolMap['hyphen:minus'];
}
return `${prefix}${indent}${color}${symbol}${title}${colors.white}${error}`;
}

module.exports = {
__proto__: null,
reporterUnicodeSymbolMap,
reporterColorMap,
formatTestReport,
indent,
};
Loading