nest
nest copied to clipboard
chore: tests run with jest
PR Checklist
Please check if your PR fulfills the following requirements:
- [X] The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
- [X] Tests for the changes have been added (for bug fixes / features)
- [ ] Docs have been added / updated (for bug fixes / features) N/A
PR Type
What kind of change does this PR introduce?
- [ ] Bugfix
- [ ] Feature
- [ ] Code style update (formatting, local variables)
- [X] Refactoring (no functional changes, no api changes)
- [ ] Build related changes
- [ ] CI related changes
- [ ] Other... Please describe:
What is the current behavior?
Currently we are running our tests using mocha
, chai
, and sinon
. It's been discussed before about possibly using jest
for everything for it's speed and common use in many JS projects now
What is the new behavior?
Unit and integration tests use jest
and sinon
. No more use of chai
or mocha
Does this PR introduce a breaking change?
- [ ] Yes
- [X] No
This should only be affecting tests. It shouldn't touch the main API at all
Other information
I'm aware that this in itself is a huge PR. If t would be more beneficial and easier to review I can split this up into four or so PRs into a separate branch, replacing mocha with jest for unit tests, replacing chai with jest for unit tests, replacing mocha with jest for integration tests, and replacing chai with jest for integration tests. That way each step can be validated more thoroughly and then we can more easily have confidence in the large PR to master.
Let me know what y'all'd (you all would, for those who don't speak Texan) like me to do
Thanks @jmcdo29, this is great! Would love to finally migrate to Jest.
If t would be more beneficial and easier to review I can split this up into four or so PRs into a separate branch, replacing mocha with jest for unit tests, replacing chai with jest for unit tests, replacing mocha with jest for integration tests, and replacing chai with jest for integration tests. That way each step can be validated more thoroughly and then we can more easily have confidence in the large PR to master.
Splitting it into multiple PRs would definitely make reviewing process simpler, but I understand that even in this case, all PRs will be quite heavy (and that's totally fine).
@jmcdo29 idk if already know but you could add the following to jest.config.js
/jest.integration.js
:
globals: {
'ts-jest': {
// Disable type-checking ability and some TS features in favor of speed!
isolatedModules: true,
},
},
As stated here, this flag for ts-jest
will make tests run faster 🔥
However, since the CI system will use the same jest config file, you will need to create another one to use isolatedModules: false
instead.
In summary: locally, to decrease the time taken when running npm run test
(and others), no type-checking will be performed.
I benchmarked both with hyperfine on my Dell XPS 13 Ubuntu:
$ hyperfine -w 2 './node_modules/.bin/jest --runInBand -c jest.config.js' './node_modules/.bin/jest --runInBand -c jest.config2.js'
Benchmark #1: ./node_modules/.bin/jest --runInBand -c jest.config.js
Time (mean ± σ): 29.207 s ± 0.234 s [User: 36.223 s, System: 2.015 s]
Range (min … max): 28.823 s … 29.465 s 10 runs
Benchmark #2: ./node_modules/.bin/jest --runInBand -c jest.config2.js
Time (mean ± σ): 25.578 s ± 0.161 s [User: 29.397 s, System: 1.764 s]
Range (min … max): 25.306 s … 25.840 s 10 runs
Summary
'./node_modules/.bin/jest --runInBand -c jest.config2.js' ran
1.14 ± 0.01 times faster than './node_modules/.bin/jest --runInBand -c jest.config.js'
got 1.48 ± 0.16
without --runInBand
and with --maxWorkers 25%
-
jest.config2.js
usesisolatedModules: true
-
jest.config.js
usesisolatedModules: false
Maybe this isn't needed since Jest can cache things, and also because it prevents using few TS features (like const enum
)
@jmcdo29 idk if already know but you could add the following to
jest.config.js
/jest.integration.js
:globals: { 'ts-jest': { // Disable type-checking ability and some TS features in favor of speed! isolatedModules: true, }, },
As stated here, this flag for
ts-jest
will make tests run faster 🔥However, since the CI system will use the same jest config file, you will need to create another one to use
isolatedModules: false
instead.In summary: locally, to decrease the time taken when running
npm run test
(and others), no type-checking will be performed.I benchmarked both with hyperfine on my Dell XPS 13 Ubuntu:
$ hyperfine -w 2 './node_modules/.bin/jest --runInBand -c jest.config.js' './node_modules/.bin/jest --runInBand -c jest.config2.js' Benchmark #1: ./node_modules/.bin/jest --runInBand -c jest.config.js Time (mean ± σ): 29.207 s ± 0.234 s [User: 36.223 s, System: 2.015 s] Range (min … max): 28.823 s … 29.465 s 10 runs Benchmark #2: ./node_modules/.bin/jest --runInBand -c jest.config2.js Time (mean ± σ): 25.578 s ± 0.161 s [User: 29.397 s, System: 1.764 s] Range (min … max): 25.306 s … 25.840 s 10 runs Summary './node_modules/.bin/jest --runInBand -c jest.config2.js' ran 1.14 ± 0.01 times faster than './node_modules/.bin/jest --runInBand -c jest.config.js'
got
1.48 ± 0.16
without--runInBand
and with--maxWorkers 25%
* `jest.config2.js` uses `isolatedModules: true` * `jest.config.js` uses `isolatedModules: false`
Maybe this isn't needed since Jest can cache things, and alos because it prevents using few TS features (like
const enum
)
It's definitely cool that this is possible, but I've seen problems arise when types are left out of tests, and would rather have the tests be typesafe whenever possible. Sure it could be caught locally, but I'm one who'd like CI to catch it for those who don't use an IDE or similar in their local environment. The fact that it's only a 4 second time save as well, makes the risk seem not so worth it to me, but very cool to show off a use for hyperfine and the isolatedModules
flag!
I'll eventually come back to this in smaller batches so it's not such a huge change to look at