-
Notifications
You must be signed in to change notification settings - Fork 17
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
integrate jest #775
integrate jest #775
Conversation
package.json
Outdated
@@ -173,5 +177,25 @@ | |||
"@ungap/custom-elements-builtin": "^0.1.1", | |||
"document-register-element": "^1.12.0", | |||
"react": ">=0.14.0" | |||
}, | |||
"jest": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I prefer to keep configuration scoped in separate config files.
May we moved it jest.config.js
as described here: https://jestjs.io/docs/en/configuration.html
stack/tasks/bundle-demos.js
Outdated
@@ -43,7 +43,8 @@ async function buildComponents() { | |||
.then(result => result.css), | |||
}), | |||
babel({ | |||
runtimeHelpers: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By removing this completely we loose reuse of babel-helpers:
https://babeljs.io/docs/en/6.26.3/babel-plugin-transform-runtime
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is an interesting read about how babel's cli and helper optimisation is working:
https://brunoscopelliti.com/a-simple-babel-optimization-i-recently-learned/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately official docs are rather obscure:
https://www.npmjs.com/package/babel-plugin-external-helpers
stack/tasks/bundles/app.js
Outdated
const constants = require('../../constants'); | ||
const common = require('./_common'); | ||
const babel = require('rollup-plugin-babel'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should import dependencies before relative files
stack/tasks/bundles/app.js
Outdated
@@ -19,10 +18,11 @@ async function buildApp() { | |||
browser: true, | |||
preferBuiltins: false, | |||
}), | |||
ENV === constants.ENV.PROD ? uglify() : () => {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uglify should happen after transpilation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your PR, I really like jest.
Though I don't think it's the ideal test environment for custom elements.
May we discuss the drop of babel-runtime optimization.
stack/tasks/bundle-demos.js
Outdated
@@ -43,7 +43,8 @@ async function buildComponents() { | |||
.then(result => result.css), | |||
}), | |||
babel({ | |||
runtimeHelpers: true, | |||
plugins: ['external-helpers'], | |||
externalHelpers: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
further info at rollup-plugin-babel
If you do not wish the babel helpers to be included in your bundle at all (but instead reference the global
babelHelpers
object), you may set theexternalHelpers
option totrue
:
By default
externalHelpers
option is set tofalse
so babel helpers will be included in your bundle.
@AndyOGo what framework would u suggest instead? we were also thinking to try mocha |
@LucaMele |
@MKaHead ok lets stick to our original plan: Lets try Jest and Mocha/Karma etc and compare them. The idea of using Karma is actually really good. We can so test various browser locally. I would also see if u can use Karma togheter with Jest. So my suggestion is to evaluate JEST + KARMA + SNAPSHOT Capability VS Mocha + Sinon + Karma + Snapshot capability |
Agree. |
ok thanks @AndyOGo , i guess that will come up then in the evaluation |
So we are not the only team with that Jest problem of no browser environment. But only for headless chrome /~https://github.com/GoogleChrome/puppeteer |
@MKaHead also when runnning karma, when travis does the build then we can only test phantomjs. This could be a little issue. But anyway when running npm run release all the tests will done locally apart of Internet Explorer unless the releaser has a Windows PC which is very unlikley |
Agreed that's the other downside, CI/CD Pipeline support. Ideally we would have a tooling which allows us to:
All of that are different test environments. |
Jest Integration and necessary config changes
Changes proposed in this pull request:
regarding future Babel 7 and deprecated env key - add babelrc.js file, if babel 7 is integrated babelrc is obsolete
add jest, config, and jest runner script
add eslint jest recommented rules
remove babel external-helpers out of babel to fix issue, add it in rollup bundle proccess, but kicked out runtime helpers
add Documentation to Contribution file
Type of change
How Has This Been Tested?
Checklist: