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: Try to migrate server and integration tests to Jest #14364

Merged
merged 14 commits into from Sep 29, 2017

Conversation

@gziolo
Member

gziolo commented May 23, 2017

This PR migrates server and integration tests from Mocha to Jest.

Integration tests

npm run test-integration
npm run test-integration:ci - version tailored for CircleCI
npm run test-integration:watch - run with watch mode enabled, by default it executes tests only for modified files

I skipped code coverage for integration tests, because it isn't important at all in this context.

Server tests

npm run test-server
npm run test-server:ci - version tailored for CircleCI
npm run test-server:coverage - run with code coverage enabled
npm run test-server:watch - run with watch mode enabled, by default it executes tests only for modified files

Summary

This also breaks other test runners because I modified the helpers from the /test/ folder to make it work with Jest. Mocha uses before, but Jest wants beforeAll :(

I followed this awesome article to make test helpers work with Mocha and Jest at the same time.

Code coverage result for /server/ folder

screen shot 2017-06-13 at 11 07 53

TODO

  • Make skipped server test suites pass
  • Make client tests work with updated test helpers
  • Find a way to use mocha-junit-reporter like reporter on Circle CI
  • Find a way to distribute server tests between container on Circle CI
  • Add support for code coverage
  • Migrate integration tests to Jest
  • Distribute integration tests between CircleCi nodes

@gziolo gziolo self-assigned this May 23, 2017

@gziolo gziolo requested review from markryall, coderkevin, gwwar and samouri May 23, 2017

@matticbot

This comment has been minimized.

matticbot commented May 23, 2017

@markryall

This comment has been minimized.

Contributor

markryall commented May 25, 2017

Might it be possible to do this more cautiously by introducing jest alongside mocha and then incrementally migrate?

Perhaps we can use a mutually exclusive pattern for test filenames (like 'spec' instead of 'test')

We could introduce and discuss some new tests (specs!) that demonstrate use of jest features (such as snapshots), communicate these more broadly and then start the process for migration once the idea has momentum.

Codemods might then be used to get us the rest of the way.

@gziolo gziolo force-pushed the try/jest-server branch 2 times, most recently from 1669c2f to 3c0d65c May 30, 2017

} );
it( 'should return components stats only', done => {
getComponentsUsageStats( ( err, res ) => {

This comment has been minimized.

@gziolo

gziolo May 30, 2017

Member

I'm wondering how it was passing before ...

It had empty body in the identical call in the previous test which calls the same function:

expect( res.body ).not.to.be.empty;

@gziolo gziolo force-pushed the try/jest-server branch 3 times, most recently from f9ec6bd to 8bba761 May 30, 2017

circle.yml Outdated
@@ -27,8 +27,7 @@ test:
- server/**/*.jsx
- MOCHA_FILE=./test-results-client.xml npm run test-client -- -R mocha-junit-reporter -t $CIRCLE_NODE_TOTAL -i $CIRCLE_NODE_INDEX:
parallel: true
- MOCHA_FILE=./test-results-server.xml npm run test-server -- -R mocha-junit-reporter -t $CIRCLE_NODE_TOTAL -i $CIRCLE_NODE_INDEX:
parallel: true
- npm run test-server:ci

This comment has been minimized.

@gziolo

gziolo May 30, 2017

Member

With this line tests are executed only on node 0, which isn't what I would expect. This needs to be further investigated.

@gziolo

This comment has been minimized.

Member

gziolo commented May 30, 2017

@markryall thanks for sharing good ideas 👍

Might it be possible to do this more cautiously by introducing jest alongside mocha and then incrementally migrate?

I stumbled upon this article https://medium.com/@RubenOostinga/combining-chai-and-jest-matchers-d12d1ffd0303 this week. It helped me to alias Mocha specific lifecycle methods to make them work with Jest. I hope it allows to use Mocha syntax and Jest runner with the client side code, too :)

Perhaps we can use a mutually exclusive pattern for test filenames (like 'spec' instead of 'test')

We could introduce and discuss some new tests (specs!) that demonstrate use of jest features (such as snapshots), communicate these more broadly and then start the process for migration once the idea has momentum.

I don't like the idea of having two different file extensions, because it only makes everything more complicated. I like a lot the rest of suggestion though. It would be nice to be able to run the same tests with Mocha and Jest at the same time, to validate the transition. I'm still not sure if Jest is going to be faster because it initiates testing env for every test suite to ensure they are isolated. It runs tests concurrently, which might make it perform better locally, but it might be no go on Circle CI.

Codemods might then be used to get us the rest of the way.

Yes, they are very granular and have interactive mode. So you can migrate everything step by step.

@markryall

This comment has been minimized.

Contributor

markryall commented May 31, 2017

@gziolo that's awesome - if we can just make existing mocha tests work with jest using this aliasing trick then that certainly makes the transition easier.

We'd still have to make the before vs beforeAll change everywhere wouldn't we? That could also probably be handled with some test config. Maybe the same for 'context'.

I'd had performance issues with jest when using it a couple of years ago but we had automocking enabled. I think the useMockery approach we already have for mocks is superior (although less succinct) - jest mocks seem needlessly complicated to me.

return Object.assign( chaiMatchers, originalMatchers );
};
// Make sure we can share test helpers between Mocha and Jest

This comment has been minimized.

@gziolo

gziolo May 31, 2017

Member

We'd still have to make the before vs beforeAll change everywhere wouldn't we? That could also probably be handled with some test config. Maybe the same for 'context'.

@markryall this is enough to make Jest work with Mocha helpers. We can add the same for context :)

@gziolo

This comment has been minimized.

Member

gziolo commented May 31, 2017

I'd had performance issues with jest when using it a couple of years ago but we had automocking enabled. I think the useMockery approach we already have for mocks is superior (although less succinct) - jest mocks seem needlessly complicated to me.

Fortunately they disabled automocking last year and introduced several other performance improvements.

We'd still have to make the before vs beforeAll change everywhere wouldn't we? That could also probably be handled with some test config. Maybe the same for 'context'.

There is also a codemod that is able to rename all before, after and context occurrences with what Jest likes.

@gziolo

This comment has been minimized.

Member

gziolo commented May 31, 2017

@blowery parallel and files in the CircleCI config file is going to work perfectly fine with Jest. Even if their version of glob fails and provides too many files, like we encountered it in the past, Jest will filter out all tests that don't match its own glob based matcher. 👍

@gziolo gziolo force-pushed the try/jest-server branch from 60772ab to e1a7c99 May 31, 2017

module.exports = Object.assign(
{},
{
testResultsProcessor: './node_modules/jest-junit-reporter',

This comment has been minimized.

@gziolo

gziolo Jun 1, 2017

Member

It was quite easy to integrate a reporter that is going to play nicely with Circle CI 🎉

@gziolo

This comment has been minimized.

Member

gziolo commented Sep 25, 2017

@gwwar I rebased with master, updated Jest to the latest version and fixed some new issues. Can you review the last 3 commits before this gets merged? :)

@gwwar

This comment has been minimized.

Member

gwwar commented Sep 25, 2017

@nb @gziolo are we good with moving forward with Jest again? If so, should we wait until they actually publish a version without the patent clause?

@gziolo

This comment has been minimized.

Member

gziolo commented Sep 26, 2017

We can wait, it may take a couple of days until they update repository. The patent clause was the only blocker. As far as I know, people are very excited about Jest.

@gziolo gziolo force-pushed the try/jest-server branch from a80eab1 to c01624e Sep 27, 2017

@gziolo

This comment has been minimized.

Member

gziolo commented Sep 27, 2017

Okey, it is now rebased and updated to use Jest with MIT license 🎉

@gziolo gziolo force-pushed the try/jest-server branch 2 times, most recently from 0daac6a to 04a8861 Sep 28, 2017

@gziolo

This comment has been minimized.

Member

gziolo commented Sep 28, 2017

@blowery @aduth it's about time to shed a tear, our custom test runner code for Mocha gets removed with 04a8861 :)

@@ -234,8 +234,6 @@
"test-server:ci": "cross-env TEST_REPORT_FILENAME=./test-results-server.xml jest -c=test/server/jest.config.ci.js -w=2",
"test-server:coverage": "npm run -s test-server -- --coverage",
"test-server:watch": "npm run -s test-server -- --watch",
"test-test": "cross-env NODE_ENV=test NODE_PATH=test:client:client/extensions TEST_ROOT=test node test/runner.js",

This comment has been minimized.

@blowery

blowery Sep 28, 2017

Contributor

yaaaaay

@@ -269,9 +267,6 @@
"jscodeshift": "0.3.30",
"md5-file": "3.1.0",
"mixedindentlint": "1.2.0",
"mocha": "3.1.0",
"mocha-junit-reporter": "1.12.0",
"mockery": "1.7.0",

This comment has been minimized.

@blowery

blowery Sep 28, 2017

Contributor

yay

@gziolo gziolo force-pushed the try/jest-server branch from 04a8861 to 11be7ed Sep 29, 2017

@gziolo gziolo merged commit 2459c50 into master Sep 29, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@gziolo gziolo deleted the try/jest-server branch Sep 29, 2017

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