Skip to content

add tests for named-asset-imports plugin - #5575

Merged
iansu merged 8 commits into
react:masterfrom
NShahri:named-asset-plugin-tests
Oct 31, 2018
Merged

add tests for named-asset-imports plugin#5575
iansu merged 8 commits into
react:masterfrom
NShahri:named-asset-plugin-tests

Conversation

@NShahri

@NShahriNShahri commented Oct 26, 2018

Copy link
Copy Markdown
Contributor

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

Hot to run

yarn test

OR

npm test

@NShahriNShahri 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
Comment threadpackages/babel-plugin-named-asset-import/.babelrc Outdated
Comment threadpackages/babel-plugin-named-asset-import/index.spec.js Outdated
Comment threadpackages/babel-plugin-named-asset-import/package.json Outdated
@iansu

iansu commented Oct 31, 2018

Copy link
Copy Markdown
Contributor

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

Timer
Timer suggested changes Oct 31, 2018
View reviewed changes
Comment threadpackages/babel-plugin-named-asset-import/.npmignore Outdated
Comment threadpackages/babel-plugin-named-asset-import/package.json Outdated
@NShahri

NShahri commented Oct 31, 2018•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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

@Timer

Timer commented Oct 31, 2018

Copy link
Copy Markdown
Contributor

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
Comment threadpackages/babel-plugin-named-asset-import/index.test.js
snapshot: false,
tests: {
defaultImport: {
code: 'import logo from "logo";',

@iansuiansuOct 31, 2018

Copy link
Copy Markdown
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';?

@NShahriNShahriOct 31, 2018

Copy link
Copy Markdown
ContributorAuthor

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

@NShahriNShahriOct 31, 2018

Copy link
Copy Markdown
ContributorAuthor

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
Comment threadtasks/e2e-simple.sh
yarn test
cd ../..

cd packages/babel-plugin-named-asset-import/

@NShahriNShahriOct 31, 2018

Copy link
Copy Markdown
ContributorAuthor

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

@TimerTimer left a comment

Copy link
Copy Markdown
Contributor

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.

@TimerTimer added this to the 2.1.1 milestone Oct 31, 2018
@iansu
iansu merged commit 337541b into react:masterOct 31, 2018
@iansu

iansu commented Oct 31, 2018

Copy link
Copy Markdown
Contributor

Thanks!

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

Reviewers

2 more reviewers
@iansuiansuiansu approved these changes
@TimerTimerTimer approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CLA Signedtag: 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