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

test: add a test whether build files are es5 or not #779

Merged
merged 6 commits into from Jun 8, 2020

Conversation

koba04
Copy link
Contributor

@koba04 koba04 commented May 14, 2020

This adds a test to validate that build files are ES5 compatible.
I've added the test as a script file, not a unit test. It doesn't make sense to be added as a unit test because the test takes about 30 seconds on my local laptop.

The script runs on CI, and you can run the script as yarn test:build-assets if you want.

@koba04 koba04 requested a review from a team as a code owner May 14, 2020 10:12
@koba04 koba04 requested review from diescake and removed request for a team May 14, 2020 10:12
@reg-suit
Copy link

reg-suit bot commented May 14, 2020

✨✨ That's perfect, there is no visual difference! ✨✨

Check out the report here.

@netlify
Copy link

netlify bot commented May 14, 2020

Deploy preview for smarthr-ui ready!

Built with commit 1da1d1e

https://deploy-preview-779--smarthr-ui.netlify.app

@koba04
Copy link
Contributor Author

koba04 commented May 14, 2020

https://circleci.com/gh/kufu/smarthr-ui/5960 is the example of failing on CI

@koba04 koba04 force-pushed the test-build-assets branch 2 times, most recently from f308c8d to 849046b Compare May 14, 2020 16:51
@@ -41,6 +42,7 @@
"husky": "^4.2.5",
"jest": "^25.5.4",
"lint-staged": "^10.2.2",
"memory-fs": "^0.5.0",
Copy link
Contributor Author

@koba04 koba04 May 14, 2020

Choose a reason for hiding this comment

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

memory-fs is already deprecated. We can use memfs instead, but an error occurred at an internal of memfs when we use it, so I use memory-fs temporarily. Here is the log that I used memfs.
https://circleci.com/gh/kufu/smarthr-ui/5966

I'll investigate this later.

@koba04
Copy link
Contributor Author

koba04 commented May 14, 2020

npm seems to be down... So I'll retry after npm has recovered.

@koba04
Copy link
Contributor Author

koba04 commented May 15, 2020

I've re-run the CI and have passed it now.

@diescake diescake self-assigned this May 15, 2020
@diescake
Copy link
Contributor

@koba04
I'm really sorry for my late review. 🙇

It works fine on my local PC. When I've added query-string which doesn't support IE11 (not compatible ES5), the scripts have detected and printed error correctly.

Great job !! 🎉

diescake
diescake previously approved these changes May 21, 2020
@diescake
Copy link
Contributor

Would you rebase your branch to resolve the conflict ?

@koba04
Copy link
Contributor Author

koba04 commented May 21, 2020

@diescake I've fixed the merge conflict, but yarn audit seems to be failed because of https://www.npmjs.com/advisories/1219😢

@diescake
Copy link
Contributor

kieeeehhhhh 😭

@koba04
Copy link
Contributor Author

koba04 commented Jun 6, 2020

@diescake I've fixed the merge conflict.

diescake
diescake previously approved these changes Jun 6, 2020
Copy link
Contributor

@diescake diescake left a comment

Choose a reason for hiding this comment

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

LGTM !! 🎉 🎉

@moonstruckdrops moonstruckdrops merged commit 43fe9fb into master Jun 8, 2020
@moonstruckdrops moonstruckdrops deleted the test-build-assets branch June 8, 2020 02:32
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.

None yet

3 participants