Skip to content

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

Closed
#52401
Closed
async error inside before won't cause test error#52399
#52401
Labels
test_runnerIssues and PRs related to the test runner subsystem.

Description

@himself65
himself65
opened on Apr 7, 2024

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

Activity

  1. himself65
    added
    test_runnerIssues and PRs related to the test runner subsystem.
    on Apr 7, 2024
  2. himself65
    changed the title [-]error inside before won't cause test error[/-] [+]async error inside before won't cause test error[/+] on Apr 7, 2024

  3. himself65 commented on Apr 7, 2024

    @himself65
    himself65
    on Apr 7, 2024
    MemberAuthor
    import { before, test } from 'node:test'

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

test('ok', () => {

})

  4. benjamingr commented on Apr 7, 2024

    @benjamingr
    benjamingr
    on Apr 7, 2024
    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.

  5. himself65 commented on Apr 7, 2024

    @himself65
    himself65
    on Apr 7, 2024
    MemberAuthor

    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

  6. benjamingr commented on Apr 7, 2024

    @benjamingr
    benjamingr
    on Apr 7, 2024
    Member

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

  7. himself65 commented on Apr 7, 2024

    @himself65
    himself65
    on Apr 7, 2024
    MemberAuthor

    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

  8. himself65 commented on Apr 7, 2024

    @himself65
    himself65
    on Apr 7, 2024
    MemberAuthor

    OK, i'm opening PR now

  9. nodejs-github-bot
    closed this as completedin #52401on Apr 15, 2024
  10. nodejs-github-bot
    added a commit that references this issue on Apr 15, 2024

    test_runner: better error handing for test hook

    f098b7a
  11. aduh95
    added a commit that references this issue on Apr 29, 2024

    test_runner: better error handing for test hook

    e066ba2
  12. marco-ippolito
    added 2 commits that reference this issue on May 2, 2024

    test_runner: better error handing for test hook

    2568cef

    test_runner: better error handing for test hook

    29d2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    test_runnerIssues and PRs related to the test runner subsystem.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions