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.

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

Add tests for snowpack babel plugin. #162

Merged
merged 6 commits into from
Jan 19, 2020

Conversation

yukukotani
Copy link
Contributor

@yukukotani yukukotani commented Jan 19, 2020

Resolve #155.

Add tests for babel-plugin in __tests__/babel-plugin.
I'm not sure about directory structure and naming. Please give me some advice!

Details

runner.js find test files that end with .test.json.
*.test.json requires three keys. options is the option for babel plugin, input is the code before transpiled, and output is the code after transpiled.

@@ -8,10 +8,10 @@ function getWebDependencyName(dep) {
return dep.replace(/\.js$/, '');
}

function getPackageVersion(package) {
function getPackageVersion(packageName) {
Copy link
Contributor Author

@yukukotani yukukotani Jan 19, 2020

Choose a reason for hiding this comment

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

package is reserved word (surprisingly!), so not working in strict mode.

@yukukotani yukukotani marked this pull request as ready for review January 19, 2020 11:23
Copy link
Owner

@FredKSchott FredKSchott left a comment

Choose a reason for hiding this comment

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

This is great! Thanks for tackling, @monchi. I'm having trouble leaving comments on a PR this big, so I'll just leave them here.


Renaming/moving the entire integration directory has caused a lot of noise, so I went ahead and moved that myself in master. If you rebase this PR against master now, you should see a much cleaner diff/PR. If that causes any merge conflicts in tests/integration for you, you should be able to ignore them / just use master.


Instead of building our own custom config/test data format, why don't we use the fixtures flow already supported by the util library? https://github.com/babel-utils/babel-plugin-tester#fixtures I'd rather just use their support/docs then having to manage our own data format as our tests grow.


Screen Shot 2020-01-19 at 9 40 33 AM

When I tested this locally, I saw that tests don't have names. We should find a way to grab the name from the options fixture. (probably supported automatically if we use their fixtures directory flow).


Instead of installing a real dependency ("hyperapp") I'd recommend using a mocked out "example-dep" package in this test directory:

// Manually create node_modules/example-dep/package.json + test.cjs + test.mjs files
{
  "name": "example-dep",
  "version": "1.2.3",
  "main": "test.cjs",
  "module": "test.mjs"
}

You can also get rid of the package.json & package-lock.json files in the babel-plugin directory.

@yukukotani
Copy link
Contributor Author

yukukotani commented Jan 19, 2020

I'm having trouble leaving comments on a PR this big, so I'll just leave them here.

Thank you for reviewing, maybe you can leave comments on each commit page like fdac30f!

so I went ahead and moved that myself in master.

👍

Instead of building our own custom config/test data format, why don't we use the fixtures flow already supported by the util library?

Wow I didn't notice fixtures! Applied in bc6d8e6.

When I tested this locally, I saw that tests don't have names.

Resolved by using fixtures.

I'd recommend using a mocked out "example-dep" package in this test directory

Sounds good! Applied in 48a135b.

Also thank you for inviting me as a collaborator!

@FredKSchott
Copy link
Owner

LGTM!

Also thank you for inviting me as a collaborator!

Of course, thank you for your help maintaining & improving Snowpack for everyone!

@FredKSchott FredKSchott merged commit 1172ad0 into FredKSchott:master Jan 19, 2020
@pika-ci
Copy link

pika-ci bot commented Jan 19, 2020

🚀 This PR has been merged! Once a new release is created, any changes will become available on npm. Until then, you can load and install it directly from the Pika CDN:

npm install https://cdn.pika.dev/snowpack/master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add tests for our Babel plugin
2 participants