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

Merged
merged 9 commits into from Jul 6, 2017

Conversation

@gziolo
Member

gziolo commented Jun 22, 2017

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
screen shot 2017-06-23 at 12 51 28

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

screen shot 2017-06-23 at 13 13 54

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

334 passing (295ms)
96 pending

real 0m16.125s
user 0m16.797s
sys 0m0.895s

Jest without cache

test npm run test-unit -- --no-cache

Test Suites: 2 skipped, 28 passed, 28 of 30 total
Tests: 96 skipped, 334 passed, 430 total
Snapshots: 0 total
Time: 9.363s
Ran all test suites.

real 0m13.121s
user 1m11.006s
sys 0m3.656s

Jest

test npm run test-unit

Test Suites: 2 skipped, 28 passed, 28 of 30 total
Tests: 96 skipped, 334 passed, 430 total
Snapshots: 0 total
Time: 3.484s
Ran all test suites.

real 0m5.961s
user 0m30.956s
sys 0m2.355s

TODO

  • Fix build/test/full-content.js test.
  • Replace Chai and Sinon with Jest matchers and spies.
  • Enable Eslint rule jest/valid-expect.
  • Remove Mocha? :trollface:
  • Update testing docs
@@ -86,7 +86,7 @@ function normalizeParsedBlocks( blocks ) {
} );
}
describe( 'full post content fixture', () => {
describe.skip( 'full post content fixture', () => {

This comment has been minimized.

@gziolo

gziolo Jun 23, 2017

Member

I still need to find a way to make this test suite pass.

This comment has been minimized.

@nylen

nylen Jun 29, 2017

Member

@gziolo This suite can probably be rewritten to use Jest snapshots. What is the current issue with it?

This comment has been minimized.

@gziolo

gziolo Jun 30, 2017

Member

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 ) {

This comment has been minimized.

@gziolo

gziolo Jun 23, 2017

Member

Jest doesn't provide such method.

if ( ! process.env.RUN_SLOW_TESTS ) {
return;
}
const maybeDescribe = process.env.RUN_SLOW_TESTS ? describe : describe.skip;

This comment has been minimized.

@gziolo

gziolo Jun 23, 2017

Member

You can't have test suite without any tests so it complains without skipping.

"**/utils/**/*.js"
],
"coveragePathIgnorePatterns": [
"<rootDir>/components/clipboard-button/index.js",

This comment has been minimized.

@gziolo

gziolo Jun 23, 2017

Member

Coverage errors on those 3 files. I didn't try to understand why.

This comment has been minimized.

@gziolo

gziolo Jul 6, 2017

Member

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

This comment has been minimized.

@gziolo

gziolo Jun 23, 2017

Member

I followed pegjs-loader from Webpack and it looks like it works properly :)

@@ -0,0 +1,21 @@
require( 'core-js/modules/es7.object.values' );

This comment has been minimized.

@gziolo

gziolo Jun 23, 2017

Member

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.

This comment has been minimized.

@nylen

nylen Jul 7, 2017

Member

This should probably be a code comment.

This comment has been minimized.

@gziolo

gziolo Jul 8, 2017

Member

Yes, will add in another PR.

global.before = global.beforeAll;
global.context = global.describe;
global.wp = {

This comment has been minimized.

@gziolo

gziolo Jun 23, 2017

Member

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() {

This comment has been minimized.

@gziolo

gziolo Jun 23, 2017

Member

It is copy and paste from the test above.

@gziolo gziolo force-pushed the try/jest branch from d9e3cd5 to cb91afa Jun 23, 2017

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Jun 23, 2017

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.

@ntwb

This comment has been minimized.

Member

ntwb commented Jul 2, 2017

Found this an interesting read, results were not what I expected:

https://medium.com/dailyjs/javascript-test-runners-benchmark-3a78d4117b4

p.s. I do like watch, cache, snapshots features of Jest FWIW

@gziolo

This comment has been minimized.

Member

gziolo commented Jul 2, 2017

Found this an interesting read, results were not what I expected

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.

@gziolo

This comment has been minimized.

Member

gziolo commented Jul 3, 2017

@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.

@gziolo gziolo force-pushed the try/jest branch 2 times, most recently from 0c3fde7 to e118d4d Jul 5, 2017

@gziolo

This comment has been minimized.

Member

gziolo commented Jul 6, 2017

Fixed build/test/full-content.js test which was the only blocker. It turned out I missed to include blocks import in the setup phase.

If we are fine with using Jest I'm more than happy to remove Mocha and update all references in documentation.

@gziolo gziolo force-pushed the try/jest branch from e118d4d to df73d90 Jul 6, 2017

.gitignore Outdated
gutenberg.pot
.idea

This comment has been minimized.

@ntwb

ntwb Jul 6, 2017

Member

IDE specific entries in .gitignore should not be in the repo, users should manage these locally in ~/.gitignore_global

See also https://core.trac.wordpress.org/ticket/34345

This comment has been minimized.

@gziolo

gziolo Jul 6, 2017

Member

Thanks for tip, this was bothering me since forever :)

@@ -59,12 +59,14 @@
"enzyme": "^2.8.2",
"eslint": "^3.17.1",
"eslint-config-wordpress": "^1.1.0",
"eslint-plugin-jest": "20.0.3",

This comment has been minimized.

@ntwb

ntwb Jul 6, 2017

Member

This should include a ~ for updating via minor semantic version releases.

This comment has been minimized.

@gziolo

gziolo Jul 6, 2017

Member

Updated

"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",

This comment has been minimized.

@ntwb

ntwb Jul 6, 2017

Member

This should include a ~ for updating via minor semantic version releases.

This comment has been minimized.

@gziolo

gziolo Jul 6, 2017

Member

I see ^ is used in other places, shouldn't it be used here, too?

This comment has been minimized.

@youknowriad

youknowriad Jul 6, 2017

Contributor

I think we should use ^ to include all non-blocking updates, I guess it's also the default when using npm install --save

This comment has been minimized.

@ntwb

ntwb Jul 6, 2017

Member

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 (~) in package.json e.g. "eslint": "~3.1.0" to guarantee the results of your builds."

This comment has been minimized.

@nylen

nylen Jul 6, 2017

Member

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.

This comment has been minimized.

@gziolo

gziolo Jul 6, 2017

Member

Updated

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Jul 6, 2017

Let's remove mocha in this PR, we don't want to maintain two runners IMO.

@gziolo gziolo force-pushed the try/jest branch from ab411ff to 89ed92b Jul 6, 2017

@gziolo gziolo force-pushed the try/jest branch from 89ed92b to 46478d0 Jul 6, 2017

@gziolo gziolo force-pushed the try/jest branch from 46478d0 to 7375464 Jul 6, 2017

@gziolo

This comment has been minimized.

Member

gziolo commented Jul 6, 2017

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:

  • Replace Chai and Sinon with Jest matchers and spies.
  • Enable Eslint rule jest/valid-expect and remove mocha Eslint env.
"mocha": true
},
"mocha": true,
"jest/globals": true

This comment has been minimized.

@nylen

nylen Jul 6, 2017

Member

This doesn't include before and after, which means we still need mocha: true? I'm a bit surprised by that.

This comment has been minimized.

@gziolo

gziolo Jul 7, 2017

Member

I know. I will fix it with jest-codemods soon.

This comment has been minimized.

@gziolo

gziolo Jul 7, 2017

Member

Done with #1788.

@nylen

This comment has been minimized.

Member

nylen commented Jul 6, 2017

Not having to wait for webpack builds to run the tests is a huge improvement. Let's try it.

@nylen nylen merged commit 1406fe8 into master Jul 6, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@nylen nylen deleted the try/jest branch Jul 6, 2017

@gziolo

This comment has been minimized.

Member

gziolo commented Jul 7, 2017

Full API migration from Mocha to Jest is ready to review #1788.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment