Upgrade jsdom #31751

uladzimirdev · 2023-06-21T06:49:04Z

Description

Upgrade allows to cut 1.5-2min from jest run.

jsdom lib had a perf problem with getComputedStyle method, which is used a lot with css in js solutions. The problem was addressed in JSDOM 22.1.0, but jest itself waits for the major upgrade to take it into account. The temporary proposed solution is to use yarn resolutions.

Latest jsdom made window not configurable, so even router libs have to provide a fake configurable window object to bypass it. Unfortunately we can't do it right now as we're using old version of react-router, which doesn't allow overriding window in browserHistory

Issues about no longer possible window manipulation in jsdom
jsdom/jsdom#3492
jsdom/jsdom#3492 (comment)
jestjs/jest#5124

The issue about perf investigation
jsdom/jsdom#3234

codecov · 2023-06-21T07:58:11Z

Codecov Report

Patch coverage: 66.66% and project coverage change: +0.07 🎉

Comparison is base (7d01270) 74.15% compared to head (b24bc86) 74.23%.

❗ Current head b24bc86 differs from pull request most recent head 8f04789. Consider uploading reports for the commit 8f04789 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #31751      +/-   ##
==========================================
+ Coverage   74.15%   74.23%   +0.07%     
==========================================
  Files        2914     2911       -3     
  Lines      103970   103567     -403     
  Branches    12889    12868      -21     
==========================================
- Hits        77103    76882     -221     
+ Misses      21039    20857     -182     
  Partials     5828     5828

Flag	Coverage Δ
back-end	`86.70% <ø> (-0.08%)`	⬇️
front-end	`60.34% <66.66%> (+0.25%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
frontend/src/metabase/lib/dom.js	`44.19% <0.00%> (-0.40%)`	⬇️
...e/frontend/src/metabase-enterprise/auth/actions.ts	`100.00% <100.00%> (ø)`
frontend/src/metabase/auth/actions.ts	`75.40% <100.00%> (ø)`

... and 163 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

github-actions · 2023-06-23T11:09:12Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff ed6cdc7...8f04789.

Notify	File(s)
@alxnddr	frontend/src/metabase/visualizations/visualizations/LinkViz/LinkViz.unit.spec.tsx
@ranquild	frontend/src/metabase/auth/actions.ts frontend/src/metabase/auth/components/Logout/Logout.unit.spec.tsx

uladzimirdev · 2023-06-23T11:09:40Z

frontend/src/metabase/auth/actions.ts

@@ -92,11 +93,11 @@ export const logout = createAsyncThunk(
  async (redirectUrl: string | undefined, { dispatch, rejectWithValue }) => {
    try {
      await deleteSession();
-      await dispatch(clearCurrentUser());
+      dispatch(clearCurrentUser());


clearCurrentUser is sync

WiNloSt · 2023-06-26T09:59:20Z

frontend/test/metabase/lib/expressions/fuzz.recursive-parser.unit.spec.js

 import { parse } from "metabase-lib/expressions/recursive-parser";

 import { generateExpression } from "./generator";

-const fuzz = process.env.MB_FUZZ ? describe : describe.skip;
+const fuzz = process.env.MB_FUZZ ? describe : _.noop;


what's the reason behind this change? Changing from describe.something to _.noop looks questionable since that affects the test output.

it's simple, if you check any CI run and frontend action, you'll see

but with this change you'll see

Notice, there is no 70k of the skipped tests anymore

love this change

Does that help improve the test time as well (2186s -> 1652s)? that'll be another huge win.

@WiNloSt I don't think it affects perf somehow, maybe runners were busy so the result was ~2k seconds, but overall with this jsdom upgrade we should have 1.7-1.8k seconds per run.

jest should definitely count those describe.skip, but to my mind it's just javascript, so 10s is more than enough to go through it

frontend/src/metabase/lib/dom.js

iethree

works for me!

iethree · 2023-06-26T21:53:03Z

frontend/test/metabase/lib/expressions/fuzz.recursive-parser.unit.spec.js

 import { parse } from "metabase-lib/expressions/recursive-parser";

 import { generateExpression } from "./generator";

-const fuzz = process.env.MB_FUZZ ? describe : describe.skip;
+const fuzz = process.env.MB_FUZZ ? describe : _.noop;


love this change

iethree · 2023-06-26T21:59:00Z

frontend/src/metabase/lib/dom.js

This doesn't bother me that much, but I don't see these wrapper methods being all that useful when we just have to remember to use them instead of the window.location methods everyone already knows.

Does mocking the window.location methods directly not work with the new version of jsdom?

answered here

one more time: unfortunately with latest jsdom it's not possible to mock window properties anymore (at least ways I tried didn't help)

jsdom/jsdom#3492
jsdom/jsdom#3492 (comment)

deploysentinel · 2023-06-27T11:33:05Z

No failed tests 🎉

WiNloSt

Great work!

kamilmielnik

LGTM 👍

jsdom lib had a perf problem with getComputedStyle method, which is used a lot with css in js solutions. The problem was addressed in JSDOM 22.1.0, but jest itself waits for the major upgrade to take it into account. The temporary proposed solution is to use yarn resolutions.

jsdom lib had a perf problem with getComputedStyle method, which is used a lot with css in js solutions. The problem was addressed in JSDOM 22.1.0, but jest itself waits for the major upgrade to take it into account. The temporary proposed solution is to use yarn resolutions. Co-authored-by: Uladzimir Havenchyk <125459446+uladzimirdev@users.noreply.github.com>

metabase-bot bot assigned uladzimirdev Jun 21, 2023

metabase-bot bot added the .Team/42 🌌 label Jun 21, 2023

metabase-bot bot added the visual Run Percy visual testing label Jun 23, 2023

uladzimirdev force-pushed the upgrade-jsdom branch from af83c23 to 1b75280 Compare June 23, 2023 11:09

uladzimirdev commented Jun 23, 2023

View reviewed changes

uladzimirdev requested a review from a team June 23, 2023 21:10

WiNloSt reviewed Jun 26, 2023

View reviewed changes

WiNloSt requested review from a team and removed request for a team June 26, 2023 10:13

uladzimirdev requested a review from WiNloSt June 26, 2023 19:07

iethree approved these changes Jun 26, 2023

View reviewed changes

uladzimirdev requested a review from kamilmielnik June 27, 2023 19:56

WiNloSt approved these changes Jun 28, 2023

View reviewed changes

kamilmielnik approved these changes Jun 28, 2023

View reviewed changes

uladzimirdev added 7 commits June 29, 2023 16:12

Upgrade jsdom

5015e54

Fix window.location assignment problem

353c1ff

Drop skipped describe blocks

864f710

Bypass not configurable window object

3bd1345

Cleanup

07ea9f1

Yarn.lock

046ca50

Add comments

869d25f

paoliniluis force-pushed the upgrade-jsdom branch from a03fdc7 to 869d25f Compare June 29, 2023 19:12

uladzimirdev added 2 commits June 30, 2023 14:32

Merge branch 'master' into upgrade-jsdom

b24bc86

Merge branch 'master' into upgrade-jsdom

8f04789

uladzimirdev enabled auto-merge (squash) July 6, 2023 20:26

uladzimirdev merged commit 8fa8350 into master Jul 6, 2023
94 of 96 checks passed

uladzimirdev deleted the upgrade-jsdom branch July 6, 2023 21:00

uladzimirdev added the backport Automatically create PR on current release branch on merge label Aug 1, 2023

metabase-bot bot mentioned this pull request Aug 1, 2023

🤖 backported "Upgrade jsdom" #32788

Merged

uladzimirdev mentioned this pull request Nov 3, 2023

[jest] Move builtin plugins to specific specs #34167

Merged

This was referenced Mar 15, 2024

🍒 Manual backport #39007 to 0.47 #40173

Merged

Backport security fixes for 47 #40284

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade jsdom #31751

Upgrade jsdom #31751

uladzimirdev commented Jun 21, 2023 •

edited

codecov bot commented Jun 21, 2023 •

edited

github-actions bot commented Jun 23, 2023 •

edited

uladzimirdev Jun 23, 2023

WiNloSt Jun 26, 2023

uladzimirdev Jun 26, 2023

iethree Jun 26, 2023

WiNloSt Jun 27, 2023 •

edited

uladzimirdev Jun 27, 2023

iethree left a comment

iethree Jun 26, 2023

iethree Jun 26, 2023

uladzimirdev Jun 27, 2023

deploysentinel bot commented Jun 27, 2023 •

edited

WiNloSt left a comment

kamilmielnik left a comment

Upgrade jsdom #31751

Upgrade jsdom #31751

Conversation

uladzimirdev commented Jun 21, 2023 • edited

Description

codecov bot commented Jun 21, 2023 • edited

Codecov Report

github-actions bot commented Jun 23, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WiNloSt Jun 27, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iethree left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deploysentinel bot commented Jun 27, 2023 • edited

WiNloSt left a comment

Choose a reason for hiding this comment

kamilmielnik left a comment

Choose a reason for hiding this comment

uladzimirdev commented Jun 21, 2023 •

edited

codecov bot commented Jun 21, 2023 •

edited

github-actions bot commented Jun 23, 2023 •

edited

WiNloSt Jun 27, 2023 •

edited

deploysentinel bot commented Jun 27, 2023 •

edited