Skip to content

fixed jest tests: added /website/ folder to jest ignore list #6170

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

Closed
wants to merge 1 commit into from

Conversation

bestander
Copy link
Contributor

If website has node_modules installed npm tests

Test plan (required)

cd website && npm install
cd .. && npm test

Before this fix we see this error:
Error: Failed to build DependencyGraph: @providesModule naming collision:
Duplicate module name: ViewportMetrics
Paths: react-native/node_modules/react/lib/ViewportMetrics.js collides with react-native/website/node_modules/react/lib/ViewportMetrics.js

@bestander
Copy link
Contributor Author

Don't merge it until Monday, giving a chance to merge a large diff with as little number of conflicts

@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @cpojer, @sahrens and @skevy to be potential reviewers.

@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Feb 26, 2016
@cpojer
Copy link
Contributor

cpojer commented Feb 26, 2016

Why do you have a website folder in jest-cli? Did you npm link the github repo? I guess I'm not opposed to this change but it isn't something you are going to run into as a user of react-native. It only happens when you work on react-native and jest. And you link it. This happened to me before, so it makes sense. I would recommend changing the pattern to node_modules/jest-cli/website/ however :)

@bestander
Copy link
Contributor Author

@cpojer, the website folder is in the root of repo.
It is used to generate the docs website and has its own package.json.
Did I screw up with leading /?
I just thought it is similar to .gitignore syntax

@facebook-github-bot
Copy link
Contributor

@bestander updated the pull request.

@cpojer
Copy link
Contributor

cpojer commented Feb 26, 2016

oh, you mean the react-native website. That makes sense, <rootDir>/website/ might be the better option. I was thinking about the jest-cli website (which is set-up similarly) – if you npm link jest for development, the same error might be thrown.

@bestander
Copy link
Contributor Author

👍

@facebook-github-bot
Copy link
Contributor

@bestander updated the pull request.

@bestander
Copy link
Contributor Author

@facebook-github-bot shipit

@facebook-github-bot
Copy link
Contributor

Thanks for importing. If you are an FB employee go to Phabricator to review.

@bestander
Copy link
Contributor Author

@facebook-github-bot shipit

@facebook-github-bot
Copy link
Contributor

Thanks for importing. If you are an FB employee go to Phabricator to review.

@bestander bestander force-pushed the jest-ignore-website branch from 667fd6e to 2193028 Compare March 3, 2016 14:43
@bestander
Copy link
Contributor Author

@facebook-github-bot shipit

@facebook-github-bot
Copy link
Contributor

@bestander updated the pull request.

@facebook-github-bot
Copy link
Contributor

Thanks for importing. If you are an FB employee go to Phabricator to review.

@mkonicek
Copy link
Contributor

mkonicek commented Mar 8, 2016

@bestander Looks legit 👍

@ghost ghost closed this in d24db57 Mar 10, 2016
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants