Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Install canary versions of react and react-dom 18.3.0 #1

Closed
wants to merge 18 commits into from

Conversation

henrycatalinismith
Copy link
Owner

@henrycatalinismith henrycatalinismith commented Feb 26, 2024

Context

zetkin#1756 is being held up by playwright test failures. The update to react-dom 18.3.0 introduces lots of new failures. I don't want to bloat that PR with the investigatory work necessary to fix those failures. I also want to do that work out to the side in my own fork where nobody's builds are held up by my exploration, and where I can temporarily ignore some of the usual norms around issues and pull requests.

Tests

1) tests/organize/campaigns/list/list.spec.ts:18:3 › Campaigns list page  › shows list of campaigns  
  Error: expect(received).toEqual(expected) // deep equality
  Expected: 2
  Received: 0
    29 |       (items) => items.length
    30 |     );
  > 31 |     expect(numCampaignCards).toEqual(2);
        |                              ^
    32 |   });
    33 | });
    34 |
      at /home/runner/work/app.zetkin.org/app.zetkin.org/integrationTesting/tests/organize/campaigns/list/list.spec.ts:31:30
2) tests/organize/journeys/instance-detail/display.spec.ts:18:3 › Journey instance detail page › displays journey instance 
  Error: expect(received).toEqual(expected) // deep equality
  Expected: 1
  Received: 0
    37 |         )
    38 |         .count()
  > 39 |     ).toEqual(1);
        |       ^
    40 |
    41 |     // Check that the title is also in the breadcrumbs
    42 |     expect(
      at /home/runner/work/app.zetkin.org/app.zetkin.org/integrationTesting/tests/organize/journeys/instance-detail/display.spec.ts:39:7
3) tests/organize/journeys/instance-detail/reopen.spec.ts:19:5 › Journey instance details page › lets user reopen a closed instance › by clicking the reopen button on a closed case 
  TypeError: Cannot read properties of undefined (reading 'data')
    50 |       // Check patch request has correct data
    51 |       expect(
  > 52 |         patchInstanceMock.log<ZetkinJourneyInstance>()[0].data?.closed
        |         ^
    53 |       ).toEqual(null);
    54 |     });
    55 |   });
      at /home/runner/work/app.zetkin.org/app.zetkin.org/integrationTesting/tests/organize/journeys/instance-detail/reopen.spec.ts:52:9
4) tests/organize/journeys/instance-detail/sidebar.spec.ts:187:3 › Journey instance detail page sidebar › shows a list of subjects. 
  Error: expect(received).toEqual(expected) // deep equality
  Expected: 1
  Received: 0
    205 |         )
    206 |         .count()
  > 207 |     ).toEqual(1);
        |       ^
    208 |   });
    209 |
    210 |   //put request works
      at /home/runner/work/app.zetkin.org/app.zetkin.org/integrationTesting/tests/organize/journeys/instance-detail/sidebar.spec.ts:207:7
5) tests/organize/journeys/instance-milestones/milestones.spec.ts:20:3 › Journey instance page Milestones tab › displays milestones. 
  Error: expect(received).toEqual(expected) // deep equality
  Expected: 3
  Received: 0
    37 |     );
    38 |
  > 39 |     expect(journeyMilestoneCards).toEqual(3);
        |                                   ^
    40 |   });
    41 |
    42 |   test('displays message if there are no milestones.', async ({
      at /home/runner/work/app.zetkin.org/app.zetkin.org/integrationTesting/tests/organize/journeys/instance-milestones/milestones.spec.ts:39:35
6) tests/organize/journeys/list.spec.ts:18:3 › Journeys list page › shows list of journeys =======
  Error: expect(received).toEqual(expected) // deep equality
  Expected: 2
  Received: 0
    29 |     );
    30 |
  > 31 |     expect(numJourneysCards).toEqual(2);
        |                              ^
    32 |   });
    33 | });
    34 |
      at /home/runner/work/app.zetkin.org/app.zetkin.org/integrationTesting/tests/organize/journeys/list.spec.ts:31:30
7) tests/organize/search.spec.ts:27:3 › Search › opens the search dialog when entering "/" hotkey 
  Error: expect(received).toBeTruthy()
  Received: false
    44 |     expect(
    45 |       await page.locator('#SearchDialog-inputField').isVisible()
  > 46 |     ).toBeTruthy();
        |       ^
    47 |   });
    48 |
    49 |   test('shows error indicator if error fetching results', async ({
      at /home/runner/work/app.zetkin.org/app.zetkin.org/integrationTesting/tests/organize/search.spec.ts:46:7
8) tests/organize/tasks/detail/cover-image.spec.ts:94:3 › Task detail page › lets user reset cover image 
  Error: expect(received).toBeTruthy()
  Received: false
    125 |         .locator('data-testid=TaskPreviewSection-section >> img')
    126 |         .isVisible()
  > 127 |     ).toBeTruthy();
        |       ^
    128 |
    129 |     await Promise.all([
    130 |       page.waitForResponse((res) => res.request().method() == 'PATCH'),
      at /home/runner/work/app.zetkin.org/app.zetkin.org/integrationTesting/tests/organize/tasks/detail/cover-image.spec.ts:127:7
9) tests/organize/views/detail/display.spec.ts:28:3 › View detail page › displays view title and content to the user 
  Error: expect(received).toEqual(expected) // deep equality
  Expected: 1
  Received: 0
    33 |     expect(
    34 |       await page.locator('text=All KPD members >> visible=true').count()
  > 35 |     ).toEqual(1);
        |       ^
    36 |     expect(await page.locator('main >> text=Clara').count()).toEqual(1);
    37 |     expect(await page.locator('main >> text=Rosa').count()).toEqual(1);
    38 |   });
      at /home/runner/work/app.zetkin.org/app.zetkin.org/integrationTesting/tests/organize/views/detail/display.spec.ts:35:7

@henrycatalinismith
Copy link
Owner Author

Cool, 3010797 fixes Campaigns list page shows list of campaigns, so here's the thinking behind what I'm up to in that commit.

When I ran yarn playwright --debug and stepped through the test, it passed every time. Here's a screenshot of that, where you can see the two cards it's supposed to find.

Screenshot 2024-02-26 at 20 09 02

Whenever one of these browser automation tests starts passing when you step through it I always think that's a smoking gun for flakiness. So I figure before we do our assertion about how many cards there are, let's just chill a moment and give the cards a chance to render first.

As these tests only become flaky when the canary 18.3.0 version of react-dom is installed, my guess here is that they're changing something in this version which maybe causes some elements to render a frame or two earlier/later than before, and this is enough to break a bunch of our tests. Will work through the remaining failing tests in the same way.

@henrycatalinismith
Copy link
Owner Author

b6eb3a5 fixes Journey instance detail page sidebar shows a list of subjects in the same way as above.

@henrycatalinismith
Copy link
Owner Author

6d6a005 fixes Journey instance page Milestones tab displays milestones the same way again

@henrycatalinismith
Copy link
Owner Author

@henrycatalinismith
Copy link
Owner Author

@henrycatalinismith
Copy link
Owner Author

@henrycatalinismith
Copy link
Owner Author

d3c7007 fixes Journey instance details page lets user reopen a closed instance by clicking the reopen button on a closed case

This one was a little bit more fancy. There was a race condition between the Promise.all resolving and the patchInstanceMock updating. I reckon this is because the patchInstanceMock assertion is happening sooner than before. So I added a new test ID on the status card, and made the test wait for it to change to reflect the new status before checking patchInstanceMock. The element in question is the red "Closed" pill that becomes a green "Open" pill in this GIF.

2024-02-26 22 06 45

@henrycatalinismith
Copy link
Owner Author

Starting to get green builds here now but continuing to run yarn playwright:skipbuild --repeat-each=4 locally over and over and fixing the flaky tests that it surfaces.

@henrycatalinismith
Copy link
Owner Author

Finished, I think. Awaiting final review & merge over at zetkin#1807

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant