Skip to content
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.

Sign up for GitHub

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

Jump to bottom

add tests for named-asset-imports plugin #5575

Merged
merged 8 commits into from
Oct 31, 2018
Merged

add tests for named-asset-imports plugin #5575

merged 8 commits into from
Oct 31, 2018

Conversation

NShahri
Copy link
Contributor

@NShahri NShahri commented Oct 26, 2018

Some automation tests added to test named-asset-imports plugin

Hot to run

yarn test

OR

npm test

@NShahri NShahri mentioned this pull request Oct 26, 2018
Merged
@Timer Timer self-requested a review October 26, 2018 19:15
iansu
iansu suggested changes Oct 31, 2018
View reviewed changes
packages/babel-plugin-named-asset-import/.babelrc Outdated Show resolved Hide resolved
packages/babel-plugin-named-asset-import/index.spec.js Outdated Show resolved Hide resolved
packages/babel-plugin-named-asset-import/package.json Outdated Show resolved Hide resolved
@iansu
Copy link
Contributor

iansu commented Oct 31, 2018

This is great. I suggested a few changes and we'll need to get this passing on CI.

@NShahri
Copy link
Contributor Author

NShahri commented Oct 31, 2018 •
edited
Loading

Tests have been updated. please let me know if you think we need more tests to cover more cases. Thanks

@Timer
Copy link
Contributor

Timer commented Oct 31, 2018

Can we make sure these tests run as part of e2e? Just add it somewhere in tasks/e2e-simple.sh.

iansu
iansu reviewed Oct 31, 2018
View reviewed changes
packages/babel-plugin-named-asset-import/index.test.js
snapshot: false,
tests: {
defaultImport: {
code: 'import logo from "logo";',
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 reason these don't include the file name? Why not import logo from 'logo.svg';?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first 3 tests are testing cases when file extension does NOT match to plugin config (.svg in here),
and rest can cover tests when imported file extension matches to config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

svgDefaultImport covers default import of logo.svg

NShahri
NShahri commented Oct 31, 2018
View reviewed changes
tasks/e2e-simple.sh
@@ -118,6 +118,10 @@ cd packages/react-dev-utils/
yarn test
cd ../..

cd packages/babel-plugin-named-asset-import/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Timer tests run as part of e2e

Timer
Timer approved these changes Oct 31, 2018
View reviewed changes
Copy link
Contributor

@Timer Timer left a comment

Choose a reason for hiding this comment

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

LGTM, @iansu proceed with merge when you're happy.

@Timer Timer added this to the 2.1.1 milestone Oct 31, 2018
@iansu iansu merged commit 337541b into facebook:master Oct 31, 2018
@iansu
Copy link
Contributor

iansu commented Oct 31, 2018

Thanks!

@lock lock bot locked and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Timer Timer Timer approved these changes

@iansu iansu iansu approved these changes

Assignees
No one assigned
Labels
CLA Signed tag: internal
Projects
None yet
Milestone
2.1.1
Development

Successfully merging this pull request may close these issues.
4 participants
@NShahri @iansu @Timer @facebook-github-bot