Prevent a ENOENT crash by checking for file existence in jest-message-util #5405
Conversation
Jest tries to load the stack-trace based on the top frame. The problem is, the filename may be provided by the source-map, which is unreliable since it can point to a non-existent file. In such a case, the code would cause a hard crash of "Test suite failed to run" with a stack-trace pointing at "exports.formatStackTrace", which I have corrected here.
niieani
changed the title
|
Note: The fail on Node 8 is not related to this PR. |
| @@ -227,7 +227,7 @@ export const formatStackTrace = ( | |||
| if (topFrame) { | |||
| const filename = topFrame.file; | |||
|
|
|||
| if (path.isAbsolute(filename)) { | |||
| if (path.isAbsolute(filename) && fs.existsSync(filename)) { | |||
There was a problem hiding this comment.
Would it be better to just catch the readFileSync to save an io operation?
There was a problem hiding this comment.
What we should do is check the hastefs cache and just read from that
There was a problem hiding this comment.
See #5087 (comment). Fixing that (ignoring files not in the cache) is the real fix
There was a problem hiding this comment.
@SimenB I don't know how HasteFS works or how to read its cache, however I suspect there could be cases where the file would not appear in it either. One of those cases could be running vm.runInThisContext or vm.runInNewContext from within a test, and the contents of the file is evaled (rather than being read from the disk). Unless that somehow ends up in the HasteFS too? I'm only mentioning it, since it's also exactly what happens in the webpack case.
Could I try/catch the readFileSync for now, so you could ship this bugfix as soon as possible, and we can prepare the proper fix at a later time, since I don't have enough knowledge about HasteFS? Unless it's something really simple, and you could point me to the right place?
Cheers!
There was a problem hiding this comment.
It's not a simple change as we have to somehow thread the cache all the way through.
Sticking the readFileSync in a try should be good enough
CHANGELOG.md
Outdated
| @@ -7,6 +7,7 @@ | |||
|
|
|||
| ### Fixes | |||
|
|
|||
| * `[jest-message-util]` Prevent an `ENOENT` crash when the test file contained a malformed source-map. | |||
There was a problem hiding this comment.
please add a link to this PR (and run yarn lint:md to format the file)
Codecov Report
@@ Coverage Diff @@
## master #5405 +/- ##
=======================================
Coverage 61.32% 61.32%
=======================================
Files 205 205
Lines 6925 6925
Branches 3 3
=======================================
Hits 4247 4247
Misses 2677 2677
Partials 1 1
Continue to review full report at Codecov.
|
|
@SimenB I've pushed both changes. |
|
CI is drunk, it doesn't pick up our config... |
|
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
Jest tries to load the stack-trace based on the top frame. The problem is, the filename may be provided by the source-map, which is unreliable since it can point to a non-existent file. In such a case, the code would cause a hard crash of "Test suite failed to run" with a stack-trace pointing at "exports.formatStackTrace", which I have corrected here.
I've run into this issue while migrating
webpack, to usejest.(CC: @ooflorent).
Test plan
Added an integration test for malformed source-maps.
Watch the video comparing before and after.