Update react & react-dom to 18.3.0 and fix playwright flakiness #1807

henrycatalinismith · 2024-02-26T22:48:41Z

Description

The new survey page depends on useFormState which depends on the canary release channel versions of react and react-dom. The new version of react-dom causes mayhem with the Playwright tests.

The new survey page is a big enough change in its own right. I wanted to separate this other work into this PR in order to get that solved and unblock the survey page.

Screenshots

None.

Changes

Full excruciating details of the process of producing this PR are available at henrycatalinismith#1.

My hunch is that react-dom 18.3.0's performance characteristics are somehow different than before. I think this exposes pre-existing race conditions in our playwright tests.

The rough idea of my fixes here is that I'm splitting up page.locator('…').something() chains. Instead I'm doing await locator.waitFor({ state: 'visible' }) on the locators to ensure the elements they refer to have rendered before allowing the tests to progress to the next phase that depends on those elements being visible to do something() with them.

Generally speaking, I'm adding a lot of Promise.all blocks containing a pair of promises where the first one is doing something (e.g. clicking a button, pressing a key) and the second is observing the outcome (e.g. waiting for a menu to open). It's a structure that sets a nice steady drumbeat to the progression of the tests which seems to help keep things from getting tangled up in each other.

Notes to reviewer

I've been doing a lot of yarn playwright:skipbuild --repeat-each=4, personally. If you're feeling patient, crank that 4 up to a bigger number, go make yourself a coffee while it runs, and see if you turn up anything interesting.

Related issues

None.

richardolsson

Nice work on figuring this out! I think you've solved the problem, and the core of the solution seems to be a bunch of waitFor(). However, I'm not sure all the Promise.all() are needed, and I've elaborated on my hunch below.

richardolsson · 2024-02-27T04:11:51Z

integrationTesting/tests/organize/campaigns/list/list.spec.ts

@@ -22,12 +22,13 @@ test.describe('Campaigns list page ', () => {
      WelcomeNewMembers,
    ]);

-    await page.goto(appUri + '/organize/1/projects');
+    const campaignCards = page.locator('data-testid=campaign-card');


This (using locators instead of $$eval) is a clear improvement IMO.

richardolsson · 2024-02-27T04:16:16Z

integrationTesting/tests/organize/campaigns/list/list.spec.ts

+    await Promise.all([
+      page.goto(appUri + '/organize/1/projects'),
+      campaignCards.first().waitFor({ state: 'visible' }),
+    ]);


Is this really necessary though?

I've used the Promise.all([ someAction, someWait ]) construct a lot for cases where the two can happen simultaneously, e.g. where someAction is a click or a navigation, and someWait is, for example, an HTTP request that gets made as a result of that click. In those cases, the request can sometimes happen while the click evaluates, and when that happens the test would stall if we first waited for the click, and then waited for the request (which by then had already happened and won't happen again).

But in this situation, and others in this PR, I feel like the campaignCards.first().waitFor() can always happen after the navigation. Isn't that right?

Here's what the documentation on waitFor says:

Returns when element specified by locator satisfies the state option.

If target element already satisfies the condition, the method returns immediately. Otherwise, waits for up to timeout milliseconds until the condition is met.

That makes me think that if await page.goto(appUri + '/organize/1/projects') finishes "late", once the campaign cards have already shown up, that won't be a problem because waitFor() will just return immediately and the test can proceed.

If I'm right, it means we don't have to use the Promise.all() construct here and I think that would be better as it would make the test more readable and "sequential".

richardolsson · 2024-02-27T04:25:26Z

integrationTesting/tests/organize/journeys/instance-detail/display.spec.ts

+    await Promise.all([
+      page.goto(appUri + '/organize/1/journeys/1/1'),
+      title.waitFor({ state: 'visible' }),
+      breadcrumbTitle.waitFor({ state: 'visible' }),
+    ]);
+
+    const numTitles = await title.count();
+    const numBreadcrumbTitles = await breadcrumbTitle.count();
+
+    expect(numTitles).toEqual(1);
+    expect(numBreadcrumbTitles).toEqual(1);


As an example, I think the important part of the fix here is the added uses of waitFor(), not the Promise.all(). I'm pretty sure you could do this instead?

await page.goto(appUri + '/organize/1/journeys/1/1'); await title.waitFor({ state: 'visible' }); await breadcrumbTitle.waitFor({ state: 'visible' }); const numTitles = await title.count(); const numBreadcrumbTitles = await breadcrumbTitle.count(); expect(numTitles).toEqual(1); expect(numBreadcrumbTitles).toEqual(1);

Do you agree that this pattern would make the code more readable?

You're 100% correct here I think. Reckon I've accidentally picked this up from those other ones you mentioned and not really thought much about it. Crossed my mind briefly at one point I think, but guess I got a little lost in the fun here. Will make this change real quick, thanks for raising this!

richardolsson · 2024-02-27T04:40:45Z

One more thing! Something seems to have changed about the fonts in this PR. The GIF below compares the current main (on app.dev.zetkin.org) with the preview of this branch. When I look in the network panel for font loading, I see that two different fonts are loaded. I have no idea what could be causing that. Do you know?

UPDATE: It looks like this code in _document.tsx is yielding an empty <script> tag on your branch, but one that contains the correct JS on main. Maybe some new React security feature?

<script>{'try{Typekit.load({ async: true })}catch(e){}'}</script>

henrycatalinismith · 2024-02-27T06:53:03Z

I've re-run yarn playwright:skipbuild --repeat-each=4 on aba1c20 and got 288 passing tests and zero failures, so looks good there! Font issue next!

henrycatalinismith · 2024-02-27T07:06:40Z

Okay the font issue is a pain! It doesn't reproduce in the dev server!!

I've tried running yarn build & yarn start to see it in the built version of the app, but that fails to boot for me due to setup steps I have yet to complete.

I'm tempted to push a commit along these lines into a branch and use the Vercel preview builds to check if it fixes the issue. Seems like you set those things up at the exact right time lol.

-          <script>{'try{Typekit.load({ async: true })}catch(e){}'}</script>
+          <script
+            // eslint-disable-next-line react/no-danger
+            dangerouslySetInnerHTML={{
+              __html: 'try{Typekit.load({ async: true })}catch(e){}',
+            }}
+          />

Reading the TypeKit docs at https://helpx.adobe.com/fonts/using/embed-codes.html#Changingtheembedcodeinawebsite, it sounds to me like this is an older form of embed code anyway. Another option here could be to try using the <link rel="stylesheet" href="https://use.typekit.net/xxxxxxx.css"> embed that they seem to prefer nowadays?

richardolsson · 2024-02-27T07:10:55Z

Okay the font issue is a pain! It doesn't reproduce in the dev server!!

I've tried running yarn build & yarn start to see it in the built version of the app, but that fails to boot for me due to setup steps I have yet to complete.

Try just using the .env.development for "production" (i.e. your local build) as well:

cp .env.development .env.production
yarn build
yarn start -p 3000

Reading the TypeKit docs at https://helpx.adobe.com/fonts/using/embed-codes.html#Changingtheembedcodeinawebsite, it sounds to me like this is an older form of embed code anyway. Another option here could be to try using the <link rel="stylesheet" href="https://use.typekit.net/xxxxxxx.css"> embed that they seem to prefer nowadays?

Yeah, I saw that, and if you want to try that change I'd be happy to make the switch. I'm not sure we will be using typekit much longer anyway. We'll see what happens with our current design system work, but for now it would be nice to fix this issue so that we will be able to merge this branch before that work is complete (which will be months).

henrycatalinismith · 2024-02-27T20:54:57Z

Okay cool, I ran with TypeKit's most up-to-date embed code in 1b40c75 and then used your handy advice about running yarn build locally to test the font loading before and after that commit!

Before	After

Seems to be working very nicely!

richardolsson

This is amazing. I've looked at this again because I'm frustrated that tests in other PRs seem flakier than usual, and I am almost ready to merge, but I have some comments below that I would like to discuss before doing so.

integrationTesting/tests/organize/views/detail/delete.spec.ts

richardolsson · 2024-03-17T05:28:26Z

integrationTesting/tests/organize/tags/remove.spec.ts

    ]);

    moxy.setZetkinApiMock(`/orgs/1/people/${ClaraZetkin.id}/tags`, 'get', []);

+    await tagToDelete.nth(2).waitFor({ state: 'hidden' });


The nth(2) feels a bit risky. Is this because the title of the tag is "Active" and that string also occurs in other places on the page?

This is now await tagToDelete.waitFor({ state: 'hidden' }); thanks to changing the tag from Activist to Plays guitar.

integrationTesting/tests/organize/tags/add.spec.ts

richardolsson

Let's get this thing merged! I believe in us. 🙏

henrycatalinismith added 18 commits February 26, 2024 19:23

Install canary versions of react and react-dom 18.3.0

0b6ce07

Try to fix campaign list test

3010797

Wait for render before counting elements

b6eb3a5

Wait for render before counting elements

6d6a005

Wait for render before counting elements

2873b26

Wait for visible state instead of asserting it

7ef42c4

Wait for render before counting elements

ebd0f49

Wait for render before counting elements

00c834a

Wait for status to visibly change before counting API calls

d3c7007

Wait for search button to render before invoking keyboard shortcut

d0a5b9a

Wait for render before counting elements

c7e7fe8

Wait for tag to appear on page before counting API calls

964ab3a

Wait for delete button to appear before clicking it

4e2d8db

Wait for date picker to appear before clicking it

8eab52d

Wait for tag to disappear from page before counting API calls

be0defc

Wait for buttons to appear before clicking them

a30f8c1

Add missing /orgs/1/people/views mock

d156b92

Wait for June 24 to appear before clicking it

8dc2578

henrycatalinismith requested a review from richardolsson February 26, 2024 22:48

vercel bot deployed to Preview February 27, 2024 04:00 View deployment

richardolsson reviewed Feb 27, 2024

View reviewed changes

Don't use Promise.all unnecessarily

aba1c20

vercel bot deployed to Preview February 27, 2024 07:33 View deployment

Load TypeKit using CSS link embed code instead

1b40c75

vercel bot deployed to Preview February 28, 2024 07:21 View deployment

This was referenced Mar 16, 2024

New survey page based on Next.js v13 using the app router and server actions #1756

Open

Install canary versions of react and react-dom 18.3.0 henrycatalinismith/app.zetkin.org#1

Closed

richardolsson reviewed Mar 17, 2024

View reviewed changes

Remove superstitious Promise.all and brittle nth() usage

b5ab00e

vercel bot deployed to Preview March 17, 2024 08:19 View deployment

Fix mistakes in previous commit

d5d7c50

richardolsson approved these changes Mar 17, 2024

View reviewed changes

richardolsson merged commit 3b6c2fa into zetkin:main Mar 17, 2024
4 of 5 checks passed

henrycatalinismith deleted the undocumented/react-18.3.0-plus-playwright-fixes branch March 17, 2024 09:07

vercel bot deployed to Preview March 17, 2024 09:13 View deployment

henrycatalinismith mentioned this pull request Mar 17, 2024

Reinstate .first() calls in playwright tests #1844

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update react & react-dom to 18.3.0 and fix playwright flakiness #1807

Update react & react-dom to 18.3.0 and fix playwright flakiness #1807

henrycatalinismith commented Feb 26, 2024 •

edited

richardolsson left a comment

richardolsson Feb 27, 2024

richardolsson Feb 27, 2024

richardolsson Feb 27, 2024

henrycatalinismith Feb 27, 2024

richardolsson commented Feb 27, 2024 •

edited

henrycatalinismith commented Feb 27, 2024

henrycatalinismith commented Feb 27, 2024

richardolsson commented Feb 27, 2024

henrycatalinismith commented Feb 27, 2024 •

edited

richardolsson left a comment

richardolsson Mar 17, 2024

henrycatalinismith Mar 17, 2024

richardolsson left a comment

Update react & react-dom to 18.3.0 and fix playwright flakiness #1807

Update react & react-dom to 18.3.0 and fix playwright flakiness #1807

Conversation

henrycatalinismith commented Feb 26, 2024 • edited

Description

Screenshots

Changes

Notes to reviewer

Related issues

richardolsson left a comment

Choose a reason for hiding this comment

richardolsson Feb 27, 2024

Choose a reason for hiding this comment

richardolsson Feb 27, 2024

Choose a reason for hiding this comment

richardolsson Feb 27, 2024

Choose a reason for hiding this comment

henrycatalinismith Feb 27, 2024

Choose a reason for hiding this comment

richardolsson commented Feb 27, 2024 • edited

henrycatalinismith commented Feb 27, 2024

henrycatalinismith commented Feb 27, 2024

richardolsson commented Feb 27, 2024

henrycatalinismith commented Feb 27, 2024 • edited

richardolsson left a comment

Choose a reason for hiding this comment

richardolsson Mar 17, 2024

Choose a reason for hiding this comment

henrycatalinismith Mar 17, 2024

Choose a reason for hiding this comment

richardolsson left a comment

Choose a reason for hiding this comment

henrycatalinismith commented Feb 26, 2024 •

edited

richardolsson commented Feb 27, 2024 •

edited

henrycatalinismith commented Feb 27, 2024 •

edited