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: add testNamePatterns param to run() #46522

Closed
wants to merge 9 commits into from
Closed

test: add testNamePatterns param to run() #46522

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2287515
test: implemented testNamePattens param in run
italojs Feb 25, 2023
0092479
test: new test for test_runner
italojs Feb 27, 2023
546538d
file: removed unecessary file
italojs Feb 27, 2023
ce32bc4
test: test coverage
italojs Mar 11, 2023
5adff53
Merge branch 'main' into italo/run-testNamePatterns-option
italojs Mar 11, 2023
3b8c1ff
test: using ObjectHasOwn
italojs Mar 11, 2023
c64c198
test: setting async to function
italojs Mar 11, 2023
9edd7dc
test: update comment
italojs Mar 11, 2023
e222a29
doc: add docs for testNamePatterns
italojs Mar 20, 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
8 changes: 8 additions & 0 deletions doc/api/errors.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -697,6 +697,14 @@ message thrown by `block` because that usage suggests that the user believes
`message` is the expected message rather than the message the `AssertionError`
will display if `block` does not throw.

<a id="ERR_ARG_NOT_ALLOWED"></a>

### `ERR_ARG_NOT_ALLOWED`

A function argument that can't be accept depending of some logic restriction.
italojs marked this conversation as resolved.
Show resolved Hide resolved
e.g. The function `run(...)` from module `node:test` can't accept `testNamePatterns`
param from programatic function and from CLI at same time. <a id="ERR_ARG_NOT_ITERABLE"></a>

<a id="ERR_ARG_NOT_ITERABLE"></a>

### `ERR_ARG_NOT_ITERABLE`
Expand Down
1 change: 1 addition & 0 deletions lib/internal/errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -960,6 +960,7 @@ module.exports = {
// Note: Node.js specific errors must begin with the prefix ERR_

E('ERR_AMBIGUOUS_ARGUMENT', 'The "%s" argument is ambiguous. %s', TypeError);
E('ERR_ARG_NOT_ALLOWED', '%s is already being assigned', Error);
Copy link
Contributor

Choose a reason for hiding this comment

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

I can see people being confused by this error message (I was when first reading it, and I have the context of this PR).

Copy link
Member

Choose a reason for hiding this comment

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

I'd use ERR_INVALID_ARG_TYPE

E('ERR_ARG_NOT_ITERABLE', '%s must be iterable', TypeError);
E('ERR_ASSERTION', '%s', Error);
E('ERR_ASYNC_CALLBACK', '%s must be a function', TypeError);
Expand Down
15 changes: 8 additions & 7 deletions lib/internal/test_runner/runner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,8 +254,9 @@ class FileTest extends Test {
const runningProcesses = new SafeMap();
const runningSubtests = new SafeMap();

function runTestFile(path, root, inspectPort, filesWatcher) {
const subtest = root.createSubtest(FileTest, path, async (t) => {
function runTestFile(options) {
italojs marked this conversation as resolved.
Show resolved Hide resolved
const { path, root, inspectPort, filesWatcher, testNamePatterns } = options;
const subtest = root.createSubtest(FileTest, path, { testNamePatterns }, async (t) => {
const args = getRunArgs({ path, inspectPort });
const stdio = ['pipe', 'pipe', 'pipe'];
const env = { ...process.env };
Expand Down Expand Up @@ -328,7 +329,7 @@ function runTestFile(path, root, inspectPort, filesWatcher) {
return subtest.start();
}

function watchFiles(testFiles, root, inspectPort) {
function watchFiles(testFiles, root, inspectPort, testNamePatterns) {
const filesWatcher = new FilesWatcher({ throttle: 500, mode: 'filter' });
filesWatcher.on('changed', ({ owners }) => {
filesWatcher.unfilterFilesOwnedBy(owners);
Expand All @@ -342,7 +343,7 @@ function watchFiles(testFiles, root, inspectPort) {
await once(runningProcess, 'exit');
}
await runningSubtests.get(file);
runningSubtests.set(file, runTestFile(file, root, inspectPort, filesWatcher));
runningSubtests.set(file, runTestFile({ file, root, inspectPort, filesWatcher, testNamePatterns }));
}, undefined, (error) => {
triggerUncaughtException(error, true /* fromPromise */);
}));
Expand All @@ -354,7 +355,7 @@ function run(options) {
if (options === null || typeof options !== 'object') {
options = kEmptyObject;
}
const { concurrency, timeout, signal, files, inspectPort, watch, setup } = options;
const { concurrency, timeout, signal, files, inspectPort, watch, setup, testNamePatterns } = options;

if (files != null) {
validateArray(files, 'options.files');
Expand All @@ -372,11 +373,11 @@ function run(options) {
let postRun = () => root.postRun();
let filesWatcher;
if (watch) {
filesWatcher = watchFiles(testFiles, root, inspectPort);
filesWatcher = watchFiles(testFiles, root, inspectPort, testNamePatterns);
postRun = undefined;
}
const runFiles = () => SafePromiseAllSettledReturnVoid(testFiles, (path) => {
const subtest = runTestFile(path, root, inspectPort, filesWatcher);
const subtest = runTestFile({ path, root, inspectPort, filesWatcher, testNamePatterns });
runningSubtests.set(path, subtest);
return subtest;
});
Expand Down
14 changes: 11 additions & 3 deletions lib/internal/test_runner/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ const { AbortController } = require('internal/abort_controller');
const {
codes: {
ERR_INVALID_ARG_TYPE,
ERR_ARG_NOT_ALLOWED,
ERR_TEST_FAILURE,
},
AbortError,
Expand Down Expand Up @@ -163,9 +164,16 @@ class Test extends AsyncResource {
constructor(options) {
super('Test');

let { fn, name, parent, skip } = options;
let { fn, name, parent, skip, testNamePatterns: _testNamePatterns } = options;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let { fn, name, parent, skip, testNamePatterns: _testNamePatterns } = options;
let { fn, name, parent, skip, testNamePatterns: testNamePatternsArg } = options;

Copy link
Contributor

Choose a reason for hiding this comment

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

We need input validation on this new option, as well as tests for the validation.

const { concurrency, only, timeout, todo, signal } = options;

if (_testNamePatterns && testNamePatterns) {
italojs marked this conversation as resolved.
Show resolved Hide resolved
throw new ERR_ARG_NOT_ALLOWED('testNamePatterns');
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need a test added for the case where the user tries to specify both.

}
if (!_testNamePatterns) {
_testNamePatterns = testNamePatterns;
}

if (typeof fn !== 'function') {
fn = noop;
}
Expand Down Expand Up @@ -224,10 +232,10 @@ class Test extends AsyncResource {
this.timeout = timeout;
}

if (testNamePatterns !== null) {
if (_testNamePatterns !== null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would move all of the logic from above down to here so that test name patterns are only checked in one place (for performance and to keep the code cleaner). I would do something like this:

if (testNamePatterns !== null || testNamePatternArgs) {
  if (testNamePatterns && testNamePatternArgs) {
    // throw here
  }

  const patterns = testNamePatterns ?? testNamePatternArgs;
}

// eslint-disable-next-line no-use-before-define
const match = this instanceof TestHook || ArrayPrototypeSome(
testNamePatterns,
_testNamePatterns,
(re) => RegExpPrototypeExec(re, name) !== null,
);

Expand Down
2 changes: 2 additions & 0 deletions test/fixtures/test-runner/test/random.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,5 @@
const test = require('node:test');

test('this should pass');

test('this should pass too');
italojs marked this conversation as resolved.
Show resolved Hide resolved
12 changes: 12 additions & 0 deletions test/parallel/test-runner-cli.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,18 @@ const testFixtures = fixtures.path('test-runner');
assert.match(child.stderr.toString(), /^Could not find/);
}


{
// Returned only test taht maches with the filter
italojs marked this conversation as resolved.
Show resolved Hide resolved
const args = ['--test', '--test-name-pattern="too"', join(testFixtures, 'test/random.cjs')];
Copy link
Contributor

Choose a reason for hiding this comment

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

We already have tests for the CLI flag. I'm not sure why this is being added.

const child = spawnSync('./node', args);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const child = spawnSync('./node', args);
const child = spawnSync(process.execPath, args);


assert.strictEqual(child.stderr.toString(), '');
const stdout = child.stdout.toString();
assert.match(stdout, /ok 1 - this should pass # SKIP test name does not match pattern/);
assert.match(stdout, /ok 2 - this should pass too/);
}

{
// Default behavior. node_modules is ignored. Files that don't match the
// pattern are ignored except in test/ directories.
Expand Down
10 changes: 9 additions & 1 deletion test/parallel/test-runner-run.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,14 @@ describe('require(\'node:test\').run', { concurrency: true }, () => {
it('should succeed with a file', async () => {
const stream = run({ files: [join(testFixtures, 'test/random.cjs')] });
stream.on('test:fail', common.mustNotCall());
stream.on('test:pass', common.mustCall(2));
// eslint-disable-next-line no-unused-vars
for await (const _ of stream);
});

it('should succeed with a file and filter', async () => {
const stream = run({ files: [join(testFixtures, 'test/random.cjs')], testNamePatterns: [/too/] });
stream.on('test:fail', common.mustNotCall());
stream.on('test:pass', common.mustCall(1));
// eslint-disable-next-line no-unused-vars
for await (const _ of stream);
Expand All @@ -35,7 +43,7 @@ describe('require(\'node:test\').run', { concurrency: true }, () => {
it('should run same file twice', async () => {
const stream = run({ files: [join(testFixtures, 'test/random.cjs'), join(testFixtures, 'test/random.cjs')] });
stream.on('test:fail', common.mustNotCall());
stream.on('test:pass', common.mustCall(2));
stream.on('test:pass', common.mustCall(4));
// eslint-disable-next-line no-unused-vars
for await (const _ of stream);
});
Expand Down