chore(deps): upgrades jest to 28 #4783
Conversation
I really have little or no idea why this is failing without this (and what effect simply updating jest had on this) but... so it goes.
| @@ -1,3 +1,5 @@ | |||
| import { describe, expect, it } from '@jest/globals'; | |||
There was a problem hiding this comment.
after this PR, we can go ahead and add this to every test file in the project, which will allow us to no longer depend on the globals and also will mean that TypeScript will correctly tell us we cannot use jest stuff from our source code (by accident, for example).
There was a problem hiding this comment.
ok I see. I actually like the clarity. Thank you for doing this work.
There was a problem hiding this comment.
for anyone interested to learn more about @jest/globals DefinitelyTyped/DefinitelyTyped#44365 is a decent primer. the TLDR; is that it allows us to use jest without globals (which, for us, means that we can't then accidentally call jest globals from our non-test source code).
| @@ -3,7 +3,7 @@ module.exports = { | |||
| collectCoverage: false, | |||
| globals: { | |||
| 'ts-jest': { | |||
| isolatedModules: true, | |||
| isolatedModules: false, | |||
There was a problem hiding this comment.
it's time for this default to flip. it means our tests will be typechecked by default (which, then means we'll have to turn it off in those that have errors rather than turn it on in those that don't, and new packages will have it by default).
| import { ByRoleMatcher, ByRoleOptions, fireEvent, render } from '@testing-library/react'; | ||
| import React from 'react'; | ||
|
|
||
| import { Tooltip } from './tooltip'; | ||
|
|
||
| const pause = (time: number) => new Promise(resolve => setTimeout(resolve, time)); | ||
|
|
There was a problem hiding this comment.
this test was actually pausing in real wall-clock time before. now.. it doesn't.
| @@ -13,7 +13,6 @@ | |||
| "node_modules", | |||
| "dist", | |||
| "src/jest", | |||
| ".babelrc.js", | |||
There was a problem hiding this comment.
this file was deleted a while back and it looks like we just missed deleting it here.
| @@ -1,15 +0,0 @@ | |||
| /** @type { import('@babel/core').TransformOptions } */ | |||
There was a problem hiding this comment.
this is unused insofar as I can tell. I thought maybe jest uses it, but I tested that and it appears it does not.
| @@ -1,29 +1,20 @@ | |||
| /** @type { import('@jest/types').Config.InitialOptions } */ | |||
| module.exports = { | |||
| // preset: '../../jest-preset.js', // DOES NOT WORK | |||
There was a problem hiding this comment.
I figured out why this doesn't work see jestjs/jest#12817 for what the solution was
| 'styled-components': '<rootDir>/../node_modules/styled-components', | ||
| '\\.(css|less|png)$': '<rootDir>/src/__mocks__/dummy.ts', | ||
| 'styled-components': '<rootDir>/node_modules/styled-components', | ||
| 'jsonpath-plus': '<rootDir>/node_modules/jsonpath-plus/dist/index-node-cjs.cjs', |
There was a problem hiding this comment.
it may not seem like much, but this line of code took me about 5 hours, maybe more, to arrive at. there's some funky business for jsonpath-plus that we have to work around and it was quite a bear.
There was a problem hiding this comment.
Related reference: https://nodejs.org/api/packages.html#conditional-exports
Scoped out of this PR are changes to testEnvironment: jsdom
Which resolves "browser" modules by default, rather than the commonjs "node" modules which would be resolved in electron context currently. Also related to electron api "contextIsolation"
| const resultDecrypted = crypt.decryptAESToBuffer(key, resultEncrypted); | ||
| expect(resultDecrypted).toEqual(Buffer.from('Hello World!', 'utf8')); | ||
| expect(resultDecrypted.toString()).toEqual(source.toString()); |
There was a problem hiding this comment.
it's non-ideal to have to toString these before comparison, but if we don't the match fails because one buffer comes from the npm and one comes from node. and if we try to flip that around we get stuck because we can't polyfill what's in the hkdf npm (which doesn't use the buffer npm, and uses the global).
| expect(setTimeout).toHaveBeenCalledTimes(3); | ||
| expect(setTimeout).nthCalledWith(1, expect.anything(), 100); | ||
| expect(setTimeout).nthCalledWith(2, expect.anything(), 100); | ||
| expect(setTimeout).nthCalledWith(3, expect.anything(), 100); |
There was a problem hiding this comment.
for these kinds of things it was tricky and everyone on the team agreed that asserting on the result is plenty verification. we don't need to mock and assert on the internals (e.g. setInterval). I still wish I understood why it didn't work, but so it goes.
| @@ -1,4 +1,4 @@ | |||
| import { mocked } from 'ts-jest/utils'; | |||
| import { mocked } from 'jest-mock'; | |||
There was a problem hiding this comment.
this happened in ts-jest 27.1.0. see https://github.com/kulshekhar/ts-jest/blob/main/CHANGELOG.md#2710-2021-11-30
| @@ -348,7 +342,9 @@ export const database = { | |||
| config, | |||
| ), | |||
| ); | |||
| collection.persistence.setAutocompactionInterval(DB_PERSIST_INTERVAL); | |||
| if (!config.inMemoryOnly) { | |||
There was a problem hiding this comment.
this should be an effective noop in runtime and has the benefit of helping out the tests
|
@marckong see #4783 (comment). I was doing a self review (actually I went through the PR 3 separate times top-to-bottom and found things each time. The signal that it's ready to review would be pulling it out of draft and assigning reviewers, which I will do immediately after typing this message. :) |
|
Yeah I didn't realize this was the draft lol I automatically jumped onto it because it reminded me of the zoom call excitement! Amazing work! |
In lieu of the normal description, I have written a bunch of comments on specific topics that came up during this work. The one and only goal of this PR is to upgrade jest with the smallest footprint possible. If you have any ideas on how to further reduce the footprint, lemme know!