playwright: fix race conditions causing flakiness

README.md

@@ -413,6 +413,59 @@ PLAYWRIGHT_STATIC_PASSWORD="<your_static_user_password>"
 
 For local development purposes, you can use the same credentials as for `PLAYWRIGHT_USER` and `PLAYWRIGHT_PASSWORD` if you are using your stage account for those.
 
+### Finding flaky tests with injected latency
+
+Races between the browser and the API are hard to reproduce on demand. The
+window is usually a few hundred milliseconds wide, and it only matters when a
+test happens to click inside it. Setting `PW_CHAOS=1` delays the app's own API
+responses so those windows open wide enough to hit deliberately.
+
+```bash
+PW_CHAOS=1 npx playwright test --project="UI tests" --repeat-each=5
+```
+
+What finds bugs here is **reordering**, not slowness. Playwright waits for
+elements to become actionable, so uniformly slower responses are absorbed and
+nothing fails. Delays are drawn from a heavy tailed distribution instead: most
+requests pass straight through and a small share are held for up to two
+seconds, which is long enough for one response to overtake another issued
+before it. Code that assumes replies arrive in the order they were sent breaks
+under this. Code that does not, does not.
+
+Delays are capped below `actionTimeout`, so slowness on its own can never fail
+a test - anything that goes red under chaos is a real defect. Only requests to
+`**/api/**` are delayed, since holding bundles and fonts adds wall clock
+without producing anything worth finding.
+
+#### Reproducing a specific failure
+
+Every test records the seed it used as an annotation, visible in the HTML
+report:
+
+```
+chaos-seed     PW_CHAOS_SEED=2223827117 (test seed 729962994)
+chaos-summary  175 requests delayed, max 1982ms, total 19208ms
+```
+
+Passing that seed back replays the same delays:
+
+```bash
+PW_CHAOS=1 PW_CHAOS_SEED=2223827117 npx playwright test --project="UI tests"
+```
+
+Reproduction is close but not exact. Delays are drawn per request in dispatch
+order, so a run that issues its requests in a different order gets a different
+assignment - narrowing to a single spec is enough to change that. When a seed
+does not reproduce, raising `--repeat-each` is usually faster than chasing it.
+
+#### What it tends to find
+
+Failures under chaos point at state that outlives the request it came from:
+a cached answer that a slower, older response overwrites; a control that is
+enabled before the data behind it has arrived; an effect that re-runs after the
+component has already moved on. A failure that only appears under chaos is
+still real. It just needs an unlucky user rather than an unlucky test.
+
 ## Playwright Boot tests
 
 This section describes what Playwright Boot tests are, how they work and how to run them locally.

playwright.config.ts

@@ -28,24 +28,39 @@ export default defineConfig({
   reporter: reporters,
   globalTimeout: 89.5 * 60 * 1000, // 1h29.5m, Set because of codebuild, we want PW to timeout before CB to get the results.
   timeout: 3 * 60 * 1000, // 3m
-  expect: { timeout: 50_000 }, // 50s
+  // Assertions that genuinely need to wait longer say so at the call site.
+  // A high default hides which those are, and turns every ordinary failure
+  // into a slow one.
+  expect: { timeout: 15_000 }, // 15s
   use: {
     actionTimeout: 30_000, // 30s
     navigationTimeout: 30_000, // 30s
     headless: true,
+    // Specs assert on rendered dates. Without this the browser follows the
+    // host clock, so a date the wizard stores as UTC midnight renders as the
+    // previous day anywhere west of Greenwich - passing in CI and failing on
+    // a developer's machine.
+    timezoneId: 'UTC',
     baseURL: process.env.BASE_URL
       ? process.env.BASE_URL
       : 'http://127.0.0.1:9090',
     video: 'retain-on-failure',
-    trace: 'on',
+    // 'on' kept a trace for every passing test too, which is hundreds of
+    // megabytes of artifacts nobody opens. 'on-first-retry' would record
+    // nothing locally, where retries are 0, so match the video setting and
+    // keep a trace whenever there is a failure to look at.
+    trace: 'retain-on-failure',
 
     ignoreHTTPSErrors: true,
   },
   projects: [
     { name: 'Setup', testMatch: /.*\.setup\.ts/ },
     {
       name: 'UI tests',
-      timeout: 29.5 * 60 * 1000, // 29.5m
+      // The slowest attempt observed is under 3m. This is only a ceiling on
+      // how long a hung test can burn before it is killed, and at 29.5m one
+      // could take a third of globalTimeout with it.
+      timeout: 5 * 60 * 1000, // 5m
       use: {
         ...devices['Desktop Chrome'],
         storageState: '.auth/user.json',

playwright/Basic/imageMode.spec.ts

@@ -44,7 +44,7 @@ test('Image mode blueprint create, edit, export, import', async ({
   // Skip the test if the image mode flag is not enabled in this environment
   const imageModeToggle = frame.getByRole('button', { name: 'Image mode' });
   try {
-    await expect(imageModeToggle).toBeVisible({ timeout: 10000 });
+    await expect(imageModeToggle).toBeVisible();
   } catch {
     test.skip(true, 'Image mode flag not enabled');
   }
@@ -64,8 +64,8 @@ test('Image mode blueprint create, edit, export, import', async ({
     let imageSourceDropdown = frame.getByRole('button', {
       name: /Red Hat Enterprise Linux|RHEL/i,
     });
-    await expect(imageSourceDropdown).toBeVisible({ timeout: 10000 });
-    await expect(imageSourceDropdown).toBeEnabled({ timeout: 5000 });
+    await expect(imageSourceDropdown).toBeVisible();
+    await expect(imageSourceDropdown).toBeEnabled();
     await imageSourceDropdown.click();
     await frame.getByRole('option', { name: 'Fedora Hummingbird' }).click();
 
@@ -88,8 +88,8 @@ test('Image mode blueprint create, edit, export, import', async ({
       name: /fedora hummingbird/i,
     });
     await imageSourceDropdown.click();
-    await expect(imageSourceDropdown).toBeVisible({ timeout: 10000 });
-    await expect(imageSourceDropdown).toBeEnabled({ timeout: 5000 });
+    await expect(imageSourceDropdown).toBeVisible();
+    await expect(imageSourceDropdown).toBeEnabled();
     await frame
       .getByRole('option', { name: 'Red Hat Enterprise Linux (RHEL)' })
       .click();
@@ -99,14 +99,14 @@ test('Image mode blueprint create, edit, export, import', async ({
     const imageSourceDropdown = frame.getByRole('button', {
       name: /Red Hat Enterprise Linux|RHEL/i,
     });
-    await expect(imageSourceDropdown).toBeVisible({ timeout: 10000 });
-    await expect(imageSourceDropdown).toBeEnabled({ timeout: 5000 });
+    await expect(imageSourceDropdown).toBeVisible();
+    await expect(imageSourceDropdown).toBeEnabled();
     await imageSourceDropdown.click();
 
     const rhelSourceOption = frame
       .getByRole('option', { name: /RHEL/i })
       .first();
-    await expect(rhelSourceOption).toBeVisible({ timeout: 10000 });
+    await expect(rhelSourceOption).toBeVisible();
     await rhelSourceOption.click();
 
     await expect(
@@ -126,7 +126,6 @@ test('Image mode blueprint create, edit, export, import', async ({
     const reviewImageButton = frame.getByRole('button', {
       name: 'Review image',
     });
-    await expect(reviewImageButton).toBeEnabled({ timeout: 10000 });
     await reviewImageButton.click();
     await createBlueprint(frame, blueprintName);
   });
@@ -158,7 +157,6 @@ test('Image mode blueprint create, edit, export, import', async ({
     const reviewImageButton = frame.getByRole('button', {
       name: 'Review image',
     });
-    await expect(reviewImageButton).toBeEnabled({ timeout: 10000 });
     await reviewImageButton.click();
     await frame
       .getByRole('button', { name: 'Save changes to blueprint' })
@@ -185,7 +183,7 @@ test('Image mode blueprint create, edit, export, import', async ({
     const importedImageMode = frame.getByRole('button', {
       name: 'Image mode',
     });
-    await expect(importedImageMode).toBeVisible({ timeout: 10000 });
+    await expect(importedImageMode).toBeVisible();
     await expect(importedImageMode).toHaveAttribute('aria-pressed', 'true');
 
     // Export doesn't include image_requests, so image types must be re-selected

playwright/Customizations/Registration.spec.ts

@@ -415,7 +415,6 @@ registrationModes.forEach(
           const saveButton = frame.getByRole('button', {
             name: 'Save changes to blueprint',
           });
-          await expect(saveButton).toBeEnabled();
           await saveButton.click();
         });
 

playwright/Customizations/Repositories.spec.ts

@@ -5,6 +5,7 @@ import {
   createRepositoryViaApi,
   deleteRepositoryByUrlViaApi,
   deleteRepositoryViaApi,
+  waitForIntrospection,
 } from '../helpers/apiHelpers';
 import { isHosted } from '../helpers/helpers';
 import { ensureAuthenticated } from '../helpers/login';
@@ -21,8 +22,11 @@ import {
   registerLater,
 } from '../helpers/wizardHelpers';
 
-const REPOSITORY_URL =
-  'https://jlsherrill.fedorapeople.org/fake-repos/really-empty/';
+// Content sources allows one repository per url per organization, and this
+// spec claims the url by deleting whatever already holds it. Every spec
+// therefore needs a url of its own - sharing one with RepeatableBuild meant
+// this test deleted that test's repository out from under it mid-run.
+const REPOSITORY_URL = 'https://jlsherrill.fedorapeople.org/fake-repos/signed/';
 
 test('Create blueprint with repository and test edit mode removal', async ({
   page,
@@ -50,6 +54,9 @@ test('Create blueprint with repository and test edit mode removal', async ({
       snapshot: false,
     });
     repositoryUuid = repository.uuid;
+    // The wizard lists repositories filtered by architecture and version, and
+    // a repository does not qualify until content sources has introspected it.
+    await waitForIntrospection(page, repositoryName);
   });
 
   cleanup.add(() => deleteBlueprint(page, blueprintName));

playwright/fixtures/customizations.ts

@@ -5,6 +5,7 @@ import { test as ariaHiddenTest } from './ariaHiddenWorkaround';
 import { blockAnalyticsTest } from './blockAnalytics';
 import { test as cleanupTest } from './cleanup';
 import { test as coverageTest } from './coverage';
+import { test as networkChaosTest } from './networkChaos';
 import { test as popupTest } from './popupHandler';
 
 // Combine the fixtures into one
@@ -13,5 +14,6 @@ export const test = mergeTests(
   blockAnalyticsTest,
   cleanupTest,
   coverageTest,
+  networkChaosTest,
   popupTest,
 );

playwright/fixtures/networkChaos.ts

@@ -0,0 +1,121 @@
+import { test as base } from '@playwright/test';
+
+// Fault injection for shaking out client-side request races.
+//
+// Uniform latency finds very little: Playwright's auto-waiting absorbs a
+// slower-but-still-ordered backend. What breaks code is RESPONSE REORDERING -
+// an earlier request resolving after a later one, so a stale closure wins and
+// writes obsolete state. Reordering only happens when the spread between
+// delays exceeds the natural spacing between requests, so the distribution is
+// deliberately heavy tailed rather than uniform.
+//
+// Enable with PW_CHAOS=1. Every test records the seed it used as an
+// annotation; replay a specific failure with PW_CHAOS_SEED=<that seed>.
+
+const CHAOS_ENABLED = process.env.PW_CHAOS === '1';
+
+// Only the app's own API traffic. Delaying bundles and fonts adds wall clock
+// and load timeouts without producing any reordering worth finding.
+const TARGET = '**/api/**';
+
+type Band = { weight: number; min: number; max: number };
+
+// Measured stage latency sits around 320ms, so the tail has to reach well past
+// that for one response to overtake another.
+const BANDS: Band[] = [
+  // Most requests pass straight through. A few milliseconds of jitter would
+  // not reorder anything, and holding every request open while it elapsed put
+  // real load on the dev proxy.
+  { weight: 70, min: 0, max: 0 },
+  { weight: 20, min: 50, max: 400 }, // same order as real latency
+  { weight: 10, min: 400, max: 2000 }, // the tail that causes overtaking
+];
+
+const TOTAL_WEIGHT = BANDS.reduce((sum, b) => sum + b.weight, 0);
+
+// mulberry32 - small, fast, and good enough that a seed reproduces a run.
+const makeRng = (seed: number) => {
+  let a = seed >>> 0;
+  return () => {
+    a = (a + 0x6d2b79f5) >>> 0;
+    let t = Math.imul(a ^ (a >>> 15), 1 | a);
+    t = (t + Math.imul(t ^ (t >>> 7), 61 | t)) ^ t;
+    return ((t ^ (t >>> 14)) >>> 0) / 4294967296;
+  };
+};
+
+const pickDelay = (rng: () => number): number => {
+  let roll = rng() * TOTAL_WEIGHT;
+  for (const band of BANDS) {
+    roll -= band.weight;
+    if (roll <= 0) {
+      return Math.floor(band.min + rng() * (band.max - band.min));
+    }
+  }
+  return 0;
+};
+
+// Distinct per test so parallel workers scramble differently, but derived from
+// the run seed so the whole suite replays from one env var.
+const deriveSeed = (runSeed: number, testId: string): number => {
+  let h = runSeed >>> 0;
+  for (let i = 0; i < testId.length; i++) {
+    h = (Math.imul(h ^ testId.charCodeAt(i), 0x01000193) + 1) >>> 0;
+  }
+  return h;
+};
+
+export type NetworkChaosFixture = {
+  networkChaos: void;
+};
+
+export const test = base.extend<NetworkChaosFixture>({
+  networkChaos: [
+    async ({ page }, use, testInfo) => {
+      if (!CHAOS_ENABLED) {
+        await use(undefined);
+        return;
+      }
+
+      const runSeed = process.env.PW_CHAOS_SEED
+        ? Number(process.env.PW_CHAOS_SEED)
+        : Math.floor(Math.random() * 0xffffffff);
+      const seed = deriveSeed(
+        runSeed,
+        `${testInfo.titlePath.join(' > ')}#${testInfo.repeatEachIndex}`,
+      );
+      const rng = makeRng(seed);
+
+      testInfo.annotations.push({
+        type: 'chaos-seed',
+        description: `PW_CHAOS_SEED=${runSeed} (test seed ${seed})`,
+      });
+
+      const delays: number[] = [];
+
+      await page.route(TARGET, async (route) => {
+        const delay = pickDelay(rng);
+        delays.push(delay);
+        if (delay > 0) {
+          await new Promise((resolve) => setTimeout(resolve, delay));
+        }
+        // fallback rather than continue so other route handlers still apply
+        await route.fallback();
+      });
+
+      await use(undefined);
+
+      await page.unroute(TARGET);
+
+      if (delays.length > 0) {
+        const max = Math.max(...delays);
+        const total = delays.reduce((a, b) => a + b, 0);
+        testInfo.annotations.push({
+          type: 'chaos-summary',
+          description: `${delays.length} requests delayed, max ${max}ms, total ${total}ms`,
+        });
+      }
+    },
+    { auto: true },
+  ],
+});