test: do not swallow uncaughtException errors in exit code tests

[nektro]

cannot use node:assert module in cases where uncaughtException handler is registered as an failure will be eaten by it

to that point: zeroExitWithUncaughtHandler had partially wrong values and i've updated it along with the patch.

[Trott]

First and foremost: Thanks for both finding and fixing this problem! Is this the direct result of working on Bun compatibility with Node.js? I love when stuff like that ends up improving things for everyone.

The approach taken here seems fine to me and I'll happily approve it, but might it result in easier-to-read code if we remove the uncaughtException listener inside the exit listener rather than working around the listener? That might look something like this:

  function zeroExitWithUncaughtHandler() {
    const noop = () => { };
    process.on('exit', (code) => {
      process.off('uncaughtException', noop);
      assert.strictEqual(process.exitCode, undefined);
      assert.strictEqual(code, 0);
    });
    process.on('uncaughtException', noop);
    throw new Error('ok');
  }
  cases.push({ func: zeroExitWithUncaughtHandler, result: 0 });

  function changeCodeInUncaughtHandler() {
    const modifyExitCode = () => { process.exitCode = 97; };
    process.on('exit', (code) => {
      process.off('uncaughtException', modifyExitCode);
      assert.strictEqual(process.exitCode, 97);
      assert.strictEqual(code, 97);
    });
    process.on('uncaughtException', modifyExitCode);
    throw new Error('ok');
  }
  cases.push({ func: changeCodeInUncaughtHandler, result: 97 });

[Trott]

To placate the commit message validator, make the first line of your commit message something like test: do not swallow uncaughtException errors in exit code tests or something like that. You'll need to force push it to your branch. (I'm sure you're more than capable of amending a git commit and force pushing, but if you're allergic to spending time pleasing automated linters, I'm happy to do it for you.)

[nektro]

First and foremost: Thanks for both finding and fixing this problem! Is this the direct result of working on Bun compatibility with Node.js? I love when stuff like that ends up improving things for everyone.

yep!

EventEmitter.off

TIL, makes sense

To placate the commit message validator, make the first line of your commit message something like test: do not swallow uncaughtException errors in exit code tests or something like that

roger

[Trott]

I landed a different PR that causes a conflict with this PR. I'll update this PR as it's just a moved file.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/60677/

[Trott]

@nodejs/testing This could use another collaborator review/approval.

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/54039
✔  Done loading data for nodejs/node/pull/54039
----------------------------------- PR info ------------------------------------
Title      test: do not swallow uncaughtException errors in exit code tests (#54039)
Author     Meghan Denny <hello@nektro.net> (@nektro, first-time contributor)
Branch     nektro:patch-1 -> nodejs:main
Labels     test, author ready, needs-ci
Commits    1
 - test: do not swallow uncaughtException errors in exit code tests
Committers 1
 - Rich Trott <rtrott@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/54039
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/54039
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Thu, 25 Jul 2024 10:44:26 GMT
   ✔  Approvals: 2
   ✔  - Rich Trott (@Trott): https://github.com/nodejs/node/pull/54039#pullrequestreview-2203199600
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/54039#pullrequestreview-2205540572
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-07-27T18:29:46Z: https://ci.nodejs.org/job/node-test-pull-request/60677/
- Querying data for job/node-test-pull-request/60677/
   ✔  Last Jenkins CI successful
   ⚠  PR author is a new contributor: @nektro(hello@nektro.net)
   ⚠  - commit 3c3e428ef668 is authored by meghan@bun.sh
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/10151832847

[Trott]

Landed in d7941b2

Thanks for the fix!