test_runner: emit test-only diagnostic warning by richiemccoll · Pull Request #46540 · nodejs/node

richiemccoll · 2023-02-07T10:00:49Z

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 · 2023-02-07T10:00:55Z

Review requested:

@nodejs/test_runner

MoLow

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

richiemccoll · 2023-02-10T09:39:51Z

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 flagconst{ test }=require('node:test');test('top level test',async(t)=>{t.runOnly(true);awaitt.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 · 2023-02-11T15:20:12Z

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 · 2023-02-13T10:44:33Z

@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 · 2023-02-13T10:45:59Z

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 · 2023-02-18T19:05:20Z

@richiemccoll this PR requires a rebase

nodejs-github-bot · 2023-02-20T13:30:07Z

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

nodejs-github-bot · 2023-02-20T14:51:44Z

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

nodejs-github-bot · 2023-02-21T06:10:56Z

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

nodejs-github-bot · 2023-02-21T08:39:20Z

Landed in c90ea93

PR-URL: nodejs#46540Fixes: 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>

PR-URL: #46540 Backport-PR-URL: #46839Fixes: #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>

PR-URL: #46540Fixes: #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>

`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.

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 marked this pull request as ready for review February 7, 2023 10:02

MoLow reviewed Feb 7, 2023
View reviewed changes

Comment threadtest/message/test_runner_only_warning.js Outdated

MoLow reviewed Feb 7, 2023
View reviewed changes

Comment threadlib/internal/test_runner/test.js Outdated

richiemccoll force-pushed the test-runner/emit-only-warning branch from 3ee8044 to e9bf7c1Compare February 7, 2023 15:10

cjihrig reviewed Feb 8, 2023
View reviewed changes

Comment threadlib/internal/test_runner/test.js Outdated
Comment threadlib/internal/test_runner/test.js Outdated
Comment threadtest/message/test_runner_output.out Outdated

richiemccoll force-pushed the test-runner/emit-only-warning branch 2 times, most recently from 8292f1b to 514fd0bCompare February 9, 2023 12:11

cjihrig reviewed Feb 9, 2023
View reviewed changes

Comment threadlib/internal/test_runner/test.js Outdated

richiemccoll force-pushed the test-runner/emit-only-warning branch from 514fd0b to 5669292Compare February 9, 2023 12:28

cjihrig approved these changes Feb 9, 2023
View reviewed changes

MoLow approved these changes Feb 9, 2023
View reviewed changes

benjamingr approved these changes Feb 9, 2023
View reviewed changes

jasnell approved these changes Feb 11, 2023
View reviewed changes

test_runner: emit test-only diagnostic warning
7696218

richiemccoll force-pushed the test-runner/emit-only-warning branch from 5669292 to 7696218Compare February 20, 2023 09:29

MoLow approved these changes Feb 20, 2023
View reviewed changes

MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 20, 2023

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 20, 2023

github-actionsBot mentioned this pull request Feb 21, 2023
CI Reliability 2023-02-21 nodejs/reliability#502
Open
26 tasks

MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 21, 2023

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 merged commit c90ea93 into nodejs:mainFeb 21, 2023

github-actionsBot mentioned this pull request Feb 22, 2023
CI Reliability 2023-02-22 nodejs/reliability#503
Open
34 tasks

MoLow added the backport-open-v18.x label Feb 25, 2023

juanarbol mentioned this pull request Mar 3, 2023
V18.15.0 proposal #46920
Merged

targos mentioned this pull request Mar 14, 2023
v19.8.0 release proposal #47087
Merged

andrew-ge-wu mentioned this pull request Jun 20, 2026
fix(files): handle client abort in /api/v1/files/[name] CircuitWall/jarela#323
Merged

Uh oh!

Uh oh!

Conversation

richiemccoll commented Feb 7, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Feb 7, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MoLow left a comment

Choose a reason for hiding this comment

Uh oh!

richiemccoll commented Feb 10, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjihrig commented Feb 11, 2023

Uh oh!

richiemccoll commented Feb 13, 2023

Uh oh!

MoLow commented Feb 13, 2023

Uh oh!

MoLow commented Feb 18, 2023

Uh oh!

nodejs-github-bot commented Feb 20, 2023

Uh oh!

nodejs-github-bot commented Feb 20, 2023

Uh oh!

nodejs-github-bot commented Feb 21, 2023

Uh oh!

nodejs-github-bot commented Feb 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

richiemccoll commented Feb 7, 2023•
edited
Loading

richiemccoll commented Feb 10, 2023•
edited
Loading