Migrate tests to jest runner #1336
Conversation
| const path = require('path'); | ||
|
|
||
| module.exports = { | ||
| testMatch: ['**/?(*-)+(test|tests).[tj]s?(x)'], |
There was a problem hiding this comment.
@chrisirhc @Xiot For now I put a regex to match our current test files, i.e foo-test.js and bar-tests.js
Many repo use the .test. convention, I don't have a strong opinion on this kind of naming, but I think this PR is the opportunity to rename all the test files if you want to
There was a problem hiding this comment.
Do we want to keep tests the separate top level tests folder, or use the side-by-side __tests__ folder. Or just the files completely side by side.
I tend to to prefer the top level test folder as I find it keeps things neater.
There was a problem hiding this comment.
I'm used to have the files side by side at it's easier for me to spot files without tests and to navigate between a file and its test.
But I also work on projects with a top folder and we have the coverage reports to track missing tests so it's not a big deal for me
| @@ -27,9 +27,9 @@ | |||
| "build": "yarn run clean && babel --root-mode upward src -d dist --copy-files && BABEL_ENV=es babel --root-mode upward src -d es --copy-files && node-sass src/main.scss dist/style.css --output-style compressed && yarn run build:browser", | |||
| "lint-styles": "stylelint src/styles/*.scss --syntax scss", | |||
| "test:windows": "babel-node --inspect ./tests/index.js", | |||
| "test": "node --inspect ./node_modules/.bin/babel-node --root-mode upward ./tests/index.js --only='../showcase/**/*.js,src,tests'", | |||
| "test": "NODE_ENV=development jest", | |||
There was a problem hiding this comment.
I had to add this because by default jest sets NODE_END to test, see this issue
We can also add a test key to babel.config.json but since all the plugins are copy-pasted between each key I didn't want to add one more, it would make it more likely to forget a plugin somewhere when we update the file.
There was a problem hiding this comment.
good call... I need to take another pass to clean up that babel config a little bit.
| "jsdom": "^9.9.1", | ||
| "node-sass": "^4.9.3", | ||
| "nyc": "^13.0.1", |
There was a problem hiding this comment.
According to nyc docs:
If you use jest or tap, you do not need to install nyc. Those runners already have the IstanbulJS libraries to provide coverage for you.
| @@ -297,21 +237,16 @@ test('Trigger all onParentMouse handlers on Series components', t => { | |||
| .at(0) | |||
| .simulate('touchmove'); | |||
|
|
|||
| t.end(); | |||
| expect(onParentMouseHandler).toHaveBeenCalledTimes(14); | |||
There was a problem hiding this comment.
imo we could improve this test but I wanted to migrate to jest by keeping the tests identical if possible and not bring any opinionated change
There was a problem hiding this comment.
What are your thoughts about moving towards
describe('component', () => {
it('should do something fun', () => {
...
expect(...)
}
}
I find that the grouping that the describe block provides is really nice.
It also allows us to easily set up [before|after]Each blocks.
The it('should naming is something that I got used to but I find that it helps with the readability of the test names.
| @@ -0,0 +1,18 @@ | |||
| /*eslint-env node*/ | |||
There was a problem hiding this comment.
Do we want to create a jest folder (sibling of src and tests) that will contain jest.setup.js and the transform.
It keeps the jest stuff a little more together.
There was a problem hiding this comment.
If I need to use the describe blocks I am afraid I will have to do it manually. It is quite tedious and can take a while so I think we shouldn't block this PR for that.
As for using it instead of test imo it can improve readability when the tests are named properly. If we are to keep current names it would give something like:
it('Axis: Showcase Example - Even more Custom axes')
which doesn't makes more sense than:
test(Axis: Showcase Example - Even more Custom axes')
Even if we use the describe block, it would still read it('Even more Custom axes')
So I guess my take is the same for both questions: we would need to update a lot of stuff manually so it can be done in a second PR in order to merge this one quicker. That way we can get rid of the really verbose logs we have with current tests asap
There was a problem hiding this comment.
ya.. I didn't mean for it to block this review, just a general question.
|
We should probably add the |
See #1328
The downside compared to the previous setup is that with jest it's not as easy to add custom message on assertions liek this one
If we want to keep the messages, I think we can try jest-expect-message but I didn't want to add a new dependency before discussing it