-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Make process.exit optional #1018
Conversation
the reason we have that is because most node libraries don't clean up connections etc, plus it's a huge burden on the people writing tests to manually close down everything that might be active in the lib/app (timers, connections, blah blah). -1 from me in general, I don't think it would go too well |
I totally understand why this exists, and for the 90% case I agree it's the absolutely the way to go in mocha to let it You're right most node libraries don't clean up connections & timers and all that and it is indeed a huge burden to have to do that in tests. I'm probably not a normal case in that I'd really like to use mocha to test node-postgres, but one of the things I need is deterministic proof the process exited cleanly on it's own. Since I'm responsible for writing the code that does the actual cleaning up of database connections and connection pools and streams and all that crap I need to know it's working. So the burden is on me to make sure things exit cleanly in my tests. Which means I'd really love it if I could tell mocha "nah, it's okay, I don't want you to exit for me. If the test hangs I'll consider it an error." Mostly because node-postgres is so freakin' old I wrote it before mocha existed and it's just a I do see the other side that maybe some lower level libraries should use a lower level testing method...I've just become spoiled on the niceness of mocha in all my apps for testing & would love to use it everywhere. 😄 Either way thank you & the team & contributors for a great testing lib. 👍 an example would be like... $ mocha test/ --no-exit To tell the suite explicitly "do not call process.exit under any circumstances" |
--no-exit would be ok with me, tiny amount of code at least so that's not a huge deal, I know what you mean, for certain types of libraries it's definitely a good sanity check to see that everything is shutting down properly |
--no-exit's ok here |
This provides an optional means to leave it up to the developer to propertly clean up their references and let the event loop die on its own. It dovetails nicely with `--bail` in that `--bail` will still allow early termination of the test process on the first error.
Alrighty - code attached. 😄 I wasn't super sure the best place to put the tests, so I made an acceptance test for them. From what I could tell that's where the trickier "uhh, you kinda have to run the actual command to test this" tests go. One nice thing about this is if you use the Please let me know if you'd like me to modify or change this at all & I'll do so asap! |
LGTM |
Cool, just needed to squash that extra commit |
Any interest in making the automatic process exiting an optional thing that happens? By default the behavior is the same, but there's an option to disable the process.exit() call and you become responsible for making your tests clean up and exit node properly? A lot of the testing I do is with database stuff, and it's important to make sure nothing is keeping the event loop alive after my tests run. I'd happily send a pull request over if you're open to it.