fix: replace page.textContent with page.locator in integration tests
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:
- [x] 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
- [x] This message body should clearly illustrate what problems it solves.
- [ ] Ideally, include a test that fails without this PR but passes with it.
Tests
- [x] Run the tests with
pnpm testand lint the project withpnpm lintandpnpm check
Changesets
- [ ] If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running
pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.
Edits
- [x] 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/*usepage.textContentassertions. This is problematic because they are flaky by default -- they do not poll. Playwright recommends usingexpect(locator).toHaveTextinstead. Visit all of the projects inpackages/kit/test/apps, find their test files, and replace thepage.textContentusages withpage.locatorinstead. This may require you to move the locaiton ofawaitcalls from inside theexpectto 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.
⚠️ No Changeset found
Latest commit: 3199618f2d12582715641e4bf156c91798d21406
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
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.
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.
That being said, I think the ideal policy here would basically be:
- Switch everything to
page.locatorunless 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")
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:
https://github.com/sveltejs/kit/blob/9d1c523ad9965bd41352c363458718d9cb6fcad9/packages/kit/test/utils.js#L117
looks like the change introduced some strict mode violations which will need to be resolved
@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".
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: https://github.com/sveltejs/kit/issues/14668