From 71f2df324f18bece22d34c3df4692e653b476db0 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Sat, 19 Sep 2026 11:48:47 +0300 Subject: [PATCH 1/2] [#218] Make usecase1 e2e spec independent of recon thread ordering The spec asserted that dependent users must NOT exist after the 1st and 2nd reconciliation. Recon processes source rows on 10 task threads, so whether a dependent user's CREATE runs before or after its manager's CREATE has committed within the same pass is a scheduling accident: CI saw user.4 created in pass 2 (9 users instead of the README's 12), and a local run even created a direct report in pass 1. Keep only the outcomes the three-level hierarchy guarantees: superadmin after pass 1, the six direct reports after pass 2, all 23 users after pass 3. A retry could never recover either: the beforeAll purge deleted managed/user but silently left every repo/link in place, because "If-Match: *" is turned into a null revision by CREST and rejected with 409 by the OrientDB repository. The next recon then classified every row as MISSING (policy UNLINK) and created nothing, so step 3 polled for superadmin until the 180 s test timeout. Delete with the exact _rev, check every response and verify both tables are empty afterwards. Cap the polling helpers at 150 s so a missing user fails with its own message instead of an opaque test timeout. Fixes #218 --- e2e/usecase1.spec.mjs | 133 +++++++++++++++++++++++++----------------- 1 file changed, 81 insertions(+), 52 deletions(-) diff --git a/e2e/usecase1.spec.mjs b/e2e/usecase1.spec.mjs index a8388de90..c02718572 100644 --- a/e2e/usecase1.spec.mjs +++ b/e2e/usecase1.spec.mjs @@ -72,13 +72,20 @@ async function fetchManagedUser(request, userName) { return await res.json(); } +// Upper bound for the polling helpers below. Kept comfortably under the +// 180 s Playwright test timeout (playwright.config.mjs) so a genuinely +// missing user surfaces as the descriptive assertion message rather than as +// an opaque "Test timeout exceeded". +const POLL_BUDGET_MS = 150000; + async function expectManagedUserExists(request, userName) { // Generous polling: after the recon helper returns, individual CREATEs // can still be committing asynchronously through the relationship // resolver, so we may need to wait notably longer than runReconcileNow // itself took. let user = null; - for (let i = 0; i < 180; i++) { + const deadline = Date.now() + POLL_BUDGET_MS; + while (Date.now() < deadline) { user = await fetchManagedUser(request, userName); if (user) break; await new Promise(r => setTimeout(r, 1000)); @@ -87,9 +94,34 @@ async function expectManagedUserExists(request, userName) { expect(user.userName).toBe(userName); } -async function expectManagedUserMissing(request, userName) { - const user = await fetchManagedUser(request, userName); - expect(user, `managed/user/${userName} should NOT exist yet`).toBeNull(); +/** Return the ids of every managed/user currently in the repository. */ +async function listManagedUserIds(request) { + const res = await request.get( + `${BASE_URL}${CONTEXT_PATH}/managed/user?_queryFilter=true&_fields=_id`, + { + headers: { + "X-OpenIDM-Username": ADMIN_USER, + "X-OpenIDM-Password": ADMIN_PASS, + "Accept": "application/json", + }, + } + ); + expect(res.ok(), `query managed/user -> ${res.status()}`).toBeTruthy(); + const body = await res.json(); + return (body.result || []).map(r => r._id); +} + +async function expectManagedUserCount(request, expected) { + // Same asynchronous-commit caveat as expectManagedUserExists. + let ids = []; + const deadline = Date.now() + POLL_BUDGET_MS; + while (Date.now() < deadline) { + ids = await listManagedUserIds(request); + if (ids.length >= expected) break; + await new Promise(r => setTimeout(r, 1000)); + } + expect(ids, `managed/user should hold exactly ${expected} users`) + .toHaveLength(expected); } test.describe.serial("Usecase1 - Initial Reconciliation", () => { @@ -97,13 +129,13 @@ test.describe.serial("Usecase1 - Initial Reconciliation", () => { "Only runs when OPENIDM_SAMPLE=samples/usecase/usecase1"); // The README walk-through assumes a fresh deployment: the 1st recon - // creates only superadmin, the 2nd adds 12 users, the 3rd adds 10 more. - // Re-running the suite against a populated repo would invalidate every - // "user X should not exist yet" assertion, so purge managed/user (and - // the synchronisation link table - otherwise leftover links from a - // previous run keep recon in UNQUALIFIED/CONFIRMED states and no new - // managed users are created) once up-front. Idempotent: a 404 on an - // empty repo is fine. + // creates only superadmin and the 3rd completes the hierarchy. A + // Playwright retry re-runs this whole serial block (in a new worker, so + // this hook runs again) against the same OpenIDM instance, so purge + // managed/user AND the synchronisation link table up-front - leftover + // links whose target is gone put every source row into MISSING, whose + // policy is UNLINK, and that first recon then creates nothing at all. + // Idempotent: an empty repo simply yields nothing to delete. test.beforeAll(async ({ request }) => { if (!IS_USECASE1) return; const headers = { @@ -112,18 +144,29 @@ test.describe.serial("Usecase1 - Initial Reconciliation", () => { "Accept": "application/json", }; for (const resource of ["managed/user", "repo/link"]) { - const list = await request.get( - `${BASE_URL}${CONTEXT_PATH}/${resource}?_queryFilter=true&_fields=_id`, - { headers } - ); - if (!list.ok()) continue; + const listUrl = + `${BASE_URL}${CONTEXT_PATH}/${resource}?_queryFilter=true&_fields=_id`; + const list = await request.get(listUrl, { headers }); + expect(list.ok(), `query ${resource} -> ${list.status()}`).toBeTruthy(); const body = await list.json(); for (const r of (body.result || [])) { - await request.delete( + // Send the exact revision rather than "If-Match: *". The + // managed/user handler tolerates "*" (it re-reads the object + // and uses its own _rev), but the OrientDB repository behind + // repo/link requires an integer revision and answers 409 to + // "*", which would silently leave every link in place. The + // query above returns _rev alongside _id for both resources. + const del = await request.delete( `${BASE_URL}${CONTEXT_PATH}/${resource}/${encodeURIComponent(r._id)}`, - { headers: { ...headers, "If-Match": "*" } } + { headers: { ...headers, "If-Match": `"${r._rev}"` } } ); + expect(del.ok(), `delete ${resource}/${r._id} -> ${del.status()}`) + .toBeTruthy(); } + const check = await request.get(listUrl, { headers }); + expect(check.ok(), `query ${resource} -> ${check.status()}`).toBeTruthy(); + const remaining = ((await check.json()).result || []).map(r => r._id); + expect(remaining, `${resource} should be empty after purge`).toHaveLength(0); } }); @@ -158,10 +201,14 @@ test.describe.serial("Usecase1 - Initial Reconciliation", () => { // Step 3) "Query the managed users created by reconciliation" test("3) Query the managed users created by the first reconciliation", async ({ page, request }) => { // README: "On this first recon there should be only one user - // created, superadmin". Verify superadmin exists and a typical - // dependent user (user.0) does not yet. + // created, superadmin". superadmin has no manager, so its CREATE + // cannot fail and its presence is the one guaranteed outcome of + // this pass. We deliberately do not assert that anybody else is + // still absent: recon processes source rows on a pool of threads + // (taskThreads, default 10), so whether a dependent user's CREATE + // runs before or after its manager's CREATE has committed within + // the same pass is a scheduling accident, not a contract. await expectManagedUserExists(request, "superadmin"); - await expectManagedUserMissing(request, "user.0"); await page.goto(USERS_LIST_URL); await expect(page.locator(".backgrid.table")) .toContainText("superadmin", { timeout: 30000 }); @@ -176,12 +223,15 @@ test.describe.serial("Usecase1 - Initial Reconciliation", () => { // Step 5) "Query the managed users created by the second reconciliation" test("5) Query the managed users created by the second reconciliation", async ({ page, request }) => { // README: "12 new additional users created. These users have - // superadmin as their manager". user.0 (HR manager, reports to - // superadmin) is one of them; user.4 (HR contractor, reports to - // user.0) is still failing because its manager was just created in - // *this* recon and the validation snapshot was taken before then. - await expectManagedUserExists(request, "user.0"); - await expectManagedUserMissing(request, "user.4"); + // superadmin as their manager". hr_data.ldif actually holds six + // direct reports of superadmin (the four department managers plus + // hradmin and systemadmin); the README's "12" already includes + // second-level users that happened to be processed after their + // manager in the same pass. Only the six are guaranteed here, since + // superadmin existed before this recon started. + for (const userName of ["user.0", "user.5", "user.10", "user.15", "hradmin", "systemadmin"]) { + await expectManagedUserExists(request, userName); + } await page.goto(USERS_LIST_URL); await expect(page.locator(".backgrid.table")) .toContainText("user.0", { timeout: 30000 }); @@ -197,13 +247,11 @@ test.describe.serial("Usecase1 - Initial Reconciliation", () => { test("7) Query the managed users created by the third reconciliation", async ({ page, request }) => { // README: "10 new additional users created, bringing the total to 23 // users. ... The default password of the imported users is Passw0rd." - // Verify the previously-failing dependent user is now present, then - // exercise the documented credentials by logging into the Self-Service - // UI as user.0 / Passw0rd (which is the user the rest of the use - // cases authenticate as). + // The hierarchy is three levels deep, so after three passes every + // manager exists and all 23 source rows must have been created + // regardless of the ordering inside the earlier passes. + await expectManagedUserCount(request, 23); await expectManagedUserExists(request, "user.4"); - await expectManagedUserExists(request, "user.10"); - await expectManagedUserExists(request, "user.19"); await assertNoErrors(page); // Cross-verify the documented default password by signing in to the @@ -217,22 +265,3 @@ test.describe.serial("Usecase1 - Initial Reconciliation", () => { }); }); }); - - - - - - - - - - - - - - - - - - - From e355b4f5cadfd958a471adc70153d42354b4aa82 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Sat, 3 Oct 2026 08:58:20 +0300 Subject: [PATCH 2/2] [#218] Address review of the usecase1 e2e spec Pin the manager-existence check directly: with the per-pass absence assertions gone, nothing failed if "validate" : true on managed/user manager or RelationshipValidator's NotFound -> 400 regressed. A PUT of a user whose manager does not exist must answer 400 "... does not exist"; the body carries every property the sample schema requires, otherwise policy validation answers 403 first. Share one polling deadline per test, derived from the test's start and timeout in beforeEach, instead of a fresh 150 s budget per call that ignored the admin login and restarted for every polled user. Document that the purge does not wait for or cancel a recon left running by a timed-out attempt, and stop the step 2 comment from asserting the pass-1 ordering the spec no longer relies on. --- e2e/usecase1.spec.mjs | 75 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 62 insertions(+), 13 deletions(-) diff --git a/e2e/usecase1.spec.mjs b/e2e/usecase1.spec.mjs index c02718572..5602e0170 100644 --- a/e2e/usecase1.spec.mjs +++ b/e2e/usecase1.spec.mjs @@ -72,11 +72,15 @@ async function fetchManagedUser(request, userName) { return await res.json(); } -// Upper bound for the polling helpers below. Kept comfortably under the -// 180 s Playwright test timeout (playwright.config.mjs) so a genuinely -// missing user surfaces as the descriptive assertion message rather than as -// an opaque "Test timeout exceeded". -const POLL_BUDGET_MS = 150000; +// Shared deadline for all polling helpers of the running test, set by the +// beforeEach hook from the test's own start time and timeout. A per-call +// budget would restart with every call (step 5 polls six users in a row) and +// would not account for the admin login in beforeEach, so a genuinely +// missing user could still end in an opaque "Test timeout exceeded" instead +// of its own assertion message. The margin leaves room for the final +// request and the assertion itself. +const DEADLINE_MARGIN_MS = 20000; +let testDeadline = 0; async function expectManagedUserExists(request, userName) { // Generous polling: after the recon helper returns, individual CREATEs @@ -84,8 +88,7 @@ async function expectManagedUserExists(request, userName) { // resolver, so we may need to wait notably longer than runReconcileNow // itself took. let user = null; - const deadline = Date.now() + POLL_BUDGET_MS; - while (Date.now() < deadline) { + while (Date.now() < testDeadline) { user = await fetchManagedUser(request, userName); if (user) break; await new Promise(r => setTimeout(r, 1000)); @@ -114,8 +117,7 @@ async function listManagedUserIds(request) { async function expectManagedUserCount(request, expected) { // Same asynchronous-commit caveat as expectManagedUserExists. let ids = []; - const deadline = Date.now() + POLL_BUDGET_MS; - while (Date.now() < deadline) { + while (Date.now() < testDeadline) { ids = await listManagedUserIds(request); if (ids.length >= expected) break; await new Promise(r => setTimeout(r, 1000)); @@ -136,6 +138,13 @@ test.describe.serial("Usecase1 - Initial Reconciliation", () => { // links whose target is gone put every source row into MISSING, whose // policy is UNLINK, and that first recon then creates nothing at all. // Idempotent: an empty repo simply yields nothing to delete. + // + // Limitation: the purge neither waits for nor cancels a recon that a + // timed-out attempt left running on the server (ReconciliationService + // only cancels on an explicit action), so a retry only recovers from + // failures that happen after the recon has finished. A recon of these + // 23 rows takes seconds; one that outlives the test timeout points at a + // server-side hang that a retry would not fix anyway. test.beforeAll(async ({ request }) => { if (!IS_USECASE1) return; const headers = { @@ -170,7 +179,8 @@ test.describe.serial("Usecase1 - Initial Reconciliation", () => { } }); - test.beforeEach(async ({ page }) => { + test.beforeEach(async ({ page }, testInfo) => { + testDeadline = Date.now() + testInfo.timeout - DEADLINE_MARGIN_MS; await loginToAdmin(page); }); @@ -189,9 +199,10 @@ test.describe.serial("Usecase1 - Initial Reconciliation", () => { // Step 2) "Run reconciliation for the first time." test("2) Run reconciliation for the first time", async ({ page }) => { - // First pass: only superadmin (no manager attribute) is expected to - // succeed; the remaining 22 source rows fail the manager-existence - // relationship check. We do not pin an exact success counter - + // First pass: only superadmin (no manager attribute) is guaranteed to + // succeed; most of the other 22 source rows fail the manager-existence + // relationship check, but how many depends on recon thread scheduling + // (see step 3). We do not pin an exact success counter - // the README itself documents the partial failures - we just // require the recon to actually run to completion (which the // helper confirms by polling for a fresh audit/recon summary). @@ -264,4 +275,42 @@ test.describe.serial("Usecase1 - Initial Reconciliation", () => { timeout: 30000, }); }); + + // Not a README step. With the per-pass absence assertions gone, this is + // what pins the manager-existence check the walk-through relies on + // ("validate" : true on managed/user manager, RelationshipValidator + // turning a missing reference into 400): without it the first recon + // would create all 23 users and every step above would still pass. + // Runs last so that, should the check regress and the user be created, + // the count asserted in step 7 is not affected. + test("Reject a managed user whose manager does not exist", async ({ request }) => { + const res = await request.put( + `${BASE_URL}${CONTEXT_PATH}/managed/user/e2e-dangling-manager`, + { + headers: { + "X-OpenIDM-Username": ADMIN_USER, + "X-OpenIDM-Password": ADMIN_PASS, + "Accept": "application/json", + "Content-Type": "application/json", + "If-None-Match": "*", + }, + // Every property the sample's managed/user schema requires, + // so the request reaches relationship validation instead of + // stopping at policy validation (403). + data: { + userName: "e2e-dangling-manager", + givenName: "E2E", + sn: "Dangling", + displayName: "E2E Dangling", + employeeNumber: "e2e-dangling", + mail: "e2e-dangling@example.com", + password: "Passw0rd", + manager: { _ref: "managed/user/does-not-exist" }, + }, + } + ); + expect(res.status(), "dangling manager reference must be rejected").toBe(400); + // A policy-validation failure must not satisfy this case by accident. + expect((await res.json()).message).toContain("does not exist"); + }); });