Skip to content
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

#1944 converting test suite from Mocha to Jest #1951

Merged
merged 6 commits into from
Sep 21, 2016

Conversation

HeinrichFilter
Copy link
Contributor

Just wanted to get something going where the tests are passing on my machine.

The replacement for expect.spyOn() is not ideal. It is quite verbose and the linter doesn't like that console is accessed. It might make sense to use jestjs/jest#1592 (comment) when it becomes available.

Please let me know what else I can do.

While looking into reduxjs#1944 I noticed that the examples in the documentation
still uses expect's createSpy() instead of Jest's fn() approach
Added Jest dependencies
Removed expect from files
Updated mocks to use `jest.fn()`
Current `expect.spyOn` replacement can be improved on
TODO: Fix linter not to complain about using `console`
@@ -304,7 +304,7 @@ import Header from '../../components/Header'

function setup() {
const props = {
addTodo: expect.createSpy()
addTodo: jest.fn(() => {})

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be wrong but I think

addTodo: jest.fn()

is sufficient for a noop.

Copy link
Contributor Author

@HeinrichFilter HeinrichFilter Sep 13, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Florian-R indeed, according to the documentation that should work.

But weirdly, when I run jest with a no-args jest.fn() in applyMiddleware.spec.js I get the following error which, to me, seems totally unrelated:

 FAIL  test/applyMiddleware.spec.js
  ● Test suite failed to run

    /Users/heinrichfilter/dev/src/redux/test/applyMiddleware.spec.js:1
    ({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,global,jest){import { createStore, applyMiddleware } from '../src/index';
                                                                                             ^^^^^^
    SyntaxError: Unexpected token import

      at transformAndBuildScript (../../../.nvm/v6.5.0/lib/node_modules/jest/node_modules/jest-runtime/build/transform.js:284:10)

I'm a bit confused by it and thought I'd leave it in until someone more knowledgeable can provide a definite answer.

Maybe someone else can run the tests and see if it gives get the same error?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I give this issue a quick try too and noticed this kind of errors (my branch is roughly the same than yours). Didn't had much time to dig further though.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@HeinrichFilter Played with your branch a bit, you might want to change your package.json scripts to

"test": "cross-env BABEL_ENV=commonjs jest",

otherwise you wont use the transform-es2015-modules-commonjs plugin.

Also, make sure to clear Jest cache on first launch, e.g.

npm run test -- --no-cache

Having just jest.fn() seems OK with this setup.

@HeinrichFilter
Copy link
Contributor Author

@Florian-R Thanks for the suggestions. I made the changes and updated the PR

@@ -105,6 +106,7 @@
"gitbook-cli": "^2.3.0",
"glob": "^6.0.4",
"isparta": "^4.0.0",
"jest": "^15.1.1",
"mocha": "^2.2.5",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can remove mocha, expect and isparta from the package.json now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ellbee

I removed the dependencies from package.json and updated test:cov to use jest

Updated test:cov command to use jest's --coverage option
@@ -2,7 +2,7 @@ import * as tt from 'typescript-definition-tester'


describe('TypeScript definitions', function () {
this.timeout(0)
//this.timeout(0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this just be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the Mocha homepage:

Use this.timeout(0) to disable timeouts all together

On Travis (and my machine) the new Jest test passes without hitting the default timeout. Thus, in my opinion, the line should be removed.

But if I'm missing anything, let me know.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's kill it then

@gaearon gaearon merged commit 0e73bcc into reduxjs:master Sep 21, 2016
@gaearon
Copy link
Contributor

gaearon commented Sep 21, 2016

Thanks!

seantcoyote pushed a commit to seantcoyote/redux that referenced this pull request Jan 14, 2018
* Fix Jest spy code in documentation

While looking into reduxjs#1944 I noticed that the examples in the documentation
still uses expect's createSpy() instead of Jest's fn() approach

* reduxjs#1944 Converting tests from Mocha to Jest

Added Jest dependencies
Removed expect from files
Updated mocks to use `jest.fn()`
Current `expect.spyOn` replacement can be improved on
TODO: Fix linter not to complain about using `console`

* Disable eslint no-console for console related tests

* Made changes suggested by @Florian-R that enable no-args jest.fn() calls

https://github.com/reactjs/redux/pull/1951/files/927bf45f1c7666e42ae8c4c390cc726a75cab903#r78733124

* Removed moch, expect and isparta

Updated test:cov command to use jest's --coverage option

* Update typescript.spec.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants