From e39ca24486d71fb4292e6f2bfc5a3f4cb31c2466 Mon Sep 17 00:00:00 2001 From: Thibault Le Ouay Date: Tue, 20 Oct 2026 13:00:55 +0200 Subject: [PATCH] tests: use factory (#2953) * tests: use factory * ci: apply automated fixes * tests: race * ci: apply automated fixes --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> --- .../monitor/__tests__/monitor.test.ts | 42 ++++++++----- .../__tests__/notification.test.ts | 51 ++++++++------- .../status-page/__tests__/status-page.test.ts | 49 ++++++++++----- packages/db/.busy-probe.mjs | 18 ++++++ .../__tests__/stream-monitor-preview.test.ts | 4 +- .../__tests__/page-subscriber.test.ts | 2 +- .../services/src/sso/__tests__/sso.test.ts | 10 +-- packages/services/test/helpers.ts | 63 ++++++++++++++++++- 8 files changed, 174 insertions(+), 65 deletions(-) create mode 100644 packages/db/.busy-probe.mjs diff --git a/apps/server/src/routes/rpc/handlers/monitor/__tests__/monitor.test.ts b/apps/server/src/routes/rpc/handlers/monitor/__tests__/monitor.test.ts index 09c31b603..efc4963ec 100644 --- a/apps/server/src/routes/rpc/handlers/monitor/__tests__/monitor.test.ts +++ b/apps/server/src/routes/rpc/handlers/monitor/__tests__/monitor.test.ts @@ -448,22 +448,31 @@ describe("MonitorService.ListMonitors", () => { }); test("uses offset for pagination", async () => { - // First page - const res1 = await connectRequest( - "ListMonitors", - { limit: 1 }, - { "x-openstatus-key": "1" }, - ); + // Isolate in a dedicated workspace so concurrent monitor inserts/deletes + // on the seeded workspace can't shift the pagination window mid-test. + // Distinct createdAt values give the `desc(createdAt)` order a + // deterministic sequence (it has no id tiebreaker). + const ws = await createWorkspace(); + const key = String(ws.id); + const now = Date.now(); + await createMonitor(ws.id, { createdAt: new Date(now - 1000) }); + await createMonitor(ws.id, { createdAt: new Date(now) }); - expect(res1.status).toBe(200); - const data1 = await res1.json(); + try { + const res1 = await connectRequest( + "ListMonitors", + { limit: 1 }, + { "x-openstatus-key": key }, + ); + + expect(res1.status).toBe(200); + const data1 = await res1.json(); + expect(data1.totalSize).toBe(2); - if (data1.totalSize > 1) { - // Second page using offset const res2 = await connectRequest( "ListMonitors", { limit: 1, offset: 1 }, - { "x-openstatus-key": "1" }, + { "x-openstatus-key": key }, ); expect(res2.status).toBe(200); @@ -482,11 +491,12 @@ describe("MonitorService.ListMonitors", () => { ...(data2.dnsMonitors || []).map((m: { id: string }) => m.id), ]; - // Should have no overlap - const overlap = firstPageIds.filter((id: string) => - secondPageIds.includes(id), - ); - expect(overlap.length).toBe(0); + expect(firstPageIds).toHaveLength(1); + expect(secondPageIds).toHaveLength(1); + expect(firstPageIds[0]).not.toBe(secondPageIds[0]); + } finally { + await db.delete(monitor).where(eq(monitor.workspaceId, ws.id)); + await db.delete(workspace).where(eq(workspace.id, ws.id)); } }); diff --git a/apps/server/src/routes/rpc/handlers/notification/__tests__/notification.test.ts b/apps/server/src/routes/rpc/handlers/notification/__tests__/notification.test.ts index 0b905f328..a7e872a27 100644 --- a/apps/server/src/routes/rpc/handlers/notification/__tests__/notification.test.ts +++ b/apps/server/src/routes/rpc/handlers/notification/__tests__/notification.test.ts @@ -2,8 +2,13 @@ import { db, eq } from "@openstatus/db"; import { notification, notificationsToMonitors, + workspace, } from "@openstatus/db/src/schema"; -import { createTestWorkspace } from "@openstatus/db/src/test/factories"; +import { + createNotification, + createTestWorkspace, + createWorkspace, +} from "@openstatus/db/src/test/factories"; import { afterAll, beforeAll, describe, expect, test } from "vitest"; import { app } from "../../../../../index"; @@ -444,36 +449,36 @@ describe("NotificationService.ListNotifications", () => { }); test("respects offset parameter", async () => { - // Get total count first - const res1 = await connectRequest( - "ListNotifications", - {}, - { "x-openstatus-key": "1" }, - ); - const data1 = await res1.json(); - const totalSize = data1.totalSize; + // Isolate in a dedicated workspace so concurrent notification + // inserts/deletes on the seeded workspace can't shift the pagination + // window mid-test. In dev/test the `x-openstatus-key` header is the + // workspace id. + const ws = await createWorkspace(); + const key = String(ws.id); + await createNotification(ws.id); + await createNotification(ws.id); - if (totalSize > 1) { - // Get first page - const res2 = await connectRequest( + try { + const res1 = await connectRequest( "ListNotifications", { limit: 1, offset: 0 }, - { "x-openstatus-key": "1" }, + { "x-openstatus-key": key }, ); - const data2 = await res2.json(); - - // Get second page - const res3 = await connectRequest( + const res2 = await connectRequest( "ListNotifications", { limit: 1, offset: 1 }, - { "x-openstatus-key": "1" }, + { "x-openstatus-key": key }, ); - const data3 = await res3.json(); + const data1 = await res1.json(); + const data2 = await res2.json(); - // Should have different notifications - if (data2.notifications?.length > 0 && data3.notifications?.length > 0) { - expect(data2.notifications[0].id).not.toBe(data3.notifications[0].id); - } + expect(data1.totalSize).toBe(2); + expect(data1.notifications).toHaveLength(1); + expect(data2.notifications).toHaveLength(1); + expect(data1.notifications[0].id).not.toBe(data2.notifications[0].id); + } finally { + await db.delete(notification).where(eq(notification.workspaceId, ws.id)); + await db.delete(workspace).where(eq(workspace.id, ws.id)); } }); diff --git a/apps/server/src/routes/rpc/handlers/status-page/__tests__/status-page.test.ts b/apps/server/src/routes/rpc/handlers/status-page/__tests__/status-page.test.ts index bc495398a..7b7bef673 100644 --- a/apps/server/src/routes/rpc/handlers/status-page/__tests__/status-page.test.ts +++ b/apps/server/src/routes/rpc/handlers/status-page/__tests__/status-page.test.ts @@ -8,8 +8,13 @@ import { pageSubscriber, statusReport, statusReportsToPageComponents, + workspace, } from "@openstatus/db/src/schema"; -import { createTestWorkspace } from "@openstatus/db/src/test/factories"; +import { + createPage, + createTestWorkspace, + createWorkspace, +} from "@openstatus/db/src/test/factories"; import { afterAll, beforeAll, @@ -684,25 +689,35 @@ describe("StatusPageService.ListStatusPages", () => { }); test("respects offset parameter", async () => { - // Get first page - const res1 = await connectRequest( - "ListStatusPages", - { limit: 1, offset: 0 }, - { "x-openstatus-key": "1" }, - ); - const data1 = await res1.json(); + // Isolate in a dedicated workspace so concurrent page inserts/deletes on + // the seeded workspace can't shift the pagination window mid-test. In + // dev/test the `x-openstatus-key` header is the workspace id. + const ws = await createWorkspace(); + const key = String(ws.id); + await createPage(ws.id); + await createPage(ws.id); - // Get second page - const res2 = await connectRequest( - "ListStatusPages", - { limit: 1, offset: 1 }, - { "x-openstatus-key": "1" }, - ); - const data2 = await res2.json(); + try { + const res1 = await connectRequest( + "ListStatusPages", + { limit: 1, offset: 0 }, + { "x-openstatus-key": key }, + ); + const res2 = await connectRequest( + "ListStatusPages", + { limit: 1, offset: 1 }, + { "x-openstatus-key": key }, + ); + const data1 = await res1.json(); + const data2 = await res2.json(); - // Should have different pages if multiple exist - if (data1.statusPages?.length > 0 && data2.statusPages?.length > 0) { + expect(data1.totalSize).toBe(2); + expect(data1.statusPages).toHaveLength(1); + expect(data2.statusPages).toHaveLength(1); expect(data1.statusPages[0].id).not.toBe(data2.statusPages[0].id); + } finally { + await db.delete(page).where(eq(page.workspaceId, ws.id)); + await db.delete(workspace).where(eq(workspace.id, ws.id)); } }); }); diff --git a/packages/db/.busy-probe.mjs b/packages/db/.busy-probe.mjs new file mode 100644 index 000000000..0c094405e --- /dev/null +++ b/packages/db/.busy-probe.mjs @@ -0,0 +1,18 @@ +import { createClient } from "@libsql/client"; +const a = createClient({ url: "http://127.0.0.1:8090" }); +const b = createClient({ url: "http://127.0.0.1:8090" }); +await a.execute("create table if not exists t (x int)"); +const txA = await a.transaction(); +await txA.execute("insert into t values (1)"); +const t0 = Date.now(); +const txB = await b.transaction(); +try { + await txB.execute("select 1"); + console.log("B started after", Date.now() - t0, "ms (no busy)"); +} catch (e) { + console.log("B failed after", Date.now() - t0, "ms:", e.code, e.message); +} +setTimeout(async () => { + await txA.rollback(); + console.log("A released at", Date.now() - t0); +}, 3000); diff --git a/packages/services/src/monitor/__tests__/stream-monitor-preview.test.ts b/packages/services/src/monitor/__tests__/stream-monitor-preview.test.ts index dcb211dca..f2e7c1924 100644 --- a/packages/services/src/monitor/__tests__/stream-monitor-preview.test.ts +++ b/packages/services/src/monitor/__tests__/stream-monitor-preview.test.ts @@ -55,8 +55,8 @@ describe("streamMonitorPreview", () => { }); test("throws NotFoundError when monitor belongs to a different workspace", async () => { + const ws = (await createWorkspaceFixture("team")).workspace; await withTestTransaction(async (tx) => { - const ws = (await createWorkspaceFixture("team")).workspace; const ctx = { ...makeUserCtx(ws), db: tx }; // Use a monitor id that does not exist for this workspace. @@ -76,8 +76,8 @@ describe("streamMonitorPreview", () => { }); test("yields one result per region for an owned monitor", async () => { + const ws = (await createWorkspaceFixture("team")).workspace; await withTestTransaction(async (tx) => { - const ws = (await createWorkspaceFixture("team")).workspace; const ctx = { ...makeUserCtx(ws), db: tx }; const created = await createMonitor({ ctx, diff --git a/packages/services/src/page-subscriber/__tests__/page-subscriber.test.ts b/packages/services/src/page-subscriber/__tests__/page-subscriber.test.ts index 563db004f..15c490d1d 100644 --- a/packages/services/src/page-subscriber/__tests__/page-subscriber.test.ts +++ b/packages/services/src/page-subscriber/__tests__/page-subscriber.test.ts @@ -1028,8 +1028,8 @@ describe("hasPendingSubscriber", () => { describe("createPageSubscriber", () => { test("rejects read-only actor", async () => { + const team = (await createWorkspaceFixture("team")).workspace; await withTestTransaction(async (tx) => { - const team = (await createWorkspaceFixture("team")).workspace; const readOnlyCtx = { ...makeApiKeyCtx(team, { keyId: "k-read", diff --git a/packages/services/src/sso/__tests__/sso.test.ts b/packages/services/src/sso/__tests__/sso.test.ts index 2ad919f8a..31aa1fa35 100644 --- a/packages/services/src/sso/__tests__/sso.test.ts +++ b/packages/services/src/sso/__tests__/sso.test.ts @@ -715,10 +715,10 @@ describe("joinSsoWorkspace", () => { } test("provisions the member and audits the join", async () => { + // `createUser` writes on the default db — do it before the test tx takes + // the write lock, or the two deadlock. + const newcomer = await createUser(); await withTestTransaction(async (tx) => { - // `createUser` writes on the default db — do it before the test tx takes - // a write lock, or the two deadlock. - const newcomer = await createUser(); await enableSsoOn(tx); const ctx = makeUserCtx(scaleCtx.workspace, { userId: newcomer.id }); @@ -774,8 +774,8 @@ describe("joinSsoWorkspace", () => { }); test("rejects a workspace with sso switched off", async () => { + const newcomer = await createUser(); await withTestTransaction(async (tx) => { - const newcomer = await createUser(); await expect( joinSsoWorkspace({ ctx: { ...scaleCtx, db: tx }, @@ -786,8 +786,8 @@ describe("joinSsoWorkspace", () => { }); test("a sign-in that overlaps disableSso does not provision", async () => { + const newcomer = await createUser(); await withTestTransaction(async (tx) => { - const newcomer = await createUser(); await enableSsoOn(tx); // The ctx snapshot was taken while SSO was on; the owner turns it off // before the callback lands. diff --git a/packages/services/test/helpers.ts b/packages/services/test/helpers.ts index 584359bd7..0aa0988ae 100644 --- a/packages/services/test/helpers.ts +++ b/packages/services/test/helpers.ts @@ -1,4 +1,4 @@ -import { and, db, desc, eq, inArray } from "@openstatus/db"; +import { and, db, desc, eq, inArray, sql } from "@openstatus/db"; import { type Scope, auditLog, @@ -8,10 +8,41 @@ import { createTestWorkspace } from "@openstatus/db/src/test/factories"; import { expect } from "vitest"; import type { Actor, DB, DrizzleTx, ServiceContext } from "../src/context"; +import { isRetryableDbError } from "../src/retry"; import type { Workspace } from "../src/types"; class RollbackSignal extends Error {} +/** The transaction never started: `fn` did not run, so it's safe to retry. */ +class BeginFailed extends Error { + constructor(readonly original: unknown) { + super("test transaction failed to begin"); + } +} + +// `db.transaction()` opens with `BEGIN IMMEDIATE`, so every test transaction +// takes the single write lock up front. Parallel test files queue on it, and a +// waiter that loses gets SQLITE_BUSY ("database is locked") or sqld's +// TRANSACTION_TIMEOUT — surfaced on the test's first query, since libsql +// pipelines BEGIN with it. +const BEGIN_MAX_ATTEMPTS = 8; +const BEGIN_BASE_DELAY_MS = 50; +const BEGIN_MAX_DELAY_MS = 1_000; + +function isLockContention(err: unknown): boolean { + if (isRetryableDbError(err)) return true; + let current: unknown = err; + for (let depth = 0; depth < 10; depth++) { + if (typeof current !== "object" || current === null) return false; + if ("code" in current && current.code === "TRANSACTION_TIMEOUT") { + return true; + } + if (!("cause" in current) || current.cause === current) return false; + current = current.cause; + } + return false; +} + /** * Wrap a test body in a transaction that always rolls back, so per-test * writes (rows, audit log entries, etc.) never reach the committed db. @@ -30,13 +61,43 @@ class RollbackSignal extends Error {} * - Inside the callback, replace any `db.select(...)` / * `db.insert(...)` / `db.delete(...)` with the same call on `tx`. * Reads via committed `db` will not see the in-tx writes. + * + * Lock contention while BEGIN acquires the write lock is retried with + * backoff. Once the transaction has started, `fn` holds the lock, so its + * own queries are never retried (and `fn` never runs twice). */ export async function withTestTransaction( fn: (tx: DrizzleTx) => Promise, ): Promise { + for (let attempt = 1; ; attempt++) { + try { + return await runRolledBack(fn); + } catch (e) { + if (!(e instanceof BeginFailed)) throw e; + if (attempt >= BEGIN_MAX_ATTEMPTS || !isLockContention(e.original)) { + throw e.original; + } + const delay = Math.min( + BEGIN_BASE_DELAY_MS * 2 ** (attempt - 1), + BEGIN_MAX_DELAY_MS, + ); + await new Promise((resolve) => + setTimeout(resolve, delay / 2 + Math.random() * (delay / 2)), + ); + } + } +} + +async function runRolledBack(fn: (tx: DrizzleTx) => Promise): Promise { let result!: T; try { await db.transaction(async (tx) => { + // Force the BEGIN now so lock contention is isolated from `fn`. + try { + await tx.run(sql`select 1`); + } catch (e) { + throw new BeginFailed(e); + } result = await fn(tx); throw new RollbackSignal(); }); -- 2.54.0