Web 1014 playwright test data factories client group user and reverse order cleanup guard#3686
Open
devvaansh wants to merge 1 commit into
Conversation
…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)
|
Note
|
| Layer / File(s) | Summary |
|---|---|
E2E test-data shapes and naming playwright/types/test-data.types.ts, playwright/utils/naming.ts, playwright/utils/naming.spec.ts |
Exports shared identity, status, timeline, client, group, user, and loan projections, and adds deterministic E2E name generation with validation coverage. |
Cleanup guard stack playwright/utils/cleanup-guard.ts, playwright/utils/cleanup-guard.spec.ts |
Adds a LIFO cleanup stack with registration validation, flush summaries, and unit coverage for ordering, failure isolation, and concurrency. |
API setup cache manager playwright/utils/api-setup-manager.ts, playwright/utils/api-setup-manager.spec.ts |
Adds in-flight API setup deduplication, cache reset helpers, and unit coverage for rejection handling, retries, and cache-key stability. |
Integration project and fixture wiring playwright.config.ts, playwright/fixtures/fineract-api.ts, playwright/fixtures/test-fixtures.ts, playwright/factories/_shared.ts |
Adds the factory integration project, shared first-office resolution, Fineract API wrappers, and the apiSetup and cleanupGuard fixtures. |
Client factory playwright/factories/client.factory.ts, playwright/factories/client.factory.spec.ts |
Adds client creation defaults, override handling, cleanup registration, and live create/get/delete tests. |
Group factory playwright/factories/group.factory.ts, playwright/factories/group.factory.spec.ts |
Adds group creation defaults, override handling, cleanup registration, and live create/get/delete tests. |
User factory playwright/factories/user.factory.ts, playwright/factories/user.factory.spec.ts |
Adds password generation, user creation defaults, cleanup registration, and live create/get/delete and teardown-order tests. |
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~60 minutes
Suggested reviewers
- alberto-art3ch
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title names the main additions: client/group/user factories and reverse-order cleanup guard. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% 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. |
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
✏️ 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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
playwright/factories/client.factory.ts (2)
102-136: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
displayNameprojection can diverge from the payload whenfirstname/lastnameare passed viaextra.The projection on Line 134 uses the locally-resolved
firstname/lastnamevariables, but...overrides.extra(Line 111) is merged into the payload afterward. A caller settingextra: { firstname: 'X' }would POST one name while the returnedTestClient.displayNamereports the generated default — and the livegetClientround-trip in the spec would mismatch. Consider documenting that name fields must use the dedicated overrides, notextra, or derive the projection from the merged payload.🤖 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/factories/client.factory.ts` around lines 102 - 136, The returned TestClient displayName can drift from the actual create payload because createTestClient in client.factory.ts builds payload with ...overrides.extra after resolving firstname/lastname, but still projects displayName from the local firstname/lastname values. Update createTestClient so the displayName is derived from the final merged client payload (or explicitly prevent name fields from being set through extra and document the intended overrides), and keep the payload/projection logic aligned with the createClient and getClient flow.
117-124: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
resourceIdtyped asnumberbut assigned a possibly-undefinedvalue.
responseisany, so TypeScript acceptsconst resourceId: number = response.resourceId ?? response.clientIdeven though the value can beundefined(the very case the guard on Line 118 handles). Annotate it honestly so the narrowing carries real type information.♻️ Proposed tweak
- const resourceId: number = response.resourceId ?? response.clientId; - if (typeof resourceId !== 'number') { + const resourceId: unknown = response.resourceId ?? response.clientId; + if (typeof resourceId !== 'number') { throw new Error( `createTestClient: Fineract create-client response missing numeric resourceId/clientId, got ${JSON.stringify( response )}` ); }🤖 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/factories/client.factory.ts` around lines 117 - 124, The `createTestClient` resource ID handling is misleadingly typed as a guaranteed number even though `response.resourceId ?? response.clientId` can still be undefined; update the `resourceId` declaration in `client.factory.ts` to reflect the possible undefined value so the later `typeof resourceId !== 'number'` check performs real narrowing. Keep the existing guard and error path, but make the annotation honest so TypeScript correctly models the response from the Fineract create-client call.playwright/factories/client.factory.spec.ts (1)
29-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer the shared naming contract over the hardcoded
'E2E_client_S'literal.The literal prefix on Line 29 couples this assertion to
generateE2EName's internal segment layout; a change toE2E_NAME_PREFIXor the prefix separator would silently break this test rather than the source. SinceE2E_NAME_PATTERNis already imported (and used on Line 32), the suffix check on Line 30 plus the pattern match largely cover the intent. Consider asserting against the exported prefix constant instead of the inline string. As per path instructions (stable selectors, minimal brittle 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/factories/client.factory.spec.ts` around lines 29 - 32, The client displayName test is using a brittle hardcoded prefix literal instead of the shared naming contract. Update the assertion in client.factory.spec.ts to rely on the exported E2E naming constant used by generateE2EName/E2E_NAME_PREFIX rather than the inline 'E2E_client_S' string, while keeping the existing suffix check and E2E_NAME_PATTERN match so the test stays aligned with the source naming behavior.Source: Path instructions
playwright/factories/user.factory.spec.ts (1)
68-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid
Date.now()as the sole uniqueness source for the override username.Under the
integrationproject with multiple workers, two tests can hit the same millisecond and produce identical usernames, causing a duplicate-username failure and flakiness. Prefer the deterministic, shard+random-tagged generator already used by the factory.♻️ Suggested change
-import { E2E_NAME_PATTERN } from '../utils/naming'; +import { E2E_NAME_PATTERN, generateE2EName } from '../utils/naming';- const overrideUsername = `OverrideUser_${Date.now()}`; + const overrideUsername = generateE2EName('override');As per path instructions: "minimal 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/factories/user.factory.spec.ts` around lines 68 - 75, The override username in the user factory spec relies on Date.now() alone, which can collide across parallel integration workers and cause duplicate usernames. Update the test around createTestUser in user.factory.spec.ts to use the same deterministic shard-plus-random-tagged username generator already used by the factory, while still asserting the override is honored. Keep the existing firstname/lastname override checks and make the username value unique without depending on millisecond timing.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/group.factory.spec.ts`:
- Around line 40-46: The override name in group.factory.spec.ts is only
timestamp-based, so parallel workers can still generate the same group name and
collide with the unique-name constraint in createTestGroup/createGroup. Update
the overrideName construction in the test to include a random suffix in addition
to Date.now(), keeping it unique per worker without relying on millisecond
timing. Use the existing createTestGroup call site and the overrideName variable
as the place to make this change.
In `@playwright/utils/naming.ts`:
- Around line 154-168: The doc comment for sanitiseRandomToken does not match
the actual implementation: the loop currently zero-pads with literal '0'
characters instead of using another RNG draw. Update the comment to describe
zero-padding, or change sanitiseRandomToken to consume additional random input
if you want the stated entropy behavior; make the implementation and comment
consistent.
---
Nitpick comments:
In `@playwright/factories/client.factory.spec.ts`:
- Around line 29-32: The client displayName test is using a brittle hardcoded
prefix literal instead of the shared naming contract. Update the assertion in
client.factory.spec.ts to rely on the exported E2E naming constant used by
generateE2EName/E2E_NAME_PREFIX rather than the inline 'E2E_client_S' string,
while keeping the existing suffix check and E2E_NAME_PATTERN match so the test
stays aligned with the source naming behavior.
In `@playwright/factories/client.factory.ts`:
- Around line 102-136: The returned TestClient displayName can drift from the
actual create payload because createTestClient in client.factory.ts builds
payload with ...overrides.extra after resolving firstname/lastname, but still
projects displayName from the local firstname/lastname values. Update
createTestClient so the displayName is derived from the final merged client
payload (or explicitly prevent name fields from being set through extra and
document the intended overrides), and keep the payload/projection logic aligned
with the createClient and getClient flow.
- Around line 117-124: The `createTestClient` resource ID handling is
misleadingly typed as a guaranteed number even though `response.resourceId ??
response.clientId` can still be undefined; update the `resourceId` declaration
in `client.factory.ts` to reflect the possible undefined value so the later
`typeof resourceId !== 'number'` check performs real narrowing. Keep the
existing guard and error path, but make the annotation honest so TypeScript
correctly models the response from the Fineract create-client call.
In `@playwright/factories/user.factory.spec.ts`:
- Around line 68-75: The override username in the user factory spec relies on
Date.now() alone, which can collide across parallel integration workers and
cause duplicate usernames. Update the test around createTestUser in
user.factory.spec.ts to use the same deterministic shard-plus-random-tagged
username generator already used by the factory, while still asserting the
override is honored. Keep the existing firstname/lastname override checks and
make the username value unique without depending on millisecond timing.
🪄 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: 53206aa9-1516-4b44-b12f-f9364208503d
📒 Files selected for processing (17)
playwright.config.tsplaywright/factories/_shared.tsplaywright/factories/client.factory.spec.tsplaywright/factories/client.factory.tsplaywright/factories/group.factory.spec.tsplaywright/factories/group.factory.tsplaywright/factories/user.factory.spec.tsplaywright/factories/user.factory.tsplaywright/fixtures/fineract-api.tsplaywright/fixtures/test-fixtures.tsplaywright/types/test-data.types.tsplaywright/utils/api-setup-manager.spec.tsplaywright/utils/api-setup-manager.tsplaywright/utils/cleanup-guard.spec.tsplaywright/utils/cleanup-guard.tsplaywright/utils/naming.spec.tsplaywright/utils/naming.ts
Comment on lines
+40
to
+46
| // Use a timestamp suffix so the override is still unique enough | ||
| // not to clash with a previous test run. | ||
| const overrideName = `OverrideGroup_${Date.now()}`; | ||
| const group = await createTestGroup(apiSetup, cleanupGuard, { | ||
| name: overrideName, | ||
| submittedOnDate: '15 March 2024' | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Override name can collide across parallel workers.
OverrideGroup_${Date.now()} relies solely on a millisecond timestamp, with no random component. Since group names are unique within an office in Fineract (see group.factory.ts Lines 20-22), two workers executing this test in the same millisecond would collide and the second createGroup would fail, producing flaky runs. Add a random suffix to make the override deterministically unique.
🔧 Proposed fix
- const overrideName = `OverrideGroup_${Date.now()}`;
+ const overrideName = `OverrideGroup_${Date.now()}_${Math.random().toString(36).slice(2, 8)}`;As per path instructions: tests should have "minimal brittle timing dependencies".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Use a timestamp suffix so the override is still unique enough | |
| // not to clash with a previous test run. | |
| const overrideName = `OverrideGroup_${Date.now()}`; | |
| const group = await createTestGroup(apiSetup, cleanupGuard, { | |
| name: overrideName, | |
| submittedOnDate: '15 March 2024' | |
| }); | |
| // Use a timestamp suffix so the override is still unique enough | |
| // not to clash with a previous test run. | |
| const overrideName = `OverrideGroup_${Date.now()}_${Math.random().toString(36).slice(2, 8)}`; | |
| const group = await createTestGroup(apiSetup, cleanupGuard, { | |
| name: overrideName, | |
| submittedOnDate: '15 March 2024' | |
| }); |
🤖 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/factories/group.factory.spec.ts` around lines 40 - 46, The
override name in group.factory.spec.ts is only timestamp-based, so parallel
workers can still generate the same group name and collide with the unique-name
constraint in createTestGroup/createGroup. Update the overrideName construction
in the test to include a random suffix in addition to Date.now(), keeping it
unique per worker without relying on millisecond timing. Use the existing
createTestGroup call site and the overrideName variable as the place to make
this change.
Source: Path instructions
Comment on lines
+154
to
+168
| /** | ||
| * Fold a `[0, 1)` RNG draw into a fixed-length lower-case base36 token. | ||
| * `Math.random().toString(36).slice(2)` can occasionally yield a token | ||
| * shorter than expected (when the value is very small), so this helper | ||
| * pads with the next draw rather than emitting a name shorter than the | ||
| * pattern documents. | ||
| */ | ||
| function sanitiseRandomToken(value: number, length: number): string { | ||
| const normalised = Number.isFinite(value) && value >= 0 && value < 1 ? value : 0; | ||
| let token = normalised.toString(36).replace('0.', ''); | ||
| while (token.length < length) { | ||
| token += '0'; | ||
| } | ||
| return token.slice(0, length); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Doc comment contradicts the padding implementation.
The comment states the helper "pads with the next draw rather than emitting a name shorter than the pattern documents," but the loop appends literal '0' characters instead of folding in another RNG draw. For very small values this yields deterministic trailing zeros (lower entropy than the comment implies). Either update the comment to reflect zero-padding, or actually consume an additional draw if the entropy guarantee matters.
📝 Align comment with behavior
/**
* Fold a `[0, 1)` RNG draw into a fixed-length lower-case base36 token.
* `Math.random().toString(36).slice(2)` can occasionally yield a token
* shorter than expected (when the value is very small), so this helper
- * pads with the next draw rather than emitting a name shorter than the
- * pattern documents.
+ * right-pads with `'0'` rather than emitting a name shorter than the
+ * pattern documents.
*/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * Fold a `[0, 1)` RNG draw into a fixed-length lower-case base36 token. | |
| * `Math.random().toString(36).slice(2)` can occasionally yield a token | |
| * shorter than expected (when the value is very small), so this helper | |
| * pads with the next draw rather than emitting a name shorter than the | |
| * pattern documents. | |
| */ | |
| function sanitiseRandomToken(value: number, length: number): string { | |
| const normalised = Number.isFinite(value) && value >= 0 && value < 1 ? value : 0; | |
| let token = normalised.toString(36).replace('0.', ''); | |
| while (token.length < length) { | |
| token += '0'; | |
| } | |
| return token.slice(0, length); | |
| } | |
| /** | |
| * Fold a `[0, 1)` RNG draw into a fixed-length lower-case base36 token. | |
| * `Math.random().toString(36).slice(2)` can occasionally yield a token | |
| * shorter than expected (when the value is very small), so this helper | |
| * right-pads with `'0'` rather than emitting a name shorter than the | |
| * pattern documents. | |
| */ | |
| function sanitiseRandomToken(value: number, length: number): string { | |
| const normalised = Number.isFinite(value) && value >= 0 && value < 1 ? value : 0; | |
| let token = normalised.toString(36).replace('0.', ''); | |
| while (token.length < length) { | |
| token += '0'; | |
| } | |
| return token.slice(0, length); | |
| } |
🤖 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/naming.ts` around lines 154 - 168, The doc comment for
sanitiseRandomToken does not match the actual implementation: the loop currently
zero-pads with literal '0' characters instead of using another RNG draw. Update
the comment to describe zero-padding, or change sanitiseRandomToken to consume
additional random input if you want the stated entropy behavior; make the
implementation and comment consistent.
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
Introduces three typed test-data factories (
createTestClient,createTestGroup,createTestUser) and a panic-safe reverse-orderCleanupGuardfor thePlaywright E2E suite. These are the foundational building blocks that every
Week 5+ client/group/loan test will use to seed and tear down data without
manual cleanup code in each spec.
Why these changes:
Without factories, every spec hard-codes its own
fineractApi.createClient(...)call with raw payloads, duplicating date formats, locale strings, and
office-id lookups across dozens of tests. Factories centralise that
boilerplate and project the response into the typed
TestClient / TestGroup / TestUsershapes defined inplaywright/types/test-data.types.ts.Without a
CleanupGuard, a test that panics mid-run leaves orphaned rows inFineract that pollute subsequent runs and make CI non-deterministic. The
auto-fixture wired in
test-fixtures.tscallsguard.flush()unconditionallyin teardown — even on test panic — so cleanup is opt-out-impossible.
CleanupGuard.flush()runs deleters in strict LIFO order (loan before client,group before client) matching Fineract's FK constraints, and wraps every call
in
Promise.allSettledso one failing deleter never blocks its siblings.A new
integrationPlaywright project (playwright.config.ts) lets CI runplaywright test --project=integrationto validate factory correctness againsta live Fineract in seconds — no browser, no auth setup, no Chromium boot cost.
Dependencies:
ApiSetupManager, test-data type interfaces,and E2E naming generator
All new files are zero-Angular-import and React-portable by design.
Related issues and discussion
https://mifosforge.jira.com/browse/WEB-1014
Screenshots, if any
Unit project (pure-logic, no backend): 54/54 pass
Integration project (live Fineract): 14/14 pass, zero cleanup warnings
Checklist
web-app/.github/CONTRIBUTING.md.