Tests: Refactor tests to use only Jest API #1788

gziolo · 2017-07-07T16:34:51Z

This is a follow up for #1382. This PR performs all remaining refactorings to get rid of Mocha specific code:

Sinon test doubles
Sinon fake timers
Chai assertions
Chai lifecycle methods

This change makes test execution a bit faster.

Before

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

After

real 0m4.529s
user 0m22.167s
sys 0m1.724s

Testing

Make sure npm test passes.

gziolo · 2017-07-07T16:41:52Z

@nb This concludes Mocha to Jest migration for Gutenberg! This PR gives an overview how much code would be updated in the context of Calypso. Unfortunately sinon changes aren't covered with jest-codemods. Maybe there are other codemods, but to be honest it was quite easy to change manually. Most of the changes were applied with the combination:

# jest-codemods <file-path>
# npm run lint -- --fix space-in-parens

nylen

Some minor revisions needed but overall I'm a fan. Can we now also remove mocha: true from the eslint config?

nylen · 2017-07-07T22:15:26Z

blocks/api/test/serializer.js

-
-/**
- * WordPress dependencies
- */
 import { createElement, Component } from 'element';


This section should now be titled WordPress dependencies, not External dependencies.

I see. jest-codemods wasn't smart enough to use those conventions :)

nylen · 2017-07-07T22:15:45Z

blocks/library/freeform/test/naiveCss2Jsx.js

-import { expect } from 'chai';
-/**
- * Internal dependencies
- */
 import { naiveCss2Jsx } from '../format-list';


This section should now be titled Internal dependencies, not External dependencies.

nylen · 2017-07-07T22:16:09Z

editor/block-mover/test/mover-label.js

-
-/**
- * Internal dependencies
- */
 import { getBlockMoverLabel, getMultiBlockMoverLabel } from '../mover-label';


This section should now be titled Internal dependencies, not External dependencies.

nylen · 2017-07-07T22:16:31Z

editor/test/actions.js

-
-/**
- * Internal dependencies
- */


This section should now be titled Internal dependencies, not External dependencies.

nylen · 2017-07-07T22:16:41Z

editor/utils/test/dom.js

-
-/**
- * Internal dependencies
- */
 import { isEditableElement } from '../dom';


This section should now be titled Internal dependencies, not External dependencies.

nylen · 2017-07-07T22:30:31Z

editor/block-mover/test/mover-label.js

-			);
+			) ).toBe(
+                `Block "${ type }" is at the beginning of the content and can’t be moved up`
+            );


The new indentation here looks wrong. In fact, I would expect this to cause a lint error and we should investigate why it isn't.

Both lines 22 & 23 here are indented with spaces and not tabs

As to why ESLint is missing that I've no idea /shrug

nylen · 2017-07-07T22:34:22Z

editor/test/state.js

 		} );

 		it( 'should have expected reducer keys', () => {
 			const store = createReduxStore();
 			const state = store.getState();

-			expect( Object.keys( state ) ).to.have.members( [
+			expect( Object.keys( state ) ).toEqual( expect.arrayContaining( [


Not a critique for this PR, but I'm not a huge fan of this syntax.

I would blame jest-codemod here. I'm sure we can refactor later using expect.objectContaining.

The toEqual( expect.someWeirdObject syntax in general is strange. Again not something we can really change here.

nylen · 2017-07-07T22:34:50Z

package.json

@@ -77,9 +75,6 @@
    "react-markdown": "^2.5.0",
    "react-test-renderer": "^15.5.4",
    "sass-loader": "^6.0.3",
-    "sinon": "^2.1.0",
-    "sinon-chai": "^2.9.0",
-    "sinon-test": "^1.0.2",


🎉 🎉 🎉

nylen · 2017-07-07T22:35:02Z

package.json

@@ -51,11 +51,9 @@
    "babel-plugin-transform-runtime": "^6.23.0",
    "babel-preset-env": "^1.4.0",
    "babel-traverse": "^6.24.1",
-    "chai": "^3.5.0",


nylen · 2017-07-07T22:35:09Z

package.json

    "concurrently": "^3.4.0",
    "cross-env": "^3.2.4",
    "deep-freeze": "0.0.1",
-    "dirty-chai": "^1.2.2",


nb · 2017-07-08T00:43:32Z

Great work, @gziolo. The sinon changes don't worry me much.

I have a feeling we will get the most benefits from jest if we start using snapshots for component/integration tests, any chance we can try this in Gutenberg? Maintaining all of those fixtures… 💣

It was as easy as executing jest-codemods and eslint --fix space-in-parens.

…re detailed info

gziolo · 2017-07-08T15:53:02Z

@nylen Thanks for catching all issues introduced with jest-codemods run. I fixed all of them. I also added try / catch clause with 8c3cc5b to make sure we display more user friendly message using removed debug like:

format( 'File '%s.parsed.json' does not match expected value', f )

Let me know if that works for you until Jest adds a better way to do it.

I guess the best way would be to refactor this code to use snapshots instead. We can do it in the follow up PR.

coveralls · 2017-07-08T15:56:43Z

Coverage remained the same at 17.816% when pulling 8c3cc5b on update/tests-jest-api into b450568 on master.

nb · 2017-07-09T08:53:25Z

I guess the best way would be to refactor this code to use snapshots instead. We can do it in the follow up PR.

What’s your thinking about what tests should use snapshots? A lot of the tests touched in this PR should better stay the way they are. Converting component and fixture tests to snapshots would be awesome, though. Agreed that a separate PR is better.

nylen · 2017-07-09T16:31:13Z

Let me know if that works for you until Jest adds a better way to do it.

In 4e481fe I've also preserved the original error message which contains the actual+expected values and the differences between them.

Thanks for working on this; it's looking good to me.

coveralls · 2017-07-09T16:36:34Z

Coverage remained the same at 17.816% when pulling 4e481fe on update/tests-jest-api into b450568 on master.

gziolo · 2017-07-10T06:46:53Z

In 4e481fe I've also preserved the original error message which contains the actual+expected values and the differences between them.

Yes, it makes perfect sense. 👍

What’s your thinking about what tests should use snapshots? A lot of the tests touched in this PR should better stay the way they are. Converting component and fixture tests to snapshots would be awesome, though. Agreed that a separate PR is better.

@nb I rather consider snapshot testing as complementary. When you develop component, you should also use TDD techniques and introduce snapshot based tests once you have something working to make sure it doesn't change later. Definitely tests where you create tons of fixtures are perfect match to use snapshots, too.

nb · 2017-07-10T10:03:18Z

I rather consider snapshot testing as complementary.

This sounds like too many tests – why can't we use snapshots to TDD components? We should start with some small output and then build up.

gziolo · 2017-07-10T17:00:55Z

This sounds like too many tests – why can't we use snapshots to TDD components? We should start with some small output and then build up.

Let's see how it goes. I don't have any experience with snapshots so I'm only blindly predicting :D
I opened #1841 which is a perfect use case for snapshots!

Release v1.21.0

gziolo added [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement. labels Jul 7, 2017

gziolo self-assigned this Jul 7, 2017

gziolo requested review from nylen, youknowriad, aduth and nb July 7, 2017 16:34

gziolo mentioned this pull request Jul 7, 2017

Tests: Setup Jest as an alternative test runner #1382

Merged

5 tasks

nylen requested changes Jul 7, 2017

View reviewed changes

nb approved these changes Jul 8, 2017

View reviewed changes

gziolo added 4 commits July 8, 2017 17:49

Tests: Update tests to use Jest API

e4941a4

It was as easy as executing jest-codemods and eslint --fix space-in-parens.

Tests: Refactor tests to use Jest test doubles instead of Sinon

b47a3e3

Tests: Fix all issues introduced when executing jest-codemods

3fff861

Tests: Wrap expectations in blocks tests with try / catch to print mo…

8c3cc5b

…re detailed info

gziolo force-pushed the update/tests-jest-api branch from 12f73f2 to 8c3cc5b Compare July 8, 2017 15:50

Preserve original error details when re-throwing

4e481fe

nylen approved these changes Jul 9, 2017

View reviewed changes

gziolo merged commit d157b5e into master Jul 10, 2017

gziolo deleted the update/tests-jest-api branch July 10, 2017 06:47

nylen mentioned this pull request Jul 24, 2017

Tests: Update full post content tests to use snapshots #1841

Closed

gziolo mentioned this pull request Aug 22, 2017

Tests: Try to migrate server and integration tests to Jest Automattic/wp-calypso#14364

Merged

7 tasks

skovhus mentioned this pull request Aug 22, 2017

Support for Sinon skovhus/jest-codemods#68

Closed

Tug pushed a commit that referenced this pull request Feb 28, 2020

Merge pull request #1788 from wordpress-mobile/release/1.21.0

beff3d2

Release v1.21.0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tests: Refactor tests to use only Jest API #1788

Tests: Refactor tests to use only Jest API #1788

gziolo commented Jul 7, 2017 •

edited

Loading

gziolo commented Jul 7, 2017 •

edited

Loading

nylen left a comment

nylen Jul 7, 2017

gziolo Jul 8, 2017

nylen Jul 7, 2017

nylen Jul 7, 2017

nylen Jul 7, 2017

nylen Jul 7, 2017

nylen Jul 7, 2017

ntwb Jul 8, 2017

nylen Jul 7, 2017

gziolo Jul 8, 2017 •

edited

Loading

nylen Jul 9, 2017

nylen Jul 7, 2017

nylen Jul 7, 2017

nylen Jul 7, 2017

nb commented Jul 8, 2017

gziolo commented Jul 8, 2017 •

edited

Loading

coveralls commented Jul 8, 2017

nb commented Jul 9, 2017 •

edited

Loading

nylen commented Jul 9, 2017

coveralls commented Jul 9, 2017

gziolo commented Jul 10, 2017

nb commented Jul 10, 2017

gziolo commented Jul 10, 2017

Tests: Refactor tests to use only Jest API #1788

Tests: Refactor tests to use only Jest API #1788

Conversation

gziolo commented Jul 7, 2017 • edited Loading

Before

After

Testing

gziolo commented Jul 7, 2017 • edited Loading

nylen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo Jul 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nb commented Jul 8, 2017

gziolo commented Jul 8, 2017 • edited Loading

coveralls commented Jul 8, 2017

nb commented Jul 9, 2017 • edited Loading

nylen commented Jul 9, 2017

coveralls commented Jul 9, 2017

gziolo commented Jul 10, 2017

nb commented Jul 10, 2017

gziolo commented Jul 10, 2017

gziolo commented Jul 7, 2017 •

edited

Loading

gziolo commented Jul 7, 2017 •

edited

Loading

gziolo Jul 8, 2017 •

edited

Loading

gziolo commented Jul 8, 2017 •

edited

Loading

nb commented Jul 9, 2017 •

edited

Loading