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: migrate test runner message tests to snapshot #47392

Merged
merged 1 commit into from
Apr 10, 2023
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
1 change: 0 additions & 1 deletion .github/CODEOWNERS
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,6 @@

# Test runner

/test/message/test_runner_* @nodejs/test_runner
/test/parallel/test-runner-* @nodejs/test_runner
Copy link
Member

Choose a reason for hiding this comment

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

Not in this PR, but perhaps we should move all the test runner stuff into /test/test-runner?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm +0 on that

/doc/api/test.md @nodejs/test_runner
/lib/test.js @nodejs/test_runner
Expand Down
6 changes: 6 additions & 0 deletions test/common/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -642,6 +642,12 @@ environment variables.
If set, `NODE_COMMON_PORT`'s value overrides the `common.PORT` default value of
12346\.

### `NODE_REGENERATE_SNAPSHOTS`

If set, test snapshots for a the current test are regenerated.
for example `NODE_REGENERATE_SNAPSHOTS=1 out/Release/node test/parallel/test-runner-output.mjs`
will update all the test runner output snapshots.

### `NODE_SKIP_FLAG_CHECK`

If set, command line arguments passed to individual tests are not validated.
Expand Down
53 changes: 53 additions & 0 deletions test/common/assertSnapshot.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
'use strict';
const common = require('.');
const path = require('node:path');
const fs = require('node:fs/promises');
const assert = require('node:assert/strict');


const stackFramesRegexp = /(\s+)((.+?)\s+\()?(?:\(?(.+?):(\d+)(?::(\d+))?)\)?(\s+\{)?(\n|$)/g;
const windowNewlineRegexp = /\r/g;

function replaceStackTrace(str) {
return str.replace(stackFramesRegexp, '$1*$7\n');
}

function replaceWindowsLineEndings(str) {
return str.replace(windowNewlineRegexp, '');
}

function transform(...args) {
return (str) => args.reduce((acc, fn) => fn(acc), str);
}

function getSnapshotPath(filename) {
const { name, dir } = path.parse(filename);
return path.resolve(dir, `${name}.snapshot`);
}

async function assertSnapshot(actual, filename = process.argv[1]) {
const snapshot = getSnapshotPath(filename);
if (process.env.NODE_REGENERATE_SNAPSHOTS) {
await fs.writeFile(snapshot, actual);
} else {
const expected = await fs.readFile(snapshot, 'utf8');
assert.strictEqual(actual, replaceWindowsLineEndings(expected));
}
}

async function spawnAndAssert(filename, transform = (x) => x) {
// TODO: Add an option to this function to alternatively or additionally compare stderr.
// For now, tests that want to check stderr or both stdout and stderr can use spawnPromisified.
const flags = common.parseTestFlags(filename);
const { stdout } = await common.spawnPromisified(process.execPath, [...flags, filename]);
await assertSnapshot(transform(stdout), filename);
}

module.exports = {
assertSnapshot,
getSnapshotPath,
replaceStackTrace,
replaceWindowsLineEndings,
spawnAndAssert,
transform,
};
83 changes: 45 additions & 38 deletions test/common/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,31 @@ const hasOpenSSL3 = hasCrypto &&

const hasQuic = hasCrypto && !!process.config.variables.openssl_quic;

function parseTestFlags(filename = process.argv[1]) {
// The copyright notice is relatively big and the flags could come afterwards.
const bytesToRead = 1500;
const buffer = Buffer.allocUnsafe(bytesToRead);
const fd = fs.openSync(filename, 'r');
const bytesRead = fs.readSync(fd, buffer, 0, bytesToRead);
fs.closeSync(fd);
const source = buffer.toString('utf8', 0, bytesRead);

const flagStart = source.indexOf('// Flags: --') + 10;

if (flagStart === 9) {
return [];
}
let flagEnd = source.indexOf('\n', flagStart);
// Normalize different EOL.
if (source[flagEnd - 1] === '\r') {
flagEnd--;
}
return source
.substring(flagStart, flagEnd)
.replace(/_/g, '-')
.split(' ');
}
GeoffreyBooth marked this conversation as resolved.
Show resolved Hide resolved

// Check for flags. Skip this for workers (both, the `cluster` module and
// `worker_threads`) and child processes.
// If the binary was built without-ssl then the crypto flags are
Expand All @@ -71,44 +96,25 @@ if (process.argv.length === 2 &&
hasCrypto &&
require('cluster').isPrimary &&
fs.existsSync(process.argv[1])) {
// The copyright notice is relatively big and the flags could come afterwards.
const bytesToRead = 1500;
const buffer = Buffer.allocUnsafe(bytesToRead);
const fd = fs.openSync(process.argv[1], 'r');
const bytesRead = fs.readSync(fd, buffer, 0, bytesToRead);
fs.closeSync(fd);
const source = buffer.toString('utf8', 0, bytesRead);

const flagStart = source.indexOf('// Flags: --') + 10;
if (flagStart !== 9) {
let flagEnd = source.indexOf('\n', flagStart);
// Normalize different EOL.
if (source[flagEnd - 1] === '\r') {
flagEnd--;
}
const flags = source
.substring(flagStart, flagEnd)
.replace(/_/g, '-')
.split(' ');
const args = process.execArgv.map((arg) => arg.replace(/_/g, '-'));
for (const flag of flags) {
if (!args.includes(flag) &&
// If the binary is build without `intl` the inspect option is
// invalid. The test itself should handle this case.
(process.features.inspector || !flag.startsWith('--inspect'))) {
console.log(
'NOTE: The test started as a child_process using these flags:',
inspect(flags),
'Use NODE_SKIP_FLAG_CHECK to run the test with the original flags.',
);
const args = [...flags, ...process.execArgv, ...process.argv.slice(1)];
const options = { encoding: 'utf8', stdio: 'inherit' };
const result = spawnSync(process.execPath, args, options);
if (result.signal) {
process.kill(0, result.signal);
} else {
process.exit(result.status);
}
const flags = parseTestFlags();
const args = process.execArgv.map((arg) => arg.replace(/_/g, '-'));
for (const flag of flags) {
if (!args.includes(flag) &&
// If the binary is build without `intl` the inspect option is
// invalid. The test itself should handle this case.
(process.features.inspector || !flag.startsWith('--inspect'))) {
console.log(
'NOTE: The test started as a child_process using these flags:',
inspect(flags),
'Use NODE_SKIP_FLAG_CHECK to run the test with the original flags.',
);
const args = [...flags, ...process.execArgv, ...process.argv.slice(1)];
const options = { encoding: 'utf8', stdio: 'inherit' };
const result = spawnSync(process.execPath, args, options);
if (result.signal) {
process.kill(0, result.signal);
} else {
process.exit(result.status);
}
}
}
Expand Down Expand Up @@ -929,6 +935,7 @@ const common = {
mustSucceed,
nodeProcessAborted,
PIPE,
parseTestFlags,
platformTimeout,
printSkipMessage,
pwdCommand,
Expand Down
2 changes: 2 additions & 0 deletions test/common/index.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ const {
getCallSite,
mustNotCall,
mustNotMutateObjectDeep,
parseTestFlags,
printSkipMessage,
skip,
nodeProcessAborted,
Expand Down Expand Up @@ -88,6 +89,7 @@ export {
getCallSite,
mustNotCall,
mustNotMutateObjectDeep,
parseTestFlags,
printSkipMessage,
skip,
nodeProcessAborted,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// Flags: --no-warnings
'use strict';
require('../common');
require('../../../common');
const test = require('node:test');

test('promise timeout signal', { signal: AbortSignal.timeout(1) }, async (t) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,15 @@ TAP version 13
# Subtest: not ok 2
not ok 6 - not ok 2
---
duration_ms: *
duration_ms: ZERO
failureType: 'cancelledByParent'
error: 'test did not finish before its parent and was cancelled'
code: 'ERR_TEST_FAILURE'
...
# Subtest: not ok 3
not ok 7 - not ok 3
---
duration_ms: *
duration_ms: ZERO
failureType: 'testAborted'
error: 'This operation was aborted'
code: 20
Expand All @@ -59,7 +59,7 @@ TAP version 13
# Subtest: not ok 4
not ok 8 - not ok 4
---
duration_ms: *
duration_ms: ZERO
failureType: 'testAborted'
error: 'This operation was aborted'
code: 20
Expand All @@ -79,7 +79,7 @@ TAP version 13
# Subtest: not ok 5
not ok 9 - not ok 5
---
duration_ms: *
duration_ms: ZERO
failureType: 'testAborted'
error: 'This operation was aborted'
code: 20
Expand Down Expand Up @@ -161,15 +161,15 @@ not ok 2 - promise abort signal
# Subtest: not ok 2
not ok 6 - not ok 2
---
duration_ms: *
duration_ms: ZERO
failureType: 'cancelledByParent'
error: 'test did not finish before its parent and was cancelled'
code: 'ERR_TEST_FAILURE'
...
# Subtest: not ok 3
not ok 7 - not ok 3
---
duration_ms: *
duration_ms: ZERO
failureType: 'testAborted'
error: 'This operation was aborted'
code: 20
Expand All @@ -189,7 +189,7 @@ not ok 2 - promise abort signal
# Subtest: not ok 4
not ok 8 - not ok 4
---
duration_ms: *
duration_ms: ZERO
failureType: 'testAborted'
error: 'This operation was aborted'
code: 20
Expand All @@ -209,7 +209,7 @@ not ok 2 - promise abort signal
# Subtest: not ok 5
not ok 9 - not ok 5
---
duration_ms: *
duration_ms: ZERO
failureType: 'testAborted'
error: 'This operation was aborted'
code: 20
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// Flags: --no-warnings
'use strict';
require('../common');
require('../../../common');
const { describe, it } = require('node:test');

describe('describe timeout signal', { signal: AbortSignal.timeout(1) }, (t) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,31 +31,31 @@ TAP version 13
# Subtest: not ok 2
not ok 6 - not ok 2
---
duration_ms: *
duration_ms: ZERO
failureType: 'cancelledByParent'
error: 'test did not finish before its parent and was cancelled'
code: 'ERR_TEST_FAILURE'
...
# Subtest: not ok 3
not ok 7 - not ok 3
---
duration_ms: *
duration_ms: ZERO
failureType: 'cancelledByParent'
error: 'test did not finish before its parent and was cancelled'
code: 'ERR_TEST_FAILURE'
...
# Subtest: not ok 4
not ok 8 - not ok 4
---
duration_ms: *
duration_ms: ZERO
failureType: 'cancelledByParent'
error: 'test did not finish before its parent and was cancelled'
code: 'ERR_TEST_FAILURE'
...
# Subtest: not ok 5
not ok 9 - not ok 5
---
duration_ms: *
duration_ms: ZERO
failureType: 'cancelledByParent'
error: 'test did not finish before its parent and was cancelled'
code: 'ERR_TEST_FAILURE'
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// Flags: --no-warnings
'use strict';
require('../common');
require('../../../common');
const assert = require('node:assert');
const { describe, it, test } = require('node:test');
const util = require('util');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -196,10 +196,10 @@ ok 21 - immediate resolve pass
*
*
*
new Promise (<anonymous>)
*
*
*
*
Array.map (<anonymous>)
...
# Subtest: mixing describe/it and test should work
ok 2 - mixing describe/it and test should work
Expand Down Expand Up @@ -469,10 +469,10 @@ not ok 53 - custom inspect symbol that throws fail
*
*
*
new Promise (<anonymous>)
*
*
*
*
Array.map (<anonymous>)
...
# Subtest: sync throw fails at second
not ok 2 - sync throw fails at second
Expand All @@ -491,7 +491,7 @@ not ok 53 - custom inspect symbol that throws fail
*
*
*
*
async Promise.all (index 0)
...
1..2
not ok 54 - subtest sync throw fails
Expand All @@ -506,7 +506,7 @@ not ok 54 - subtest sync throw fails
# Subtest: should not run
not ok 1 - should not run
---
duration_ms: *
duration_ms: ZERO
failureType: 'cancelledByParent'
error: 'test did not finish before its parent and was cancelled'
code: 'ERR_TEST_FAILURE'
Expand Down Expand Up @@ -535,7 +535,7 @@ not ok 55 - describe sync throw fails
# Subtest: should not run
not ok 1 - should not run
---
duration_ms: *
duration_ms: ZERO
failureType: 'cancelledByParent'
error: 'test did not finish before its parent and was cancelled'
code: 'ERR_TEST_FAILURE'
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// Flags: --no-warnings
'use strict';
require('../common');
require('../../../common');
const { describe, it } = require('node:test');

describe('nested - no tests', () => {
Expand Down
8 changes: 8 additions & 0 deletions test/fixtures/test-runner/output/dot_reporter.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// Flags: --no-warnings
'use strict';
require('../../../common');
const fixtures = require('../../../common/fixtures');
const spawn = require('node:child_process').spawn;

spawn(process.execPath,
['--no-warnings', '--test-reporter', 'dot', fixtures.path('test-runner/output/output.js')], { stdio: 'inherit' });
Loading