await inside expect(...) makes the check non-retrying, toBeHidden() passes on a locator that matches nothing, and a .first() is usually a strict-mode violation that got silenced rather than fixed. Then read the setup: waitForTimeout, page.route('**/*'), module-level variables and hardcoded URLs are the four that survive review and fail in CI weeks later.The pull request has eleven new spec files and 640 added lines. Every test passes locally and passes on the branch build. The reviewer opens the diff, sees code that reads like the Playwright documentation, scrolls to the bottom and approves.
\n\nSix of those tests cannot fail. Two of them pass because the page never finished loading and the assertion checked something that was already absent. One holds a five-second sleep that will become a thirty-second timeout the first time CI is under load. None of this is exotic — it is the same short list every time, because a language model producing Playwright code reaches for the same constructs. That makes review tractable. You are not auditing the model's reasoning. You are checking eight specific things.
\n\nPlaywright's own API reference marks page.waitForTimeout() Discouraged, with the note that it \"should only be used for debugging\" and that \"tests using the timer in production are going to be flaky\". Generated code uses it constantly, because a sleep is the shortest thing that makes a failing script pass.
// Generated\nawait page.getByRole('button', { name: 'Place order' }).click();\nawait page.waitForTimeout(3000);\nconst heading = await page.locator('h1').textContent();\nexpect(heading).toBe('Order confirmed');// Reviewed\nawait page.getByRole('button', { name: 'Place order' }).click();\nawait expect(page.getByRole('heading', { name: 'Order confirmed' })).toBeVisible();The arithmetic is worse than it looks. The default test timeout is 30 seconds, and time spent in the test function, fixture setup and beforeEach hooks all counts against it. Action timeouts, by contrast, have no default — actionTimeout is unset, so a click waits until the test itself runs out. Four three-second sleeps in a spec therefore hand 40% of the test's entire budget to a stopwatch. When the sleep is too short the test fails on a slow runner; when it is long enough to be safe it is charged to every run forever.
If the wait genuinely cannot be expressed as an element state — waiting on a queue to drain, say — the replacement is expect.poll() or expect(...).toPass(), both of which retry a condition rather than burning a fixed interval.
await that migrated inside expect()This is the single highest-value thing to grep for, because the two forms look nearly identical and behave completely differently.
\n\n// Generated — does not retry\nexpect(await page.getByText('welcome').isVisible()).toBe(true);\nexpect(await page.getByTestId('total').textContent()).toBe('$42.00');\n\n// Reviewed — retries until the assertion timeout\nawait expect(page.getByText('welcome')).toBeVisible();\nawait expect(page.getByTestId('total')).toHaveText('$42.00');Playwright splits its matchers into two documented groups. Auto-retrying assertions — toBeVisible(), toHaveText(), toHaveCount(), toHaveValue() and the rest of the locator and page matchers — \"will retry until the assertion passes, or the assertion timeout is reached\". Non-retrying assertions are the generic value matchers: toBe(), toEqual(), toContain(), toBeTruthy(). The documentation states plainly that \"using non-retrying assertions can lead to a flaky test\".
The moment you write await locator.isVisible(), you have resolved a boolean at one instant in time and handed it to toBe(), which has nothing left to retry. Playwright's best-practices guide gives exactly this pair as its 👎/👍 example and adds the operative sentence: with isVisible() \"the test won't wait a single second, it will just check the locator is there and return immediately.\"
The assertion timeout is 5 seconds by default and is independent of the 30-second test timeout, so the retrying form costs nothing when the element is already there. A useful review heuristic: any line containing both expect( and await where the await comes second is wrong.
.first() where a strict-mode violation used to beLocators in Playwright are strict: \"all operations on locators that imply some target DOM element will throw an exception if more than one element matches.\" When a generated locator matches three elements, the model's repair is almost always to append .first(), because that is the smallest edit that turns a red run green.
The documentation is unambiguous about the cost: first(), last() and nth() \"are not recommended because when your page changes, Playwright may click on an element you did not intend.\"
// Generated — clicks whichever \"Add to cart\" happens to render first\nawait page.getByRole('button', { name: 'Add to cart' }).first().click();// Reviewed — names the row, then acts inside it\nconst product = page\n .getByRole('listitem')\n .filter({ has: page.getByRole('heading', { name: 'Ceramic mug' }) });\n\nawait expect(product).toHaveCount(1);\nawait product.getByRole('button', { name: 'Add to cart' }).click();The toHaveCount(1) line is the part reviewers skip and shouldn't. It converts the ambiguity from a silent behaviour into an assertion: if a second matching row ever appears, the test tells you so instead of quietly acting on the wrong one.
Not every .first() is a defect. Playwright documents two legitimate uses: expect(locator.first()).toBeVisible() to assert that at least one item in a list is visible, and a.or(b).first() for the case where two alternative elements might both appear. The distinguishing question for a reviewer is whether the test means \"any of these\" or means \"the one I could not name\".
Two distinct constructions produce a test that is permanently green, and they need different fixes.
\n\nThe first exploits the definition of toBeHidden(), which \"ensures that Locator either does not resolve to any DOM node, or resolves to a non-visible one\". A locator with a typo in it resolves to nothing, and nothing is hidden:
// Generated — passes whether or not `.error-messge` was ever a real class\nawait expect(page.locator('.error-messge')).toBeHidden();\n\n// Reviewed — pin the positive state, so a wrong locator fails\nawait expect(page.getByRole('status')).toHaveText('Payment accepted');\nawait expect(page.getByRole('alert')).toHaveCount(0);toHaveCount(0) is also an auto-retrying assertion, so you lose nothing by asserting absence that way — but it is paired here with a positive assertion that a misspelled locator would break. An absence check on its own can never distinguish \"the error is gone\" from \"the test is looking in the wrong place\".
The second is the tautological mock. The test stubs a response, then asserts that the stubbed value appears on screen:
\n\n// Generated — the assertion restates the fixture\nawait page.route('**/api/cart', route => route.fulfill({\n json: { total: 4200, currency: 'USD', itemCount: 3 },\n}));\nawait page.goto('/cart');\nawait expect(page.getByTestId('total')).toContainText('4200');// Reviewed — asserts the transformation the component actually performs\nawait page.route('**/api/cart', route => route.fulfill({\n json: { total: 4200, currency: 'USD', itemCount: 3 },\n}));\nawait page.goto('/cart');\n\n// minor units -> localised currency, and pluralisation from itemCount\nawait expect(page.getByTestId('total')).toHaveText('$42.00');\nawait expect(page.getByTestId('summary')).toHaveText('3 items');The rule for the reviewer: if you can predict the expected string by reading the mock alone, the assertion is testing the mock. A useful mocked test asserts something the application computed — a format, a total, a sort order, a disabled state — that the fixture does not literally contain.
\n\npage.route() with a pattern that swallows the appInterception patterns are easy to write too widely, and the failure is delayed: the test passes today and starts failing when someone adds an endpoint.
\n\n// Generated — every request in the page, including HTML and JS, gets this body\nawait page.route('**/*', route => route.fulfill({\n status: 200,\n contentType: 'application/json',\n body: JSON.stringify({ items: [] }),\n}));// Reviewed — narrow the pattern, and let everything else through explicitly\nawait page.route('**/api/v1/orders', async route => {\n if (route.request().method() !== 'GET') return route.fallback();\n await route.fulfill({ json: { items: [] } });\n});Three documented behaviours make this section worth a reviewer's attention. Once routing is enabled, \"every request matching the url pattern will stall unless it's continued, fulfilled or aborted\" — a handler that forgets a branch hangs the request until the test times out, and the failure message points at an assertion rather than at the route. Second, \"if a request matches multiple registered routes, the most recently registered route takes precedence\", so a broad handler installed in beforeEach and a narrow one installed inside a test do not compose the way a reader expects. Third, \"enabling routing disables http cache\", which changes the timing profile of the page under test whether or not you intended to mock anything.
One more thing worth checking in the diff: page.route() does not intercept requests made by a Service Worker. Playwright's recommendation is to set serviceWorkers: 'block' in the context options when you rely on interception. A generated test against a PWA that mocks an endpoint and then asserts real data is usually this.
fullyParallelA model writing a spec file writes it as a script, and scripts keep state in variables at the top.
\n\n// Generated — one id shared by every test in the file\nlet orderId: string;\n\ntest('creates an order', async ({ page }) => {\n orderId = `order-${Date.now()}`;\n await page.goto(`/orders/new?id=${orderId}`);\n});\n\ntest('cancels the order', async ({ page }) => {\n await page.goto(`/orders/${orderId}`); // undefined under parallel mode\n});Playwright's parallelism guide states the constraint directly: \"parallel tests are executed in separate worker processes and cannot share any state or global variables.\" With fullyParallel: true the two tests above may land in different processes, where the second one reads its own module-level orderId, which is still undefined. Under the default mode — tests in one file run in order in one worker — the same code passes. That is why this defect reaches production config changes rather than review.
The documented fix for per-test data is testInfo.testId, which is unique per test:
test('cancels an order', async ({ page }, testInfo) => {\n const orderId = `order-${testInfo.testId}`;\n await page.goto(`/orders/new?id=${orderId}`);\n await page.getByRole('button', { name: 'Cancel order' }).click();\n await expect(page.getByRole('status')).toHaveText('Order cancelled');\n});Where setup is genuinely expensive, move it into a worker-scoped fixture keyed on testInfo.workerIndex, which the docs use for exactly this — one database user per worker, created once and torn down once. Note the index you key on: workerIndex is unique per worker process, while parallelIndex is between 0 and workers - 1 and is reused by a restarted worker after a failure. Playwright's own authentication recipe keys storage-state files on parallelIndex deliberately, so a restarted worker reuses the account file. Keying scratch data on it will collide.
Generated tests almost always carry an absolute URL in every goto() and a login sequence in every beforeEach, because each test was produced as a standalone artefact with no knowledge of your config.
// Generated — in every spec file in the PR\ntest.beforeEach(async ({ page }) => {\n await page.goto('https://staging.example.com/login');\n await page.getByLabel('Email').fill('qa-user@example.com');\n await page.getByLabel('Password').fill('Passw0rd!');\n await page.getByRole('button', { name: 'Sign in' }).click();\n});Set baseURL once in use and the relative form works everywhere; page.goto(), page.route(), page.waitForURL(), page.waitForRequest() and page.waitForResponse() all resolve against it. Then move the login into a setup project that writes storage state, and point the test projects at the file:
// playwright.config.ts\nexport default defineConfig({\n fullyParallel: true,\n use: { baseURL: process.env.BASE_URL ?? 'http://localhost:3000' },\n projects: [\n { name: 'setup', testMatch: /.*\\.setup\\.ts/ },\n {\n name: 'chromium',\n use: { ...devices['Desktop Chrome'], storageState: 'playwright/.auth/user.json' },\n dependencies: ['setup'],\n },\n ],\n});Playwright's guidance on the state file is worth repeating in a review comment, because generated code has no reason to know it: the storage-state file \"may contain sensitive cookies and headers that could be used to impersonate you or your test account\", and the docs strongly discourage committing it — playwright/.auth belongs in .gitignore. If you write it under the project's outputDir instead, it is cleaned up before every run.
toHaveScreenshot() used as a general-purpose assertionWhen a model cannot work out what to assert, it takes a picture. A visual assertion added on a whim behaves very differently from one added deliberately, and the first run hides it: with no baseline on disk, Playwright reports \"A snapshot doesn't exist at ..., writing actual\" and stores what it just saw. The reference committed by that PR is therefore whatever the page happened to look like on the author's machine.
\n\nSeveral defaults matter here. Baselines are named per browser and platform — example-test-1-chromium-darwin.png — so a snapshot generated on macOS has no counterpart on a Linux runner. The comparison uses pixelmatch with a threshold of 0.2 in YIQ colour space, while maxDiffPixels and maxDiffPixelRatio are unset by default, meaning a single pixel over threshold fails the assertion. Animations are already handled: animations defaults to 'disabled', and caret defaults to 'hide', so a generated test that adds a sleep \"to let animations finish\" is doing nothing.
// Reviewed — scope it, mask what is genuinely volatile, allow a small budget\nawait expect(page.getByTestId('invoice-summary')).toHaveScreenshot('invoice-summary.png', {\n mask: [page.getByTestId('generated-at'), page.getByTestId('avatar')],\n maxDiffPixels: 40,\n});Ask the author two questions: which baselines are in the PR, and on which platform were they generated. If the answer to the second is \"my laptop\" and CI is Linux, the assertion has never actually run.
\n\ntest.step that produces unreadable failures. Generated helpers wrap actions in steps, which is fine, but the failure then points at a line deep inside the helper rather than at the caller. test.step(name, fn, { box: true }) changes that: \"an error inside a boxed step points to the step call site.\" For page-object methods and shared login helpers, boxing is what makes a step useful in the HTML report rather than decorative.
expect.soft scattered through a flow. Soft assertions do not terminate the test but do mark it failed, which is right for a batch of independent field checks and wrong before a navigation — the test carries on clicking against a page it has already established is in the wrong state. Where a run of soft assertions must hold before the next step, the documented gate is explicit:
await expect.soft(page.getByTestId('status')).toHaveText('Success');\nawait expect.soft(page.getByTestId('eta')).toHaveText('1 day');\n\n// Do not continue into the next page if either check above failed.\nexpect(test.info().errors).toHaveLength(0);\nawait page.getByRole('link', { name: 'next page' }).click();Take the most recent AI-assisted test PR in your repository and run three greps over it before reading anything else: waitForTimeout, expect(await, and .first(). Those three cover the majority of what this article describes and take under a minute.
Then make the first two mechanical. Add @typescript-eslint/no-floating-promises to your test lint config — Playwright recommends it specifically to catch missing awaits before asynchronous API calls — and run tsc --noEmit in CI so signature errors surface without executing a browser. For the rest, run the branch once with fullyParallel: true and --repeat-each=3; shared module-level state and tautological mocks both show up under repetition in a way they never do in a single ordered run.
A passing test is evidence only if it is capable of failing. Break the feature deliberately — change the confirmation text, return a 500 from the endpoint the test mocks — and re-run. Any test still green after that is asserting something other than the behaviour it claims to cover, which is the exact failure mode of toBeHidden() on a mistyped locator and of an assertion that restates its own fixture.
.first() always a defect?No. Playwright documents it for \"at least one of these\" cases: expect(page.getByTestId('todo-item').first()).toBeVisible(), and newEmail.or(dialog).first() where either of two elements may appear. It is a defect when it exists to silence a strict-mode error the author could not otherwise resolve. Reading the surrounding assertion usually tells you which you have.
Not automatically — Playwright supports them and there are elements no role or text locator reaches. What the docs call out as bad practice is the long structural chain, giving #tsf > div:nth-child(2) > div.A8SBwf > div.RNNXgb > div > div.a4bIc > input as the example. A single stable class or attribute is defensible; anything with nth-child, a generated class hash, or four levels of descent is tied to markup that will change.
Partly, and it is worth doing. Beyond no-floating-promises, eslint-plugin-playwright ships no-wait-for-timeout, missing-playwright-await, valid-expect, no-force-option and no-conditional-in-test — between them, Tells 1 and 2 stop reaching review at all. What no linter can decide is whether an assertion is meaningful: whether toContainText('4200') is checking the application or restating the mock. That judgement stays human, which is why the assertions are where you should spend the review.
Regenerating changes which of these defects you get, not whether you get them, because the same constructs are the shortest path to a green run every time. The durable investment is making the environment reject them: baseURL and storageState in config so hardcoding is unnecessary, fullyParallel: true so shared state fails immediately, a banned-API lint rule, and a review pass focused on assertions.
expect.soft, test.info().errors, expect.poll and expect.toPassexpect(await locator.isVisible()), user-facing locators over CSS chains, and the recommended lint setupfirst()/last()/nth() are not recommended, filtering with has and hasText, and the long-CSS-chain anti-patterntoBeHidden() matching a non-existent node, and the toHaveScreenshot() defaults for animations, caret, threshold and maxDiffPixelswaitForTimeout() marked Discouraged, and route() stalling, precedence, cache and Service Worker behaviouractionTimeout having no defaulttestInfo.testId for per-test data, and workerIndex versus parallelIndex after a restartstorageState, the warning against committing the state file, and per-worker accountsbaseURL and which APIs resolve relative URLs against itmaxDiffPixelstest.step() and the box option that reports errors at the step call sitefallback() versus continue(), and the reverse-registration order in which matching handlers runno-wait-for-timeout, missing-playwright-await and valid-expect