test: [M3-7863] - Use `happy-dom` instead of `jsdom` in unit tests #11085

bnussman-akamai · 2024-10-10T16:24:13Z

Description 📝

Replaces jsdom with happy-dom 😄
I recommend googling jsdom vs happy-dom to understand the basic differences but here are some extra callouts
- happy-dom should perform better than jsdom 🏎️ (Not seeing a huge boost in perf, but it seems to be ~30sec to 1 min faster in CI)
- Test stability seems the same (there are no stability regressions with happy-dom) 🪨
- In my opinion, the main "risk" with happy-dom is its lack of maturity although this doesn't seems to have any significant impact on us. This was, for the most part, a drop-in replacement. 👴
- happy-dom is known to work on the Bun runtime whereas jsdom does not work (yet). This doesn't matter at all for us right now, but if we ever consider the Bun runtime in the near future, this change might be nice. 🥟
- For whatever reason, happy-dom seemed to be more correct with the css that it renders. (In the diff, you'll see some changes to toHaveStyle assertions. happy-dom seems to be more inline with what the browser does which is good) 🎨

Changes Required 💻

I had to update some style assertions (toHaveStyle)
- I compared the test output to what I see in the browser and happy-dom appears to match what my browser shows, which is a win for happy-dom
I had to update a small number of test assertions to use different selectors. I didn't dig deep into why I had to make the changes I did but all of the changes seemed logical. If you have questions on specific changes, I can comment in more detail
Some unit tests that tested actions menus never opened the ActionMenu. I had to add a userEvent.click in a few tests to open the ActionMenu before making assertions. Not sure how they were passing on jsdom
Had to increase timeout for a Database related test. Don't love this, but not too bad in the grand scheme of things

How to test 🧪

Look at diff and make sure changes actually make sense and are not just changes made to satisfy the test 👀
Verify the test-manager Github Action passes ✅
Run some tests locally and verify they pass
- This change does not fix timeouts and failures when running a large number of tests locally. This is still an issue. @jdamore-linode mentioned we may be able to increase the global timeout to improve this at some point

As an Author I have considered 🤔

bnussman-akamai · 2024-10-10T23:03:02Z

packages/manager/src/features/Databases/DatabaseLanding/DatabaseLanding.test.tsx

+    await waitFor(() => expect(getByText(database.label)).toBeVisible(), {
+      timeout: 10_000,
+    });


The is the only sketchy change, everything else when pretty smoothly

Have you tried awaiting a findByText?

I did try that but the default timeout wasn't long enough 😖 I might be able to revert to findByText but I think his accomplishes the same thing

hkhalil-akamai

I think this is a positive change, particularly for the performance improvement to our CI. Nice work 👍

hkhalil-akamai · 2024-10-11T15:24:07Z

packages/manager/src/features/Databases/DatabaseLanding/DatabaseLanding.test.tsx

+    await waitFor(() => expect(getByText(database.label)).toBeVisible(), {
+      timeout: 10_000,
+    });


Have you tried awaiting a findByText?

hkhalil-akamai · 2024-10-11T15:24:42Z

packages/manager/src/features/Linodes/CloneLanding/Disks.test.tsx

@@ -33,7 +33,7 @@ describe('Disks', () => {
    const { getByTestId } = render(wrapWithTheme(<Disks {...props} />));
    disks.forEach((eachDisk) => {
      const checkbox = getByTestId(`checkbox-${eachDisk.id}`).parentNode;
-      fireEvent.click(checkbox as any);
+      fireEvent.click(checkbox!);


Should we also rewrite this as a userEvent?

Ideally yes, but I want to keep this PR's scope as narrow as possible so it is representative of what was absolutely necessary to change to get happy-dom working

There are so many places where fireEvent is used when userEvent would be preferable 😣

github-actions · 2024-10-11T16:49:44Z

Coverage Report: ✅
Base Coverage: 86.99%
Current Coverage: 86.99%

mjac0bs

(I see you added the WIP label back, so feel free to re-request a review if you're making any changes/found issues and want 👀 again.)

This was looking good and from my reading up on happy-dom vs js-dom, it seemed like sentiment was: 'if you can easily make the swap and tests pass, why not reap the benefits?'.

Unit tests are passing in CI. The changes to tests made sense - though left a comment on one.

Perf improvement on a local run:

js-dom	happy-dom

packages/manager/.changeset/pr-11085-tech-stories-1728657019139.md

...ages/manager/src/features/Linodes/LinodesDetail/LinodeNetworking/LinodeIPAddressRow.test.tsx

packages/ui/.changeset/pr-11085-tech-stories-1728657169966.md

…tests (linode#11085)" This reverts commit efa8580.

…tests (#11085)" (#11128) This reverts commit efa8580. Co-authored-by: Banks Nussman <banks@nussman.us>

bnussman added 7 commits October 10, 2024 12:18

use happy-dom

f20da70

first batch of fixes

451a3e4

another batch of fixes

c172999

Merge branch 'develop' into poc/check-status-of-happy-dom

f34aae6

another batch of fixes

2ff4197

fix ui package color

f2d53ad

fix database test flake with longer timeout

d17fd00

bnussman-akamai commented Oct 10, 2024

View reviewed changes

bnussman-akamai changed the title ~~poc: Reassess hapy-dom~~ poc: Use happy-dom instead of jsdom in unit tests Oct 10, 2024

bnussman-akamai self-assigned this Oct 10, 2024

bnussman-akamai added Unit tests Improves unit test coverage Dependencies Pull requests that update a dependency file labels Oct 10, 2024

bnussman added 9 commits October 10, 2024 19:41

try something

0123759

try running on macOS

10213d2

oops

a9a5fb3

try different config

3335ff7

clean up test config options

2cc590e

try more forks

9a20202

try more forks

dad3c4f

go back to working config

85e5e56

run on ubuntu

94630bb

bnussman-akamai marked this pull request as ready for review October 11, 2024 14:29

bnussman-akamai requested a review from a team as a code owner October 11, 2024 14:29

bnussman-akamai requested review from mjac0bs and hkhalil-akamai and removed request for a team October 11, 2024 14:29

bnussman added 2 commits October 11, 2024 10:30

Added changeset: Use happy-dom instead of jsdom in unit tests

d618969

add changeset for ui

666cac2

bnussman-akamai changed the title ~~poc: Use happy-dom instead of jsdom in unit tests~~ change: [M3-7863] - Use happy-dom instead of jsdom in unit tests Oct 11, 2024

hkhalil-akamai approved these changes Oct 11, 2024

View reviewed changes

fix last flake

6433f3d

bnussman-akamai added the Work in Progress label Oct 11, 2024

mjac0bs approved these changes Oct 11, 2024

View reviewed changes

bnussman-akamai removed the Work in Progress label Oct 11, 2024

feedback

26e45e4

bnussman-akamai changed the title ~~change: [M3-7863] - Use happy-dom instead of jsdom in unit tests~~ test: [M3-7863] - Use happy-dom instead of jsdom in unit tests Oct 11, 2024

bnussman-akamai merged commit efa8580 into linode:develop Oct 11, 2024
23 checks passed

bnussman-akamai mentioned this pull request Oct 15, 2024

fix: AppSelect.test.tsx test flake #11104

Merged

12 tasks

bnussman-akamai pushed a commit to bnussman-akamai/manager that referenced this pull request Oct 21, 2024

Revert "test: [M3-7863] - Use happy-dom instead of jsdom in unit …

bb457f6

…tests (linode#11085)" This reverts commit efa8580.

bnussman-akamai mentioned this pull request Oct 21, 2024

Revert "test: [M3-7863] - Use happy-dom instead of jsdom in unit tests (#11085)" #11128

Merged

12 tasks

bnussman-akamai added a commit that referenced this pull request Oct 21, 2024

Revert "test: [M3-7863] - Use happy-dom instead of jsdom in unit …

3eaf8c0

…tests (#11085)" (#11128) This reverts commit efa8580. Co-authored-by: Banks Nussman <banks@nussman.us>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: [M3-7863] - Use `happy-dom` instead of `jsdom` in unit tests #11085

test: [M3-7863] - Use `happy-dom` instead of `jsdom` in unit tests #11085

bnussman-akamai commented Oct 10, 2024 •

edited

Loading

bnussman-akamai Oct 10, 2024

hkhalil-akamai Oct 11, 2024

bnussman-akamai Oct 11, 2024 •

edited

Loading

hkhalil-akamai left a comment

hkhalil-akamai Oct 11, 2024

hkhalil-akamai Oct 11, 2024

bnussman-akamai Oct 11, 2024

github-actions bot commented Oct 11, 2024 •

edited

Loading

mjac0bs left a comment

test: [M3-7863] - Use happy-dom instead of jsdom in unit tests #11085

test: [M3-7863] - Use happy-dom instead of jsdom in unit tests #11085

Conversation

bnussman-akamai commented Oct 10, 2024 • edited Loading

Description 📝

Changes Required 💻

How to test 🧪

As an Author I have considered 🤔

bnussman-akamai Oct 10, 2024

Choose a reason for hiding this comment

hkhalil-akamai Oct 11, 2024

Choose a reason for hiding this comment

bnussman-akamai Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

hkhalil-akamai left a comment

Choose a reason for hiding this comment

hkhalil-akamai Oct 11, 2024

Choose a reason for hiding this comment

hkhalil-akamai Oct 11, 2024

Choose a reason for hiding this comment

bnussman-akamai Oct 11, 2024

Choose a reason for hiding this comment

github-actions bot commented Oct 11, 2024 • edited Loading

mjac0bs left a comment

Choose a reason for hiding this comment

test: [M3-7863] - Use `happy-dom` instead of `jsdom` in unit tests #11085

test: [M3-7863] - Use `happy-dom` instead of `jsdom` in unit tests #11085

bnussman-akamai commented Oct 10, 2024 •

edited

Loading

bnussman-akamai Oct 11, 2024 •

edited

Loading

github-actions bot commented Oct 11, 2024 •

edited

Loading