upgrade jest #15642

brad-decker · 2022-08-18T16:59:54Z

Explanation

Fixes issue related to slack reports of unit tests consuming upwards of more than 20gb of memory and 4gb of swap. Note that this PR does nothing to fix the underlying memory leaks in our tests and that should be explored separately. This works around the memory issue by forcing worker processes to terminate once they've consumed 500mb of memory. I chose 500mb because our ci environment has 4gb of ram and we run two workers when running tests in jest. This would allow us to theoretically add another 6 workers without running into out of memory issues. I'll test that theory in another PR aimed at improving CI.

Consequences of this PR:

The default environment for tests is node, so had to install the jsdom environment and specify jsdom as our testEnvironment in jest.config.js. This can be changed per file.
Had to temporarily disable the button.stories.test.js file because it eventually imports a .mdx file and the transform that was previously working for 'mdx' files in jest no longer works.
Had to patch the types for jsdom to not use the import { type TypeName} syntax
Removed the yargs patch because the newest version checks for the existence of Error.createErrorBoundary before calling it.
Some minor syntax changes

Pre-Merge Checklist

PR template is filled out
IF this PR fixes a bug, a test that would have caught the bug has been added
PR is linked to the appropriate GitHub issue
PR has been added to the appropriate release Milestone

+ If there are functional changes:

Manual testing complete & passed
"Extension QA Board" label has been applied

metamaskbot · 2022-08-18T23:40:34Z

Builds ready [d5eefb4]

builds: chrome, firefox, opera
builds (beta): chrome, firefox, opera
builds (flask): chrome, firefox, opera
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1847 ± 60 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	97	1871	225	381	183
		domContentLoaded	1643	2032	1837	124	60
		load	1643	2032	1847	125	60
		domInteractive	1643	2032	1837	124	60

tmashuang

LGTM, will re-approve once the merge conflicts are resolved.

metamaskbot · 2022-08-23T15:45:10Z

Builds ready [7ede140]

builds: chrome, firefox, opera
builds (beta): chrome, firefox, opera
builds (flask): chrome, firefox, opera
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1826 ± 76 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	98	218	125	26	12
		domContentLoaded	1619	2249	1794	147	71
		load	1619	2269	1826	157	76
		domInteractive	1619	2249	1794	147	71

georgewrmarshall

As per our slack conversation I think we can probably remove the ui/components/ui/button/button.stories.skippedtest.js file altogether

ui/components/ui/button/button.stories.skippedtest.js

georgewrmarshall

LGTM!

metamaskbot · 2022-08-23T17:20:03Z

Builds ready [206510f]

builds: chrome, firefox, opera
builds (beta): chrome, firefox, opera
builds (flask): chrome, firefox, opera
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (2182 ± 137 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	107	2331	271	475	228
		domContentLoaded	1716	2620	2163	288	138
		load	1741	2620	2182	286	137
		domInteractive	1716	2620	2163	288	138

mcmire · 2022-08-23T17:22:21Z

jest.config.js

+  // installed in @metamask/controllers so I had to just blanket specify all
+  // of the @metamask/controllers folder.
+  transformIgnorePatterns: [
+    '/node_modules/(?!(multiformats|uuid|nanoid|@metamask/controllers|@metamask/snap-controllers)/)',


multiformats, uuid, and nanoid makes sense, because they have recently(ish) converted to ESM, and I've seen Jest choke on these packages in other projects.

We don't ship ESM code in @metamask/controllers or @metamask/snap-controllers, however, so why did we have to list these here?

some of the dependencies nested inside the metamask/controllers repo are ESM and i could not figure out how to specify that recursion in this ignore pattern.

Ah I see. Interesting...

mcmire · 2022-08-23T17:23:01Z

patches/@types+jsdom++parse5+7.0.0.patch

+++ b/node_modules/@types/jsdom/node_modules/parse5/dist/index.d.ts
+@@ -1,10 +1,10 @@
+-import { type ParserOptions } from './parser/index.js';
+import { ParserOptions } from './parser/index.js';


Sorry why do we have to remove type here?

running tsc throws errors for using type keyword inside the brackets. You can import types and non types without specifying a keyword

Ohh... wow. That's subtle. How do other people even use this package then 🤔

no idea. unless it has something to do with our setup.

mcmire · 2022-08-23T19:28:07Z

Had to do a bit of research on this one. Looks like the next version of Jest includes workerIdleMemoryLimit, and that version isn't out yet, so this PR bumps it to 29.alpha. The docs for this option link to this issue which explains where it came from. It also might be worth following this PR to see if that fixes things further. Or this issue in Node.

mcmire

Makes sense to me.

brad-decker · 2022-08-23T20:12:46Z

@mcmire my apologies for not posting more details in the description. You ended up doing some double work that I could have possibly saved some time for you if I would have simply transcribed @Gudahtt and I's thread in the extension channel into the PR description. I'll do better in the future.

brad-decker force-pushed the upgrade-jest branch from 2ed42b8 to 6691a0b Compare August 18, 2022 19:36

brad-decker marked this pull request as ready for review August 18, 2022 22:35

brad-decker requested a review from a team as a code owner August 18, 2022 22:35

brad-decker requested a review from hmalik88 August 18, 2022 22:35

tmashuang previously approved these changes Aug 23, 2022

View reviewed changes

upgrade jest

7ede140

brad-decker dismissed tmashuang’s stale review via 7ede140 August 23, 2022 14:47

brad-decker force-pushed the upgrade-jest branch from d5eefb4 to 7ede140 Compare August 23, 2022 14:47

tmashuang previously approved these changes Aug 23, 2022

View reviewed changes

georgewrmarshall reviewed Aug 23, 2022

View reviewed changes

ui/components/ui/button/button.stories.skippedtest.js Outdated Show resolved Hide resolved

remove skipped test

206510f

brad-decker dismissed tmashuang’s stale review via 206510f August 23, 2022 16:23

georgewrmarshall approved these changes Aug 23, 2022

View reviewed changes

mcmire reviewed Aug 23, 2022

View reviewed changes

mcmire approved these changes Aug 23, 2022

View reviewed changes

brad-decker merged commit 0d862d4 into develop Aug 23, 2022

brad-decker deleted the upgrade-jest branch August 23, 2022 20:13

github-actions bot locked and limited conversation to collaborators Aug 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

upgrade jest #15642

upgrade jest #15642

brad-decker commented Aug 18, 2022 •

edited

Loading

metamaskbot commented Aug 18, 2022

tmashuang left a comment

metamaskbot commented Aug 23, 2022

georgewrmarshall left a comment

georgewrmarshall left a comment

metamaskbot commented Aug 23, 2022

mcmire Aug 23, 2022

brad-decker Aug 23, 2022

mcmire Aug 23, 2022

mcmire Aug 23, 2022

brad-decker Aug 23, 2022

mcmire Aug 23, 2022

brad-decker Aug 23, 2022

mcmire commented Aug 23, 2022

mcmire left a comment

brad-decker commented Aug 23, 2022

upgrade jest #15642

upgrade jest #15642

Conversation

brad-decker commented Aug 18, 2022 • edited Loading

Explanation

Pre-Merge Checklist

+ If there are functional changes:

metamaskbot commented Aug 18, 2022

tmashuang left a comment

Choose a reason for hiding this comment

metamaskbot commented Aug 23, 2022

georgewrmarshall left a comment

Choose a reason for hiding this comment

georgewrmarshall left a comment

Choose a reason for hiding this comment

metamaskbot commented Aug 23, 2022

mcmire Aug 23, 2022

Choose a reason for hiding this comment

brad-decker Aug 23, 2022

Choose a reason for hiding this comment

mcmire Aug 23, 2022

Choose a reason for hiding this comment

mcmire Aug 23, 2022

Choose a reason for hiding this comment

brad-decker Aug 23, 2022

Choose a reason for hiding this comment

mcmire Aug 23, 2022

Choose a reason for hiding this comment

brad-decker Aug 23, 2022

Choose a reason for hiding this comment

mcmire commented Aug 23, 2022

mcmire left a comment

Choose a reason for hiding this comment

brad-decker commented Aug 23, 2022

brad-decker commented Aug 18, 2022 •

edited

Loading