WEB-1065: [Playwright] Active-client factory + sequential cleanup#3755
Open
devvaansh wants to merge 1 commit into
Open
WEB-1065: [Playwright] Active-client factory + sequential cleanup#3755devvaansh wants to merge 1 commit into
devvaansh wants to merge 1 commit into
Conversation
|
Note
|
| Layer / File(s) | Summary |
|---|---|
Active client creation and validation playwright/factories/client.factory.ts, playwright/factories/client.factory.spec.ts |
Adds createActiveTestClient, strict Fineract date parsing and ordering validation, default date constants, live active-client coverage, and invalid-input rejection tests. |
Factory export separation playwright/factories/index.ts, playwright/tests/clients/create-client.spec.ts |
Renames the payload builder to buildTestClientPayload, exposes API factory symbols and shared resolvers, and updates client tests accordingly. |
Sequential cleanup flushing
| Layer / File(s) | Summary |
|---|---|
Sequential LIFO cleanup playwright/utils/cleanup-guard.ts, playwright/utils/cleanup-guard.spec.ts |
Changes cleanup draining to await each reverse-insertion deleter before starting the next, while preserving failure recording and continuation behavior. |
Estimated code review effort: 3 (Moderate) | ~20 minutes
Possibly related PRs
- openMF/web-app#3686: Directly relates to
CleanupGuardflushing semantics. - openMF/web-app#3699: Touches the client payload factory and create-client test setup.
- openMF/web-app#3725: Uses the client factory and pending-to-active lifecycle test 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 clearly and concisely summarizes the two main changes: the active-client factory and sequential cleanup behavior. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| 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. |
✨ 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.
devvaansh
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
playwright/utils/cleanup-guard.ts (1)
166-177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the stale
Promise.allSettledreference fromFlushOutcomedocumentation.The implementation now invokes deleters sequentially, but the adjacent
FlushOutcomedocs still describe the result as coming fromPromise.allSettled. Update that description to refer to the underlying deleter invocation.🤖 Prompt for 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. In `@playwright/utils/cleanup-guard.ts` around lines 166 - 177, Update the FlushOutcome documentation near the sequential drain logic to remove the stale Promise.allSettled reference and describe the result as originating from the underlying deleter invocation. Leave the sequential LIFO behavior and error-recording semantics unchanged.playwright/utils/cleanup-guard.spec.ts (1)
76-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a deterministic gate instead of fixed timer delays.
The Arrange-Act-Assert structure is clear, but
setTimeout(1)/setTimeout(25)introduces unnecessary timing sensitivity. Gate the child deleter with a promise, assert that the parent has not started, then release the child.Suggested test structure
- const slowDeleter = (label: string, delayMs: number) => async () => { - events.push(`start:${label}`); - await new Promise((resolve) => setTimeout(resolve, delayMs)); - events.push(`end:${label}`); - }; + let releaseChild = () => {}; + const childGate = new Promise<void>((resolve) => { + releaseChild = resolve; + }); + let markChildStarted = () => {}; + const childStarted = new Promise<void>((resolve) => { + markChildStarted = resolve; + }); - guard.register('parent', slowDeleter('parent', 1)); - guard.register('child', slowDeleter('child', 25)); + guard.register('parent', async () => { + events.push('start:parent'); + events.push('end:parent'); + }); + guard.register('child', async () => { + events.push('start:child'); + markChildStarted(); + await childGate; + events.push('end:child'); + }); - await guard.flush(); + const flushPromise = guard.flush(); + await childStarted; + expect(events).toEqual(['start:child']); + releaseChild(); + await flushPromise;As per path instructions, keep this test in clear Arrange-Act-Assert form and minimize brittle timing dependencies.
🤖 Prompt for 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. In `@playwright/utils/cleanup-guard.spec.ts` around lines 76 - 110, Update the “awaits each deleter before starting the next” test to replace fixed setTimeout delays with a deterministic child-completion promise gate. Start flush, assert the child has started while the parent has not, resolve the gate, await flush, and retain the expected child-before-parent event ordering in clear Arrange-Act-Assert form.Source: Path instructions
🤖 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/factories/client.factory.ts`:
- Around line 221-225: Update the extra construction in createTestClient so
overrides.extra is merged before the active-client invariants, then explicitly
set active, activationDate, and submittedOnDate from the validated values.
Ensure caller-provided extra fields cannot replace any of these three fields or
bypass date-order validation.
---
Nitpick comments:
In `@playwright/utils/cleanup-guard.spec.ts`:
- Around line 76-110: Update the “awaits each deleter before starting the next”
test to replace fixed setTimeout delays with a deterministic child-completion
promise gate. Start flush, assert the child has started while the parent has
not, resolve the gate, await flush, and retain the expected child-before-parent
event ordering in clear Arrange-Act-Assert form.
In `@playwright/utils/cleanup-guard.ts`:
- Around line 166-177: Update the FlushOutcome documentation near the sequential
drain logic to remove the stale Promise.allSettled reference and describe the
result as originating from the underlying deleter invocation. Leave the
sequential LIFO behavior and error-recording semantics unchanged.
🪄 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 Plus
Run ID: ebf018e1-f2d9-498a-b50d-e9c59474cf5c
📒 Files selected for processing (6)
playwright/factories/client.factory.spec.tsplaywright/factories/client.factory.tsplaywright/factories/index.tsplaywright/tests/clients/create-client.spec.tsplaywright/utils/cleanup-guard.spec.tsplaywright/utils/cleanup-guard.ts
devvaansh
force-pushed
the
WEB-1065-playwright-active-client-factory-sequential-cleanup
branch
from
July 25, 2026 11:24
c142d53 to
3f7e0a0
Compare
2 tasks
CleanupGuard.flush() now awaits each deleter in turn instead of dispatching all of them concurrently via Promise.allSettled. LIFO ordering was previously only apparent: a child resource's DELETE could still reach Fineract after its parent's, producing an FK violation. Adds createActiveTestClient() with a monotonic date chain (submitted -> activated -> account opening) and a local pre-flight guard, so an inverted date fails with a message naming both fields rather than an opaque Fineract validation error. Fixes the factories barrel: client.ts and client.factory.ts both exported createTestClient with incompatible signatures and only the pure builder was reachable through the barrel. It is now buildTestClientPayload.
devvaansh
force-pushed
the
WEB-1065-playwright-active-client-factory-sequential-cleanup
branch
from
July 25, 2026 14:18
3f7e0a0 to
cc0810f
Compare
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
Two related fixes to the Playwright test-data foundation, needed before the client account/charge specs can be written.
1.
CleanupGuard.flush()did not actually enforce cleanup order.The guard's contract is that a test creating a client and then a loan on that client must delete the loan first, or Fineract rejects the cascade with a 403.
flush()popped the stack in LIFO order — but then handed every deleter toPromise.allSettledin the same tick. The ordering was only apparent: a slow child DELETE could still land after its parent's, which is the exact failure the guard exists to prevent.flush()now awaits each deleter before starting the next. A rejecting deleter is still caught and recorded, and the drain continues, so one dead resource cannot leak its siblings. Added a staggered-delay regression test that fails against the old implementation.2. Added
createActiveTestClient().createTestClient()deliberately creates a pending client, because pending is the only state Fineract will hard-delete. That is right for the lifecycle specs, but every upcoming flow — loan accounts, savings accounts, client charges — requires an active client, and each carries a date-ordering constraint relative to the activation date. Getting it wrong produces a Fineract validation error that names neither field.The new factory encodes the chain once (
submitted→activated→ account opening) and pre-flight checks it locally, so a mistake fails with a message that actually says which two dates are inverted. Without this, roughly twenty upcoming specs would each re-derive it by hand.Cleanup caveat, documented in the factory: Fineract will not delete an active client, so the registered deleter is expected to fail and the guard records it without throwing. E2E databases are ephemeral and the
E2E_name prefix keeps orphans greppable.3. Fixed drift in the factories barrel.
factories/client.tsandfactories/client.factory.tsboth exported a function namedcreateTestClientwith incompatible signatures — one is a pure payload builder, the other seeds via the API and registers cleanup. Only the pure builder was reachable through the barrel, soimport { createTestClient } from '../../factories'andfrom '../../factories/client.factory'silently resolved to different functions. The pure builder is now exported asbuildTestClientPayload, and the barrel re-exports both flavours with a note on when to use which.Related issues and discussion
https://mifosforge.jira.com/browse/WEB-1065
Screenshots, if any
N/A — test infrastructure only, no UI changes.
Testing
npx playwright test --project=unit— 96 passedcreateActiveTestClientintegration specs, date-guard negative cases, and a constant-chain invariant testNote:
tsc --noEmitreports a pre-existing duplicatedeleteClient()declaration infixtures/fineract-api.tsondev. It is untouched here and fixed in the follow-up savings-API PR.Checklist
web-app/.github/CONTRIBUTING.md.Summary by CodeRabbit