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

async error inside before won't cause test error #52399

Closed
himself65 opened this issue Apr 7, 2024 · 6 comments · Fixed by #52401
Closed

async error inside before won't cause test error #52399

himself65 opened this issue Apr 7, 2024 · 6 comments · Fixed by #52401
Labels
test_runner Issues and PRs related to the test runner subsystem.

Comments

@himself65
Copy link
Member

Version

18, 20, 21

Platform

macos 14

Subsystem

test_runner

What steps will reproduce the bug?

import { before, test } from 'node:test'
import { createWriteStream } from 'node:fs'

before(() => {
  const fsStream = createWriteStream(new URL('./not-exist/file.txt', import.meta.url), {
    encoding: 'utf8'
  })

  fsStream.write('Hello World')
  fsStream.end()
})

test('ok', () => {

})

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

No response

What is the expected behavior? Why is that the expected behavior?

No response

What do you see instead?

node --test node-js-fs-stream.mjs
✔ ok (1.033833ms)
ℹ tests 1
ℹ suites 0
ℹ pass 1
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 43.30675

Additional information

No response

@himself65 himself65 added the test_runner Issues and PRs related to the test runner subsystem. label Apr 7, 2024
@himself65 himself65 changed the title error inside before won't cause test error async error inside before won't cause test error Apr 7, 2024
@himself65
Copy link
Member Author

import { before, test } from 'node:test'

before(() => {
  setTimeout(() => {
    throw new Error('before')
  }, 0)
})

test('ok', () => {

})

@benjamingr
Copy link
Member

Right, there is an asynchronous action and you don't wait for it to finish (and fail) so the test passes, I'm not sure how that's unexpected behavior?

Is the ask for better debugging? (e.g. log the error) or actually fail a test? The issue with the latter is the te test could have already completed by the time some unawaited async action failed in a hook. This is not specific to hooks (it's a property of js).

I think it would be reasonable to have the same "Warning: Test generated asynchronous activity after the test ended." warning we have for testts.

@himself65
Copy link
Member Author

Right, there is an asynchronous action and you don't wait for it to finish (and fail) so the test passes, I'm not sure how that's unexpected behavior?

Not I don't want to use await, actually there're many process.nextTick logic inside nodejs source code, and test runner won't throw correct error to the user

@benjamingr
Copy link
Member

@himself65 it arguably does, the whole execution can finish before the error is thrown in the async case, though a warning seems approprite

@himself65
Copy link
Member Author

I fell like this more like a bug, error is actually be caught but never show it to user

Subject: [PATCH] feat: add log
---
Index: lib/internal/test_runner/harness.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/lib/internal/test_runner/harness.js b/lib/internal/test_runner/harness.js
--- a/lib/internal/test_runner/harness.js	(revision 47c934e464756952719927529c2d28a92a58d701)
+++ b/lib/internal/test_runner/harness.js	(date 1712475302558)
@@ -73,7 +73,7 @@
       process.exitCode = kGenericUserError;
       return;
     }
-
+    console.log('error occurred in test_runner/harness.js: createProcessEventHandler()')
     test.fail(new ERR_TEST_FAILURE(err, eventName));
     test.abortController.abort();
   };
@@ -157,6 +157,7 @@
       // Run global before/after hooks in case there are no tests
       await root.run();
     }
+    console.log('exitHandler() in test_runner/harness.js')
     root.postRun(new ERR_TEST_FAILURE(
       'Promise resolution is still pending but the event loop has already resolved',
       kCancelledByParent));
import { before, test } from 'node:test'

process.on('uncaughtException', (err) => {
  console.log('uncaughtException')
})

before(() => {
  console.log('before called')
  setTimeout(() => {
    console.log('error occurred')
    throw new Error('error')
  }, 0)
})

test('ok', () => {
  console.log('ok called')
})
 node git:(main) ✗ ./out/Debug/node ./error.mjs
before called
ok called
✔ ok (1.391708ms)
error occurred
uncaughtException
error occurred in test_runner/harness.js: createProcessEventHandler()
exitHandler() in test_runner/harness.js
ℹ tests 1
ℹ suites 0
ℹ pass 1
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 15.233792

@himself65
Copy link
Member Author

OK, i'm opening PR now

nodejs-github-bot pushed a commit that referenced this issue Apr 15, 2024
Co-authored-by: Colin Ihrig <cjihrig@gmail.com>
PR-URL: #52401
Fixes: #52399
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
aduh95 pushed a commit that referenced this issue Apr 29, 2024
Co-authored-by: Colin Ihrig <cjihrig@gmail.com>
PR-URL: #52401
Fixes: #52399
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
marco-ippolito pushed a commit that referenced this issue May 2, 2024
Co-authored-by: Colin Ihrig <cjihrig@gmail.com>
PR-URL: #52401
Fixes: #52399
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
marco-ippolito pushed a commit that referenced this issue May 3, 2024
Co-authored-by: Colin Ihrig <cjihrig@gmail.com>
PR-URL: #52401
Fixes: #52399
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants