Fix frontend unit tests for Node 20 #4607

wjrosa · 2025-08-20T15:17:45Z

Changes proposed in this Pull Request:

As part of the Node 20 upgrade project, this PR extracts most of the required frontend unit tests fixes from the source PR (#4264). The changes here are compatible with both versions (16 and 20).

What this changes:

Adds the await clause in front of every userEvent.click call
Mocks the createSelector method when mocking the @wordpress/data library
Correctly awaits getByRole calls
Apply some lint fixes (tests only)

Testing instructions

Code review
Check if the frontend tests and lint validations are still passing

Covered with tests (or have a good reason not to test in description ☝️)
Tested on mobile (or does not apply)

Changelog entry

This Pull Request does not require a changelog entry. (Comment required below)

Changelog Entry Comment

Comment

Post merge

Added testing instructions to the Release Testing Instructions wiki page (or does not apply)
Added the needs docs label (or does not apply)
Included this PR in the Release Thread scope (or does not apply)

daledupreez

@wjrosa, this seems OK at a glance, but I have some questions for you:

Why are we switching from userEvent to fireEvent? Per the Testing Library docs for userEvent, we should prefer userEvent over fireEvent as it is a more realistic reflection of user interactions.
- I am OK if this is meant to be temporary or a workaround, but I think we should clarify why we're doing this. It's possible we did it in the original PR too, in which case we should understand why we did it there!
How can/should we test that these changes still pass in node 20? It would be good to clarify how we can confirm that is true. (But I don't think it's blocking, but I'd rather make sure we confirm it is working so we can avoid later cleanup.)

daledupreez · 2025-08-21T09:01:22Z

client/settings/advanced-settings-section/__tests__/optimized-checkout-feature.test.js


-		expect( checkbox ).toBeDisabled();
-		expect( checkbox ).not.toBeChecked();
+		expect( mockSetIsOCEnabled ).toHaveBeenCalledWith( false );


Is the render calling a state setter? That feels pretty weird to me.

Hum, how would you do it instead?

My confusion stems from the fact that the previous test was waiting for the user to click on a button before checking for a state change. Simply rendering a component should generally not result in a state change, which was why I was confused.

Looking at this more closely, I now realise that we have a far more subtle situation: useIsOCEnabled() only looks at the underlying setting and not whether that setting is being overridden by UPE being disabled, and we're using an effect to update the client-side state when we detect that UPE is disabled.

It feels to me like it would be cleaner if the useIsOCEnabled() logic took into account whether UPE was enabed/disabled so our client-side state more directly represents the server-side state.

But I think that it something we should tackle in other PRs!

wjrosa · 2025-08-21T10:59:05Z

Hey Dale!

Why are we switching from userEvent to fireEvent? Per the Testing Library docs for userEvent, we should prefer userEvent over fireEvent as it is a more realistic reflection of user interactions.

That's just because it works with Node 20, while userEvent does not (at least with our changes here). I saw it in the docs for the latest versions and decided to give it a try. I will do some testing with the latest versions of testing library to see if that works for us.

How can/should we test that these changes still pass in node 20? It would be good to clarify how we can confirm that is true. (But I don't think it's blocking, but I'd rather make sure we confirm it is working so we can avoid later cleanup.)

The changes I made here are exactly the same as I did in the main PR. You can see that tests are passing there now. You can compare both.

daledupreez

I think it's worth explaining why you're using fireEvent instead of userEvent in the PR description, even if it's just "it continues to work". 😁

That said, I think we should eventually aim to switch back to userEvent at some point after we've completed the upgrade to Node 20, as I think the idea of testing user interactions is going to help us write better tests in the long run. However, I think that the big obstacle here is the 14.0 release of @testing-library/user-event, as that was when the core API changed from synchronous to asynchronous. Still, let's take that on after we have node 20 running.

daledupreez · 2025-08-21T12:12:31Z

client/settings/advanced-settings-section/__tests__/optimized-checkout-feature.test.js


-		expect( checkbox ).toBeDisabled();
-		expect( checkbox ).not.toBeChecked();
+		expect( mockSetIsOCEnabled ).toHaveBeenCalledWith( false );


My confusion stems from the fact that the previous test was waiting for the user to click on a button before checking for a state change. Simply rendering a component should generally not result in a state change, which was why I was confused.

Looking at this more closely, I now realise that we have a far more subtle situation: useIsOCEnabled() only looks at the underlying setting and not whether that setting is being overridden by UPE being disabled, and we're using an effect to update the client-side state when we detect that UPE is disabled.

It feels to me like it would be cleaner if the useIsOCEnabled() logic took into account whether UPE was enabed/disabled so our client-side state more directly represents the server-side state.

But I think that it something we should tackle in other PRs!

changelog.txt

wjrosa · 2025-08-21T13:49:12Z

I think it's worth explaining why you're using fireEvent instead of userEvent in the PR description, even if it's just "it continues to work". 😁

I conducted some tests and was able to resolve the issue by simply adding the await clause in front of all userEvent.click calls. So I reverted that replacement in both here and in the base PR. Thanks for flagging that!

daledupreez

Thanks for the updates! They look good to me!

Another thing we might be able to is split out a PR to update @testing-library/user-event to 14.0.0 (or some similarly early 14.x release), as that was released quite a while ago. We may not be able to switch to a current version until we upgrade React, but we might be able to reduce our work by tackling some of the work in an interim PR.

Co-authored-by: daledupreez <[email protected]>

Mayisha

Changes are straightforward and look good to me. 👍
I have suggested a few additional missing await.
+1 to the suggestion of updating @testing-library/user-event in a follow-up PR.

Mayisha · 2025-08-25T08:12:28Z

client/stripe-utils/__tests__/cash-app-limit-notice-handler.test.js

+	maybeShowCashAppLimitNotice,
+} from 'wcstripe/stripe-utils/cash-app-limit-notice-handler';
 import { callWhenElementIsAvailable } from 'wcstripe/blocks/upe/call-when-element-is-available';
-import { CASH_APP_NOTICE_AMOUNT_THRESHOLD } from 'wcstripe/data/constants';


Isn't CASH_APP_NOTICE_AMOUNT_THRESHOLD still part of the constant file? 🤔

client/settings/advanced-settings-section/__tests__/optimized-checkout-feature.test.js

client/components/popover/test/index.test.js

Co-authored-by: Mayisha <[email protected]>

…checkout-feature.test.js Co-authored-by: Mayisha <[email protected]>

wjrosa self-assigned this Aug 20, 2025

wjrosa marked this pull request as ready for review August 20, 2025 15:17

wjrosa mentioned this pull request Aug 20, 2025

Update NodeJS to v20 #4264

Closed

12 tasks

wjrosa requested review from a team, daledupreez and Mayisha and removed request for a team August 20, 2025 15:40

daledupreez reviewed Aug 21, 2025

View reviewed changes

daledupreez approved these changes Aug 21, 2025

View reviewed changes

wjrosa added this to the 9.9.0 milestone Aug 21, 2025

daledupreez approved these changes Aug 22, 2025

View reviewed changes

wjrosa and others added 7 commits August 22, 2025 15:21

Fix frontend unit tests for Node 20

3b89471

Test lint fixes

95f78d3

Changelog and readme entries

18d8285

Fix tests

0639b43

Revert unnecessary replacement of userEvent.click with fireEvent.click

24c253b

Update changelog.txt

5200f39

Co-authored-by: daledupreez <[email protected]>

Readme entry

08ca917

wjrosa force-pushed the dev/node-20-unit-tests-fixes branch from 4ed908b to 08ca917 Compare August 22, 2025 18:26

wjrosa added 2 commits August 22, 2025 15:27

Fix rebase

065be13

Reflecting latest changes from the main PR

2c6d067

Mayisha approved these changes Aug 25, 2025

View reviewed changes

wjrosa and others added 3 commits August 25, 2025 07:07

Update client/components/popover/test/index.test.js

4b4dfa9

Co-authored-by: Mayisha <[email protected]>

Update client/components/popover/test/index.test.js

44f8c58

Co-authored-by: Mayisha <[email protected]>

Update client/settings/advanced-settings-section/__tests__/optimized-…

748c9d8

…checkout-feature.test.js Co-authored-by: Mayisha <[email protected]>

wjrosa enabled auto-merge (squash) August 25, 2025 10:13

wjrosa merged commit 41e360d into develop Aug 25, 2025
40 checks passed

wjrosa deleted the dev/node-20-unit-tests-fixes branch August 25, 2025 10:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix frontend unit tests for Node 20 #4607

Fix frontend unit tests for Node 20 #4607

Uh oh!

wjrosa commented Aug 20, 2025 •

edited

Loading

Uh oh!

daledupreez left a comment

Uh oh!

daledupreez Aug 21, 2025

Uh oh!

wjrosa Aug 21, 2025

Uh oh!

daledupreez Aug 21, 2025

Uh oh!

wjrosa commented Aug 21, 2025

Uh oh!

daledupreez left a comment

Uh oh!

daledupreez Aug 21, 2025

Uh oh!

Uh oh!

wjrosa commented Aug 21, 2025

Uh oh!

daledupreez left a comment

Uh oh!

Mayisha left a comment

Uh oh!

Mayisha Aug 25, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Fix frontend unit tests for Node 20 #4607

Fix frontend unit tests for Node 20 #4607

Uh oh!

Conversation

wjrosa commented Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this Pull Request:

Testing instructions

Changelog entry

Comment

Uh oh!

daledupreez left a comment

Choose a reason for hiding this comment

Uh oh!

daledupreez Aug 21, 2025

Choose a reason for hiding this comment

Uh oh!

wjrosa Aug 21, 2025

Choose a reason for hiding this comment

Uh oh!

daledupreez Aug 21, 2025

Choose a reason for hiding this comment

Uh oh!

wjrosa commented Aug 21, 2025

Uh oh!

daledupreez left a comment

Choose a reason for hiding this comment

Uh oh!

daledupreez Aug 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wjrosa commented Aug 21, 2025

Uh oh!

daledupreez left a comment

Choose a reason for hiding this comment

Uh oh!

Mayisha left a comment

Choose a reason for hiding this comment

Uh oh!

Mayisha Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wjrosa commented Aug 20, 2025 •

edited

Loading