-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
fix(jest-resolve-dependencies): resolve mocks as dependencies #8952
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8952 +/- ##
==========================================
+ Coverage 64.25% 64.27% +0.02%
==========================================
Files 276 276
Lines 11706 11716 +10
Branches 2865 2868 +3
==========================================
+ Hits 7522 7531 +9
- Misses 3552 3554 +2
+ Partials 632 631 -1
Continue to review full report at Codecov.
|
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.
Awesome work @petevdp!
packages/jest-resolve-dependencies/src/__tests__/dependency_resolver.test.ts
Show resolved
Hide resolved
try { | ||
resolvedDependency = this._resolver.resolveModule( | ||
file, | ||
dependency, | ||
options, | ||
); | ||
} catch (e) { | ||
resolvedDependency = this._resolver.getMockModule(file, dependency); | ||
resolvedMockDependency = this._resolver.getMockModule(file, dependency); |
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.
I'm wondering if we still need two code paths to resolve a mock. I'm unsure what this line is even for, maybe virtual mocks that do not have a real module counterpart?
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.
I'm not entirely sure but I assumed so, as the case where an error is thrown there expects the name to exist in (i think) the metadata but not be in the filesystem.
I left a note for prosterity in this commit: 8137d38
I could try to get to the bottom of this in this PR since it's pretty related, would that be a good idea?
If you do, is there a way I could figure out if this code is exercised in the test suite?
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.
Such a nice, clean solution! Thanks
packages/jest-resolve-dependencies/src/__tests__/dependency_resolver.test.ts
Outdated
Show resolved
Hide resolved
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.
LGTM
Co-Authored-By: Simen Bekkhus <sbekkhus91@gmail.com>
Thanks guys! |
// If we resolve a dependency, then look for a mock dependency | ||
// of the same name in that dependency's directory. | ||
resolvedMockDependency = this._resolver.getMockModule( | ||
path.dirname(resolvedDependency), |
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.
Is there a reason why it's not file, dependency
like above? Would be nice if we could just try
to resolve, ignoring failure, and then look for a mock in a single call regardless of whether the resolution failed, to simplify the logic here
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.
getMockModule(file, dependency)
actually does not work as expected.
Taking another look at it though, what does work with the tests that I added is this:
resolvedMockDependency = this._resolver.getMockModule(
dependency
path.basename(dependency),
);
Which is definitely better than what I had before.
Unfortunately, this will fail for the case where the dependency is in a different directory from the dependent, and there's still the existing issue of mocks of the same name from different directories.
If this isn't good enough, I'm thinking we'll have to introduce a new method into jest-resolve
which mirrors the functionality resolveModule, or expand the functionality of resolveModule
, or change what information is collected about mocks to handle these cases. This would also serve to avoid some of the hackyness of the rest of my solution.
I do feel as though I'm slightly out of my depth here since it seems like some fairly important changes need to happen to pick up those cases, so if anyone has any guidance that would be appreciated.
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.
I don't understand, sorry. I would expect that getMockModule
does no more than look for a __mocks__/file.js
next to the file.js
. Why does it matter where the dependent is, it's not being passed to getMockModule
?
@SimenB No worries, I've been sitting on this PR for a while so it makes sense. Sorry about that, and thanks for reminding me haha. I'll give this another look to see if I can get this resolved. |
ping 🙂 |
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.
Would be cool to get this clean contribution over the line - not sure I understand the unresolved comment, maybe a failing test case would help visualize it?
); | ||
|
||
if (resolvedMockDependency) { | ||
const dependencyMockDir = path.join( |
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.
Should probably also resolve
this like the one it's being compared with, in case of symlinks or something
// If we resolve a dependency, then look for a mock dependency | ||
// of the same name in that dependency's directory. | ||
resolvedMockDependency = this._resolver.getMockModule( | ||
path.dirname(resolvedDependency), |
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.
I don't understand, sorry. I would expect that getMockModule
does no more than look for a __mocks__/file.js
next to the file.js
. Why does it matter where the dependent is, it's not being passed to getMockModule
?
Merging this would make me sooooo happy... 😄 |
Seems there's still some unresolved questions. If someone else wants to create a PR based on this and take over further development that'd be great, we're definitely accepting a fix for this. |
Yeah, if someone else could open up a pr for this that would be great.
Apologies for any wasted effort.
…On Fri, Aug 21, 2020 at 1:16 PM Tim Seckinger ***@***.***> wrote:
Seems there's still some unresolved questions. If someone else wants to
create a PR based on this and take over further development that'd be
great, we're definitely accepting a fix for this.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8952 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACDAAU4MSCFSE3JANR37QQLSB3IZPANCNFSM4IWKSWNQ>
.
|
@petevdp no wasted effort at all, I think this is great work and pretty close to landing 🙂 |
Great! I'll look into this soon @SimenB |
Landed in #10713, 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
Fixes issue #8907
dependencyResolver.resolve does not take the mocks of functional dependencies into account, meaning that, as layed out in the above issue, changes to a mock will not register as an affecting change when resolving related tests.
My changes include the mocks of any dependencies found by the resolver, based on having the same name as a given dependency.
Test plan
Here's the output of running the dependency resolver tests after my changes:
I also tested this change against the example repo from the issue referenced above, and it appears to fix the problem.
This is my first contribution, so apologies for any missteps!