WEB-1012: [Playwright] Global setup hardening with functional readiness probe and deadline-bounded polling #3681
Open
devvaansh wants to merge 1 commit into
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
devvaansh
changed the title
…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
devvaansh
force-pushed
the
WEB-1012-playwright-global-setup-hardening-with-functional-readiness-probe-and-deadline-bounded-polling
branch
from
479f615 to
3362ccc
Compare
devvaansh
added a commit
to devvaansh/web-app
that referenced
this pull request
Jun 24, 2026
…reverse-order cleanup-guard (WA-2.9 + WA-2.10)
Adds three test-data factories and a panic-safe LIFO CleanupGuard, building
on the WEB-1013 ApiSetupManager + naming + test-data type surface and the
WEB-1012 global setup hardening. Unlocks Week 5 WA-3 client E2E tests.
What ships:
* playwright/utils/cleanup-guard.ts
Zero-import LIFO cleanup stack. register(label, deleter) pushes,
flush() pops in reverse order under Promise.allSettled and returns
a structured { ok, failed, outcomes } summary. Never throws so
teardown noise cannot mask the real test failure. Re-entrant
register() during a flush is rejected; concurrent flush() calls
drain exactly once. React-portable.
* playwright/factories/{client,group,user}.factory.ts
Each takes (setup: ApiSetupManager, guard: CleanupGuard, overrides?),
names entities via generateE2EName(prefix) from WEB-1013, dedupes the
/api/v1/offices lookup through setup.dedupe('office:first', ...) so a
test creating three resources only pays one round-trip, and registers
its deleter on the guard immediately after a successful POST. Each
factory defaults to a pending shape that Fineract is willing to
hard-delete on cleanup.
* playwright/factories/_shared.ts
resolveDefaultOfficeId helper + stable FIRST_OFFICE_CACHE_KEY
constant so the three factories agree on the dedupe key.
* playwright/fixtures/test-fixtures.ts
Adds apiSetup fixture and auto cleanupGuard fixture. cleanupGuard
declares an explicit apiSetup dependency so Playwright's teardown
order is guaranteed (cleanupGuard.flush() runs BEFORE fineractApi
disposes its request context); without that anchor, deleters race
the API context teardown and reject with 'context has been closed'.
* playwright/fixtures/fineract-api.ts
Adds deleteClient, createGroup/getGroup/deleteGroup, and
createUser/getUser/deleteUser using the existing validateResponse
helper.
* playwright.config.ts
New 'integration' project (testMatch /playwright/factories/.*\.spec\.ts/)
with no browser, no auth-setup dependency. Live-backend factory
specs exit in seconds because no Chromium boot is paid.
* playwright/utils/cleanup-guard.spec.ts
11 pure-logic specs covering LIFO order, double-flush no-op,
Promise.allSettled isolation across siblings, synchronous-throw
capture, re-entrancy guard, and concurrent-flush drain-once.
* playwright/factories/{client,group,user}.factory.spec.ts
14 live-backend specs creating each entity, asserting the typed
shape, round-tripping through GET, flushing the guard, and
asserting the subsequent GET 404s.
Cross-factory ordering is verified end-to-end: client + group + user
created in that order are deleted user -> group -> client under a
single flush().
Verified against live Fineract at https://localhost:8443:
unit project : 54/54 pass
integration project : 14/14 pass
zero cleanup warnings, eslint clean, prettier clean, headers OK.
Closes: WA-2.9, WA-2.10
Depends-on: WEB-1012 (openMF#3681), WEB-1013 (openMF#3684)
devvaansh
added a commit
to devvaansh/web-app
that referenced
this pull request
Jun 24, 2026
…reverse-order CleanupGuard (WA-2.9 + WA-2.10)
Adds three test-data factories and a panic-safe LIFO CleanupGuard on top
of WEB-1013 (ApiSetupManager + naming + test-data types). Unlocks Week 5
WA-3 client E2E tests.
## What ships (WEB-1013 foundation + WEB-1014 factories)
### playwright/utils/cleanup-guard.ts [NEW]
Zero-import LIFO cleanup stack. register(label, deleter) pushes onto
a stack; flush() drains in reverse order under Promise.allSettled,
returns { ok, failed, outcomes } and never throws. React-portable.
### playwright/factories/ [NEW]
client.factory.ts — createTestClient(setup, guard, overrides?)
group.factory.ts — createTestGroup(setup, guard, overrides?)
user.factory.ts — createTestUser + Fineract-policy-compliant generateE2EPassword
_shared.ts — resolveDefaultOfficeId deduped via ApiSetupManager
Each factory: names via generateE2EName(prefix), dedupes office lookup
through setup.dedupe('office:first'), registers deleter immediately
after successful POST. Defaults to a pending shape that Fineract
hard-deletes cleanly on teardown.
### playwright/utils/api-setup-manager.ts [NEW — WEB-1013]
### playwright/utils/naming.ts [NEW — WEB-1013]
### playwright/types/test-data.types.ts [NEW — WEB-1013]
### playwright/fixtures/fineract-api.ts [EXTENDED]
Adds deleteClient, createGroup/getGroup/deleteGroup,
createUser/getUser/deleteUser using existing validateResponse helper.
### playwright/fixtures/test-fixtures.ts [EXTENDED]
Adds apiSetup fixture + auto cleanupGuard fixture. cleanupGuard
explicitly depends on apiSetup so Playwright tears down the guard
BEFORE the API request context disposes — without this ordering,
deleters race the context teardown and fail.
### playwright.config.ts [EXTENDED]
New 'integration' project (testMatch /playwright/factories/.*\.spec\.ts/)
— no browser, no auth-setup dep, exits in seconds.
## Specs
playwright/utils/cleanup-guard.spec.ts — 11 pure-logic unit specs
playwright/utils/api-setup-manager.spec.ts — 19 unit specs (WEB-1013)
playwright/utils/naming.spec.ts — 24 unit specs (WEB-1013)
playwright/factories/client.factory.spec.ts — live-backend integration
playwright/factories/group.factory.spec.ts — live-backend integration
playwright/factories/user.factory.spec.ts — live-backend integration
## Verified against live Fineract (https://localhost:8443)
unit project : 54/54 pass
integration project : 14/14 pass, zero cleanup warnings
tsc, eslint, prettier, headers : all clean
Closes: WA-2.9, WA-2.10
Depends-on: WEB-1012 (openMF#3681), WEB-1013 (openMF#3684)
Open
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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