nest icon indicating copy to clipboard operation
nest copied to clipboard

chore: tests run with jest

Open jmcdo29 opened this issue 2 years ago • 4 comments

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

jmcdo29 avatar Aug 28 '21 22:08 jmcdo29

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

jmcdo29 avatar Aug 29 '21 18:08 jmcdo29

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).

kamilmysliwiec avatar Aug 30 '21 08:08 kamilmysliwiec

@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 also because it prevents using few TS features (like const enum)

micalevisk avatar Oct 05 '21 22:10 micalevisk

@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!

jmcdo29 avatar Oct 06 '21 01:10 jmcdo29

I'll eventually come back to this in smaller batches so it's not such a huge change to look at

jmcdo29 avatar Aug 11 '22 14:08 jmcdo29