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 swallows uncaughtException #44612

Closed
tniessen opened this issue Sep 12, 2022 · 10 comments · Fixed by #45264
Closed

Test runner swallows uncaughtException #44612

tniessen opened this issue Sep 12, 2022 · 10 comments · Fixed by #45264
Labels
good first issue Issues that are suitable for first-time contributors. test_runner Issues and PRs related to the test runner subsystem.

Comments

@tniessen
Copy link
Member

Version

v18.9.0

Platform

Linux ubuntuserver 5.15.0-1019-azure #24-Ubuntu SMP Tue Aug 23 15:05:55 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

test_runner

What steps will reproduce the bug?

import test from 'node:test';

test('foo', () => {
  setTimeout(() => { throw new Error(); }, 100);
});

How often does it reproduce? Is there a required condition?

Always.

What is the expected behavior?

The exit code of the process should indicate failure, not success. This is what happens without the experimental node:test and it is what other test runners do, too.

What do you see instead?

node --test swallows the error altogether. Not even a warning.

$ node --test test.mjs && echo -e "\nTest runner reported no error!"
TAP version 13
# Subtest: /home/tniessen/dev/github-44611/test.mjs
ok 1 - /home/tniessen/dev/github-44611/test.mjs
  ---
  duration_ms: 157.248203
  ...
1..1
# tests 1
# pass 1
# fail 0
# cancelled 0
# skipped 0
# todo 0
# duration_ms 159.519197

Test runner reported no error!

Without --test, at least there is a warning, but the process exit code still indicates success.

$ node test.mjs && echo -e "\nTest runner reported no error!"
(node:21021) ExperimentalWarning: The test runner is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
TAP version 13
# Subtest: foo
ok 1 - foo
  ---
  duration_ms: 0.910498
  ...
1..1
# Warning: Test "foo" generated asynchronous activity after the test ended. This activity created the error "Error" and would have caused the test to fail, but instead triggered an uncaughtException event.
# tests 1
# pass 1
# fail 0
# cancelled 0
# skipped 0
# todo 0
# duration_ms 103.150927

Test runner reported no error!

Additional information

No response

@tniessen tniessen added the test_runner Issues and PRs related to the test runner subsystem. label Sep 12, 2022
@MoLow
Copy link
Member

MoLow commented Sep 12, 2022

@tniessen isn't this a duplicate of #44611?

@tniessen
Copy link
Member Author

@MoLow AFAICT, #44611 is only about the stack trace missing when the error is reported, but the test at least fails as expected. Here, the test runner reports success even though an uncaught exception occurred.

(It could, of course, be the same bug causing two different problems.)

@timmolendijk
Copy link

timmolendijk commented Sep 12, 2022

I think the rationale for this behavior is that the test finishes without an error, which makes the test runner conclude that it passes. After which any lingering async running code is just dismissed (and errors swallowed). This is to some extent alluded to in the docs.

I am not sure whether this is the ideal behavior, but I can at least see how it could be considered by design.

@cjihrig
Copy link
Contributor

cjihrig commented Sep 12, 2022

Without --test, at least there is a warning, but the process exit code still indicates success.

When I originally wrote that code, I went back and forth on whether or not to change the exit code when this happens. I figured I'd wait and see if anyone complained. Someone complained now, so let's just change the exit code when a warning occurs.

@MoLow
Copy link
Member

MoLow commented Sep 12, 2022

@timmolendijk are you interested in creating a PR fixing this?
if not I will

@timmolendijk
Copy link

timmolendijk commented Sep 12, 2022

@MoLow Interested yes. Available not yet sure.

@MoLow
Copy link
Member

MoLow commented Sep 12, 2022

according to @cjihrig 's comment, the fix will require setting process.exitCode withing the uncaughtException handler.
feel free to ping me if you need help

@MoLow MoLow added the good first issue Issues that are suitable for first-time contributors. label Oct 27, 2022
@fossamagna
Copy link
Contributor

@MoLow I interest in creating a PR fixing this. If @timmolendijk not work on it yet, Can I take this up?

@MoLow
Copy link
Member

MoLow commented Oct 28, 2022

please do :)

@timmolendijk
Copy link

@fossamagna Yeah please do, I hadn’t got around to it yet. Thanks!

fossamagna added a commit to fossamagna/node that referenced this issue Nov 1, 2022
fossamagna added a commit to fossamagna/node that referenced this issue Nov 7, 2022
nodejs-github-bot pushed a commit that referenced this issue Nov 7, 2022
Fixes: #44612
PR-URL: #45264
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
lucshi pushed a commit to lucshi/node that referenced this issue Nov 9, 2022
Fixes: nodejs#44612
PR-URL: nodejs#45264
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
RafaelGSS pushed a commit that referenced this issue Nov 10, 2022
Fixes: #44612
PR-URL: #45264
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MoLow pushed a commit to MoLow/node that referenced this issue Nov 23, 2022
Fixes: nodejs#44612
PR-URL: nodejs#45264
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MoLow pushed a commit to MoLow/node that referenced this issue Nov 23, 2022
Fixes: nodejs#44612
PR-URL: nodejs#45264
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MoLow pushed a commit to MoLow/node that referenced this issue Dec 9, 2022
Fixes: nodejs#44612
PR-URL: nodejs#45264
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
fossamagna added a commit to fossamagna/node that referenced this issue Dec 29, 2022
Fixes: nodejs#44612
PR-URL: nodejs#45264
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
fossamagna added a commit to fossamagna/node that referenced this issue Jan 23, 2023
Fixes: nodejs#44612
PR-URL: nodejs#45264
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MoLow pushed a commit to MoLow/node that referenced this issue Jan 26, 2023
Fixes: nodejs#44612
PR-URL: nodejs#45264
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MoLow pushed a commit to MoLow/node that referenced this issue Jan 26, 2023
Fixes: nodejs#44612
PR-URL: nodejs#45264
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MoLow pushed a commit to MoLow/node that referenced this issue Jan 26, 2023
Fixes: nodejs#44612
PR-URL: nodejs#45264
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MoLow pushed a commit to MoLow/node-core-test that referenced this issue Feb 2, 2023
Fixes: nodejs/node#44612
PR-URL: nodejs/node#45264
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
(cherry picked from commit 06603c44a5b0e92b1a3591ace467ce9770bf9658)
MoLow pushed a commit to MoLow/node-core-test that referenced this issue Feb 2, 2023
Fixes: nodejs/node#44612
PR-URL: nodejs/node#45264
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
(cherry picked from commit 06603c44a5b0e92b1a3591ace467ce9770bf9658)
MoLow pushed a commit to MoLow/node-core-test that referenced this issue Feb 2, 2023
Fixes: nodejs/node#44612
PR-URL: nodejs/node#45264
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
(cherry picked from commit 06603c44a5b0e92b1a3591ace467ce9770bf9658)
MoLow pushed a commit to MoLow/node-core-test that referenced this issue Feb 2, 2023
Fixes: nodejs/node#44612
PR-URL: nodejs/node#45264
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
(cherry picked from commit 06603c44a5b0e92b1a3591ace467ce9770bf9658)
MoLow pushed a commit to MoLow/node that referenced this issue Feb 25, 2023
Fixes: nodejs#44612
PR-URL: nodejs#45264
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
juanarbol pushed a commit that referenced this issue Mar 3, 2023
Fixes: #44612
PR-URL: #45264
Backport-PR-URL: #46839
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
juanarbol pushed a commit that referenced this issue Mar 5, 2023
Fixes: #44612
PR-URL: #45264
Backport-PR-URL: #46839
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issues that are suitable for first-time contributors. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants
@timmolendijk @fossamagna @cjihrig @tniessen @MoLow and others