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.

Sign up for GitHub

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: refactor spec reporter to generator function #50177

Closed
wants to merge 3 commits into from
Closed

test_runner: refactor spec reporter to generator function #50177

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dba68c0
test_runner: refactor spec reporter to generator function
himself65 Oct 13, 2023
e218ad6
fixup! fix: remove this
himself65 Oct 13, 2023
331f9a7
fixup! fix: code
himself65 Oct 14, 2023
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
95 changes: 46 additions & 49 deletions lib/internal/test_runner/reporter/spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@
SafeMap,
StringPrototypeRepeat,
} = primordials;

const assert = require('assert');
const Transform = require('internal/streams/transform');
const { inspectWithNoCustomRetry } = require('internal/errors');
const { green, blue, red, white, gray, shouldColorize } = require('internal/util/colors');
const { kSubtestsFailed } = require('internal/test_runner/test');
Expand All @@ -27,6 +27,7 @@
'test:pass': green,
'test:diagnostic': blue,
};

const symbols = {
'__proto__': null,
'test:fail': '\u2716 ',
Expand All @@ -36,27 +37,25 @@
'arrow:right': '\u25B6 ',
'hyphen:minus': '\uFE63 ',
};
class SpecReporter extends Transform {
#stack = [];
#reported = [];
#indentMemo = new SafeMap();
#failedTests = [];
#cwd = process.cwd();

constructor() {
super({ __proto__: null, writableObjectMode: true });
}

#indent(nesting) {
let value = this.#indentMemo.get(nesting);
module.exports = async function *specReporter(source) {
Copy link
Member

Choose a reason for hiding this comment

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

This would still need to be a transform stream, API wise (can be from a generator)

let stack = [];

Check failure on line 42 in lib/internal/test_runner/reporter/spec.js

View workflow job for this annotation

GitHub Actions / lint-js-and-md 

'stack' is never reassigned. Use 'const' instead
let reported = [];

Check failure on line 43 in lib/internal/test_runner/reporter/spec.js

View workflow job for this annotation

GitHub Actions / lint-js-and-md 

'reported' is never reassigned. Use 'const' instead
let indentMemo = new SafeMap();

Check failure on line 44 in lib/internal/test_runner/reporter/spec.js

View workflow job for this annotation

GitHub Actions / lint-js-and-md 

'indentMemo' is never reassigned. Use 'const' instead
let failedTests = [];

Check failure on line 45 in lib/internal/test_runner/reporter/spec.js

View workflow job for this annotation

GitHub Actions / lint-js-and-md 

'failedTests' is never reassigned. Use 'const' instead
let cwd = process.cwd();

Check failure on line 46 in lib/internal/test_runner/reporter/spec.js

View workflow job for this annotation

GitHub Actions / lint-js-and-md 

'cwd' is never reassigned. Use 'const' instead

function indent(nesting) {
Copy link
Member

Choose a reason for hiding this comment

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

this means all of these functions are defined anew every time this reporter is called, instead of just once at module startup.

Copy link
Member Author

Choose a reason for hiding this comment

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

these functions require closure variables

Copy link
Member

Choose a reason for hiding this comment

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

then if you're switching to a non-class-based model, the functions should take everything as arguments, so they can be defined at module level.

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

return value;
}
#formatError(error, indent) {

function formatError(error, indent) {
if (!error) return '';
const err = error.code === 'ERR_TEST_FAILURE' ? error.cause : error;
const message = ArrayPrototypeJoin(
Expand All @@ -66,7 +65,8 @@
), `\n${indent} `);
return `\n${indent} ${message}\n`;
}
#formatTestReport(type, data, prefix = '', indent = '', hasChildren = false) {

function formatTestReport(type, data, prefix = '', indent = '', hasChildren = false) {
let color = colors[type] ?? white;
let symbol = symbols[type] ?? ' ';
const { skip, todo } = data;
Expand All @@ -82,77 +82,74 @@
// If this test has had children - it was already reported, so slightly modify the output
return `${prefix}${indent}${color}${symbols['arrow:right']}${white}${title}\n`;
}
const error = this.#formatError(data.details?.error, indent);
const error = formatError(data.details?.error, indent);
if (skip !== undefined) {
color = gray;
symbol = symbols['hyphen:minus'];
}
return `${prefix}${indent}${color}${symbol}${title}${white}${error}`;
}
#handleTestReportEvent(type, data) {
const subtest = ArrayPrototypeShift(this.#stack); // This is the matching `test:start` event

function handleTestReportEvent(type, data) {
const subtest = ArrayPrototypeShift(stack); // This is the matching `test:start` event
if (subtest) {
assert(subtest.type === 'test:start');
assert(subtest.data.nesting === data.nesting);
assert(subtest.data.name === data.name);
}
let prefix = '';
while (this.#stack.length) {
while (stack.length) {
// Report all the parent `test:start` events
const parent = ArrayPrototypePop(this.#stack);
const parent = ArrayPrototypePop(stack);
assert(parent.type === 'test:start');
const msg = parent.data;
ArrayPrototypeUnshift(this.#reported, msg);
prefix += `${this.#indent(msg.nesting)}${symbols['arrow:right']}${msg.name}\n`;
ArrayPrototypeUnshift(reported, msg);
prefix += `${indent(msg.nesting)}${symbols['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);
if (reported[0] && reported[0].nesting === data.nesting && reported[0].name === data.name) {
ArrayPrototypeShift(reported);
hasChildren = true;
}
const indent = this.#indent(data.nesting);
return `${this.#formatTestReport(type, data, prefix, indent, hasChildren)}\n`;
return `${formatTestReport(type, data, prefix, indent(data.nesting), hasChildren)}\n`;
}
#handleEvent({ type, data }) {

function handleEvent({ type, data }) {
switch (type) {
case 'test:fail':
if (data.details?.error?.failureType !== kSubtestsFailed) {
ArrayPrototypePush(this.#failedTests, data);
ArrayPrototypePush(failedTests, data);
}
return this.#handleTestReportEvent(type, data);
return handleTestReportEvent(type, data);
case 'test:pass':
return this.#handleTestReportEvent(type, data);
return handleTestReportEvent(type, data);
case 'test:start':
ArrayPrototypeUnshift(this.#stack, { __proto__: null, data, type });
ArrayPrototypeUnshift(stack, { __proto__: null, data, type });
break;
case 'test:stderr':
case 'test:stdout':
return data.message;
case 'test:diagnostic':
return `${colors[type]}${this.#indent(data.nesting)}${symbols[type]}${data.message}${white}\n`;
return `${colors[type]}${indent(data.nesting)}${symbols[type]}${data.message}${white}\n`;
case 'test:coverage':
return getCoverageReport(this.#indent(data.nesting), data.summary, symbols['test:coverage'], blue, true);
return getCoverageReport(indent(data.nesting), data.summary, symbols['test:coverage'], blue, true);
}
}
_transform({ type, data }, encoding, callback) {
callback(null, this.#handleEvent({ __proto__: null, type, data }));

for await (const event of source) {
yield handleEvent(event);
}
_flush(callback) {
if (this.#failedTests.length === 0) {
callback(null, '');
return;
}

if (failedTests.length !== 0) {
const results = [`\n${colors['test:fail']}${symbols['test:fail']}failing tests:${white}\n`];
for (let i = 0; i < this.#failedTests.length; i++) {
const test = this.#failedTests[i];
const relPath = relative(this.#cwd, test.file);
const formattedErr = this.#formatTestReport('test:fail', test);
for (let i = 0; i < failedTests.length; i++) {
const test = failedTests[i];
const relPath = relative(cwd, test.file);
const formattedErr = formatTestReport('test:fail', test);
const location = `test at ${relPath}:${test.line}:${test.column}`;

ArrayPrototypePush(results, location, formattedErr);
}
callback(null, ArrayPrototypeJoin(results, '\n'));
yield ArrayPrototypeJoin(results, '\n');
}
}

module.exports = SpecReporter;
};
4 changes: 2 additions & 2 deletions lib/test/reporters.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
'use strict';

const { ObjectDefineProperties, ReflectConstruct } = primordials;

Check failure on line 3 in lib/test/reporters.js

View workflow job for this annotation

GitHub Actions / lint-js-and-md 

'ReflectConstruct' is assigned a value but never used

let dot;
let junit;
Expand Down Expand Up @@ -31,9 +31,9 @@
__proto__: null,
configurable: true,
enumerable: true,
value: function value() {
get() {
spec ??= require('internal/test_runner/reporter/spec');
return ReflectConstruct(spec, arguments);
return spec;
},
},
tap: {
Expand Down
Loading