playwright: fix race conditions causing flakiness

README.md

@@ -413,6 +413,80 @@ 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.
 
+### Diagnosing a failure that has no visible cause
+
+A blank page, a control that never appears, and a page that renders slowly all
+look identical from the outside: an assertion times out and says only what it
+was waiting for. Failing tests therefore carry a `browser-diagnostics.log`
+attachment in the HTML report, holding what the browser reported while the test
+ran - uncaught exceptions, console errors and warnings, requests that failed,
+and any response of 400 or above, each stamped with the time since the test
+started.
+
+```
++  1429ms  HTTP 404    GET .../apps/chrome/operator-generated/fed-modules.json
++  2759ms  CONSOLE WARNING Unsatisfied version * from undefined of shared
+                           singleton module react-router-dom (required =6.30.4)
++  3210ms  HTTP 400    GET .../api/rbac/v1/access/?application=inventory
+```
+
+Passing tests attach nothing. Warnings have a smaller budget than errors,
+because the console shell emits them in bursts and they would otherwise crowd
+out the entries worth reading.
+
+### 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/browserDiagnostics.ts

@@ -0,0 +1,77 @@
+import { test as base } from '@playwright/test';
+
+// Attaches what the browser reported to any test that fails. A blank page or a
+// control that never appears looks identical from the outside whether the cause
+// was a JS exception, a failed module load, or a backend returning 502, and the
+// assertion that times out can say nothing about which it was.
+//
+// Only failures produce an attachment, so a green run stays quiet.
+
+// Capped so that a page erroring in a loop cannot produce an unreadable
+// attachment. Warnings get their own smaller budget because the console shell
+// emits them in bursts, and they would otherwise crowd out the errors.
+const MAX_ERRORS = 150;
+const MAX_WARNINGS = 30;
+
+export type BrowserDiagnosticsFixture = {
+  browserDiagnostics: void;
+};
+
+export const test = base.extend<BrowserDiagnosticsFixture>({
+  browserDiagnostics: [
+    async ({ page }, use, testInfo) => {
+      const startedAt = Date.now();
+      const entries: string[] = [];
+      let errorCount = 0;
+      let warningCount = 0;
+
+      const record = (line: string, isWarning = false) => {
+        if (isWarning && warningCount >= MAX_WARNINGS) return;
+        if (!isWarning && errorCount >= MAX_ERRORS) return;
+        if (isWarning) warningCount++;
+        else errorCount++;
+        const at = String(Date.now() - startedAt).padStart(6);
+        entries.push(`+${at}ms  ${line}`);
+      };
+
+      page.on('pageerror', (error) =>
+        record(`PAGEERROR   ${error.message.split('\n')[0]}`),
+      );
+
+      page.on('console', (message) => {
+        const type = message.type();
+        if (type !== 'error' && type !== 'warning') return;
+        record(
+          `CONSOLE ${type.toUpperCase().padEnd(7)} ${message.text().slice(0, 300)}`,
+          type === 'warning',
+        );
+      });
+
+      page.on('requestfailed', (request) =>
+        record(
+          `REQ FAILED  ${request.method()} ${request.url().slice(0, 160)} ` +
+            `- ${request.failure()?.errorText ?? 'unknown'}`,
+        ),
+      );
+
+      page.on('response', (response) => {
+        if (response.status() < 400) return;
+        record(
+          `HTTP ${response.status()}    ${response.request().method()} ` +
+            `${response.url().slice(0, 160)}`,
+        );
+      });
+
+      await use(undefined);
+
+      if (testInfo.status === testInfo.expectedStatus) return;
+      if (entries.length === 0) return;
+
+      await testInfo.attach('browser-diagnostics.log', {
+        body: entries.join('\n'),
+        contentType: 'text/plain',
+      });
+    },
+    { auto: true },
+  ],
+});

playwright/fixtures/customizations.ts

@@ -3,15 +3,19 @@ import { mergeTests } from '@playwright/test';
 
 import { test as ariaHiddenTest } from './ariaHiddenWorkaround';
 import { blockAnalyticsTest } from './blockAnalytics';
+import { test as browserDiagnosticsTest } from './browserDiagnostics';
 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
 export const test = mergeTests(
   ariaHiddenTest,
   blockAnalyticsTest,
+  browserDiagnosticsTest,
   cleanupTest,
   coverageTest,
+  networkChaosTest,
   popupTest,
 );