fix: only set error codes when they are non zero #7363
Conversation
I'm working on an internal CLI that runs jest, eslint, and sasslint all in one single process for our frontend team to run from their `package.json`'s
Aka something like, `"test": "cli test"`.
The code `process.on('exit', () => (process.exitCode = code));` adds a listener that is overriding the exit codes we're setting so that when jest is successful, and eslint or sasslint fail, I can't exit with a failing code.
Therefore our CI's are reporting as passed even though the linting fails.
It seems as though there's no reason to actually set the error code at all unless it's non-zero?
Let me know what the thoughts here are. I can come back in and add more, add some additional tests, etc if this looks like an ok change.
Thanks!
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
|
Changelog and code both updated. |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
|
So, @SimenB that lint-and-typecheck step keeps failing? |
|
Sorry for reviewing this after you fixed the lint, but I left a couple questions |
|
Haha @rickhanlonii no worries, I'd rather get it right. |
Codecov Report
@@ Coverage Diff @@
## master #7363 +/- ##
=======================================
Coverage 66.72% 66.72%
=======================================
Files 241 241
Lines 9356 9356
Branches 6 6
=======================================
Hits 6243 6243
Misses 3110 3110
Partials 3 3Continue to review full report at Codecov.
|
|
Anything else y'all need from me to mergy merge? |
|
@rickhanlonii mind taking a last look? 🙂 |
|
Looks great, thanks @jcreamer898 and welcome to Jest! |
|
Thanks!! |
|
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. |
Summary
I'm working on an internal CLI that runs jest, eslint, and sasslint all in one single process for our frontend team to run from their
package.json'sAka something like,
"test": "cli test".The code
process.on('exit', () => (process.exitCode = code));adds a listener that is overriding the exit codes we're setting so that when jest is successful, and eslint or sasslint fail, I can't exit with a failing code.Therefore our CI's are reporting as passed even though the linting fails.
It seems as though there's no reason to actually set the error code at all unless it's non-zero?
Let me know what the thoughts here are. I can come back in and add more, add some additional tests, etc if this looks like an ok change.
Thanks!
Test plan
Pretty much just make sure that jest still reports a non-zero exit code with a failing test.