Skip to content

chore: upgrade Playwright #15089

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

Merged
elliott-with-the-longest-name-on-github merged 10 commits into from
Dec 24, 2025
Merged

chore: upgrade Playwright #15089

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ff08ba3
chore: Upgrade Playwright
elliott-with-the-longest-name-on-github Dec 23, 2025
895ab46
checkpoint, not sure what else is going wrong
elliott-with-the-longest-name-on-github Dec 23, 2025
7b5899c
i have never been so happy to see a test failure
elliott-with-the-longest-name-on-github Dec 23, 2025
03d937e
fix lockfile maybe
elliott-with-the-longest-name-on-github Dec 23, 2025
28cb818
fix: remove playwright
elliott-with-the-longest-name-on-github Dec 23, 2025
e6e3334
more flaky tests
elliott-with-the-longest-name-on-github Dec 24, 2025
ef0ef30
fix clicknav
elliott-with-the-longest-name-on-github Dec 24, 2025
85ac507
fix another flaky test
elliott-with-the-longest-name-on-github Dec 24, 2025
fe67356
improve further
elliott-with-the-longest-name-on-github Dec 24, 2025
54e9645
another clicknav usage
elliott-with-the-longest-name-on-github Dec 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 11 additions & 11 deletions packages/kit/test/apps/basics/test/client.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1104,23 +1104,23 @@ test.describe('data-sveltekit attributes', () => {
});

test('data-sveltekit-reload', async ({ baseURL, page, clicknav }) => {
/** @type {string[]} */
const requests = [];
page.on('request', (r) => requests.push(r.url()));

await page.goto('/data-sveltekit/reload');
let request_promise = page.waitForRequest(`${baseURL}/data-sveltekit/reload/target`);
await clicknav('#one');
expect(requests).toContain(`${baseURL}/data-sveltekit/reload/target`);
await request_promise;

requests.length = 0;
await page.goto('/data-sveltekit/reload');
request_promise = page.waitForRequest(`${baseURL}/data-sveltekit/reload/target`);
await clicknav('#two');
expect(requests).toContain(`${baseURL}/data-sveltekit/reload/target`);
await request_promise;

requests.length = 0;
await page.goto('/data-sveltekit/reload');
request_promise = page.waitForRequest(`${baseURL}/data-sveltekit/reload/target`, {
timeout: 1000
});
request_promise.catch(() => {});
await clicknav('#three');
expect(requests).not.toContain(`${baseURL}/data-sveltekit/reload/target`);
await expect(request_promise).rejects.toThrow();
});

test('data-sveltekit-noscroll', async ({ page, clicknav }) => {
Expand Down Expand Up @@ -1649,9 +1649,9 @@ test.describe('Shallow routing', () => {
});

test.describe('reroute', () => {
test('Apply reroute during client side navigation', async ({ page }) => {
test('Apply reroute during client side navigation', async ({ page, clicknav }) => {
await page.goto('/reroute/basic');
await page.click("a[href='/reroute/basic/a']");
await clicknav('a[href="/reroute/basic/a"]', { waitForURL: '/reroute/basic/a' });
expect(await page.textContent('h1')).toContain(
'Successfully rewritten, URL should still show a: /reroute/basic/a'
);
Expand Down
73 changes: 36 additions & 37 deletions packages/kit/test/apps/basics/test/cross-platform/client.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,14 @@ test.describe('a11y', () => {

await page.goto('/accessibility/a');

await clicknav('[href="/accessibility/b"]');
expect(await page.innerHTML('h1')).toBe('b');
await clicknav('[href="/accessibility/b"]', { waitForURL: '/accessibility/b' });
expect(await page.evaluate(() => (document.activeElement || {}).nodeName)).toBe('BODY');
await page.keyboard.press(tab);

expect(await page.evaluate(() => (document.activeElement || {}).nodeName)).toBe('BUTTON');
expect(await page.evaluate(() => (document.activeElement || {}).textContent)).toBe('focus me');

await clicknav('[href="/accessibility/a"]');
await clicknav('[href="/accessibility/a"]', { waitForURL: '/accessibility/a' });
expect(await page.innerHTML('h1')).toBe('a');
expect(await page.evaluate(() => (document.activeElement || {}).nodeName)).toBe('BODY');

Expand All @@ -36,7 +35,9 @@ test.describe('a11y', () => {
test('applies autofocus after a navigation', async ({ page, clicknav }) => {
await page.goto('/accessibility/autofocus/a');

await clicknav('[href="/accessibility/autofocus/b"]');
await clicknav('[href="/accessibility/autofocus/b"]', {
waitForURL: '/accessibility/autofocus/b'
});
expect(await page.innerHTML('h1')).toBe('b');
expect(await page.evaluate(() => (document.activeElement || {}).nodeName)).toBe('INPUT');
});
Expand Down Expand Up @@ -388,14 +389,14 @@ test.describe('Scrolling', () => {
await page.goto('/anchor');
await page.locator('#scroll-anchor').click();
const originalScrollY = /** @type {number} */ (await page.evaluate(() => scrollY));
await clicknav('#routing-page');
await page.goBack();
await clicknav('#routing-page', { waitForURL: '/routing/hashes/target' });

await expect(page).toHaveURL('/anchor#last-anchor-2');
await page.goBack();
await page.waitForURL('/anchor#last-anchor-2');
expect(await page.evaluate(() => scrollY)).toEqual(originalScrollY);

await page.goBack();
await expect(page).toHaveURL('/anchor');
await page.waitForURL('/anchor');
expect(await page.evaluate(() => scrollY)).toEqual(0);
});

Expand Down Expand Up @@ -734,14 +735,10 @@ test.describe('Prefetching', () => {
test('same route hash links work more than once', async ({ page, clicknav, baseURL }) => {
await page.goto('/routing/hashes/a');

await clicknav('[href="#preload"]');
expect(page.url()).toBe(`${baseURL}/routing/hashes/a#preload`);
Copy link
Member

@benmccann benmccann Dec 24, 2025

Choose a reason for hiding this comment

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

I'm not entirely sure we should remove these expect lines. perhaps the current waitForURL implementation checks that, but I'd like to fix this up so that we don't use waitForURL and I'm afraid it'd be easy to miss that we're no longer expecting anything while making that change

Copy link
Contributor Author

@elliott-with-the-longest-name-on-github elliott-with-the-longest-name-on-github Dec 24, 2025

Choose a reason for hiding this comment

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

If we're asserting that we've arrived at a URL, waitForURL is superior to expect(page.url()).toBe(...). It throws a helpful message after the configured timeout if the URL is never arrived at, and the test fails. It's not an explicit expect, but with Playwright you don't always end up with one -- like in this case, we're just making sure the URL actually has been updated to what we expected it to be.


await clicknav('[href="/routing/hashes/a"]');
expect(page.url()).toBe(`${baseURL}/routing/hashes/a`);
await clicknav('[href="#preload"]', { waitForURL: `${baseURL}/routing/hashes/a#preload` });

await clicknav('[href="#preload"]');
expect(page.url()).toBe(`${baseURL}/routing/hashes/a#preload`);
await clicknav('[href="/routing/hashes/a"]', { waitForURL: `${baseURL}/routing/hashes/a` });
await clicknav('[href="#preload"]', { waitForURL: `${baseURL}/routing/hashes/a#preload` });
});

test('does not rerun load on calls to duplicate preload hash route', async ({ app, page }) => {
Expand Down Expand Up @@ -802,19 +799,19 @@ test.describe('Routing', () => {
expect(await page.textContent('#page-url-hash')).toBe('#target');
});

test('page.url.hash is correctly set on navigation', async ({ page }) => {
test('page.url.hash is correctly set on navigation', async ({ page, clicknav }) => {
await page.goto('/routing/hashes/pagestate');
expect(await page.textContent('#window-hash')).toBe('');
expect(await page.textContent('#page-url-hash')).toBe('');
await page.locator('[href="#target"]').click();
expect(await page.textContent('#window-hash')).toBe('#target');
expect(await page.textContent('#page-url-hash')).toBe('#target');
await page.locator('[href="/routing/hashes/pagestate"]').click();
await expect(page.locator('#window-hash')).toHaveText('');
await expect(page.locator('#page-url-hash')).toHaveText('');
await clicknav('[href="#target"]');
await expect(page.locator('#window-hash')).toHaveText('#target');
await expect(page.locator('#page-url-hash')).toHaveText('#target');
await clicknav('[href="/routing/hashes/pagestate"]');
await expect(page.locator('#window-hash')).toHaveText('#target'); // hashchange doesn't fire for these
await expect(page.locator('#page-url-hash')).toHaveText('');
await page.goBack();
expect(await page.textContent('#window-hash')).toBe('#target');
expect(await page.textContent('#page-url-hash')).toBe('#target');
await expect(page.locator('#window-hash')).toHaveText('#target');
await expect(page.locator('#page-url-hash')).toHaveText('#target');
});

test('clicking on a hash link focuses the associated element', async ({ page }) => {
Expand All @@ -832,12 +829,14 @@ test.describe('Routing', () => {
baseURL
}) => {
await page.goto('/data-sveltekit/reload/hash');
await page.locator('a[href="#example"]').click();
expect(page.url()).toBe(`${baseURL}/data-sveltekit/reload/hash#example`);
await clicknav('a[href="/data-sveltekit/reload/hash/new"]');
expect(page.url()).toBe(`${baseURL}/data-sveltekit/reload/hash/new`);
await clicknav('a[href="#example"]', {
waitForURL: `${baseURL}/data-sveltekit/reload/hash#example`
});
await clicknav('a[href="/data-sveltekit/reload/hash/new"]', {
waitForURL: `${baseURL}/data-sveltekit/reload/hash/new`
});
await page.goBack();
expect(page.url()).toBe(`${baseURL}/data-sveltekit/reload/hash#example`);
await page.waitForURL(`${baseURL}/data-sveltekit/reload/hash#example`);
await expect(page.getByRole('textbox')).toBeVisible();
});

Expand All @@ -862,11 +861,9 @@ test.describe('Routing', () => {
await page.goto('/routing/hashes/a');

await page.locator('[href="#hash-target"]').click();
expect(page.url()).toBe(`${baseURL}/routing/hashes/a#hash-target`);
await page.waitForURL(`${baseURL}/routing/hashes/a#hash-target`);

await clicknav('[href="/routing/hashes/b"]');
expect(await page.textContent('h1')).toBe('b');

await expect(page.locator('h1')).toHaveText('b');
await page.goBack();
await expect(page.locator('h1')).toHaveText('a');
Expand All @@ -879,14 +876,16 @@ test.describe('Routing', () => {
}) => {
await page.goto('/routing/hashes/a');

await clicknav('[href="#hash-target"]');
expect(page.url()).toBe(`${baseURL}/routing/hashes/a#hash-target`);
Copy link
Member

@benmccann benmccann Dec 24, 2025

Choose a reason for hiding this comment

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

I don't think you should have to change these lines because clicknav does a waitForNavigation. If that's not working it's probably better to fix it inside clicknav so that there's a only a single location that needs to be updated

kit/packages/kit/test/utils.js

Line 36 in 85a57a0

await Promise.all([page.waitForNavigation(options), element.click()]);

Copy link
Contributor Author

@elliott-with-the-longest-name-on-github elliott-with-the-longest-name-on-github Dec 24, 2025

Choose a reason for hiding this comment

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

I think even if we updated clicknav it would still have to be updated to accept the "expected URL" as the second parameter. waitForNavigation probably won't ever be reliable (see microsoft/playwright#20853).

Copy link
Member

@benmccann benmccann Dec 24, 2025

Choose a reason for hiding this comment

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

Ah, interesting! It's so dumb that they have an API that is discouraged and not deprecated at the same time 😝

Doing something along the lines of microsoft/playwright#20853 (comment) would make a lot of sense. We already do something similar with body.started:

kit/packages/kit/test/utils.js

Line 117 in 85a57a0

await page.waitForSelector('body.started', { timeout: 15000 });

Perhaps we could change from a boolean body.started to a timestamp body.lastNav or something along those lines. It'd keep the API on the caller's side much simpler

Copy link
Contributor Author

@elliott-with-the-longest-name-on-github elliott-with-the-longest-name-on-github Dec 24, 2025

Choose a reason for hiding this comment

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

I don't think that'll work as it won't actually work for clientside navs (it relies on window being destroyed and recreated during the nav). That being said maybe it could wait on afterNavigation or something?

For now I'm adding waitForURL as an option to it, which will make it significantly more reliable in the case where you're going clicknav(/* I want to go to a location, and wait until I've arrived at that location */)

Copy link
Member

@benmccann benmccann Dec 24, 2025

Choose a reason for hiding this comment

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

Yeah, it looks like we probably have access to afterNavigate:

kit/packages/kit/test/utils.js

Line 20 in 85a57a0

afterNavigate: () => page.evaluate(() => afterNavigate(() => {})),

I'd really much prefer a solution using that as I think it'd be much easier to author tests that way. It's also something that I imagine could then be followed as a pattern in end user apps

Copy link
Contributor Author

@elliott-with-the-longest-name-on-github elliott-with-the-longest-name-on-github Dec 24, 2025

Choose a reason for hiding this comment

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

I don't actually think it'll work, unfortunately -- it has to be linked to a component lifecycle ☹️

await clicknav('[href="#hash-target"]', {
waitForURL: `${baseURL}/routing/hashes/a#hash-target`
});

await clicknav('[href="#replace-state"]');
expect(page.url()).toBe(`${baseURL}/routing/hashes/a#replace-state`);
await clicknav('[href="#replace-state"]', {
waitForURL: `${baseURL}/routing/hashes/a#replace-state`
});

await page.goBack();
expect(page.url()).toBe(`${baseURL}/routing/hashes/a`);
await page.waitForURL(`${baseURL}/routing/hashes/a`);
});

test('does not normalize external path', async ({ page, start_server }) => {
Expand Down
5 changes: 4 additions & 1 deletion packages/kit/test/types.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,10 @@ export const test: TestType<
preloadCode(pathname: string): Promise<void>;
preloadData(url: string): Promise<void>;
};
clicknav(selector: string, options?: Parameters<Page['waitForNavigation']>[0]): Promise<void>;
clicknav(
selector: string,
options?: { timeout?: number; waitForURL?: string }
): Promise<void>;
scroll_to(x: number, y: number): Promise<void>;
in_view(selector: string): Promise<boolean>;
get_computed_style(selector: string, prop: string): Promise<string>;
Expand Down
10 changes: 7 additions & 3 deletions packages/kit/test/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,16 @@ export const test = base.extend({
clicknav: async ({ page, javaScriptEnabled }, use) => {
/**
* @param {string} selector
* @param {{ timeout: number }} options
* @param {{ timeout?: number, waitForURL?: string }} [options]
*/
async function clicknav(selector, options) {
async function clicknav(selector, { timeout, waitForURL } = {}) {
const element = page.locator(selector);
if (javaScriptEnabled) {
await Promise.all([page.waitForNavigation(options), element.click()]);
const promises = [page.waitForNavigation({ timeout }), element.click()];
if (waitForURL) {
promises.push(page.waitForURL(waitForURL, { timeout }));
}
await Promise.all(promises);
} else {
await element.click();
}
Expand Down
42 changes: 21 additions & 21 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion pnpm-workspace.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ catalog:
'@netlify/functions': ^5.0.0
'@opentelemetry/sdk-node': ^0.208.0
'@opentelemetry/sdk-trace-node': ^2.0.1
'@playwright/test': ^1.51.1
'@playwright/test': 1.56.0
'@polka/url': ^1.0.0-next.28
'@rollup/plugin-commonjs': ^28.0.1
'@rollup/plugin-json': ^6.1.0
Expand Down