GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test_runner: remove stdout and stderr from error by cjihrig · Pull Request #45592 · nodejs/node

Conversation

@cjihrig

@cjihrig cjihrig commented Nov 23, 2022

Copy link
Copy Markdown
Contributor

The CLI test runner parses the TAP output from child processes now and displays it. This commit removes a previous workaround for displaying child process stdout and stderr when tests failures occurred.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@cjihrig
test_runner: remove stdout and stderr from error
e18aa36 
The CLI test runner parses the TAP output from child processes
now and displays it. This commit removes a previous workaround
for displaying child process stdout and stderr when tests
failures occurred.
@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 Nov 23, 2022
targos
targos approved these changes Nov 23, 2022
View reviewed changes
@MoLow

MoLow commented Nov 23, 2022

Copy link
Copy Markdown
Member

Now, arbitrary output on stderr is swallowed, I think we should at least pipe it to the parent process stderr.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig

cjihrig commented Nov 23, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

Now, arbitrary output on stderr is swallowed, I think we should at least pipe it to the parent process stderr.

From my testing stderr shows up as TAP comments because the child process stderr is (incorrectly) piped into the TAP parser.

I also have a follow up PR that stops stderr from being piped to the parser and instead just interprets the lines of stderr as unknown TAP tokens (diff below) - I just have to write a test for it.

diff --git a/lib/internal/test_runner/runner.js b/lib/internal/test_runner/runner.js
index 3f7134c6cd..eb73a46e55 100644
--- a/lib/internal/test_runner/runner.js
+++ b/lib/internal/test_runner/runner.js
@@ -246,16 +246,25 @@ function runTestFile(path, root, inspectPort, filesWatcher) {
       rl.on('line', (line) => {
         if (isInspectorMessage(line)) {
           process.stderr.write(line + '\n');
+          return;
         }
+
+        // stderr cannot be treated as TAP, per the spec. However, we want to
+        // surface stderr lines as TAP diagnostics to improve the DX. Inject
+        // each line into the test output as an unknown token as if it came
+        // from the TAP parser.
+        const node = {
+          kind: TokenKind.UNKNOWN,
+          node: {
+            value: line,
+          },
+        };
+
+        subtest.addToReport(node);
       });
     }
 
     const parser = new TapParser();
-    child.stderr.pipe(parser).on('data', (ast) => {
-      if (ast.lexeme && isInspectorMessage(ast.lexeme)) {
-        process.stderr.write(ast.lexeme + '\n');
-      }
-    });
 
     child.stdout.pipe(parser).on('data', (ast) => {
       subtest.addToReport(ast);
diff --git a/lib/internal/util/inspector.js b/lib/internal/util/inspector.js
index 038479b173..c7f18ffdb6 100644
--- a/lib/internal/util/inspector.js
+++ b/lib/internal/util/inspector.js
@@ -16,7 +16,7 @@ const { validatePort } = require('internal/validators');
 const kMinPort = 1024;
 const kMaxPort = 65535;
 const kInspectArgRegex = /--inspect(?:-brk|-port)?|--debug-port/;
-const kInspectMsgRegex = /Debugger listening on ws:\/\/\[?(.+?)\]?:(\d+)\/|Debugger attached|Waiting for the debugger to disconnect\.\.\./;
+const kInspectMsgRegex = /Debugger listening on ws:\/\/\[?(.+?)\]?:(\d+)\/|For help, see: https:\/\/nodejs\.org\/en\/docs\/inspector|Debugger attached|Waiting for the debugger to disconnect\.\.\./;
 
 const _isUsingInspector = new SafeWeakMap();
 function isUsingInspector(execArgv = process.execArgv) {
MoLow reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MoLow MoLow added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Nov 23, 2022
MoLow
MoLow approved these changes Nov 23, 2022
View reviewed changes
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Nov 23, 2022
@nodejs-github-bot

nodejs-github-bot commented Nov 23, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/48120/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Nov 23, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/48127/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MoLow MoLow added the commit-queue PRs queued for automated landing through the Commit Queue. label Nov 23, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Nov 25, 2022
@nodejs-github-bot
nodejs-github-bot merged commit fec0fbc into nodejs:main Nov 25, 2022
@nodejs-github-bot

nodejs-github-bot commented Nov 25, 2022

Copy link
Copy Markdown
Collaborator

Landed in fec0fbc
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig
cjihrig deleted the runner branch November 25, 2022 02:54
@github-actions github-actions Bot mentioned this pull request Nov 28, 2022
CI Reliability 2022-11-28 nodejs/reliability#442
Open
20 tasks
@bricss bricss mentioned this pull request Nov 28, 2022
v19.2.0 proposal #45615
Merged
targos pushed a commit that referenced this pull request Dec 12, 2022
@cjihrig @targos
test_runner: remove stdout and stderr from error
c9cbd1d 
The CLI test runner parses the TAP output from child processes
now and displays it. This commit removes a previous workaround
for displaying child process stdout and stderr when tests
failures occurred.

PR-URL: #45592
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
@targos targos mentioned this pull request Dec 12, 2022
v19.3.0 release proposal #45831
Merged
@danielleadams danielleadams mentioned this pull request Dec 30, 2022
v18.13.0 release proposal #46025
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@cjihrig @danielleadams
test_runner: remove stdout and stderr from error
70c98bb 
The CLI test runner parses the TAP output from child processes
now and displays it. This commit removes a previous workaround
for displaying child process stdout and stderr when tests
failures occurred.

PR-URL: #45592
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
@cjihrig @danielleadams
test_runner: remove stdout and stderr from error
20f6fa3 
The CLI test runner parses the TAP output from child processes
now and displays it. This commit removes a previous workaround
for displaying child process stdout and stderr when tests
failures occurred.

PR-URL: #45592
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 5, 2023
@cjihrig @danielleadams
test_runner: remove stdout and stderr from error
2f07514 
The CLI test runner parses the TAP output from child processes
now and displays it. This commit removes a previous workaround
for displaying child process stdout and stderr when tests
failures occurred.

PR-URL: #45592
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
MoLow pushed a commit to MoLow/node-core-test that referenced this pull request Feb 2, 2023
@cjihrig @MoLow
fix: remove stdout and stderr from error
808862b 
The CLI test runner parses the TAP output from child processes
now and displays it. This commit removes a previous workaround
for displaying child process stdout and stderr when tests
failures occurred.

PR-URL: nodejs/node#45592
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
(cherry picked from commit fec0fbc333c58e6ebbd2322f5120830fda880eb0)
MoLow pushed a commit to MoLow/node-core-test that referenced this pull request Feb 2, 2023
@cjihrig @MoLow
fix: remove stdout and stderr from error
ddbce53 
The CLI test runner parses the TAP output from child processes
now and displays it. This commit removes a previous workaround
for displaying child process stdout and stderr when tests
failures occurred.

PR-URL: nodejs/node#45592
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
(cherry picked from commit fec0fbc333c58e6ebbd2322f5120830fda880eb0)
MoLow pushed a commit to MoLow/node-core-test that referenced this pull request Feb 2, 2023
@cjihrig @MoLow
fix: remove stdout and stderr from error
5ba2500 
The CLI test runner parses the TAP output from child processes
now and displays it. This commit removes a previous workaround
for displaying child process stdout and stderr when tests
failures occurred.

PR-URL: nodejs/node#45592
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
(cherry picked from commit fec0fbc333c58e6ebbd2322f5120830fda880eb0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MoLow MoLow MoLow approved these changes
+1 more reviewer
@targos targos targos approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. 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.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@cjihrig @MoLow @nodejs-github-bot @targos
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.