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: changing spec from class to function #48208

Closed
wants to merge 8 commits into from
Closed

test_runner: changing spec from class to function #48208

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3ca1251
test_runner: changing spec from class to function
shubham9411 May 27, 2023
cd87a5b
test_runner: changing spec from class to function
shubham9411 May 27, 2023
f04482d
Merge branch 'making-specReporter-functional' of github.com:shubham94…
shubham9411 May 27, 2023
b4a450b
Merge branch 'main' of github.com:shubham9411/node into making-specRe…
shubham9411 Jul 9, 2023
4b33ac5
spec transform
shubham9411 Jul 15, 2023
5e43fee
Merge branch 'main' of github.com:shubham9411/node into making-specRe…
shubham9411 Jul 15, 2023
d6c0e92
Fixing tests
shubham9411 Jul 15, 2023
e1d57a9
fix: eslint issues
shubham9411 Jul 15, 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
90 changes: 44 additions & 46 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 @@ -12,7 +12,6 @@ const {
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 @@ -35,26 +34,24 @@ const symbols = {
'arrow:right': '\u25B6 ',
'hyphen:minus': '\uFE63 ',
};
class SpecReporter extends Transform {
Copy link
Member

Choose a reason for hiding this comment

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

@MoLow @cjihrig Do we still want/need this?

node/lib/internal/test_runner/utils.js

Lines 142 to 144 in c462147

if (reporter?.prototype && ObjectGetOwnPropertyDescriptor(reporter.prototype, 'constructor')) {
reporter = new reporter();
}

The docs do not show passing a constructor as an option...

Copy link
Member

Choose a reason for hiding this comment

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

yes, for custom reporters I guess

Copy link
Member

Choose a reason for hiding this comment

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

Then I guess we might want to add an example for that to the docs?

#stack = [];
#reported = [];
#indentMemo = new SafeMap();
#failedTests = [];

constructor() {
super({ writableObjectMode: true });
}
async function* specReporter(source) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we want new specReporter to keep working (and I think we do, otherwise it's a breaking change), we cannot use a generator here, it has to be a function specReporter(source), async functions and (async) generators are not "constructable".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just thinking out load, Is it possible to make specReporter function work consistently whether we use new specReporter() or just specReporter? I tried by it but it only works one way, can you suggest anything?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's what I meant, you need to use function specReporter(source) { /* … */ }, AFAIK it's the only thing is JS that is both callable and constructible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok sorry, I got confused, so I did made it like function specReporter(source) { /* … */ } and it works with new spec() and also work as spec(), but does not work standalone like the dot and tap. viz:

run(...).compose(new spec()); // this works
run(...).compose(spec()); // this works
run(...).compose(spec); // this does not work
run(...).compose(tap); // <- tap and dot work like this

the third one above is used like the dot and tap. If spec() is also acceptable, let me know I'll push the change. Or let me know if my understanding is correct or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you share what's the error you're getting? That would help understand what might be the problem.

const stack = [];
const reported = [];
const indentMemo = new SafeMap();
const failedTests = [];

#indent(nesting) {
let value = this.#indentMemo.get(nesting);
function indent(nesting) {
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 @@ -64,7 +61,8 @@ class SpecReporter extends Transform {
), `\n${indent} `);
return `\n${indent} ${message}\n`;
}
#formatTestReport(type, data, prefix = '', indent = '', hasChildren = false, skippedSubtest = false) {

function formatTestReport(type, data, prefix = '', indent = '', hasChildren = false, skippedSubtest = false) {
let color = colors[type] ?? white;
let symbol = symbols[type] ?? ' ';
const duration_ms = data.details?.duration_ms ? ` ${gray}(${data.details.duration_ms}ms)${white}` : '';
Expand All @@ -73,76 +71,76 @@ class SpecReporter extends Transform {
// 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 (skippedSubtest) {
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 skippedSubtest = subtest && data.skip && data.skip !== undefined;
const indent = this.#indent(data.nesting);
return `${this.#formatTestReport(type, data, prefix, indent, hasChildren, skippedSubtest)}\n`;
const _indent = indent(data.nesting);
return `${formatTestReport(type, data, prefix, _indent, hasChildren, skippedSubtest)}\n`;
}
#handleEvent({ type, data }) {

for await (const { type, data } of source) {
switch (type) {
case 'test:fail':
if (data.details?.error?.failureType !== kSubtestsFailed) {
ArrayPrototypePush(this.#failedTests, data);
ArrayPrototypePush(failedTests, data);
}
return this.#handleTestReportEvent(type, data);
yield handleTestReportEvent(type, data);
break;
case 'test:pass':
return this.#handleTestReportEvent(type, data);
yield handleTestReportEvent(type, data);
break;
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;
yield data.message;
break;
case 'test:diagnostic':
return `${colors[type]}${this.#indent(data.nesting)}${symbols[type]}${data.message}${white}\n`;
yield `${colors[type]}${indent(data.nesting)}${symbols[type]}${data.message}${white}\n`;
break;
case 'test:coverage':
return getCoverageReport(this.#indent(data.nesting), data.summary, symbols['test:coverage'], blue, true);
yield getCoverageReport(indent(data.nesting), data.summary, symbols['test:coverage'], blue, true);
break;
}
}
_transform({ type, data }, encoding, callback) {
callback(null, this.#handleEvent({ type, data }));
}
_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++) {
ArrayPrototypePush(results, this.#formatTestReport(
for (let i = 0; i < failedTests.length; i++) {
ArrayPrototypePush(results, formatTestReport(
'test:fail',
this.#failedTests[i],
failedTests[i],
));
}
callback(null, ArrayPrototypeJoin(results, '\n'));
yield results.join('\n');
}
}

module.exports = SpecReporter;
module.exports = specReporter;
11 changes: 8 additions & 3 deletions test/parallel/test-runner-run.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,12 +76,17 @@ describe('require(\'node:test\').run', { concurrency: true }, () => {
});

it('should be piped with spec', async () => {
const specReporter = new spec();
const result = await run({ files: [join(testFixtures, 'test/random.cjs')] }).compose(specReporter).toArray();
const result = await run({ files: [join(testFixtures, 'test/random.cjs')] }).compose(spec).toArray();
const stringResults = result.map((bfr) => bfr.toString());
assert.match(stringResults[0], /this should pass/);
assert.match(stringResults[1], /tests 1/);
assert.match(stringResults[1], /pass 1/);
assert.match(stringResults[2], /suites 0/);
assert.match(stringResults[3], /pass 1/);
assert.match(stringResults[4], /fail 0/);
assert.match(stringResults[5], /cancelled 0/);
assert.match(stringResults[6], /skipped 0/);
assert.match(stringResults[7], /todo 0/);
assert.match(stringResults[8], /duration_ms \d+\.?\d*/);
});

it('should be piped with tap', async () => {
Expand Down