test: add testNamePatterns param to run() #46522

italojs · 2023-02-06T13:25:16Z

fix: #46045
WIP: implementing unit tests
Comments about the implementation is welcome

nodejs-github-bot · 2023-02-06T13:25:21Z

Review requested:

@nodejs/test_runner

benjamingr

Start looks fine, all the lint errors inside GH make this a bit hard to review but +1 on the initiative and working on the feature.

Note you missed passing stuff down (in line 240 when doing new Test, see the test failures)

lib/internal/test_runner/runner.js

lib/internal/test_runner/test.js

lib/internal/test_runner/runner.js

lib/internal/test_runner/test.js

test/fixtures/test-runner/test/random.cjs

italojs · 2023-02-13T20:33:40Z

[support request]
I was working on failing units tests but I got a little bit confused with why it's failing

Fail test files:

node/test/message/test_runner_test_name_pattern.js
node/test/message/test_runner_test_name_pattern_with_only.js

Fail test message:
function should not have been called at [...]test_runner_test_name_pattern.js:<lineNumber>

In all these failing tests we have a test:
test('some test name', common.mustNotCall());

I take a look into common.mustNotCall() and it returns a function that always ends with assert.fail(<msg>)

so.. if I understood correctly, I could say this test will never pass, am I correct?

cjihrig · 2023-02-13T20:39:43Z

I could say this test will never pass, am I correct?

common.mustNotCall() is used in the Node test suite to assert that a function is not called. If you're getting failures due to common.mustNotCall(), it means that the changes in this PR have broken those assertions and it would need to be fixed before we can consider merging this PR.

italojs · 2023-02-13T20:51:39Z

I could say this test will never pass, am I correct?

common.mustNotCall() is used in the Node test suite to assert that a function is not called. If you're getting failures due to common.mustNotCall(), it means that the changes in this PR have broken those assertions and it would need to be fixed before we can consider merging this PR.

I know, my changes broke it of course, but sorry, maybe I'm not been clear, my question is more related to understanding why it can't be called to help me to debug it.
I google about it but I just found some PRs and issues that don't explain why this function cant be called when others must be called (e.g. test('some msg', common.mustCall());

I'm debugging to fix it, but understanding a little bit more about this test could help me to fix I faster, you know
a explanation, a reference of someone that understood about it, some article, anything is welcome

but i think i already found what's wrong... i will work on this soon

cjihrig · 2023-02-13T20:57:38Z

At least one issue is likely this line:

testNamePatterns.concat(testNamePatternsFromCLI);

concat() does not change the existing array, so you're creating a new array and throwing it away.

lib/internal/test_runner/test.js

italojs · 2023-02-20T22:52:14Z

update: I'm taking few days off for canival, i will finalize tihs pr this week

doc/api/errors.md

test/parallel/test-runner-cli.js

lib/internal/test_runner/test.js

lib/internal/test_runner/runner.js

italojs · 2023-03-11T23:28:14Z

@benjamingr @cjihrig @recrack @ljharb
I think it's done, all checks is passing, it was an interesting "good first issue" I learned a lot about nodejs structure and coverage, and I saw some new contributions opportunity while I was doing it and learned more about nodejs
Thanks for your patience with who was reviewing it :)

cjihrig · 2023-03-18T15:39:41Z

doc/api/errors.md

+A function argument that can't be accepted depending of some logic restriction.
+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>


Suggested change

A function argument that can't be accepted depending of some logic restriction.

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 function argument that can't be accepted depending of some logic restriction.

cjihrig · 2023-03-18T15:40:35Z

lib/internal/errors.js

@@ -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);


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

I'd use ERR_INVALID_ARG_TYPE

lib/internal/test_runner/runner.js

cjihrig · 2023-03-18T15:44:14Z

lib/internal/test_runner/test.js

@@ -42,6 +43,7 @@ const {
  kEmptyObject,
  once: runOnce,
 } = require('internal/util');
+const { ObjectHasOwn } = primordials;


Please add this where primordials is already destructured above (and keep the list sorted)

cjihrig · 2023-03-18T15:45:41Z

lib/internal/test_runner/test.js

@@ -163,9 +165,16 @@ class Test extends AsyncResource {
  constructor(options) {
    super('Test');

-    let { fn, name, parent, skip } = options;
+    let { fn, name, parent, skip, testNamePatterns: _testNamePatterns } = options;


Suggested change

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

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

cjihrig · 2023-03-18T15:54:39Z

lib/internal/test_runner/test.js

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

+    if (ObjectHasOwn(options, '_testNamePatterns') && testNamePatterns) {
+      throw new ERR_ARG_NOT_ALLOWED('testNamePatterns');


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

cjihrig · 2023-03-18T15:56:47Z

lib/internal/test_runner/test.js

@@ -163,9 +165,16 @@ class Test extends AsyncResource {
  constructor(options) {
    super('Test');

-    let { fn, name, parent, skip } = options;
+    let { fn, name, parent, skip, testNamePatterns: _testNamePatterns } = options;


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

cjihrig · 2023-03-18T15:57:26Z

test/parallel/test-runner-cli.js

+{
+  // Return only test that maches with the filter
+  const args = ['--test', '--test-name-pattern="too"', join(testFixtures, 'test/random.cjs')];
+  const child = spawnSync('./node', args);


Suggested change

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

const child = spawnSync(process.execPath, args);

cjihrig · 2023-03-18T15:58:36Z

test/parallel/test-runner-cli.js

+
+{
+  // Return only test that maches with the filter
+  const args = ['--test', '--test-name-pattern="too"', join(testFixtures, 'test/random.cjs')];


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

cjihrig · 2023-03-18T16:00:05Z

test/parallel/test-runner-run.mjs

@@ -8,6 +8,17 @@ const testFixtures = fixtures.path('test-runner');

 describe('require(\'node:test\').run', { concurrency: true }, () => {

+  it('should not throw error', async () => {


What is this test trying to accomplish? It seems like it could be dropped.

italojs · 2023-04-09T19:00:24Z

closing this PR, i don't want to work on this anymore

MoLow · 2023-04-09T19:04:05Z

@italojs thanks for the effort!

nodejs-github-bot added dont-land-on-v14.x needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Feb 6, 2023

italojs marked this pull request as ready for review February 9, 2023 02:39

benjamingr reviewed Feb 9, 2023

View reviewed changes

ljharb reviewed Feb 11, 2023

View reviewed changes

lib/internal/test_runner/runner.js Outdated Show resolved Hide resolved

lib/internal/test_runner/runner.js Outdated Show resolved Hide resolved

cjihrig reviewed Feb 11, 2023

View reviewed changes

cjihrig reviewed Feb 13, 2023

View reviewed changes

lib/internal/test_runner/test.js Outdated Show resolved Hide resolved

cjihrig reviewed Feb 13, 2023

View reviewed changes

lib/internal/test_runner/test.js Outdated Show resolved Hide resolved

italojs force-pushed the italo/run-testNamePatterns-option branch 2 times, most recently from 56a71bc to 69681f7 Compare February 24, 2023 23:17

test: implemented testNamePattens param in run

2287515

italojs force-pushed the italo/run-testNamePatterns-option branch from 83fd79a to 2287515 Compare February 25, 2023 14:32

italojs added 2 commits February 27, 2023 18:21

test: new test for test_runner

0092479

file: removed unecessary file

546538d

benjamingr reviewed Feb 28, 2023

View reviewed changes

doc/api/errors.md Outdated Show resolved Hide resolved

recrack reviewed Mar 9, 2023

View reviewed changes

test/parallel/test-runner-cli.js Outdated Show resolved Hide resolved

ljharb reviewed Mar 9, 2023

View reviewed changes

lib/internal/test_runner/test.js Outdated Show resolved Hide resolved

italojs added 3 commits March 11, 2023 01:10

test: test coverage

ce32bc4

Merge branch 'main' into italo/run-testNamePatterns-option

5adff53

test: using ObjectHasOwn

3b8c1ff

ljharb reviewed Mar 11, 2023

View reviewed changes

lib/internal/test_runner/runner.js Outdated Show resolved Hide resolved

italojs added 2 commits March 11, 2023 14:10

test: setting async to function

c64c198

test: update comment

9edd7dc

ljharb approved these changes Mar 12, 2023

View reviewed changes

cjihrig reviewed Mar 18, 2023

View reviewed changes

doc: add docs for testNamePatterns

e222a29

italojs closed this Apr 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add testNamePatterns param to run() #46522

test: add testNamePatterns param to run() #46522

italojs commented Feb 6, 2023 •

edited

Loading

nodejs-github-bot commented Feb 6, 2023

benjamingr left a comment

italojs commented Feb 13, 2023

cjihrig commented Feb 13, 2023

italojs commented Feb 13, 2023 •

edited

Loading

cjihrig commented Feb 13, 2023

italojs commented Feb 20, 2023

italojs commented Mar 11, 2023 •

edited

Loading

cjihrig Mar 18, 2023

cjihrig Mar 18, 2023

MoLow Mar 18, 2023

cjihrig Mar 18, 2023

cjihrig Mar 18, 2023

cjihrig Mar 18, 2023

cjihrig Mar 18, 2023

cjihrig Mar 18, 2023

cjihrig Mar 18, 2023

cjihrig Mar 18, 2023

italojs commented Apr 9, 2023

MoLow commented Apr 9, 2023

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

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

		@@ -8,6 +8,17 @@ const testFixtures = fixtures.path('test-runner');

		describe('require(\'node:test\').run', { concurrency: true }, () => {

		it('should not throw error', async () => {

test: add testNamePatterns param to run() #46522

test: add testNamePatterns param to run() #46522

Conversation

italojs commented Feb 6, 2023 • edited Loading

nodejs-github-bot commented Feb 6, 2023

benjamingr left a comment

Choose a reason for hiding this comment

italojs commented Feb 13, 2023

cjihrig commented Feb 13, 2023

italojs commented Feb 13, 2023 • edited Loading

cjihrig commented Feb 13, 2023

italojs commented Feb 20, 2023

italojs commented Mar 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

italojs commented Apr 9, 2023

MoLow commented Apr 9, 2023

italojs commented Feb 6, 2023 •

edited

Loading

italojs commented Feb 13, 2023 •

edited

Loading

italojs commented Mar 11, 2023 •

edited

Loading