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: Refactor tests to use only Jest API #1788

Merged
merged 5 commits into from Jul 10, 2017

Conversation

@gziolo
Member

gziolo commented Jul 7, 2017

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

This comment has been minimized.

Member

gziolo commented Jul 7, 2017

@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

@gziolo gziolo referenced this pull request Jul 7, 2017

Merged

Tests: Setup Jest as an alternative test runner #1382

3 of 5 tasks complete
@nylen

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

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

This comment has been minimized.

@nylen

nylen Jul 7, 2017

Member

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

This comment has been minimized.

@gziolo

gziolo Jul 8, 2017

Member

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

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

This comment has been minimized.

@nylen

nylen Jul 7, 2017

Member

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

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

This comment has been minimized.

@nylen

nylen Jul 7, 2017

Member

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

/**
* Internal dependencies
*/

This comment has been minimized.

@nylen

nylen Jul 7, 2017

Member

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

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

This comment has been minimized.

@nylen

nylen Jul 7, 2017

Member

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

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

This comment has been minimized.

@nylen

nylen Jul 7, 2017

Member

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.

This comment has been minimized.

@ntwb

ntwb Jul 8, 2017

Member

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

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

} );
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( [

This comment has been minimized.

@nylen

nylen Jul 7, 2017

Member

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

This comment has been minimized.

@gziolo

gziolo Jul 8, 2017

Member

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

This comment has been minimized.

@nylen

nylen Jul 9, 2017

Member

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

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

This comment has been minimized.

@nylen

nylen Jul 7, 2017

Member

🎉 🎉 🎉

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

This comment has been minimized.

@nylen

nylen Jul 7, 2017

Member

🎉

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

This comment has been minimized.

@nylen

nylen Jul 7, 2017

Member

🎉

@nb

This comment has been minimized.

Member

nb commented Jul 8, 2017

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… 💣

@nb

nb approved these changes Jul 8, 2017

@gziolo gziolo force-pushed the update/tests-jest-api branch from 12f73f2 to 8c3cc5b Jul 8, 2017

@gziolo

This comment has been minimized.

Member

gziolo commented Jul 8, 2017

@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

This comment has been minimized.

coveralls commented Jul 8, 2017

Coverage Status

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

@nb

This comment has been minimized.

Member

nb commented Jul 9, 2017

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

This comment has been minimized.

Member

nylen commented Jul 9, 2017

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.

@nylen

nylen approved these changes Jul 9, 2017

@coveralls

This comment has been minimized.

coveralls commented Jul 9, 2017

Coverage Status

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

@gziolo

This comment has been minimized.

Member

gziolo commented Jul 10, 2017

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.

@gziolo gziolo merged commit d157b5e into master Jul 10, 2017

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 17.816%
Details

@gziolo gziolo deleted the update/tests-jest-api branch Jul 10, 2017

@nb

This comment has been minimized.

Member

nb commented Jul 10, 2017

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

This comment has been minimized.

Member

gziolo commented Jul 10, 2017

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!

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