Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Dec 3, 2025

page.textContent assertions are flaky because they don't poll—they fetch text content once and fail immediately if the element isn't ready. Playwright recommends using expect(locator).toHaveText() which auto-retries.

Changes

  • expect(await page.textContent(sel)).toBe(x)await expect(page.locator(sel)).toHaveText(x)
  • expect(await page.textContent(sel)).toContain(x)await expect(page.locator(sel)).toContainText(x)
  • Value captures for later comparison: await page.textContent(sel)await page.locator(sel).textContent()

Updated all test files in packages/kit/test/apps/*:

  • amp, basics, dev-only, embed, no-ssr, options, options-2, prerendered-app-error-pages, writes

Before/After

// Before (no retry, flaky)
expect(await page.textContent('h1')).toBe('Hello');

// After (auto-retries, robust)
await expect(page.locator('h1')).toHaveText('Hello');

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Edits

  • Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.
Original prompt

Right now, a lot of our integration tests (things in packages/kit/test/apps/* use page.textContent assertions. This is problematic because they are flaky by default -- they do not poll. Playwright recommends using expect(locator).toHaveText instead. Visit all of the projects in packages/kit/test/apps, find their test files, and replace the page.textContent usages with page.locator instead. This may require you to move the locaiton of await calls from inside the expect to outside of it. DO NOT substantively change the execution order, test order, or semantics of tests to make them pass.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@changeset-bot
Copy link

changeset-bot bot commented Dec 3, 2025

⚠️ No Changeset found

Latest commit: 3199618

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copilot AI and others added 3 commits December 3, 2025 18:34
Co-authored-by: elliott-with-the-longest-name-on-github <76245373+elliott-with-the-longest-name-on-github@users.noreply.github.com>
Co-authored-by: elliott-with-the-longest-name-on-github <76245373+elliott-with-the-longest-name-on-github@users.noreply.github.com>
Co-authored-by: elliott-with-the-longest-name-on-github <76245373+elliott-with-the-longest-name-on-github@users.noreply.github.com>
Copilot AI changed the title [WIP] Refactor integration tests to use page.locator assertions fix: replace page.textContent with page.locator in integration tests Dec 3, 2025
@Conduitry
Copy link
Member

I had argued in the past for something similar, taking advantage of the retrying nature of various Playwright commands. The counterargument at the time was that these commands that automatically retry are more suitable for applications than they are for frameworks, and that we wanted to be able to be more precise about what we were asserting for regarding the specific timing of different things changing than was afforded by the retrying commands. I don't know whether there's been a change of heart about all that since then, which would have been a couple years ago.

@elliott-with-the-longest-name-on-github
Copy link
Contributor

I think I would agree with that if that's what we were testing. But the reality is that every Playwright suite we have is not testing SvelteKit the framework, it's testing an application build with SvelteKit the framework. If we want specific, non-application-level tests that drill deeply into instantaneous behavior, we need to accomplish that at a unit-testing level. (All of this is very general, obviously, and I'm sure there are exceptions in either direction.)

Right now, I'm just experimenting with this to gauge the efficacy of Copilot's agent capabilities and to try to cut down on flakiness. I've been hammering the test suites locally for the last few days and pretty much every "1/1000" failure on a test that is "correct but flaky" is due to us screaming past a check that needs to wait for the DOM to get up to date with everything else.

@elliott-with-the-longest-name-on-github
Copy link
Contributor

That being said, I think the ideal policy here would basically be:

  • Switch everything to page.locator unless we're actually trying to gauge instantaneous behavior
  • Stick an ESLint rule in that disallows page.textContent (so that you have to actually annotate it with "yeah, this could be flaky, but that's actually the point")

@benmccann
Copy link
Member

I don't think this will generally help because we do some magic to make sure the page is loaded before doing the non-polling assertions:

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

@Rich-Harris
Copy link
Member

looks like the change introduced some strict mode violations which will need to be resolved

@elliott-with-the-longest-name-on-github
Copy link
Contributor

@benmccann right, it doesn't really change anything for the tests that do "nav, then check page content", but it absolutely does for anything that does "nav, then do anything and check for results".

@benmccann
Copy link
Member

I think we generally use the appropriate assertion method. Rich had been worried previously that if we did the polling thing we might cover over some framework timing issues and that as framework authors we should be stricter than app authors. The only test I see fail frequently on the CI is one where we're checking network requests and I don't know how to await those properly: #14668

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants