test_runner: allow to force exit after all tests finished #49925
Comments
rluvaton
added
feature request
|
cc @nodejs/test_runner |
|
Do we need to do anything? Wouldn't a |
|
I don't quite understand the use case.
It's your test suite, so in theory you have some level of control over all of the resources. It sounds like you just aren't cleaning up after yourself properly. |
|
in this is totally valid: await setTimeout(10 * 60 * 1000) // sleep for ten minutes
test('run a test after ten minutes', () => {});and there is no way to know a test is going to be enqueued besides waiting for event loop to drain using |
|
Unfortunately, the use case is some dependency of dependency that I'm using is not cleaning up... |
@rluvaton that sounds to me like a bug within the dependency, which should be fixed there... In addition, as Moshe said, the test runner can't tell if all tests completed, as we do not build the test tree before executing the tests, which means adding such an implementation would also make executing the tests that much slower... |
|
I think this is reasonable and test-runners typically implement this because it's common enough, I'm also not sure we can implement this reasonably with our current design. We can however (with a flag) exit when no more tests are enqueued within a microtick after all already enqueued tests were run which should work well in practice maybe? |
|
I disagree. I don't think we should implement this, if for no other reason than I don't think it's worth another CLI flag. You're literally saying you have no control over your own application if you can't make it stop. |
|
another option is to add on the |
|
I don't mind that idea, but if I understand #49925 (comment) correctly, the problem is that they are not able to perform any cleanup. (I'm curious to hear more about this by the way because if the issue is that a handle is holding the event loop open, you should be able to use something like |
|
How about exposing a function that would force the reporter to print its result so that the user can use process.exit() afterward ? Maybe something like "context.report()". I think the importance of this functionality is understated in this issue. Unfortunately in a nodejs project ensuring that you have a perfectly clean state after running complex tests can be quite difficult and is not always worth the effort (_getActiveHandles was deprecated and getActiveResourcesInfo is not really sufficient). I think that providing an easy way out is important for usability. |
|
I'm +1 for implementing this. Another common use case is when Child Processes are being opened and never closed. Another is you'd use e2e tests to spin up a bunch of server instances and don't bother of closing the server on tests as in production would be only a single server instance. This is very helpful IMHO. I agree that we'd use getAcive handles and check it out ourselves or the best scenario would be checking manually for handles and closing them. But in my experience, while migrating from one test runner to another having a way to force exit after all tests have been completed would be the best user experience and would prevent some friction on how it works on other frameworks but in the native runner. We could also show a warning of the active handles while forcing exiting and help users understand what could be the problem (as we have already when we reach the maximum listeners count) |
|
A socket kept alive by an http agent is another example. Sure I can spend 1 hour tracking it down (like I did just today) and either find a way to change the agent parameters or grab a reference and destroy it in test cleanup. But there is no added value. I don't feel like I get a better control of my program as the change only concerns the test suite, and I am worried that the next time something similar happens I might get stuck for real and be forced to move the whole test suite to another runner. |
|
I'm trying to use Test Runner as testing tool for an express api and this prevents my pipeline from completing tests and continue with further steps. Am I missing something? Isn't this a good enough use case? |
|
I also have the same issue |
This commit updates the test runner to allow a forced exit once all known tests have finished running. Fixes: nodejs#49925
This commit updates the test runner to allow a forced exit once all known tests have finished running. Fixes: nodejs#49925
This commit updates the test runner to allow a forced exit once all known tests have finished running. Fixes: nodejs#49925
This commit updates the test runner to allow a forced exit once all known tests have finished running. Fixes: #49925 PR-URL: #52038 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Raz Luvaton <rluvaton@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
This commit updates the test runner to allow a forced exit once all known tests have finished running. Fixes: nodejs#49925 PR-URL: nodejs#52038 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Raz Luvaton <rluvaton@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
This commit updates the test runner to allow a forced exit once all known tests have finished running. Fixes: #49925 PR-URL: #52038 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Raz Luvaton <rluvaton@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
This commit updates the test runner to allow a forced exit once all known tests have finished running. Fixes: nodejs#49925 PR-URL: nodejs#52038 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Raz Luvaton <rluvaton@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
This commit updates the test runner to allow a forced exit once all known tests have finished running. Fixes: #49925 PR-URL: #52038 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Raz Luvaton <rluvaton@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Now we've imported and updated all our models it is time to start adding some example tests to drive research of node-test as an alternate to Lab further. In database terms, all models are accessed and used in the same way via Objection.js. However, we split them into two camps for testing; reference models and transaction models. Reference models are things we don't expect to be creating within the service as part of daily use. They are records that would be seeded, for example, regions. Transaction models are things we expect to be created, for example, bill runs or licences. When it comes to testing we seed the tables that back the reference models, hence their helpers generally have a `select()` method. Transaction models, when needed, will be created during the testing. Their helpers generally have an `add()` method. When we first tried this test, node-test wouldn't quit. It wouldn't even move to the next spec after `region.model.test.js`. This is a problem we first faced when pulling this project together. Time away has clearly helped because we were able to diagnose the cause as the database connection we create blocks node test from exiting (see [test_runner: allow to force exit after all tests finished](nodejs/node#49925) for a deeper dive into the issue which gave us our a-ha! moment). It does mean we'll need to add an `after()` block into any spec that results in a DB connection being made. But that should be simple enough. Completing this example has also highlighted that we'll probably want to extend `assert` in some way, to avoid 'messy duplication' in our tests. But for now, here is our first working model node-test!
Now that we've imported and updated all our models, it is time to start adding some example tests to further research node-test as an alternative to Lab. In database terms, all models are accessed and used via Objection.js. However, for testing, we split them into two camps: reference models and transaction models. Reference models are things we don't expect to create within the service as part of daily use. They are records that would be seeded, such as regions. Transaction models are things we expect to be created, such as bill runs or licences. When it comes to testing we seed the tables that back the reference models, hence their helpers generally have a `select()` method. Transaction models, when needed, will be created during the testing. Their helpers generally have an `add()` method. When we first tried this test, node-test wouldn't quit. It wouldn't even move to the next spec after `region.model.test.js`. This is a problem we first faced when pulling this project together. Time away has clearly helped because we were able to diagnose the cause as the database connection we create blocks node test from exiting (see [test_runner: allow to force exit after all tests finished](nodejs/node#49925) for a deeper dive into the issue which gave us our a-ha! moment). This does mean we'll need to add an `after()` block to any spec that results in a DB connection being made, but that should be simple enough. Completing this example has also highlighted that we'll probably want to extend `assert` in some way, to avoid 'messy duplication' in our tests. But for now, here is our first working model node-test!
What is the problem this feature will solve?
if you have some resource that you don't control blocks the process from exiting (socket/timer/etc) the tests will never finish
having timeout in the run options marking the test as failed
What is the feature you are proposing to solve the problem?
allow to specify force exit option that after all tests finished will exit
similar solutions in other frameworks:
mochahas--exitflagjesthas--forceExitWhat alternatives have you considered?
No response
The text was updated successfully, but these errors were encountered: