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

Mock "exit" in "process" #6815

Closed
wants to merge 1 commit into from
Closed

Mock "exit" in "process" #6815

wants to merge 1 commit into from

Conversation

mjesun
Copy link
Contributor

@mjesun mjesun commented Aug 8, 2018

Since functions exposed in process are the real ones, exit is being exposed as-is. The issue is that calling process.exit with a zero exit code might trick the workers, thinking the execution was stopped.

Pretty much like we do with send, we will override it with an empty method that does nothin.

Since functions exposed in `process` are the real ones, `exit` is being exposed as-is. The issue is that calling `process.exit` with a zero exit code might trick the workers, thinking the execution was stopped.

Pretty much like we do with `send`, we will override it with an empty method that does nothin.
@SimenB
Copy link
Member

SimenB commented Aug 8, 2018

Missing changelog :o

@SimenB
Copy link
Member

SimenB commented Aug 8, 2018

Should probably update the text in #6714 as well to say "blocked" or "stubbed" or something

@mjesun
Copy link
Contributor Author

mjesun commented Aug 9, 2018

@SimenB I wasn't aware of this, that's pretty cool! What do you think about moving the code snippet in #6714 into the create_process_object.js file, then avoid doing the realExit? I'd also like to extend the error message saying that in order to capture the error you should mock the function via jest.spyOn(process, 'exit').mockImplementation(() => {...}).

@SimenB
Copy link
Member

SimenB commented Aug 9, 2018

I left out the first part since we need access to config to properly format stack traces (e.g. rootDir for relative paths). Happy to move it there if you think it's ok to pass config through

@mjesun
Copy link
Contributor Author

mjesun commented Aug 9, 2018

I've been thinking that once you reach process.exit, you probably don't expect anything else to get executed; should we just throw instead? Then we can provide the mocking help in the exception as a UX thing.

@mjesun
Copy link
Contributor Author

mjesun commented Aug 9, 2018

(Or, even better, we should probably throw to abort test execution, then capture this thrown value and silently pass. It would probably be the closest behavior to process.exit without exiting).

@thymikee
Copy link
Collaborator

thymikee commented Aug 9, 2018

Makes sense.

@mjesun
Copy link
Contributor Author

mjesun commented Aug 9, 2018

@thymikee which one of the two? 😄

@thymikee
Copy link
Collaborator

thymikee commented Aug 9, 2018

Hah, I was replying to the first one. I'm just not sure about silently passing process.exit – when you do so, you expect the program to stop executing asap, no?

@mjesun
Copy link
Contributor Author

mjesun commented Aug 9, 2018

Yes, but code is being executed in the Jest sandbox. My gut feeling is that you expect your test to be finished, but not killing the entire worker.

@thymikee
Copy link
Collaborator

thymikee commented Aug 9, 2018

Yup, but throwing wouldn't kill the worker, just fail the whole test suite, or am I missing something?

@mjesun
Copy link
Contributor Author

mjesun commented Aug 9, 2018

The way I see it is that we'd catch this "special error" thrown (e.g. a Symbol), and silently pass the test.

@thymikee
Copy link
Collaborator

thymikee commented Aug 9, 2018

And then re-throw if it was not-0? Or display a warning that process.exit was called and do something about it?

@SimenB
Copy link
Member

SimenB commented Aug 9, 2018

I don't see any use case for process.exit to exit the test run or worker. I'm down with throwing and failing the whole suite though, and providing a nice message about how to mock process.exit if people wanna use it

@mjesun
Copy link
Contributor Author

mjesun commented Aug 9, 2018

The issue I see if we follow that approach is that mocking process.exit won't interrupt the code execution, as it would hapen on a real-life scenario. The only way I can see of stopping test execution is to throw and silently catch, but having to tell the user to do that by default is kinda overengineering.

@SimenB
Copy link
Member

SimenB commented Oct 20, 2018

I think #6714 covers this. Reopen if you disagree 🙂

@SimenB SimenB closed this Oct 20, 2018
@SimenB SimenB deleted the process-exit branch December 18, 2018 09:30
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants