Flaky Playwright test regarding Web Feature field

[DanielRyanSmith]

Twice now I've seen Playwright flake on this test - I'm not sure the cause.

Screenshot diffs

Expected

Actual

Diff

[DanielRyanSmith]

Unfortunately, I also saw this error, which looks like a problem that I thought I had figured out a fix for:

https://github.com/GoogleChrome/chromium-dashboard/actions/runs/18545859658/job/52863535731

[DanielRyanSmith]

Gemini had a lot of say about our implementation when I showed it the source code for our edit-all tests. It might be really helpful here (I have not verified if this works).

My prompt

You are a master of writing Playwright tests. I have a playwright test file that I would like you to look over. I need you to tell me if it follows best practices. Also, tell me if it contains any code that would cause the tests to be flaky.

Here is the file:
{File's source code here}

Gemini's response

You've got a great start here. This file follows several key best practices, but it also contains the single most common cause of flaky tests: arbitrary delays.

Here is a breakdown of what you're doing well, where the flakiness is, and how to fix it.

---

🚨 Flakiness Analysis: The Problem with delay()

The repeated use of await delay() is the primary source of instability in this test file.

The Problem: A delay() forces Playwright to pause for a fixed amount of time (e.g., 500ms or 1500ms). This creates a race condition:

• If your app responds faster than the delay, your test suite is now artificially slow.
• If your app responds slower than the delay (due to network lag, a slow CI runner, or heavy database load), your test will fail, even though the application is working correctly.

Playwright was built specifically to solve this. Its auto-waiting mechanism means you should almost never use a hard-coded delay. Instead, you should wait for a specific condition to be true.

1. Flakiness in gotoEditAllPage

// BEFORE
async function gotoEditAllPage(page) {
    const editButton = page.locator('a[href^="/guide/editall/"]');
    await delay(500); // <-- Unnecessary. .click() will wait for the button.
    await editButton.click();
    await delay(1500); // <-- Flaky. What if the page takes 2s to load?
}

• How to Fix: After clicking, don't wait for time; wait for the result of the click. This could be the URL changing or a unique element on the new page appearing.

// AFTER (Option 1: Wait for URL)
async function gotoEditAllPage(page) {
    const editButton = page.locator('a[href^="/guide/editall/"]');
    await editButton.click();
    // Wait for the URL to change to the new page
    await page.waitForURL('**/guide/editall/**');
}

// AFTER (Option 2: Wait for an element on the new page)
async function gotoEditAllPage(page) {
    const editButton = page.locator('a[href^="/guide/editall/"]');
    await editButton.click();
    // Wait for a heading or other stable element on the "edit all" page
    await expect(page.locator('h1:has-text("Edit All Fields")')).toBeVisible();
}

2. Flakiness in test semantic checks

// BEFORE
    await devTrialDesktopInput.fill('100');
    await devTrialDesktopInput.blur(); // Must blur to trigger change event.
    await delay(500); // <-- Flaky. The assertion below will auto-wait.

    // Check that there is no error now...
    await expect(devTrailDesktopMilestoneLocator.locator('.check-warning')).toHaveCount(0);

All the delay(500) calls in this test are unnecessary and will cause flakes. When you call await expect(...), Playwright will automatically retry the assertion for a few seconds (your 30s timeout) until it passes.

If the .check-warning element takes 700ms to disappear, your old test would fail. The new test will simply wait 700ms, see the count is 0, and pass.

---

✅ Best Practices You're Using

You're doing a lot of things right:

• Test Isolation: Using beforeEach and afterEach to login and logout is excellent. This ensures each test starts from a clean, known state.
• Utility Functions: Abstracting logic into login, logout, and createNewFeature is a core concept of the Page Object Model (POM) and keeps your tests clean, readable, and maintainable (DRY).
• Clear Naming: Your test titles ('editall page', 'test semantic checks') are descriptive.
• Type Checking: Using // @ts-check and JSDoc comments (@param) is a great way to get type safety in JavaScript files.

---

💡 Recommendations for Improvement

Beyond removing the delays, here are two more "master-level" suggestions:

1. Use Functional Assertions, Not Just Screenshots

In 'test semantic checks', you use a screenshot to verify an error:

// Test that the error message is shown for invalid shipped date
await expect(page).toHaveScreenshot('shipped-desktop-error.png');

This is a visual test, but you're trying to do functional testing. This test is brittle. If a designer changes the error color from #FF0000 to #FE0000, your test will fail, even though the error message correctly appeared.

Recommendation: First, functionally assert that the error exists. Then, (optionally) take a screenshot to lock in the layout.

// AFTER
// 1. Functional Test: Verify the error message is visible and has the right text.
const shippedDesktopMilestoneLocator = page.locator('chromedash-form-field[name="shipped_milestone"]');
const errorLocator = shippedDesktopMilestoneLocator.locator('.check-warning');

await expect(errorLocator).toBeVisible();
await expect(errorLocator).toHaveText('Shipped milestone must be different from dev trial milestone.');

// 2. Visual Test: Now that functionality is confirmed, snapshot the look.
await expect(shippedDesktopMilestoneLocator).toHaveScreenshot('shipped-desktop-error.png');

2. Use User-Facing Locators

Your locators like input[name="shipped_milestone"] are good, but they are coupled to the HTML implementation. A more resilient best practice is to use locators that a user would see.

• Instead of: page.locator('input[name="shipped_milestone"]')

• Use: page.getByLabel('Shipped milestone') (This assumes there is a <label>)

• Instead of: page.locator('a[href^="/guide/editall/"]')

• Use: page.getByRole('link', { name: 'Edit all fields' })

This makes your tests more robust against code refactors that don't change the user-visible UI.

---

🔧 Refactored Test File

Here is the complete file with all flakiness removed and best practices applied.

// @ts-check
import { test, expect } from '@playwright/test';
import { captureConsoleMessages, login, logout, createNewFeature } from './test_utils';

/**
 * Starting from the feature page, goto 'Edit all fields'.
 * @param {import('@playwright/test').Page} page
 */
async function gotoEditAllPage(page) {
    // Use a more resilient, user-facing locator
    const editButton = page.getByRole('link', { name: 'Edit all fields' });
    await editButton.click();

    // Wait for the URL to update, indicating navigation is complete.
    // This is far more reliable than a fixed delay.
    await page.waitForURL('**/guide/editall/**');
}

test.beforeEach(async ({ page }, testInfo) => {
    captureConsoleMessages(page);
    // Note: The default timeout is 30000ms, so this line may be redundant
    // unless your global config has a shorter timeout.
    testInfo.setTimeout(30000);

    // Login before running each test.
    await login(page);
});

test.afterEach(async ({ page }) => {
    // Logout after running each test.
    await logout(page);
});

test('editall page', async ({ page }) => {
    await createNewFeature(page);
    await gotoEditAllPage(page);

    // Visual regression test is great here for a whole page.
    await expect(page).toHaveScreenshot('edit-all-fields.png');
});

test('test semantic checks', async ({ page }) => {
    await createNewFeature(page);
    await gotoEditAllPage(page);

    // Use user-facing locators where possible
    const devTrialDesktopInput = page.getByLabel('Dev trial desktop start'); // Assuming label exists
    await devTrialDesktopInput.fill('100');
    await devTrialDesktopInput.blur(); // Must blur to trigger change event.

    // Check that there is no error now for the dev trial milestone field
    // Playwright's `expect` will auto-wait for the count to become 0. No delay needed.
    const devTrailDesktopMilestoneLocator = page.locator('chromedash-form-field[name="dt_milestone_desktop_start"]');
    await expect(devTrailDesktopMilestoneLocator.locator('.check-warning')).toHaveCount(0);

    // Enter shipped desktop milestone of same number
    const shippedDesktopInput = page.getByLabel('Shipped desktop'); // Assuming label exists
    await shippedDesktopInput.fill('100');
    await shippedDesktopInput.blur(); // Must blur to trigger change event.

    // Scroll next field into view, so we can see the error.
    const shippedAndroidInput = page.getByLabel('Shipped Android'); // Assuming label exists
    await shippedAndroidInput.scrollIntoViewIfNeeded();

    // Test that the error message is shown for invalid shipped date
    const shippedDesktopMilestoneLocator = page.locator('chromedash-form-field[name="shipped_milestone"]');
    const errorLocator = shippedDesktopMilestoneLocator.locator('.check-warning');

    // 1. Functional check: Assert the error is visible and has correct text
    await expect(errorLocator).toBeVisible();
    await expect(errorLocator).toHaveText('Milestones must be different.'); // Example text

    // 2. Visual check: Now snapshot the element
    await expect(shippedDesktopMilestoneLocator).toHaveScreenshot('shipped-desktop-error.png');

    // Remove the cause of the error.
    await shippedDesktopInput.fill('');
    await shippedDesktopInput.blur(); // Must blur to trigger change event.

    // Check that there is no error. `expect` will auto-wait for the element to disappear.
    await expect(errorLocator).toHaveCount(0);
});

Would you like me to review your test_utils.js file next to see how we can optimize those helper functions?

[jrobbins]

I think I found the problem. There are a few places where we have
await page.locator(...);
but it needs to be
await page.locator(...).waitFor();
or some other action after the locator.
I'll try fixing those today.