fix(Jest): Fix Jest middleware always exit with zero code #636
Conversation
|
Strange, the test for this failed in travis, even though jest says the test passed. |
|
Very strange, indeed. I've looked at Jest's results Object and it's says it's all good. Not sure where the 1 code is coming from. For some reason I also see the error code printed twice: when adding a |
|
My assumption is that it was passing before since the Jest would never reject. And since the code for failing test is 18 days old, it was never technically correct with the bug in the jest preset. @eliperelman would you mind taking a look at it? |
| (result.numFailedTests || result.numFailedTestSuites ? | ||
| reject() : | ||
| resolve())); | ||
| jest.runCLI(cliOptions, options.roots || [options.rootDir]) |
There was a problem hiding this comment.
I was just thinking, since we are no longer using a callback here, we can remove the return new Promise on L99 and instead just return the runCLI call, right?
return jest.runCLI(cliOptions, options.roots || [options.rootDir]);Do we even then need to capture results.success, or will jest reject if success is false?
There was a problem hiding this comment.
It won't reject as far as I can say by looking at their code. I think their promise is for some unexpected inside a try ... catch. That's why they have the success property.
There was a problem hiding this comment.
OK, so we will need to manually throw or reject or whatever. Gotcha.
There was a problem hiding this comment.
Yeah. The implementation I've submitted in this PR works in my repository. I dunno why are tests failing, though.
There was a problem hiding this comment.
OK, I'll see if I can clone locally and replicate.
There was a problem hiding this comment.
If you make this change, I will get it out in a patch release today. :)
There was a problem hiding this comment.
🤔 why did it work for me? Did they break the API again or I’m just stupid? I’ll test the code again with different Jest versions to check that.
There was a problem hiding this comment.
So, the just tests pass, the only thing was we were using the results properties from jest to determine pass or fail incorrectly, hence why it was failing our tests.
There was a problem hiding this comment.
No, I mean I tried this fix on my project and it fixed the issue. That's why I submitted the fix in the first place.
There was a problem hiding this comment.
Hmm not sure. You did fix the fact that it should be a promise now, so maybe that was part of it.
|
You decided to go with it? I’m sorry I still didn’t have time to investigate it |
Closes #632
Since this regression was introduced in v7, I think it should be released as a patch release in v7 and v8 accordingly.
After spending ~30 mins trying to write tests with Ava I gave up and offering my help with #635