fix(jest-runtime): make sure a module can never be its own parent #5241
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5241 +/- ##
==========================================
+ Coverage 61.18% 61.19% +<.01%
==========================================
Files 202 202
Lines 6771 6767 -4
Branches 4 4
==========================================
- Hits 4143 4141 -2
+ Misses 2627 2625 -2
Partials 1 1
Continue to review full report at Codecov.
|
|
I think this is the right fix. Test files, or the first module required in a context, are to be considered entry points. However, I'm wondering if you are seeing module.parent being null more often than not because the real path doesn't match a symlinked file name? Let's make sure we aren't breaking module.parent in other places. |
|
Any cases in particular you've got in mind beyond symlinks? And ideas for a test I can add to that effect? What if the path is to a virtual mock, I can't do |
| @@ -509,6 +509,10 @@ class Runtime { | |||
| ({ | |||
| enumerable: true, | |||
| get() { | |||
| if (localModule.filename === from) { | |||
There was a problem hiding this comment.
I originally had moduleRegistry[from] === localModule, but I think it's the same thing in practice
|
Before merging, can you make a list here with the modules that don't have a parent after this change? For virtual mocks, the parent should be the module that first requires the mock. |
Sure! @jdalton said this in the original issue:
Which I sorta agree with, and how I expected it to work. I think I have to dig a bit deeper into it. |
|
I don't agree with that. Test files should not have a parent because they are entry points. The fact that they are run via Jest shouldn't matter. Basically, it should be "the same" as doing |
|
That makes sense. I'll see if I can get some flag or something indicating if a file is a test file or not, and explicitly set parent to |
|
Keep in mind that jest-runtime may be used standalone with things other than tests, but what you are saying makes sense. You can theoretically use a flag that you set after the first file has been loaded in the context. Note that "first" file here means user file, all the other test related code is loaded using the internal require functions. Maybe we can just set this automatically after using the first non-internal require call on runtime? |
|
Closing this, but we still need to fix it. |
|
Agreed. Should be easier with #5618 merged. I'll try to find the time 🙂 |
Done at the request of @SimenB from PR jestjs#5972
* Do not include `from` information when its not valid (#5235) If you include the information then when the _execModule routine loads the module without a from context then it will incorrectly setup a circular dependency by declaring the parent is itself. By checking for a module name and not including that information when passed in, the issue is avoided. * Copy in integrations tests from pull #5241 Done at the request of @SimenB from PR #5972
|
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
Fixes #5235.
I'm not sure if this is the correct fix. Technically the parent of the test files is something internal to jest. And the condition I've added seems to be true for lots of different files, and I don't really understand why...
Test plan
Integration test added. Would love some help adding a unit test.