Jest failed, Travis Passed

[jimfb]

Calling it out as a separate issue so it doesn't get lost in the noise.

Jest clearly failed, but travis says everything is green:
https://travis-ci.org/facebook/react/jobs/112649566

Discovered in: #6121 (comment)

cc @zpao @spicyj

[zpao]

I pinged @cpojer to look at it. We might just want to downgrade our jest in the meantime.

[cpojer]

I would recommend upgrading to 0.9.0-fb3 first. fb1 had some bugs.

[sophiebits]

My fault. Move the set -e out to after the "$TEST_TYPE" = test check so that it fails the build if any command fails. Maybe even move it to the top of the whole script -- but I thought there was some reason I didn't do that originally.

[cpojer]

@spicyj can you elaborate what's going on here, just in case someone else runs into this and asks me? Also, please upgrade to fb3, the current one sometimes doesn't complete even if all tests pass. 0.9.0 is ready soon, with much better babel and npm3 support :)

[sophiebits]

Jest is properly returning 1, as is grunt, but since we run stuff after that Travis doesn't notice. That is, if you run

false  # failing command
true   # passing command

then that counts as a pass because only the last command's status matters. Unless you do set -e, in which case it bails out at the first error.

[cpojer]

I see. The original problem was however about a worker thread unexpectedly quitting. This might be because one of the tests had a side-effect of killing a node-process for some bad reason.

[sophiebits]

Yeah, maybe that is a -fb1 problem.

[zpao]

We have -fb3 running (I cleaned the caches so we fetched the latest that matched): https://travis-ci.org/facebook/react/jobs/113204333#L242

[jimfb]

I think travis is doing the right thing now.

[sophiebits]

@jimfb That seems unlikely since set -e wasn't added to .travis.yml. You sure?

[jimfb]

No, I'm not sure, but I did see a travis jest failure on a recent diff. Though, that diff may have had a syntax error, so it's possible it failed harder because it was unable to parse.

[sophiebits]

Reading the current travis file, I wouldn't expect a jest failure to cause the travis job to fail.

[jimfb]

¯\_(ツ)_/¯ and yet I'm 99.999% sure that I did witness travis saying the jest task was red :). But yeah, like I said, I'm now thinking it was because it the jest task failed at a different point in the task due to it being a syntax error instead of a less severe expectation violation.

[gaearon]

I removed set -e in d138b28. I wasn’t sure if it’s necessary or not, and since I was trying to make master behaves the same way as branches, I removed it from master for consistency. Should we add it back?

[sophiebits]

Yes, I think we have to.

[gaearon]

Should we also enable it for PRs? That discrepancy surprised me.

[sophiebits]

Yes. This bug was that it was on for master and not for PRs -- it should be on for all builds.