-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
[jest-worker] Don't crash when running node with flags disallowed in workers #12128
Conversation
Ok I need to clone the repo locally to understand what's going on; I thought |
Passing
If someone needs the behavior introduced in #12069 (i.e. enforce validation of the exec argv of the parent thread), they can explicitly pass |
Codecov Report
@@ Coverage Diff @@
## main #12128 +/- ##
==========================================
- Coverage 67.71% 67.71% -0.01%
==========================================
Files 328 328
Lines 16990 16989 -1
Branches 4817 4817
==========================================
- Hits 11505 11504 -1
Misses 5452 5452
Partials 33 33
Continue to review full report at Codecov.
|
I also opened nodejs/node#41103 to lift this restriction from the Node.js side. |
+1 for fixing it filtering
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for digging into this! I was similarly surprised by the lacking docs in this area, thanks for opening an issue there!
/cc @kherock FYI |
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 noticed that Babel's CI started failing with this error a few days ago:
Then, I noticed #12103 which fixes the problem for a specific option, and I found the root cause in #12069. I don't understand why #12069 specifies
execArgv: process.execArgv
, but removing it should fix the bug for all the different options that currently makejest-worker
crash (I don't even know if this list is documented, but I can try finding it again in the Node.js source if needed).<rant>
I am really annoyed by this Node.js behavior. Reading the docs it looks like the default value of
execArgv
isprocess.execArgv
, but it's not. Yesterday I spent some time trying to fix this same identical bug in a non-jest-related PR I was working on for Babel (babel/babel#14025 (comment)) and just after reading the Node.js source code I realized that it's an important difference: if you passexecArgv: process.execArgv
Node.js validates it; if you don't pass anything it usesprocess.execArgv
without performing any validation.If you read the docs knowing this difference you can see that it never actually says that
process.execArgv
, but it's really annoying that this difference is not explicitly stated and they make you think that it's the default value.</rant>
Test plan
CI 🤞
I did this change from the GH UI; lets see if it works.