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 global event handlers after run is completed #53868

Closed
mcollina opened this issue Jul 16, 2024 · 4 comments · Fixed by #53878
Closed

test_runner: remove global event handlers after run is completed #53868

mcollina opened this issue Jul 16, 2024 · 4 comments · Fixed by #53878
Labels
confirmed-bug Issues with confirmed bugs. good first issue Issues that are suitable for first-time contributors. test_runner Issues and PRs related to the test runner subsystem.

Comments

@mcollina
Copy link
Member

process.on('uncaughtException', exceptionHandler);
process.on('unhandledRejection', rejectionHandler);
process.on('beforeExit', exitHandler);
// TODO(MoLow): Make it configurable to hook when isTestRunner === false.
if (globalOptions.isTestRunner) {
process.on('SIGINT', terminationHandler);
process.on('SIGTERM', terminationHandler);
register some event handlers on process. After all the tests are finished, we should unregister them because multiple run() call could be done from the same process.

These warnings are currently shown by running

$ ./node --expose-internals test/parallel/test-runner-run.mjs
(node:50708) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 uncaughtException listeners added to [process]. MaxListeners is 10. Use emitter.setMaxListeners() to increase limit
(Use `node --trace-warnings ...` to show where the warning was created)
(node:50708) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 unhandledRejection listeners added to [process]. MaxListeners is 10. Use emitter.setMaxListeners() to increase limit
(node:50708) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 beforeExit listeners added to [process]. MaxListeners is 10. Use emitter.setMaxListeners() to increase limit
▶ require('node:test').run
  ✔ should run with no tests (123.090125ms)
  ✔ should fail with non existing file (129.730834ms)
/Users/matteo/Repositories/node/test/fixtures/test-runner/shards/a.cjs
  ✔ should succeed with a file (300.698708ms)
/Users/matteo/Repositories/node/test/fixtures/test-runner/shards/f.cjs
  ✔ should run same file twice (435.653792ms)
  ✔ should run a failed test (271.949208ms)
  ✔ should support timeout (208.373834ms)
  ✔ should be piped with dot (309.843ms)
  ▶ should be piped with spec reporter
    ✔ new spec (306.557334ms)
    ✔ spec() (295.876667ms)
    ✔ spec (283.325875ms)
  ▶ should be piped with spec reporter (314.147333ms)
  ✔ should be piped with tap (308.117917ms)
  ✔ should skip tests not matching testNamePatterns - RegExp (305.547959ms)
  ✔ should skip tests not matching testNamePatterns - string (301.231583ms)
  ✔ should pass only to children (310.300084ms)
  ✔ should emit "test:watch:drained" event on watch mode (113.593959ms)
  ▶ AbortSignal
    ✔ should accept a signal (105.751667ms)
    ✔ should stop watch mode when abortSignal aborts (290.386167ms)
    ✔ should abort when test succeeded (300.463708ms)
    ✔ should abort when test failed (310.130792ms)
  ▶ AbortSignal (314.604833ms)
  ▶ sharding
    ▶ validation
      ✔ should require shard.total when having shard option (7.7285ms)
      ✔ should require shard.index when having shards option (1.060042ms)
      ✔ should require shard.total to be greater than 0 when having shard option (1.074709ms)
      ✔ should require shard.index to be greater than 0 when having shard option (0.914833ms)
      ✔ should require shard.index to not be greater than the shards total when having shard option (0.867417ms)
      ✔ should require watch mode to be disabled when having shard option (0.930333ms)
    ▶ validation (58.07875ms)
    ✔ should run only the tests files matching the shard index (401.742875ms)
    ✔ different shards should not run the same file (426.680209ms)
    ✔ combination of all shards should be all the tests (439.040334ms)
  ▶ sharding (454.904625ms)
  ▶ validation
    ✔ should only allow array in options.files (83.029041ms)
    ✔ should only allow object as options (81.610708ms)
    ✔ should pass instance of stream to setup (281.134208ms)
  ▶ validation (299.106417ms)
  ✔ should run with no files (119.761708ms)
  ℹ 'only' and 'runOnly' require the --test-only command-line option.
  ✔ should run with no files and use spec reporter (115.012084ms)
  ✔ should run with no files and use dot reporter (114.977625ms)
  ✔ should avoid running recursively (296.492291ms)
▶ require('node:test').run (474.936791ms)
▶ forceExit
  ✔ throws for non-boolean values (0.275833ms)
  ✔ throws if enabled with watch mode (0.088125ms)
▶ forceExit (0.436042ms)
ℹ tests 37
ℹ suites 7
ℹ pass 37
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 479.010458
@mcollina mcollina added confirmed-bug Issues with confirmed bugs. test_runner Issues and PRs related to the test runner subsystem. labels Jul 16, 2024
@mcollina
Copy link
Member Author

cc @MeowShe

@MeowShe
Copy link
Contributor

MeowShe commented Jul 16, 2024

cc @MeowShe

Maybe you are calling @MoLow ?
(I'm meowshe, he is moshe 😆)

@mcollina
Copy link
Member Author

mcollina commented Jul 16, 2024

ouch, sorry!

@cjihrig
Copy link
Contributor

cjihrig commented Jul 16, 2024

If anyone wants to pick this up, there is already an exitHandler() function in node/lib/internal/test_runner/harness.js that performs the cleanup. That same function is also exposed throughout the rest of the test runner as root.harness.teardown(). We probably just need a call to that somewhere.

Right now, root.harness.teardown() only removes the uncaughtException and unhandledRejection handlers. It should probably clean up the other event handlers as well.

@MoLow MoLow added the good first issue Issues that are suitable for first-time contributors. label Jul 16, 2024
nodejs-github-bot pushed a commit that referenced this issue Jul 23, 2024
PR-URL: #53878
Fixes: #53868
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit that referenced this issue Jul 28, 2024
PR-URL: #53878
Fixes: #53868
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
RafaelGSS pushed a commit that referenced this issue Aug 5, 2024
PR-URL: #53878
Fixes: #53868
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
marco-ippolito pushed a commit that referenced this issue Aug 19, 2024
PR-URL: #53878
Fixes: #53868
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
marco-ippolito pushed a commit that referenced this issue Aug 19, 2024
PR-URL: #53878
Fixes: #53868
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
marco-ippolito pushed a commit that referenced this issue Aug 19, 2024
PR-URL: #53878
Fixes: #53868
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@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
confirmed-bug Issues with confirmed bugs. 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.

4 participants