test_runner: fix wrongfully printed diagnostic warning while `only: false` #54116

pmarchini · 2024-07-30T08:22:52Z

This PR should address #54052 🐍

The issue (a diagnostic warning printed when only is set to false) is caused by runOnlySubtests being set as the negation of this.only on the subtest.
We have added the repro provided by @UziTech to the snapshot tests.

A question arose while checking the logic:

Given:

test('nested tests with run only', {only: true}, common.mustCall(async (t) => {
  ...
  // The test context can be updated to run subtests with the 'only' option.
  await t.runOnly(true);
  await t.test('this subtest is now skipped - 2', common.mustNotCall());
  await t.test('this subtest is run - 3', { only: true }, common.mustCall(async (t) => {
    await t.test('this subtest is run - 4', common.mustCall());
    await t.test('this subtest is run - 5', { only: true }, common.mustCall());
  }));

  // Switch the context back to execute all tests.
  await t.runOnly(false);
  await t.test('this subtest is now run - 6');
  ...
}))

Is it correct that this subtest is run - 4 is being run even though runOnly is set in the parent test?
Should runOnly be propagated to subtests as well, or is it intended only for "sibling" tests?

Additionally, we recognized the runOnlySubtests propagation change, but we may have missed other changes that might result in breaking functionality. All the current tests seem to be passing, so no relevant changes have been found on the tested paths. Do you notice anything else that we might have broken here?

Co-author: rstagi r.stagi96@gmail.com

Co-author: rstagi <r.stagi96@gmail.com>

nodejs-github-bot · 2024-07-30T08:22:57Z

Review requested:

@nodejs/test_runner

mcollina

lgtm

nodejs-github-bot · 2024-07-30T12:58:49Z

CI: https://ci.nodejs.org/job/node-test-pull-request/60752/

UziTech · 2024-07-30T15:05:41Z

lgtm. Thanks for the quick fix 💯

cjihrig · 2024-07-31T00:23:44Z

test/fixtures/test-runner/output/output.js

@@ -403,3 +404,41 @@ test('assertion errors display actual and expected properly', async () => {
    throw err;
  }
 });
+
+describe('should NOT print --test-only diagnostic warning - describe-only-false', {only: false}, () => {


Can we put these changes in a new test. This fixture is run with a variety of reporters and makes the diff get really noisy. Some of these are already getting quite large and more difficult to debug as new changes are introduced.

Hey @cjihrig, thanks for your super-fast feedback 🚀
I moved the tests to a new file and added it to test-runner-output.mjs.

Hope that's fine!

mcollina

lgtm

nodejs-github-bot · 2024-07-31T14:36:33Z

CI: https://ci.nodejs.org/job/node-test-pull-request/60772/

pmarchini · 2024-08-05T15:20:55Z

I have taken a look at the failing tests and I don't think they are related to the content of this PR.
I believe some of them are flaky.

Is there anything I can do?

nodejs-github-bot · 2024-08-05T15:47:39Z

CI: https://ci.nodejs.org/job/node-test-pull-request/60887/

nodejs-github-bot · 2024-08-06T07:32:09Z

CI: https://ci.nodejs.org/job/node-test-pull-request/60904/

nodejs-github-bot · 2024-08-07T20:34:34Z

Commit Queue failed

- Loading data for nodejs/node/pull/54116 ✔ Done loading data for nodejs/node/pull/54116 ----------------------------------- PR info ------------------------------------ Title test_runner: fix wrongfully printed diagnostic warning while `only: false` (#54116) Author Pietro Marchini <pietro.marchini94@gmail.com> (@pmarchini) Branch pmarchini:issue/54052 -> nodejs:main Labels needs-ci, test_runner Commits 2 - test_runner: fix erroneous diagnostic warning when only: false - test_runner: move --test-only diagnostic warning tests to separate file Committers 1 - Pietro Marchini <pietro.marchini94@gmail.com> PR-URL: https://github.com/nodejs/node/pull/54116 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/54116 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> -------------------------------------------------------------------------------- ℹ This PR was created on Tue, 30 Jul 2024 08:22:52 GMT ✔ Approvals: 3 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/54116#pullrequestreview-2210346753 ✔ - Colin Ihrig (@cjihrig): https://github.com/nodejs/node/pull/54116#pullrequestreview-2210195050 ✔ - Moshe Atlow (@MoLow) (TSC): https://github.com/nodejs/node/pull/54116#pullrequestreview-2214826060 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2024-08-06T07:32:09Z: https://ci.nodejs.org/job/node-test-pull-request/60904/ - Querying data for job/node-test-pull-request/60904/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 54116 From https://github.com/nodejs/node * branch refs/pull/54116/merge -> FETCH_HEAD ✔ Fetched commits as a3ff3e8cd47a..bbe850404425 -------------------------------------------------------------------------------- Auto-merging lib/internal/test_runner/test.js [main 6061a8fca4] test_runner: fix erroneous diagnostic warning when only: false Author: Pietro Marchini <pietro.marchini94@gmail.com> Date: Tue Jul 30 10:21:34 2024 +0200 11 files changed, 744 insertions(+), 259 deletions(-) Auto-merging test/parallel/test-runner-output.mjs [main b826a7de97] test_runner: move --test-only diagnostic warning tests to separate file Author: Pietro Marchini <pietro.marchini94@gmail.com> Date: Wed Jul 31 08:50:07 2024 +0200 11 files changed, 414 insertions(+), 679 deletions(-) create mode 100644 test/fixtures/test-runner/output/test-diagnostic-warning-without-test-only-flag.js create mode 100644 test/fixtures/test-runner/output/test-diagnostic-warning-without-test-only-flag.snapshot ✔ Patches applied There are 2 commits in the PR. Attempting autorebase. Rebasing (2/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
test_runner: fix erroneous diagnostic warning when only: false

Co-author: rstagi <r.stagi96@gmail.com>
PR-URL: #54116
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

[detached HEAD 356d983cae] test_runner: fix erroneous diagnostic warning when only: false
Author: Pietro Marchini <pietro.marchini94@gmail.com>
Date: Tue Jul 30 10:21:34 2024 +0200
11 files changed, 744 insertions(+), 259 deletions(-)
Rebasing (3/4)
Rebasing (4/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
test_runner: move --test-only diagnostic warning tests to separate file

PR-URL: #54116
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

[detached HEAD f72c181a85] test_runner: move --test-only diagnostic warning tests to separate file
Author: Pietro Marchini <pietro.marchini94@gmail.com>
Date: Wed Jul 31 08:50:07 2024 +0200
11 files changed, 414 insertions(+), 679 deletions(-)
create mode 100644 test/fixtures/test-runner/output/test-diagnostic-warning-without-test-only-flag.js
create mode 100644 test/fixtures/test-runner/output/test-diagnostic-warning-without-test-only-flag.snapshot

Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/10291338532

nodejs-github-bot · 2024-08-08T05:50:34Z

Landed in 88bac52

Co-author: rstagi <r.stagi96@gmail.com> PR-URL: #54116 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

test_runner: fix erroneous diagnostic warning when only: false

52d8766

Co-author: rstagi <r.stagi96@gmail.com>

nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Jul 30, 2024

mcollina approved these changes Jul 30, 2024

View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 30, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 30, 2024

This comment was marked as outdated.

Sign in to view

cjihrig reviewed Jul 31, 2024

View reviewed changes

pmarchini force-pushed the issue/54052 branch from 5913862 to a7fe166 Compare July 31, 2024 06:49

test_runner: move --test-only diagnostic warning tests to separate file

bbe8504

pmarchini force-pushed the issue/54052 branch from a7fe166 to bbe8504 Compare July 31, 2024 06:50

cjihrig approved these changes Jul 31, 2024

View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 31, 2024

mcollina approved these changes Jul 31, 2024

View reviewed changes

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 31, 2024

MoLow approved these changes Aug 2, 2024

View reviewed changes

marco-ippolito added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 7, 2024

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Aug 7, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 8, 2024

nodejs-github-bot merged commit 88bac52 into nodejs:main Aug 8, 2024
65 checks passed

cjihrig mentioned this pull request Aug 13, 2024

comment about --test-only if only is set to false #54052

Closed

RafaelGSS mentioned this pull request Aug 19, 2024

v22.7.0 proposal #54452

Merged

targos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Sep 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test_runner: fix wrongfully printed diagnostic warning while `only: false` #54116

test_runner: fix wrongfully printed diagnostic warning while `only: false` #54116

pmarchini commented Jul 30, 2024

nodejs-github-bot commented Jul 30, 2024

mcollina left a comment

This comment was marked as outdated.

nodejs-github-bot commented Jul 30, 2024

UziTech commented Jul 30, 2024

cjihrig Jul 31, 2024

pmarchini Jul 31, 2024

mcollina left a comment

nodejs-github-bot commented Jul 31, 2024

pmarchini commented Aug 5, 2024

nodejs-github-bot commented Aug 5, 2024

nodejs-github-bot commented Aug 6, 2024

nodejs-github-bot commented Aug 7, 2024

Co-author: rstagi <r.stagi96@gmail.com>
PR-URL: #54116
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

PR-URL: #54116
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

Successfully rebased and updated refs/heads/main.

nodejs-github-bot commented Aug 8, 2024

test_runner: fix wrongfully printed diagnostic warning while only: false #54116

test_runner: fix wrongfully printed diagnostic warning while only: false #54116

Conversation

pmarchini commented Jul 30, 2024

nodejs-github-bot commented Jul 30, 2024

mcollina left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

nodejs-github-bot commented Jul 30, 2024

UziTech commented Jul 30, 2024

cjihrig Jul 31, 2024

Choose a reason for hiding this comment

pmarchini Jul 31, 2024

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Jul 31, 2024

pmarchini commented Aug 5, 2024

nodejs-github-bot commented Aug 5, 2024

nodejs-github-bot commented Aug 6, 2024

nodejs-github-bot commented Aug 7, 2024

Co-author: rstagi <r.stagi96@gmail.com> PR-URL: #54116 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

PR-URL: #54116 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

Successfully rebased and updated refs/heads/main.

nodejs-github-bot commented Aug 8, 2024

test_runner: fix wrongfully printed diagnostic warning while `only: false` #54116

test_runner: fix wrongfully printed diagnostic warning while `only: false` #54116

Co-author: rstagi <r.stagi96@gmail.com>
PR-URL: #54116
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

PR-URL: #54116
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>