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

workaround for windows to ensure console.log is always flushed before pr... #111

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants
@malonecj

malonecj commented Aug 6, 2014

...ocess.exit

This is a potential solution for #110

I don't have much experience with Node yet so I might be missing something here, but it at least fixed my problem and am now seeing all test results logged to console.

@@ -321,7 +321,9 @@ function _main(onComplete) {
if (argv.help) {
optimist.showHelp();
process.exit(0);
process.on('exit', function(){

This comment has been minimized.

@jeffmo

jeffmo Aug 21, 2014

Member

Interesting, hadn't heard of this before...but seems to be a KP: https://www.npmjs.org/package/exit

One thing we need to be sure of on these changes is that execution doesn't continue after exiting (which was part of the intent of those exits before). So we either need to refactor the scripts so they can return early after setting up this listener, or find a way to make sure that the remaining code doesn't continue to execute if we hit this branch in the code path.

@jeffmo

jeffmo Aug 21, 2014

Member

Interesting, hadn't heard of this before...but seems to be a KP: https://www.npmjs.org/package/exit

One thing we need to be sure of on these changes is that execution doesn't continue after exiting (which was part of the intent of those exits before). So we either need to refactor the scripts so they can return early after setting up this listener, or find a way to make sure that the remaining code doesn't continue to execute if we hit this branch in the code path.

@tcoopman

This comment has been minimized.

Show comment
Hide comment
@tcoopman

tcoopman Oct 1, 2014

What's the status of this?

tcoopman commented Oct 1, 2014

What's the status of this?

@jeffmo

This comment has been minimized.

Show comment
Hide comment
@jeffmo

jeffmo Oct 1, 2014

Member

I think we're just waiting on the update to the PR per the inline comment.
@malonecj: Did you want to finish this out?

Member

jeffmo commented Oct 1, 2014

I think we're just waiting on the update to the PR per the inline comment.
@malonecj: Did you want to finish this out?

@malonecj

This comment has been minimized.

Show comment
Hide comment
@malonecj

malonecj Oct 13, 2014

Sorry, have not had a chance to use Jest again since submitting this. I should hopefully get some time to look into this in the next week or so

Sorry, have not had a chance to use Jest again since submitting this. I should hopefully get some time to look into this in the next week or so

@ForbesLindesay ForbesLindesay referenced this pull request Dec 2, 2014

Open

Testing #4

@ForbesLindesay

This comment has been minimized.

Show comment
Hide comment
@ForbesLindesay

ForbesLindesay Dec 2, 2014

Contributor

I'm not sure the problem with tests "continuing to execute" makes sense. If all jest tests are synchronous, then the initial thrown exception should terminate the tests anyway, so there shouldn't be any need to worry about that?

@jeffmo If you want me to re-write this to use the 'exit' module I'm happy to make that change.

Contributor

ForbesLindesay commented Dec 2, 2014

I'm not sure the problem with tests "continuing to execute" makes sense. If all jest tests are synchronous, then the initial thrown exception should terminate the tests anyway, so there shouldn't be any need to worry about that?

@jeffmo If you want me to re-write this to use the 'exit' module I'm happy to make that change.

@jeffmo

This comment has been minimized.

Show comment
Hide comment
@jeffmo

jeffmo Dec 2, 2014

Member

In the code change I commented on, we want the jest CLI to stop then-and-there if --help was passed
(i.e. not print the help and go on to continue executing whatever command it might have if --help weren't specified)

Member

jeffmo commented Dec 2, 2014

In the code change I commented on, we want the jest CLI to stop then-and-there if --help was passed
(i.e. not print the help and go on to continue executing whatever command it might have if --help weren't specified)

@jeffmo

This comment has been minimized.

Show comment
Hide comment
@jeffmo

jeffmo Dec 2, 2014

Member

Also I'm not quite sure what you meant by tests executing synchronously -- but test files themselves run asynchronously, so a failing test would not stop the Jest cli in it's tracks

Member

jeffmo commented Dec 2, 2014

Also I'm not quite sure what you meant by tests executing synchronously -- but test files themselves run asynchronously, so a failing test would not stop the Jest cli in it's tracks

@mkristo

This comment has been minimized.

Show comment
Hide comment
@mkristo

mkristo Dec 2, 2014

@jeffmo I just tried using the proposed 'exit' module instead of process.exit, and it seems to work just as intended. Both with and without the --help directive. I might not understand the problem though.

mkristo commented Dec 2, 2014

@jeffmo I just tried using the proposed 'exit' module instead of process.exit, and it seems to work just as intended. Both with and without the --help directive. I might not understand the problem though.

@ForbesLindesay

This comment has been minimized.

Show comment
Hide comment
@ForbesLindesay

ForbesLindesay Dec 2, 2014

Contributor

@jeffmo Thanks, I think I understand now. I had assumed you meant that comment to apply to all the process.exit calls, but it makes far more sense for just the --help case. I think it would be ok to just use the original process.exit code in the --help case, since at that point stdio is still in blocking mode. We would only need to use the exit module for the other cases where stdio may have been put into non-blocking mode.

Contributor

ForbesLindesay commented Dec 2, 2014

@jeffmo Thanks, I think I understand now. I had assumed you meant that comment to apply to all the process.exit calls, but it makes far more sense for just the --help case. I think it would be ok to just use the original process.exit code in the --help case, since at that point stdio is still in blocking mode. We would only need to use the exit module for the other cases where stdio may have been put into non-blocking mode.

@jquense

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense Jan 1, 2015

Contributor

any word on when this might hit? I'd be happy to help get it done. Without this Jest doesn't provide any feedback on windows, rendering it useless as a testing tool

Contributor

jquense commented Jan 1, 2015

any word on when this might hit? I'd be happy to help get it done. Without this Jest doesn't provide any feedback on windows, rendering it useless as a testing tool

@jeffmo

This comment has been minimized.

Show comment
Hide comment
@jeffmo

jeffmo Jan 23, 2015

Member

I am happy to take this, but it's in a place where it needs some refactoring (and bin/jest.js has changed since this was written). I would go in and just finish this up myself (seems fairly straightforward), but I don't have a working Windows machine to try to port this to the latest code...so I wouldn't feel comfortable doing so without being able to verify and test.

Could either @malonecj or someone else who's running on windows finish this out and confirm that it fixes things? I'm happy to take it if we can get the stop-execution problem worked out (the thing I commented inline)

Member

jeffmo commented Jan 23, 2015

I am happy to take this, but it's in a place where it needs some refactoring (and bin/jest.js has changed since this was written). I would go in and just finish this up myself (seems fairly straightforward), but I don't have a working Windows machine to try to port this to the latest code...so I wouldn't feel comfortable doing so without being able to verify and test.

Could either @malonecj or someone else who's running on windows finish this out and confirm that it fixes things? I'm happy to take it if we can get the stop-execution problem worked out (the thing I commented inline)

@jquense

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense Jan 23, 2015

Contributor

@jeffmo I threw something together let me know if its enough!

Contributor

jquense commented Jan 23, 2015

@jeffmo I threw something together let me know if its enough!

@jeffmo

This comment has been minimized.

Show comment
Hide comment
@jeffmo

jeffmo Jan 23, 2015

Member

Awesome, thanks a ton @jquense !

Member

jeffmo commented Jan 23, 2015

Awesome, thanks a ton @jquense !

@jeffmo jeffmo closed this Jan 23, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment