-
Notifications
You must be signed in to change notification settings - Fork 534
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
chore: upgrade to jest@27 #1968
Conversation
Try adding a timeout for tests. Looking at explore-node-modules.ts, it appears that file I/O is done synchronously which might result in horrendous performance when scanning the entire node_modules folder. |
@RA80533 Jest defaults to 5 second timeouts, while these are going on for 10mins+ and CircleCI is shutting it down. More than likely a bug in Jest that needs further investigation (or it might be fixed by now). Good point about performance. It's worth considering adding some performance tests for large node_modules or at least manually testing that scenario. However in this test's case, node_modules is pretty small, and it's only a probelm in Jest 27. |
aa35968
to
bc86d99
Compare
54d6b6d
to
bdb640a
Compare
@@ -1,9 +1,5 @@ | |||
module.exports = { | |||
preset: 'ts-jest', | |||
testEnvironment: 'node', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is default now.
collectCoverage: false, // not collecting coverage for now | ||
collectCoverageFrom: ['src/**/*.ts'], | ||
coverageReporters: ['text-summary', 'html'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coverage is false
by default and we never use it so can just remove it.
"devDependencies": { | ||
"@types/jest": "26.0.20", | ||
"@types/node": "14.14.25", | ||
"jest": "26.6.3", | ||
"ts-jest": "^26.5.3", | ||
"typescript": "4.2.4" | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@snyk/fix can use devDeps from parent since we're usng npm 7 workspaces. Avoids getting dev deps out of sync.
@@ -91,7 +91,7 @@ describe('test --json-file-output ', () => { | |||
'`test --json-file-output produces same JSON output as normal JSON output to stdout`', | |||
(done) => { | |||
const jsonOutputFilename = `${uuidv4()}.json`; | |||
return exec( | |||
exec( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jest 27 does not like tests using done
and returning a promise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why use done
and not just make the test async?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More work unrelated to Jest 27's migration. I can make that a separate PR.
@@ -165,7 +165,7 @@ | |||
"eslint": "6.8.0", | |||
"eslint-config-prettier": "^6.1.0", | |||
"fs-extra": "^9.1.0", | |||
"jest": "^26.6.3", | |||
"jest": "^27.0.6", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about @types/jest
looks like that can be updated to ^26.0.24
, though maybe it doesn't matter.
Also, it's weird that the major version of @types/jest
types is out of sync with the jest
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
types/jest is community-maintained so it's a matter of waiting for someone to do the work. Though, jest does say it comes with typedefs so this shouldn't be needed. 🤔 I'll try removing types/jest and see what happens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't work so rolling it back. Luckily 26's type defs work fine for now since 27 didn't change most of the API.
@@ -91,7 +91,7 @@ describe('test --json-file-output ', () => { | |||
'`test --json-file-output produces same JSON output as normal JSON output to stdout`', | |||
(done) => { | |||
const jsonOutputFilename = `${uuidv4()}.json`; | |||
return exec( | |||
exec( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why use done
and not just make the test async?
Doesn't seem to be much in the way of breaking changes, mostly optimisations. https://jestjs.io/blog/2021/05/25/jest-27
Note: there was some issues with this PR but after upgrading our CircleCI instance to Large, it fixed itself.