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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 22 commits into from
Mar 17, 2024
Merged

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

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
0b6ce07
Install canary versions of react and react-dom 18.3.0
henrycatalinismith Feb 26, 2024
3010797
Try to fix campaign list test
henrycatalinismith Feb 26, 2024
b6eb3a5
Wait for render before counting elements
henrycatalinismith Feb 26, 2024
6d6a005
Wait for render before counting elements
henrycatalinismith Feb 26, 2024
2873b26
Wait for render before counting elements
henrycatalinismith Feb 26, 2024
7ef42c4
Wait for visible state instead of asserting it
henrycatalinismith Feb 26, 2024
ebd0f49
Wait for render before counting elements
henrycatalinismith Feb 26, 2024
00c834a
Wait for render before counting elements
henrycatalinismith Feb 26, 2024
d3c7007
Wait for status to visibly change before counting API calls
henrycatalinismith Feb 26, 2024
d0a5b9a
Wait for search button to render before invoking keyboard shortcut
henrycatalinismith Feb 26, 2024
c7e7fe8
Wait for render before counting elements
henrycatalinismith Feb 26, 2024
964ab3a
Wait for tag to appear on page before counting API calls
henrycatalinismith Feb 26, 2024
4e2d8db
Wait for delete button to appear before clicking it
henrycatalinismith Feb 26, 2024
8eab52d
Wait for date picker to appear before clicking it
henrycatalinismith Feb 26, 2024
be0defc
Wait for tag to disappear from page before counting API calls
henrycatalinismith Feb 26, 2024
a30f8c1
Wait for buttons to appear before clicking them
henrycatalinismith Feb 26, 2024
d156b92
Add missing /orgs/1/people/views mock
henrycatalinismith Feb 26, 2024
8dc2578
Wait for June 24 to appear before clicking it
henrycatalinismith Feb 26, 2024
aba1c20
Don't use Promise.all unnecessarily
henrycatalinismith Feb 27, 2024
1b40c75
Load TypeKit using CSS link embed code instead
henrycatalinismith Feb 27, 2024
b5ab00e
Remove superstitious Promise.all and brittle nth() usage
henrycatalinismith Mar 17, 2024
d5d7c50
Fix mistakes in previous commit
henrycatalinismith Mar 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 6 additions & 5 deletions integrationTesting/tests/organize/campaigns/list/list.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".


const numCampaignCards = await page.$$eval(
'data-testid=campaign-card',
(items) => items.length
);
const numCampaignCards = await campaignCards.count();
expect(numCampaignCards).toEqual(2);
});
});
37 changes: 18 additions & 19 deletions integrationTesting/tests/organize/journeys/instance-detail/display.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,24 +27,23 @@ test.describe('Journey instance detail page', () => {
ClarasOnboarding
);

await page.goto(appUri + '/organize/1/journeys/1/1');

// Check that the title is visible in the right place
expect(
await page
.locator(
`[data-testid=page-title]:has-text("${ClarasOnboarding.title}")`
)
.count()
).toEqual(1);

// Check that the title is also in the breadcrumbs
expect(
await page
.locator(
`[aria-label=breadcrumb]:has-text("${ClarasOnboarding.title}")`
)
.count()
).toEqual(1);
const title = page.locator(
`[data-testid=page-title]:has-text("${ClarasOnboarding.title}")`
);
const breadcrumbTitle = page.locator(
`[aria-label=breadcrumb]:has-text("${ClarasOnboarding.title}")`
);

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

});
});
10 changes: 9 additions & 1 deletion integrationTesting/tests/organize/journeys/instance-detail/reopen.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,13 @@ test.describe('Journey instance details page', () => {
ClarasOnboarding
);

const openStatus = page.locator(
'data-testid=journey-status >> text=Open'
);
const reopenButton = page.locator(
'data-testid=JourneyInstanceReopenButton'
);

await page.goto(appUri + '/organize/1/journeys/1/1');

await Promise.all([
Expand All @@ -44,7 +51,8 @@ test.describe('Journey instance details page', () => {
`**/orgs/${KPD.id}/journey_instances/${ClarasOnboarding.id}`
),
// Click close instance button
page.locator('data-testid=JourneyInstanceReopenButton').click(),
reopenButton.click(),
openStatus.waitFor({ state: 'visible' }),
]);

// Check patch request has correct data
Expand Down
34 changes: 18 additions & 16 deletions integrationTesting/tests/organize/journeys/instance-detail/sidebar.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,15 +30,16 @@ test.describe('Journey instance detail page sidebar', () => {
ClarasOnboarding
);

await page.goto(appUri + '/organize/1/journeys/1/1');
const assignees = page.locator(
`[data-testid=ZetkinSection-assignees] [data-testid=JourneyPerson-${ClarasOnboarding.assignees[0].id}]`
);
await Promise.all([
page.goto(appUri + '/organize/1/journeys/1/1'),
assignees.first().waitFor({ state: 'visible' }),
]);

expect(
await page
.locator(
`[data-testid=ZetkinSection-assignees] [data-testid=JourneyPerson-${ClarasOnboarding.assignees[0].id}]`
)
.count()
).toEqual(1);
const numAssignees = await assignees.count();
expect(numAssignees).toEqual(1);
});

test('lets user assign new person to the journey instance', async ({
Expand Down Expand Up @@ -196,15 +197,16 @@ test.describe('Journey instance detail page sidebar', () => {
ClarasOnboarding
);

await page.goto(appUri + '/organize/1/journeys/1/1');
const subjects = page.locator(
`[data-testid=ZetkinSection-subjects] [data-testid=JourneyPerson-${ClarasOnboarding.subjects[0].id}]`
);
await Promise.all([
page.goto(appUri + '/organize/1/journeys/1/1'),
subjects.first().waitFor({ state: 'visible' }),
]);

expect(
await page
.locator(
`[data-testid=ZetkinSection-subjects] [data-testid=JourneyPerson-${ClarasOnboarding.subjects[0].id}]`
)
.count()
).toEqual(1);
const numSubjects = await subjects.count();
expect(numSubjects).toEqual(1);
});

//put request works
Expand Down
36 changes: 22 additions & 14 deletions integrationTesting/tests/organize/journeys/instance-milestones/milestones.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,16 @@ test.describe('Journey instance page Milestones tab', () => {
ClarasOnboarding
);

await page.goto(appUri + '/organize/1/journeys/1/1/milestones');

const journeyMilestoneCards = await page.$$eval(
'data-testid=JourneyMilestoneCard',
(items) => items.length
const journeyMilestoneCards = page.locator(
'data-testid=JourneyMilestoneCard'
);
await Promise.all([
page.goto(appUri + '/organize/1/journeys/1/1/milestones'),
journeyMilestoneCards.first().waitFor({ state: 'visible' }),
]);

expect(journeyMilestoneCards).toEqual(3);
const numCards = await journeyMilestoneCards.count();
expect(numCards).toEqual(3);
});

test('displays message if there are no milestones.', async ({
Expand Down Expand Up @@ -90,22 +92,28 @@ test.describe('Journey instance page Milestones tab', () => {
{ ...AttendMeeting, deadline: newDeadline }
);

await page.goto(appUri + '/organize/1/journeys/1/1/milestones');
const datePicker = page.locator(
'[data-testid=JourneyMilestoneCard] [data-testid=JourneyMilestoneCard-datePicker] button[aria-label*="Choose"]'
);
const june24 = page.locator('button:has-text("24")');

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

//Click datepicker in first JourneyMilestoneCard
await page
.locator(
'[data-testid=JourneyMilestoneCard] [data-testid=JourneyMilestoneCard-datePicker] button[aria-label*="Choose"]'
)
.first()
.click();
await Promise.all([
datePicker.first().click(),
june24.waitFor({ state: 'visible' }),
]);

await Promise.all([
page.waitForResponse(
`**/orgs/${KPD.id}/journey_instances/${ClarasOnboarding.id}/milestones/${AttendMeeting.id}`
),
//Click June 24 to trigger setting new deadline
await page.locator('button:has-text("24")').click(),
await june24.click(),
]);

//Expect the deadline to be the newly set deadline
Expand Down
12 changes: 6 additions & 6 deletions integrationTesting/tests/organize/journeys/list.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,13 @@ test.describe('Journeys list page', () => {
MarxistTraining,
]);

await page.goto(appUri + '/organize/1/journeys');

const numJourneysCards = await page.$$eval(
'data-testid=journey-card',
(items) => items.length
);
const journeyCards = page.locator('data-testid=journey-card');
await Promise.all([
page.goto(appUri + '/organize/1/journeys'),
journeyCards.first().waitFor({ state: 'visible' }),
]);

const numJourneysCards = await journeyCards.count();
expect(numJourneysCards).toEqual(2);
});
});
20 changes: 14 additions & 6 deletions integrationTesting/tests/organize/search.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,21 @@ test.describe('Search', async () => {
SpeakToFriendAboutReferendum,
]);

await page.goto(appUri + '/organize/1/projects/1');
await page.keyboard.press('/');
const searchButton = page.locator('data-testid=SearchDialog-activator');
const searchField = page.locator('#SearchDialog-inputField');

// Check dialog open
expect(
await page.locator('#SearchDialog-inputField').isVisible()
).toBeTruthy();
await Promise.all([
page.goto(appUri + '/organize/1/projects/1'),
searchButton.waitFor({ state: 'visible' }),
]);

await Promise.all([
page.keyboard.press('/'),
searchField.waitFor({ state: 'visible' }),
]);

const isVisible = await searchField.isVisible();
expect(isVisible).toBeTruthy();
});

test('shows error indicator if error fetching results', async ({
Expand Down
18 changes: 15 additions & 3 deletions integrationTesting/tests/organize/tags/add.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,21 +33,33 @@ test.describe('Tag manager', () => {
'get',
[]
);

const { log: putTagLog } = moxy.setZetkinApiMock(
`/orgs/1/people/${ClaraZetkin.id}/tags/${ActivistTag.id}`,
'put'
);

await page.goto(appUri + `/organize/1/people/${ClaraZetkin.id}`);
const addTagButton = page.locator('text=Add tag');
const activist = page.locator('text=Activist');

await Promise.all([
page.goto(appUri + `/organize/1/people/${ClaraZetkin.id}`),
addTagButton.first().waitFor({ state: 'visible' }),
]);

await page.locator('text=Add tag').click();

await activist.first().waitFor({ state: 'visible' });

// Select tag
await activist.first().click();

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

// Select tag
await page.click('text=Activist');
// Wait for the tag to appear on the page
await activist.nth(1).waitFor({ state: 'visible' });
richardolsson marked this conversation as resolved.
Show resolved Hide resolved

// Expect to have made request to put tag
expect(putTagLog().length).toEqual(1);
Expand Down
18 changes: 15 additions & 3 deletions integrationTesting/tests/organize/tags/remove.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,16 +37,28 @@ test.describe('Tags manager', () => {
'delete'
);

await page.goto(appUri + `/organize/1/people/${ClaraZetkin.id}`);
const tagToDelete = page.locator(`text="${ActivistTag.title}"`);
const deleteButton = page.locator('[data-testid=TagChip-deleteButton]');

await Promise.all([
page.goto(appUri + `/organize/1/people/${ClaraZetkin.id}`),
tagToDelete.waitFor({ state: 'visible' }),
]);

await Promise.all([
tagToDelete.hover(),
deleteButton.waitFor({ state: 'visible' }),
]);

await page.locator(`text="${ActivistTag.title}"`).hover();
await Promise.all([
page.waitForRequest((req) => req.method() == 'DELETE'),
page.locator('[data-testid=TagChip-deleteButton]').click(),
deleteButton.click(),
]);

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

await tagToDelete.nth(2).waitFor({ state: 'hidden' });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// Expect to have made request to delete tag
expect(deleteTagLog().length).toEqual(1);
});
Expand Down
12 changes: 5 additions & 7 deletions integrationTesting/tests/organize/tasks/detail/cover-image.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,13 +118,11 @@ test.describe('Task detail page', () => {
SpeakToFriend
);

await page.goto(appUri + '/organize/1/projects/1/calendar/tasks/1');

expect(
await page
.locator('data-testid=TaskPreviewSection-section >> img')
.isVisible()
).toBeTruthy();
const image = page.locator('data-testid=TaskPreviewSection-section >> img');
await Promise.all([
page.goto(appUri + '/organize/1/projects/1/calendar/tasks/1'),
image.waitFor({ state: 'visible' }),
]);

await Promise.all([
page.waitForResponse((res) => res.request().method() == 'PATCH'),
Expand Down