Skip to content

test_runner: emit test-only diagnostic warning#46540

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
richiemccoll:test-runner/emit-only-warning
Feb 21, 2023
Merged

test_runner: emit test-only diagnostic warning#46540
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
richiemccoll:test-runner/emit-only-warning

Conversation

@richiemccoll

@richiemccoll richiemccoll commented Feb 7, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes: #46448

This PR:

  • emits a test diagnostic when running tests with { only: true } or runOnlyTests(true) without the --test-only CLI flag.

@nodejs-github-bot

nodejs-github-bot commented Feb 7, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot 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 7, 2023
@richiemccoll richiemccoll marked this pull request as ready for review February 7, 2023 10:02
MoLow
MoLow reviewed Feb 7, 2023
View reviewed changes
Comment thread test/message/test_runner_only_warning.js Outdated
MoLow
MoLow reviewed Feb 7, 2023
View reviewed changes
Comment thread lib/internal/test_runner/test.js Outdated
@richiemccoll richiemccoll force-pushed the test-runner/emit-only-warning branch from 3ee8044 to e9bf7c1 Compare February 7, 2023 15:10
cjihrig
cjihrig reviewed Feb 8, 2023
View reviewed changes
Comment thread lib/internal/test_runner/test.js
Comment thread lib/internal/test_runner/test.js Outdated
Comment thread test/message/test_runner_output.out Outdated
@richiemccoll richiemccoll force-pushed the test-runner/emit-only-warning branch 2 times, most recently from 8292f1b to 514fd0b Compare February 9, 2023 12:11
cjihrig
cjihrig reviewed Feb 9, 2023
View reviewed changes
Comment thread lib/internal/test_runner/test.js Outdated
@richiemccoll richiemccoll force-pushed the test-runner/emit-only-warning branch from 514fd0b to 5669292 Compare February 9, 2023 12:28
MoLow
MoLow approved these changes Feb 9, 2023
View reviewed changes

@MoLow MoLow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, But I think we should emit a similar diagnostic when calling runOnly when no flag

@richiemccoll

richiemccoll commented Feb 10, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I think we should emit a similar diagnostic when calling runOnly when no flag

@MoLow Are you referring to this scenario with runOnly below or something else? I've tested this locally and the diagnostic is there.

// running without the --test-only flag
const { test } = require('node:test');

test('top level test', async (t) => {
  t.runOnly(true);
  await t.test('this subtest is run');
});
TAP version 13
# Subtest: top level test
    # Subtest: this subtest is run
    ok 1 - this subtest is run
      ---
      duration_ms: 4.77543
      ...
    # 'only' and 'runOnly' require the --test-only command-line option.
    1..1
ok 1 - top level test
  ---
  duration_ms: 6.194772
  ...
1..1
# tests 1
# pass 1

@cjihrig

cjihrig commented Feb 11, 2023

Copy link
Copy Markdown
Contributor

I think we should emit a similar diagnostic when calling runOnly when no flag

I think @MoLow is saying to add the diagnostic when runOnly() is called, even if no tests are started. If this change is made, please ensure we don't end up in a situation where the diagnostic is included multiple times for the same test.

@richiemccoll

richiemccoll commented Feb 13, 2023

Copy link
Copy Markdown
Contributor Author

@cjihrig @MoLow Is that something you'd like to see as part of this PR or can that be tackled in a follow up commit?

@MoLow

MoLow commented Feb 13, 2023

Copy link
Copy Markdown
Member

Is that something you'd like to see as part of this PR or can that be tackled in a follow up commit?

A follow-up PR is ok

@MoLow

MoLow commented Feb 18, 2023

Copy link
Copy Markdown
Member

@richiemccoll this PR requires a rebase

@richiemccoll richiemccoll force-pushed the test-runner/emit-only-warning branch from 5669292 to 7696218 Compare February 20, 2023 09:29
@MoLow MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 20, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 20, 2023
@nodejs-github-bot

nodejs-github-bot commented Feb 20, 2023

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Feb 20, 2023

Copy link
Copy Markdown
Collaborator

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

@github-actions github-actions Bot mentioned this pull request Feb 21, 2023
Open
26 tasks
@nodejs-github-bot

nodejs-github-bot commented Feb 21, 2023

Copy link
Copy Markdown
Collaborator

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

@MoLow MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 21, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 21, 2023
@nodejs-github-bot nodejs-github-bot merged commit c90ea93 into nodejs:main Feb 21, 2023
@nodejs-github-bot

nodejs-github-bot commented Feb 21, 2023

Copy link
Copy Markdown
Collaborator

Landed in c90ea93

@github-actions github-actions Bot mentioned this pull request Feb 22, 2023
Open
34 tasks
MoLow pushed a commit to MoLow/node that referenced this pull request Feb 25, 2023
@richiemccoll @MoLow
test_runner: emit test-only diagnostic warning
3983267 
PR-URL: nodejs#46540
Fixes: nodejs#46448
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MoLow pushed a commit to MoLow/node that referenced this pull request Feb 25, 2023
@richiemccoll @MoLow
test_runner: emit test-only diagnostic warning
0c8f40d 
PR-URL: nodejs#46540
Fixes: nodejs#46448
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MoLow pushed a commit to MoLow/node that referenced this pull request Feb 25, 2023
@richiemccoll @MoLow
test_runner: emit test-only diagnostic warning
b50cfb3 
PR-URL: nodejs#46540
Fixes: nodejs#46448
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
juanarbol pushed a commit that referenced this pull request Mar 3, 2023
@richiemccoll @juanarbol
test_runner: emit test-only diagnostic warning
94c870b 
PR-URL: #46540
Backport-PR-URL: #46839
Fixes: #46448
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@juanarbol juanarbol mentioned this pull request Mar 3, 2023
Merged
juanarbol pushed a commit that referenced this pull request Mar 5, 2023
@richiemccoll @juanarbol
test_runner: emit test-only diagnostic warning
cd3aaa8 
PR-URL: #46540
Backport-PR-URL: #46839
Fixes: #46448
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Mar 13, 2023
@richiemccoll @targos
test_runner: emit test-only diagnostic warning
62d0c0f 
PR-URL: #46540
Fixes: #46448
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Mar 14, 2023
@richiemccoll @targos
test_runner: emit test-only diagnostic warning
ffa86f7 
PR-URL: #46540
Fixes: #46448
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Mar 14, 2023
Merged
@andrew-ge-wu andrew-ge-wu mentioned this pull request Jun 20, 2026
Merged
andrew-ge-wu added a commit to CircuitWall/jarela that referenced this pull request Jun 20, 2026
@andrew-ge-wu
fix(files): handle client abort in /api/v1/files/[name]
b3e35f5 
`Readable.toWeb(nodeStream)` is racy in current Node: when the client
aborts mid-stream (iOS Safari range-probing an image then closing the
socket, tab close mid-download, HTTP/2 RST_STREAM), the web-stream
controller is closed first, but the adapter still tries to
`controller.enqueue` any chunk the fs.ReadStream emits afterwards and
throws `ERR_INVALID_STATE: Controller is already closed` as an
uncaughtException, crashing the server.

See nodejs/node#46540, #49936.

Hand-roll the `ReadableStream` so we can:
 - destroy the node stream on consumer `cancel()`,
 - swallow late enqueue attempts after close,
 - keep `error` events from escaping to the process.

Behaviour for the happy path (full read, range read, conditional GET)
is unchanged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@benjamingr benjamingr benjamingr approved these changes

@cjihrig cjihrig cjihrig approved these changes

@MoLow MoLow MoLow approved these changes

Assignees

No one assigned

Labels

needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

test_runner: warn if only is used without properly enabling only tests

6 participants

@richiemccoll @nodejs-github-bot @cjihrig @MoLow @jasnell @benjamingr