Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test_runner: remove stdout and stderr from error #45592

Merged
merged 1 commit into from
Nov 25, 2022

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Nov 23, 2022

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.

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
@MoLow
Copy link
Member

MoLow commented Nov 23, 2022

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

@cjihrig
Copy link
Contributor Author

cjihrig commented Nov 23, 2022

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 MoLow added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Nov 23, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 23, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@MoLow MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 23, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 25, 2022
@nodejs-github-bot nodejs-github-bot merged commit fec0fbc into nodejs:main Nov 25, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in fec0fbc

@cjihrig cjihrig deleted the runner branch November 25, 2022 02:54
@bricss bricss mentioned this pull request Nov 28, 2022
ErickWendel pushed a commit to ErickWendel/node that referenced this pull request Nov 30, 2022
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#45592
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
targos pushed a commit that referenced this pull request Dec 12, 2022
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 <targos@protonmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
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 <targos@protonmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
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 <targos@protonmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
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 <targos@protonmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
danielleadams pushed a commit that referenced this pull request Jan 4, 2023
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 <targos@protonmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
danielleadams pushed a commit that referenced this pull request Jan 5, 2023
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 <targos@protonmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
MoLow pushed a commit to MoLow/node-core-test that referenced this pull request Feb 2, 2023
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 <targos@protonmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
(cherry picked from commit fec0fbc333c58e6ebbd2322f5120830fda880eb0)
MoLow pushed a commit to MoLow/node-core-test that referenced this pull request Feb 2, 2023
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 <targos@protonmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
(cherry picked from commit fec0fbc333c58e6ebbd2322f5120830fda880eb0)
MoLow pushed a commit to MoLow/node-core-test that referenced this pull request Feb 2, 2023
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 <targos@protonmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
(cherry picked from commit fec0fbc333c58e6ebbd2322f5120830fda880eb0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants