Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(jest-resolve-dependencies): resolve mocks as dependencies #10713

Merged
merged 16 commits into from
Oct 27, 2020

Conversation

brapifra
Copy link
Contributor

@brapifra brapifra commented Oct 26, 2020

Summary

This is the continuation of #8952, which solves #8907

Test plan

I've used the unit tests from the original PR. In one of them, I've added a new assertion to check that mocks of node modules are also included as dependencies.

@SimenB SimenB requested a review from jeysal October 26, 2020 15:13
@brapifra brapifra requested a review from SimenB October 26, 2020 15:35
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll defer to @jeysal here 👍

resolvedMockDependency = path.resolve(resolvedMockDependency);

// make sure mock is in the correct directory
if (dependencyMockDir === path.dirname(resolvedMockDependency)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a test that fails if this acc.push is unconditional?

Copy link
Contributor Author

@brapifra brapifra Oct 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! Both tests fail if this gets removed. Removing it would basically add an extra non-valid dependency to the list.

@jeysal
Copy link
Contributor

jeysal commented Oct 26, 2020

Just one question about a potentially missing test case (or maybe I missed how the existing tests would already cover it), other than that LGTM

@jeysal jeysal changed the title Brapifra/resolve mock dependencies fix(jest-resolve-dependencies): resolve mocks as dependencies Oct 26, 2020
This was referenced Mar 12, 2021
@github-actions
Copy link

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants