WEB-1012: [Playwright] Global setup hardening with functional readiness probe and deadline-bounded polling #3681
Conversation
|
Note
|
| Layer / File(s) | Summary |
|---|---|
Readiness polling contracts and probe implementations playwright/utils/readiness.ts |
Exports POLL_INTERVAL_MS, MAX_READINESS_ATTEMPTS, DEFAULT_READINESS_DELAYS_MS, READINESS_CEILING_MS constants; probe input types (FineractProbeInput, WebAppProbeInput); option interfaces (PollFineractOptions, PollWebAppOptions); ReadinessProbeFailure error class; defaultFineractProbe (checks /actuator/health and /api/v1/offices); defaultWebAppProbe (GETs root URL); pollFineract and pollWebApp functions that wrap probes with fixed-cadence transient retry and outer deadline enforcement. |
Docker log dumping utility playwright/utils/docker-logs.ts |
Exports SpawnFn type, DumpDockerLogsOptions interface, and dumpDockerLogs function that spawns docker compose logs --tail --no-color, streams child stdout/stderr into configurable writable streams, and always resolves with soft-failure handling for missing docker binary, synchronous spawn errors, non-zero exit codes, and wall-clock timeout (SIGKILL best-effort). |
Global setup orchestration playwright/global-setup.ts |
Replaces direct sequential HTTP GETs with concurrent Promise.all([pollFineract(), pollWebApp()]), adds FINERACT_SKIP_READINESS=1 fast-skip mode, reads E2E_FINERACT_URL, E2E_WEBAPP_URL, and compose configuration from environment, logs per-attempt retry diagnostics via onRetry and describe() error formatter, and on failure invokes dumpDockerLogs before throwing a fatal error with explicit recovery instructions. |
Unit tests playwright/utils/readiness.spec.ts |
Covers polling constant assertions (90s total), full pollFineract/pollWebApp test matrices (first-success return, fixed 5s cadence retries, 90s ceiling enforcement with last-error rethrow, transient 401-like classification, option/URL forwarding, onRetry diagnostics), ReadinessProbeFailure contract validation, and dumpDockerLogs tests using CapturingStream and FakeChild doubles for command args, stream routing, ENOENT/spawn error tolerance, timeout/kill behavior, custom compose/service/tail options, non-zero exit codes, and stream write error robustness. |
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
- openMF/web-app#3639: The new
playwright/utils/readiness.tsimportsRetryAttemptInfoand relies on theretry()primitive introduced in that PR's shared retry infrastructure.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title accurately reflects the main changes: Playwright global setup hardening with functional readiness probe and deadline-bounded polling, which aligns with the substantive refactor of readiness checking and the introduction of new polling mechanisms. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@playwright/global-setup.ts`:
- Line 113: The current return statement using JSON.stringify(error) in the
describe function can return undefined for certain values like undefined,
functions, or symbols, violating the string return contract and producing weak
diagnostics. Replace the direct JSON.stringify() call with a conditional check
that falls back to String(error) when JSON.stringify returns undefined, ensuring
the function always returns a valid string representation of the error.
In `@playwright/utils/docker-logs.ts`:
- Around line 73-88: The code makes multiple unguarded calls to out.write() and
err.write() across 8 locations without error handling. If any of these streams
throw an error, it will crash the helper and mask the original readiness
failure. Create a best-effort helper function that safely wraps write operations
and catches any errors thrown by the streams without propagating them. Then
replace all direct calls to out.write() and err.write() (at lines 87, 102, 112,
118, 127, 128, 136, and 145) with calls to this safe wrapper helper to ensure
the diagnostic logging never crashes the helper function.
In `@playwright/utils/readiness.ts`:
- Around line 154-160: The readiness probes in the retry() mechanism do not
enforce an absolute wall-clock deadline, causing the actual elapsed time to
exceed the 90-second timeout advertised in error messages and comments. Thread
an absolute deadline parameter through the retry() function and pass it to each
probe attempt (the health endpoint check via ctx.get() and the offices endpoint
check), then adjust the timeout values in each ctx.get() call to use the
remaining time budget from the deadline rather than fixed POLL_INTERVAL_MS
values, ensuring that the total elapsed time respects the contract.
Alternatively, if this architectural change is too complex, update the contract
documentation, error messages in ReadinessProbeFailure, and test expectations to
accurately reflect that the 90s timeout only covers retry sleep delays and does
not include probe I/O time.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5d3066d2-1fd0-4b9b-8dac-b20c6216c44d
📒 Files selected for processing (4)
playwright/global-setup.tsplaywright/utils/docker-logs.tsplaywright/utils/readiness.spec.tsplaywright/utils/readiness.ts
…ss probe and deadline-bounded polling (WA-2.5 + WA-2.6) Rewrites playwright/global-setup.ts as a thin orchestrator over two new utilities so the E2E suite can survive slow Docker boots without hiding real outages. - playwright/utils/readiness.ts (new): pollFineract() + pollWebApp() built on WEB-975's retry() utility. Fixed 5s cadence, ~90s ceiling (18 retries x 5s, jitter disabled). Fineract readiness is a functional probe -- /actuator/health 200 AND GET /api/v1/offices returns >=1 office -- so empty seed data no longer manifests as a flaky login. All I/O (HTTP probe, sleep) is injectable. - playwright/utils/docker-logs.ts (new): dumpDockerLogs() spawns 'docker compose -f <file> logs --tail=200 <service>' and streams output to stderr with a clear banner. Never throws -- ENOENT, spawn errors, non-zero exit, and timeouts all degrade to a soft warning so the original readiness error is what surfaces. Service, compose file, tail, output stream, and spawn impl are all injectable so the React port can lift the helper verbatim. - playwright/global-setup.ts: short-circuits when FINERACT_SKIP_READINESS=1 is set, otherwise runs both probes via Promise.all (fast-fail). On rejection it dumps the Fineract logs and rethrows with an actionable message pointing at the compose up/logs/skip-flag commands. - playwright/utils/readiness.spec.ts (new): 8 unit tests covering the 5s cadence, 90s ceiling, functional probe contract, transient classification of boot-time 4xx, ENOENT tolerance, timeout kill, custom compose file/service for portability, and non-zero exit handling. Runs under the existing 'unit' Playwright project (no config change) with no browser, no app, no backend. Closes: WA-2.5, WA-2.6
479f615 to
3362ccc
Compare
Description
Rewrites global-setup.ts to replace the one-shot 15s health check with a parallel, deadline-bounded polling loop that verifies functional Fineract readiness (seed data present) and dumps
docker compose logson timeout.Changes made:
Promise.all([pollFineract(), pollWebApp()])with 5s interval, ~90s ceiling. Short-circuits onFINERACT_SKIP_READINESS=1.playwright/utils/readiness.ts— newpollFineract()/pollWebApp()built on WEB-975'sretry(). Fineract readiness =/actuator/health200 ANDGET /api/v1/offices≥1 office (functional, not just alive).playwright/utils/docker-logs.ts— newdumpDockerLogs()spawnsdocker compose logs --tail=200on timeout. Never throws; degrades ENOENT/spawn errors to soft warnings. Parameterised for React port.playwright/utils/readiness.spec.ts— new unit tests (8 tests, 31 total with existing retry/sleep). Covers 5s cadence, 90s ceiling, functional probe, ENOENT tolerance, timeout, custom service/compose.Why:
/actuator/health— doesn't catch empty seed data (documented source of flaky CI logins)Dependencies:
retry()utility (reused, no new dep)child_process(built-in)@playwright/test(existing)Related Issues and Discussion
WEB-1012
Screenshots, if any
N/A
Checklist
Summary by CodeRabbit