Skip to content

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

Merged
SimenB merged 16 commits intojestjs:masterfrom
brapifra:brapifra/resolve-mock-dependencies
Oct 27, 2020
Merged

fix(jest-resolve-dependencies): resolve mocks as dependencies#10713
SimenB merged 16 commits intojestjs:masterfrom
brapifra:brapifra/resolve-mock-dependencies

Conversation

@brapifra
Copy link
Copy Markdown
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
Comment thread packages/jest-resolve-dependencies/src/__tests__/dependency_resolver.test.ts Outdated
Comment thread packages/jest-resolve-dependencies/src/index.ts Outdated
Comment thread packages/jest-resolve-dependencies/src/index.ts
@brapifra brapifra requested a review from SimenB October 26, 2020 15:35
Copy link
Copy Markdown
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
Copy Markdown
Collaborator

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
Copy Markdown
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
Copy Markdown
Collaborator

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
Copy Markdown

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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants