diff --git a/README.md b/README.md index f8ac9622f4..877ed30c7a 100644 --- a/README.md +++ b/README.md @@ -413,6 +413,80 @@ PLAYWRIGHT_STATIC_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. diff --git a/playwright.config.ts b/playwright.config.ts index a5d71f389a..d03d763ddf 100644 --- a/playwright.config.ts +++ b/playwright.config.ts @@ -28,16 +28,28 @@ 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, }, @@ -45,7 +57,10 @@ export default defineConfig({ { 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', diff --git a/playwright/Basic/imageMode.spec.ts b/playwright/Basic/imageMode.spec.ts index fb8340cc8e..3199d2f9c3 100644 --- a/playwright/Basic/imageMode.spec.ts +++ b/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 diff --git a/playwright/Customizations/Registration.spec.ts b/playwright/Customizations/Registration.spec.ts index c04d7d6646..30db865504 100644 --- a/playwright/Customizations/Registration.spec.ts +++ b/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(); }); diff --git a/playwright/Customizations/Repositories.spec.ts b/playwright/Customizations/Repositories.spec.ts index c39eee4e56..d383b6ebac 100644 --- a/playwright/Customizations/Repositories.spec.ts +++ b/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)); diff --git a/playwright/fixtures/browserDiagnostics.ts b/playwright/fixtures/browserDiagnostics.ts new file mode 100644 index 0000000000..1a894b7d1d --- /dev/null +++ b/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({ + 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 }, + ], +}); diff --git a/playwright/fixtures/customizations.ts b/playwright/fixtures/customizations.ts index 783abacfe1..3f408af703 100644 --- a/playwright/fixtures/customizations.ts +++ b/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, ); diff --git a/playwright/fixtures/networkChaos.ts b/playwright/fixtures/networkChaos.ts new file mode 100644 index 0000000000..31dedb6b67 --- /dev/null +++ b/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=. + +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({ + 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 }, + ], +}); diff --git a/playwright/helpers/apiHelpers.ts b/playwright/helpers/apiHelpers.ts index ea17abf6de..c9366c0769 100644 --- a/playwright/helpers/apiHelpers.ts +++ b/playwright/helpers/apiHelpers.ts @@ -1,4 +1,4 @@ -import { APIResponse, Page } from '@playwright/test'; +import { APIResponse, expect, Page } from '@playwright/test'; /** * Call API and return the response @@ -93,8 +93,15 @@ type RepositoryResponse = { uuid: string; name: string; url: string; + snapshot?: boolean; + status?: string; + last_introspection_time?: string; }; +// Introspection of a small repository takes seconds, but the backend gives no +// completion signal, so we poll. +const INTROSPECTION_TIMEOUT = 90_000; + export const createRepositoryViaApi = async ( page: Page, repository: RepositoryRequest, @@ -138,58 +145,98 @@ export const deleteRepositoryViaApi = async ( } }; -// Ensures a repository with the given name exists. If it already exists, -// returns the existing repo. If not, creates it. Handles race conditions -// where a concurrent test run creates the repo between the check and -// the create attempt. -export const ensureRepositoryExists = async ( +const findRepositoryByName = async ( page: Page, - repository: RepositoryRequest, -): Promise => { + name: string, +): Promise => { const headers = await getAuthHeaders(page); - - // Check if the repo already exists by name - const searchResponse = await page + const response = await page .context() .request.get( - `/api/content-sources/v1/repositories/?name=${encodeURIComponent(repository.name)}`, + `/api/content-sources/v1/repositories/?name=${encodeURIComponent(name)}`, { headers }, ); - if (searchResponse.status() === 200) { - const body = await searchResponse.json(); - const existing = body.data?.find( - (r: { name: string }) => r.name === repository.name, - ); - if (existing) { - return existing; - } + if (response.status() !== 200) { + return undefined; + } + + const body = await response.json(); + return body.data?.find((r: RepositoryResponse) => r.name === name); +}; + +const normalizeUrl = (url: string) => url.replace(/\/+$/, ''); + +const matchesRequest = ( + repo: RepositoryResponse, + request: RepositoryRequest, +): boolean => + normalizeUrl(repo.url) === normalizeUrl(request.url) && + !!repo.snapshot === !!request.snapshot; + +// A repository is only usable by a test once it has been introspected. Before +// that the wizard still disables it, but with "we are still learning about it" +// rather than whatever reason the test is asserting on, so waiting here keeps +// that timing out of the specs. +export const waitForIntrospection = async ( + page: Page, + name: string, +): Promise => { + await expect + .poll( + async () => { + const repo = await findRepositoryByName(page, name); + if (!repo) return 'missing'; + if (!repo.last_introspection_time) return 'not introspected yet'; + return repo.status ?? 'unknown'; + }, + { + message: `Repository "${name}" never became usable`, + timeout: INTROSPECTION_TIMEOUT, + intervals: [500, 1000, 2000, 5000], + }, + ) + .toBe('Valid'); +}; + +// Ensures a repository with the given name exists, matches the requested +// configuration, and has been introspected. Handles a concurrent run creating +// the repository between the search and the create attempt. +export const ensureRepositoryExists = async ( + page: Page, + repository: RepositoryRequest, +): Promise => { + const existing = await findRepositoryByName(page, repository.name); + + // Matching on the name alone would reuse a repository left behind with the + // wrong url or snapshot setting, which then fails every later run for a + // reason that looks nothing like stale fixture data. + if (existing && !matchesRequest(existing, repository)) { + await deleteRepositoryViaApi(page, existing.uuid); } - // Repo doesn't exist, create it - try { - return await createRepositoryViaApi(page, repository); - } catch { - // Another run may have created it concurrently, try searching again - const retryResponse = await page - .context() - .request.get( - `/api/content-sources/v1/repositories/?name=${encodeURIComponent(repository.name)}`, - { headers }, - ); - - if (retryResponse.status() === 200) { - const body = await retryResponse.json(); - const existing = body.data?.find( - (r: { name: string }) => r.name === repository.name, - ); - if (existing) { - return existing; + if (!existing || !matchesRequest(existing, repository)) { + try { + await createRepositoryViaApi(page, repository); + } catch { + // A concurrent run may have created it in the meantime. + if (!(await findRepositoryByName(page, repository.name))) { + throw new Error( + `Failed to create or find repository "${repository.name}"`, + ); } } + } + + await waitForIntrospection(page, repository.name); - throw new Error(`Failed to create or find repository "${repository.name}"`); + const repo = await findRepositoryByName(page, repository.name); + if (!repo) { + throw new Error( + `Repository "${repository.name}" disappeared after introspection`, + ); } + return repo; }; export const deleteRepositoryByUrlViaApi = async ( diff --git a/playwright/helpers/login.ts b/playwright/helpers/login.ts index af56dd5371..857998338f 100644 --- a/playwright/helpers/login.ts +++ b/playwright/helpers/login.ts @@ -1,6 +1,6 @@ import path from 'path'; -import { expect, type Page } from '@playwright/test'; +import { expect, type Locator, type Page } from '@playwright/test'; import { closePopupsIfExist, isHosted } from './helpers'; import { ibFrame } from './navHelpers'; @@ -36,38 +36,92 @@ export const login = async (page: Page, staticUser: boolean = false) => { return loginCockpit(page, user, password); }; -/** - * Checks if the user is already authenticated, if not, logs them in - * @param page - the page object - * @param staticUser - if true, use the static user instead of dynamically created one - */ +// How long the app gets to render before we give up. The landing page loads +// chrome, the federated module, and the blueprint and image lists, so this is +// not instant on a loaded CI runner. +const AUTH_TIMEOUT = 30_000; + +type AuthState = 'app' | 'login' | 'neither'; + +const detectAuthState = async ( + appHeading: Locator, + loginField: Locator, +): Promise => { + // Checked before the login form so that an app which has already rendered + // is never mistaken for a login prompt. + if (await appHeading.isVisible().catch(() => false)) return 'app'; + if (await loginField.isVisible().catch(() => false)) return 'login'; + return 'neither'; +}; + +// Waits for the app or the login form, whichever arrives, and logs in only if +// the login form is the one that showed up. +// +// This used to wait for the app alone and treat a timeout as "not logged in". +// A slow render therefore sent the run off to fill a login form that was not +// there, and the failure surfaced 30 seconds later as a missing "Red Hat login" +// textbox - which reads as broken authentication no matter what actually went +// wrong. Stage outages, a chrome-service 502, a saturated dev proxy and a slow +// render have all been reported that way. export const ensureAuthenticated = async ( page: Page, staticUser: boolean = false, ) => { - // Navigate to the target page - if (isHosted()) { - await page.goto('/insights/image-builder/landing'); - } else { - await page.goto('/cockpit-image-builder'); - } + await page.goto( + isHosted() ? '/insights/image-builder/landing' : '/cockpit-image-builder', + ); - // Check for authentication success indicator - const successIndicator = isHosted() + const appHeading = isHosted() ? page.getByRole('heading', { name: 'Image builder' }) : ibFrame(page).getByRole('heading', { name: 'Image builder' }); - let isAuthenticated = false; - try { - // Give it a 30 second period to load, it's less expensive than having to rerun the test - await expect(successIndicator).toBeVisible({ timeout: 30000 }); - isAuthenticated = true; - } catch { - isAuthenticated = false; + const loginField = isHosted() + ? page.getByRole('textbox', { name: 'Red Hat login' }) + : page.getByRole('textbox', { name: 'User name' }); + + // Held in an object because TypeScript does not track assignments made + // inside the poll callback. + const seen: { state: AuthState } = { state: 'neither' }; + const settle = async () => { + try { + await expect + .poll( + async () => { + seen.state = await detectAuthState(appHeading, loginField); + return seen.state; + }, + { timeout: AUTH_TIMEOUT, intervals: [250, 500, 1000] }, + ) + .not.toBe('neither'); + return true; + } catch { + return false; + } + }; + + if (!(await settle())) { + // A blank page usually means the federated module never mounted. Reloading + // recovers it often enough to be worth one attempt before failing. + await page.reload(); + if (!(await settle())) { + // Distinguishes "chrome never loaded" from "chrome loaded but our module + // did not", which need to be chased in completely different places. + const chromeRendered = await page + .getByRole('banner') + .isVisible() + .catch(() => false); + throw new Error( + `Neither image builder nor the login form appeared within ` + + `${(AUTH_TIMEOUT / 1000) * 2}s (including one reload) at ` + + `${page.url()}. The console chrome ` + + `${chromeRendered ? 'rendered, so the image builder module failed to mount' : 'did not render either, so this is upstream of image builder'}. ` + + `This is not an authentication failure - check the browser console ` + + `and network log in the trace.`, + ); + } } - if (!isAuthenticated) { - // Not authenticated, need to login + if (seen.state === 'login') { await login(page, staticUser); } }; diff --git a/playwright/helpers/wizardHelpers.ts b/playwright/helpers/wizardHelpers.ts index 0f93fbca76..84403f31ae 100644 --- a/playwright/helpers/wizardHelpers.ts +++ b/playwright/helpers/wizardHelpers.ts @@ -32,8 +32,15 @@ export const createBlueprint = async ( // An informational modal may appear on first create if localStorage // does not have 'imageBuilder.saveAndBuildModalSeen'. Dismiss it and // click the button again. + // isVisible() ignores its timeout and reports the state right now, so a + // modal that took a moment to paint read as absent, the second click never + // happened, and the blueprint was silently never created. const closeBtn = page.getByTestId('close-button-saveandbuild-modal'); - if (await closeBtn.isVisible({ timeout: 3000 }).catch(() => false)) { + const modalAppeared = await closeBtn + .waitFor({ state: 'visible', timeout: 3000 }) + .then(() => true) + .catch(() => false); + if (modalAppeared) { await closeBtn.click(); await page.getByRole('button', { name: 'Create blueprint' }).click(); } diff --git a/src/Components/CreateImageWizard/CreateImageWizard.tsx b/src/Components/CreateImageWizard/CreateImageWizard.tsx index f26989158d..2c58c46ecc 100644 --- a/src/Components/CreateImageWizard/CreateImageWizard.tsx +++ b/src/Components/CreateImageWizard/CreateImageWizard.tsx @@ -51,7 +51,9 @@ import { } from '@/store/slices/wizard'; import { closeWizardModal, + markWizardInitialized, openWizardModal, + selectHasWizardInitialized, selectIsWizardModalOpen, selectWizardModalMode, } from '@/store/slices/wizardModal'; @@ -123,7 +125,7 @@ const CreateImageWizard = () => { const imageSource = useAppSelector(selectImageSource); const [searchParams, setSearchParams] = useSearchParams(); const resolvePath = useAppSelector(selectPathResolver); - const hasInitialized = useRef(false); + const hasInitialized = useAppSelector(selectHasWizardInitialized); const { analytics, auth } = useChrome(); const { userData } = useGetUser(auth); const hasTrackedInitialStepRef = useRef(false); @@ -183,6 +185,8 @@ const CreateImageWizard = () => { imagePullValidation.disabledNext || (restrictions.users.isStandalone && usersHaveErrors); + const baseSettingsIsPending = !!detailsValidation.isPending; + const advancedSettingsHasErrors = filesystemValidation.disabledNext || timezoneValidation.disabledNext || @@ -207,9 +211,9 @@ const CreateImageWizard = () => { } if (mode === 'create' && showWizardModal) { - if (!hasInitialized.current) { + if (!hasInitialized) { dispatch(initializeWizard()); - hasInitialized.current = true; + dispatch(markWizardInitialized()); } // Initialize registration URLs @@ -387,7 +391,6 @@ const CreateImageWizard = () => { const handleClose = () => { dispatch(closeWizardModal()); dispatch(initializeWizard()); - hasInitialized.current = false; hasTrackedInitialStepRef.current = false; hasTrackedWizardOpenedRef.current = false; @@ -488,6 +491,7 @@ const CreateImageWizard = () => { } diff --git a/src/Components/CreateImageWizard/components/CustomWizardFooter.tsx b/src/Components/CreateImageWizard/components/CustomWizardFooter.tsx index 9c25ac749d..3400f7cc42 100644 --- a/src/Components/CreateImageWizard/components/CustomWizardFooter.tsx +++ b/src/Components/CreateImageWizard/components/CustomWizardFooter.tsx @@ -21,6 +21,7 @@ import { scrollToFirstError } from '../utilities/scrollToFirstError'; type CustomWizardFooterPropType = { disableBack?: boolean; hasErrors: boolean; + isPending?: boolean; beforeNext?: () => boolean; isOnPremise: boolean; }; @@ -28,6 +29,7 @@ type CustomWizardFooterPropType = { export const CustomWizardFooter = ({ disableBack, hasErrors, + isPending, beforeNext, isOnPremise, }: CustomWizardFooterPropType) => { @@ -38,6 +40,13 @@ export const CustomWizardFooter = ({ const reviewAndFinishBtnID = 'wizard-review-and-finish-btn'; const cancelBtnID = 'wizard-cancel-btn'; + // While a check is still running there is no error to show, so clicking would + // do nothing at all. Disable instead: an enabled button that silently + // discards the click is indistinguishable from a broken page, both to a user + // and to anything automating one. Real errors keep their enabled button so + // that clicking still reveals them. + const isWaitingOnValidation = !!isPending && !hasErrors; + const handleNext = () => { if (hasErrors) { flushSync(() => { @@ -89,10 +98,18 @@ export const CustomWizardFooter = ({ > Back - -