-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Tests: Setup Jest as an alternative test runner #1382
Conversation
blocks/test/full-content.js
Outdated
@@ -86,7 +86,7 @@ function normalizeParsedBlocks( blocks ) { | |||
} ); | |||
} | |||
|
|||
describe( 'full post content fixture', () => { | |||
describe.skip( 'full post content fixture', () => { |
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.
I still need to find a way to make this test suite pass.
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.
@gziolo This suite can probably be rewritten to use Jest snapshots. What is the current issue with it?
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.
All of them fail, but I didn't even try to fix them. I just wanted to validate if Gutenberg can work with Jest without any changes in test files.
I started my parental leave this week so I might not have time to debug it in the upcoming days or weeks, but I'm happy to help with reviews using my mobile :)
// We need a high timeout here to accomodate Travis CI | ||
this.timeout( 10000 ); | ||
// We need a high timeout for Mocha here to accomodate Travis CI | ||
if ( this.timeout ) { |
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.
Jest doesn't provide such method.
if ( ! process.env.RUN_SLOW_TESTS ) { | ||
return; | ||
} | ||
const maybeDescribe = process.env.RUN_SLOW_TESTS ? describe : describe.skip; |
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.
You can't have test suite without any tests so it complains without skipping.
"**/utils/**/*.js" | ||
], | ||
"coveragePathIgnorePatterns": [ | ||
"<rootDir>/components/clipboard-button/index.js", |
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.
Coverage errors on those 3 files. I didn't try to understand why.
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.
It fails because nyc
or istanbul
doesn't like param destructuring in function definition that uses a default value ... Example:
function Notice( { status, content, onRemove = noop } ) { ... }
I think it needs to be fixed somewhere else. I can investigate it further later.
|
||
module.exports = { | ||
process( src ) { | ||
// Description of PEG.js options: https://github.com/pegjs/pegjs#javascript-api |
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.
I followed pegjs-loader
from Webpack and it looks like it works properly :)
@@ -0,0 +1,21 @@ | |||
require( 'core-js/modules/es7.object.values' ); |
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.
This is tricky part. I had to add it explicitly here. babel-jest
should do it instead, but apparently there is something happening that breaks it.
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.
This should probably be a code comment.
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.
Yes, will add in another PR.
test/setup-test-framework.js
Outdated
global.before = global.beforeAll; | ||
global.context = global.describe; | ||
|
||
global.wp = { |
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 need it to support wp.element.*
syntax. I should probably add comment in code, too :)
@@ -438,12 +438,6 @@ describe( 'FormTokenField', function() { | |||
expect( textInputNode.prop( 'value' ) ).to.equal( ' quux' ); | |||
} ); | |||
|
|||
it( 'should skip empty tokens at the beginning of a paste', function() { |
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.
It is copy and paste from the test above.
Copying my comments for our slack discussion here: Before introducing a new testing framework to Core, we need to consider the different options and Jest is a great test runner (maybe the best right now) 🙂 One of the issues we currently have with our mocha setup is that we build the tests using webpack (pegs-loader, sass-loader, wp global variables are all handled by webpack) before running them, this has some costs. We can’t run a single test without building the entire suite which is not very performant. This PR solves this issue 🎉 but the solution has some downsides: we need to “recreate” (or find a hack) the webpack loaders one by one. Also, this makes our test code and built code slightly different. I personally think the niceties that come with Jest surpass the downsides: Performance, coverage, snapshots. It would be great to have others' opinion. |
Found this an interesting read, results were not what I expected:
p.s. I do like watch, cache, snapshots features of Jest FWIW |
I saw that the other day, too. This benchmark is very interesting but I noticed that Jest is executed with default configuration, which means it needs to setup a fresh jsdom instance for every test suite. You can pick node test env instead which should drastically speed up things and will align with what Mocha and Jasmine offer out of the box. Jest's default setup is optimized for code that is run in the browser. I asked post's author to add Jest setup that uses node as test env. I'm looking forward to see how it would impact results. |
@ntwb I got confirmation form benchmark's author that Jest is 30% faster with test env set to node. See https://medium.com/@vitaliypotapov/thanks-for-suggestion-jest-is-really-30-faster-with-env-node-option-the-chart-4309ba8aba81. It still slower in this particular case though. |
0c3fde7
to
e118d4d
Compare
Fixed If we are fine with using Jest I'm more than happy to remove Mocha and update all references in documentation. |
.gitignore
Outdated
gutenberg.pot | ||
.idea |
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.
IDE specific entries in .gitignore
should not be in the repo, users should manage these locally in ~/.gitignore_global
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 tip, this was bothering me since forever :)
package.json
Outdated
@@ -59,12 +59,14 @@ | |||
"enzyme": "^2.8.2", | |||
"eslint": "^3.17.1", | |||
"eslint-config-wordpress": "^1.1.0", | |||
"eslint-plugin-jest": "20.0.3", |
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.
This should include a ~
for updating via minor semantic version releases.
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.
Updated
package.json
Outdated
"eslint-plugin-jsx-a11y": "^4.0.0", | ||
"eslint-plugin-react": "^6.10.3", | ||
"expose-loader": "^0.7.3", | ||
"extract-text-webpack-plugin": "^2.1.0", | ||
"gettext-parser": "^1.2.2", | ||
"glob": "^7.1.1", | ||
"jest": "20.0.4", |
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.
This should include a ~
for updating via minor semantic version releases.
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.
I see ^
is used in other places, shouldn't it be used here, too?
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.
I think we should use ^
to include all non-blocking updates, I guess it's also the default when using npm install --save
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.
It should be ~
, linters and for the most part rules will inherit the same semantic behaviour from the linter, in this case ESLint and by proxy it's shared plugins and shared configs:
Via ESLint Semantic Version Policy:
"According to our policy, any minor update may report more errors than the previous release (ex: from a bug fix). As such, we recommend using the tilde (~
) inpackage.json
e.g."eslint": "~3.1.0"
to guarantee the results of your builds."
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.
I don't follow:
- How this relates to Jest specifically
- Why this means we can't use
^
I think this is best left for a separate PR.
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.
Updated
Let's remove mocha in this PR, we don't want to maintain two runners IMO. |
I removed Mocha and updated most of its references. There are two cleanup tasks left which can be done in the follow up PR if you agree:
|
"mocha": true | ||
}, | ||
"mocha": true, | ||
"jest/globals": 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.
This doesn't include before
and after
, which means we still need mocha: true
? I'm a bit surprised by that.
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.
I know. I will fix it with jest-codemods soon.
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.
Done with #1788.
Not having to wait for webpack builds to run the tests is a huge improvement. Let's try it. |
Full API migration from Mocha to Jest is ready to review #1788. |
Work around for link selection.
This is a proof of concept for Jest integration with Gutenberg.
Wondering why Jest? See: https://gziolo.pl/2017/06/17/picking-jest-over-mocha/.
Sidenotes: It doesn't need Webpack processing. It still uses Chai and Sinon to make it possible to run with both Mocha and Jest and perform comparisons on one branch. I had to disable one test suite, because I didn't find a quick way to make it pass. It can be done later if we ever decide we replace Mocha with Jest.
Regular test run
npm run test-unit
With slow tests:
RUN_SLOW_TESTS=1 npm run test-unit
Only one test file (element/test/index.js):
npm run test-unit element/test/index.js
Code coverage
npm run test-unit:coverage
Watch mode
npm run test-unit:watch
By default it executes test for modified files only. You can change it using UI for subsequent runs.
IDE integration
Performance
Jest is configured to omit Webpack processing step. It makes it faster even when executed with all caches disabled. Jest shines on subsequent runs when it's able to take advantage of its caching mechanism.
Mocha
Before:
test npm run test-unit
Jest without cache
test npm run test-unit -- --no-cache
Jest
test npm run test-unit
TODO
build/test/full-content.js
test.jest/valid-expect
.