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

Setup unit test with jest, ts-jest and enzyme #17

Merged
merged 11 commits into from
Mar 13, 2019
Merged

Conversation

chenesan
Copy link
Contributor

@chenesan chenesan commented Mar 7, 2019

Purpose

Add unit tests script with jest and typescript.

Change

  • Add test and test:watch scripts in root directory and every single package.

  • Add jest, ts-jest, enzyme, enzyme-adapter-react-16 dependency.

  • Replace lodash-es with lodash.

    It's possible to transpile single es module with { transformIgnorePatterns: ["node_modules/(?!(lodash-es)/)"] } in jest config (See the option doc and the trick), but it's deadly slow (~300s) even there's only one test. I'm sure with the option it only transpile lodash-es and not really sure what makes it so slow, but I decide to directly use lodash by import xxx from 'lodash/xxx' so we don't worry about bundle size.

  • Add a sample test of ResponsiveLayer

Risk

May not work in others' machine (should work I think). I hope everyone try to run it in their own machine and try if (1) yarn test passed (2) yarn prepublish successfully build with no error (except the HoverLayer one which should be fixed in #14.

Todos

None.

@chenesan chenesan self-assigned this Mar 7, 2019
@@ -0,0 +1,10 @@
module.exports = {
preset: 'ts-jest',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See usage in the ts-jest document

@@ -3,6 +3,7 @@
"baseUrl": "./packages",
"outDir": "lib",
"module": "es6",
"esModuleInterop": true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a must-added option or the jest will treat default import as undefined :-( See this issue

jest.config.js Outdated
@@ -0,0 +1,3 @@
module.exports = {
projects: ['<rootDir>/packages/*'],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This leverage jest project config to run tests in different projects

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice settings. I think this could be combined into the /config folder, and keep just one /config/jest.config.js file.

@@ -3,5 +3,9 @@
"compilerOptions": {
"module": "commonjs",
"outDir": "../lib/cjs"
}
},
"exclude": [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

exclude test files when run yarn build.

Copy link
Contributor

@hsunpei hsunpei left a comment

Choose a reason for hiding this comment

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

The test works like a charm on my Mac 👍

The settings for tests are configured for each package. However, I think we could just run them from the root level, since there might be rare chances that we want to run tests of a single package, and each package folder will be calculated as Jest runs the coverage.

You may refer to the settings of gypcrete, and I think we might apply similar settings to the charting library:

"test": "jest ./packages --config=configs/jest.config.json --coverage"

package.json Outdated
@@ -9,16 +9,25 @@
"build": "docz build",
"prepublish": "lerna run prepublish",
"lint": "lerna run lint --parallel --stream",
"clean": "lerna run --parallel clean"
"clean": "lerna run --parallel clean",
"test": "jest",
Copy link
Contributor

Choose a reason for hiding this comment

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

We might not need separate Jest config for each package, for Jest supports monorepo libraries and the stats of each package folder will be calculated as it runs the coverage.

You may refer to the settings of gypcrete:

"test": "jest ./packages --config=configs/jest.config.json --coverage"

jest.config.js Outdated
@@ -0,0 +1,3 @@
module.exports = {
projects: ['<rootDir>/packages/*'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice settings. I think this could be combined into the /config folder, and keep just one /config/jest.config.js file.

"build:cjs": "tsc -p ./src/tsconfig.cjs.json",
"clean": "rimraf ./dist ./lib ./es5 ./deploy",
"lint": "tslint --project ./src"
"lint": "tslint --project ./src",
"test": "jest",
Copy link
Contributor

Choose a reason for hiding this comment

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

There might be rare chances that we want to run tests of a single package, so I think that removing it and keeping it in the root package folder might be nicer.

@chenesan
Copy link
Contributor Author

Sounds reasonable. I wasn't sure if the tsconfig.json would be the same in every package in the future. But that makes sense to have only one jest config.

@chenesan
Copy link
Contributor Author

@garfieldduck I just remove test config in each package and make yarn test run from project root directly. This could be reviewed now :)

</div>
);
};
act(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if you need act() when using enzyme though.

Copy link
Contributor Author

@chenesan chenesan Mar 13, 2019

Choose a reason for hiding this comment

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

Currently, it does if we set state in the wrapped function of useEffect; However just few days ago I made enzymejs/enzyme#2034 to wrap enzyme's render with act so after enzyme release this I guess it should be fine without act() wrapper explicitly.

Copy link
Contributor Author

@chenesan chenesan Mar 13, 2019

Choose a reason for hiding this comment

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

But yes, in this test case we don't need act wrap since we just mock the useContainerDimension out. I'll remove this.

Copy link
Contributor

@zhusee2 zhusee2 left a comment

Choose a reason for hiding this comment

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

Nice and clean setup 👍
Looks like ts-jest works like a charm.

@zhusee2
Copy link
Contributor

zhusee2 commented Mar 12, 2019

Encountering this on build, but it looks like expected:

src/line/LineChart.tsx(206,12): error TS2739: Type '{ setHoveredPosAndIndex: (hoveredIndex: number, xPos: number, yPos: number) => void; clearHovering: () => void; collisionComponents: Element[]; }' is missing the following properties from type 'HoverLayerProps': handleHover, throttleTime

src/line/LineChart.tsx(206,12): error TS2739: Type '{ setHoveredPosAndIndex: (hoveredIndex: number, xPos: number, yPos: number) => void; clearHovering: () => void; collisionComponents: Element[]; }' is missing the following properties from type 'HoverLayerProps': handleHover, throttleTime

Tests are working fine.

Copy link
Contributor

@hsunpei hsunpei left a comment

Choose a reason for hiding this comment

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

Looks great to me 👍 💯

@chenesan chenesan merged commit 64e2832 into develop Mar 13, 2019
@chenesan chenesan mentioned this pull request Mar 18, 2019
@zhusee2 zhusee2 deleted the chore/jest-setup branch September 9, 2019 10:22
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.

3 participants