From 883cb7ebc37ae2111e51346a7700369f7bb4108a Mon Sep 17 00:00:00 2001 From: "molecule-ai[bot]" <276602405+molecule-ai[bot]@users.noreply.github.com> Date: Tue, 21 Apr 2026 03:07:28 +0000 Subject: [PATCH] fix(canvas): eliminate flaky timer state between orgs-page tests (#1207) (#1243) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause: tests used try/finally { vi.useRealTimers() / vi.useFakeTimers() } back-and-forth. When any test's finally-block called vi.useFakeTimers(), subsequent tests inherited fake timer state causing 50ms real setTimeouts to not fire and mockFetch to accumulate calls across test boundaries. Fix: consolidate timer management to beforeEach/afterEach hooks. - beforeEach: vi.useFakeTimers() — all tests start from known fake state - afterEach: cleanup() + vi.useRealTimers() — restore real timers for next test - Individual tests: use vi.advanceTimersByTimeAsync(50) instead of real setTimeout Also removed duplicate afterEach(cleanup()) and unused waitFor import. Closes #1207. Co-authored-by: Molecule AI Core-FE Co-authored-by: Claude Sonnet 4.6 --- canvas/src/app/__tests__/orgs-page.test.tsx | 494 +++++++++----------- 1 file changed, 218 insertions(+), 276 deletions(-) diff --git a/canvas/src/app/__tests__/orgs-page.test.tsx b/canvas/src/app/__tests__/orgs-page.test.tsx index fb0a1f75..e6cbf39b 100644 --- a/canvas/src/app/__tests__/orgs-page.test.tsx +++ b/canvas/src/app/__tests__/orgs-page.test.tsx @@ -15,7 +15,7 @@ * - Polling: provisioning orgs schedule a 5s refresh (fake timers) */ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; -import { render, screen, waitFor, cleanup } from "@testing-library/react"; +import { render, screen, cleanup } from "@testing-library/react"; // ── Hoisted mocks ──────────────────────────────────────────────────────────── // vi.mock factories are hoisted above imports; any captured references must @@ -90,6 +90,8 @@ beforeEach(() => { // vi.useFakeTimers() in the polling tests would be a no-op — causing // setTimeout(0) to hang and the test to time out. vi.useRealTimers(); + // Now install fake timers for this test's deterministic timing. + vi.useFakeTimers(); vi.clearAllMocks(); // Reset mock return values so each test starts fresh. // The mock functions (vi.fn) persist across tests; only their @@ -100,62 +102,47 @@ beforeEach(() => { }); afterEach(() => { - // Ensure fake timers are never left active after a test — even one that - // failed before reaching its own finally-block. - vi.useRealTimers(); cleanup(); + // Restore real timers so subsequent tests (and vitest internals) + // aren't polluted by fake timer state from a previous test. + vi.useRealTimers(); }); // ── Tests ──────────────────────────────────────────────────────────────────── describe("/orgs — auth guard", () => { it("redirects to login when session is null", async () => { - vi.useRealTimers(); - try { - mockFetchSession.mockResolvedValueOnce(null); - render(); - await new Promise((r) => setTimeout(r, 50)); - expect(mockRedirectToLogin).toHaveBeenCalled(); - // Must not attempt to fetch /cp/orgs before auth is established - expect(mockFetch).not.toHaveBeenCalledWith( - expect.stringContaining("/cp/orgs"), - expect.anything() - ); - } finally { - vi.useFakeTimers(); - } + mockFetchSession.mockResolvedValueOnce(null); + render(); + await vi.advanceTimersByTimeAsync(50); + expect(mockRedirectToLogin).toHaveBeenCalled(); + // Must not attempt to fetch /cp/orgs before auth is established + expect(mockFetch).not.toHaveBeenCalledWith( + expect.stringContaining("/cp/orgs"), + expect.anything() + ); }); }); describe("/orgs — error state", () => { it("shows error + Retry button when /cp/orgs fails", async () => { - vi.useRealTimers(); - try { - mockFetchSession.mockResolvedValue({ userId: "u-1" }); - mockFetch.mockResolvedValueOnce(notOk(500, "db down")); - render(); - await new Promise((r) => setTimeout(r, 50)); - expect(screen.getByText(/Error:/)).toBeTruthy(); - expect(screen.getByRole("button", { name: /retry/i })).toBeTruthy(); - } finally { - vi.useFakeTimers(); - } + mockFetchSession.mockResolvedValue({ userId: "u-1" }); + mockFetch.mockResolvedValueOnce(notOk(500, "db down")); + render(); + await vi.advanceTimersByTimeAsync(50); + expect(screen.getByText(/Error:/)).toBeTruthy(); + expect(screen.getByRole("button", { name: /retry/i })).toBeTruthy(); }); }); describe("/orgs — empty list", () => { it("renders EmptyState with CreateOrgForm when user has zero orgs", async () => { - vi.useRealTimers(); - try { - mockFetchSession.mockResolvedValue({ userId: "u-1" }); - mockFetch.mockResolvedValueOnce(okJson({ orgs: [] })); - render(); - await new Promise((r) => setTimeout(r, 50)); - expect(screen.getByText(/don't have any organizations/i)).toBeTruthy(); - expect(screen.getByRole("button", { name: /create organization/i })).toBeTruthy(); - } finally { - vi.useFakeTimers(); - } + mockFetchSession.mockResolvedValue({ userId: "u-1" }); + mockFetch.mockResolvedValueOnce(okJson({ orgs: [] })); + render(); + await vi.advanceTimersByTimeAsync(50); + expect(screen.getByText(/don't have any organizations/i)).toBeTruthy(); + expect(screen.getByRole("button", { name: /create organization/i })).toBeTruthy(); }); }); @@ -163,160 +150,130 @@ describe("/orgs — CTAs by status", () => { const session = { userId: "u-1" }; it("running → Open link targets {slug}.moleculesai.app", async () => { - vi.useRealTimers(); - try { - mockFetchSession.mockResolvedValue(session); - mockFetch.mockResolvedValueOnce( - okJson({ - orgs: [ - { - id: "o-1", - slug: "acme", - name: "Acme", - plan: "pro", - status: "running", - created_at: "", - updated_at: "", - }, - ], - }) - ); - render(); - await new Promise((r) => setTimeout(r, 50)); - const link = screen.getByRole("link", { name: /open/i }) as HTMLAnchorElement; - expect(link.href).toBe("https://acme.moleculesai.app/"); - } finally { - vi.useFakeTimers(); - } + mockFetchSession.mockResolvedValue(session); + mockFetch.mockResolvedValueOnce( + okJson({ + orgs: [ + { + id: "o-1", + slug: "acme", + name: "Acme", + plan: "pro", + status: "running", + created_at: "", + updated_at: "", + }, + ], + }) + ); + render(); + await vi.advanceTimersByTimeAsync(50); + const link = screen.getByRole("link", { name: /open/i }) as HTMLAnchorElement; + expect(link.href).toBe("https://acme.moleculesai.app/"); }); it("awaiting_payment → Complete payment link to /pricing?org=", async () => { - vi.useRealTimers(); - try { - mockFetchSession.mockResolvedValue(session); - mockFetch.mockResolvedValueOnce( - okJson({ - orgs: [ - { - id: "o-2", - slug: "beta-co", - name: "Beta", - plan: "", - status: "awaiting_payment", - created_at: "", - updated_at: "", - }, - ], - }) - ); - render(); - await new Promise((r) => setTimeout(r, 50)); - const link = screen.getByRole("link", { - name: /complete payment/i, - }) as HTMLAnchorElement; - expect(link.getAttribute("href")).toBe("/pricing?org=beta-co"); - } finally { - vi.useFakeTimers(); - } + mockFetchSession.mockResolvedValue(session); + mockFetch.mockResolvedValueOnce( + okJson({ + orgs: [ + { + id: "o-2", + slug: "beta-co", + name: "Beta", + plan: "", + status: "awaiting_payment", + created_at: "", + updated_at: "", + }, + ], + }) + ); + render(); + await vi.advanceTimersByTimeAsync(50); + const link = screen.getByRole("link", { + name: /complete payment/i, + }) as HTMLAnchorElement; + expect(link.getAttribute("href")).toBe("/pricing?org=beta-co"); }); it("failed → mailto support link", async () => { - vi.useRealTimers(); - try { - mockFetchSession.mockResolvedValue(session); - mockFetch.mockResolvedValueOnce( - okJson({ - orgs: [ - { - id: "o-3", - slug: "boom", - name: "Boom", - plan: "", - status: "failed", - created_at: "", - updated_at: "", - }, - ], - }) - ); - render(); - await new Promise((r) => setTimeout(r, 50)); - const link = screen.getByRole("link", { - name: /contact support/i, - }) as HTMLAnchorElement; - expect(link.getAttribute("href")).toBe("mailto:support@moleculesai.app"); - } finally { - vi.useFakeTimers(); - } + mockFetchSession.mockResolvedValue(session); + mockFetch.mockResolvedValueOnce( + okJson({ + orgs: [ + { + id: "o-3", + slug: "boom", + name: "Boom", + plan: "", + status: "failed", + created_at: "", + updated_at: "", + }, + ], + }) + ); + render(); + await vi.advanceTimersByTimeAsync(50); + const link = screen.getByRole("link", { + name: /contact support/i, + }) as HTMLAnchorElement; + expect(link.getAttribute("href")).toBe("mailto:support@moleculesai.app"); }); }); describe("/orgs — post-checkout banner", () => { it("renders CheckoutBanner when ?checkout=success and scrubs the URL", async () => { - vi.useRealTimers(); - try { - setLocation("https://moleculesai.app/orgs?checkout=success"); - const replaceState = vi.spyOn(window.history, "replaceState"); - mockFetchSession.mockResolvedValue({ userId: "u-1" }); - mockFetch.mockResolvedValueOnce( - okJson({ - orgs: [ - { - id: "o-1", - slug: "acme", - name: "Acme", - plan: "pro", - status: "running", - created_at: "", - updated_at: "", - }, - ], - }) - ); - render(); - await new Promise((r) => setTimeout(r, 50)); - expect(screen.getByText(/Payment confirmed/i)).toBeTruthy(); - // URL must be rewritten to drop the ?checkout flag so reload doesn't re-show the banner - expect(replaceState).toHaveBeenCalled(); - const callArgs = replaceState.mock.calls[0]; - expect(callArgs[2]).toBe("/orgs"); - } finally { - vi.useFakeTimers(); - } + setLocation("https://moleculesai.app/orgs?checkout=success"); + const replaceState = vi.spyOn(window.history, "replaceState"); + mockFetchSession.mockResolvedValue({ userId: "u-1" }); + mockFetch.mockResolvedValueOnce( + okJson({ + orgs: [ + { + id: "o-1", + slug: "acme", + name: "Acme", + plan: "pro", + status: "running", + created_at: "", + updated_at: "", + }, + ], + }) + ); + render(); + await vi.advanceTimersByTimeAsync(50); + expect(screen.getByText(/Payment confirmed/i)).toBeTruthy(); + // URL must be rewritten to drop the ?checkout flag so reload doesn't re-show the banner + expect(replaceState).toHaveBeenCalled(); + const callArgs = replaceState.mock.calls[0]; + expect(callArgs[2]).toBe("/orgs"); }); it("does NOT render CheckoutBanner without ?checkout=success", async () => { - vi.useRealTimers(); - try { - mockFetchSession.mockResolvedValue({ userId: "u-1" }); - mockFetch.mockResolvedValueOnce(okJson({ orgs: [] })); - render(); - await new Promise((r) => setTimeout(r, 50)); - expect(screen.getByText(/don't have any organizations/i)).toBeTruthy(); - expect(screen.queryByText(/Payment confirmed/i)).toBeNull(); - } finally { - vi.useFakeTimers(); - } + mockFetchSession.mockResolvedValue({ userId: "u-1" }); + mockFetch.mockResolvedValueOnce(okJson({ orgs: [] })); + render(); + await vi.advanceTimersByTimeAsync(50); + expect(screen.getByText(/don't have any organizations/i)).toBeTruthy(); + expect(screen.queryByText(/Payment confirmed/i)).toBeNull(); }); }); describe("/orgs — fetch includes credentials + timeout signal", () => { it("/cp/orgs fetch is called with credentials:include and an AbortSignal", async () => { - vi.useRealTimers(); - try { - mockFetchSession.mockResolvedValue({ userId: "u-1" }); - mockFetch.mockResolvedValueOnce(okJson({ orgs: [] })); - render(); - await new Promise((r) => setTimeout(r, 50)); - const callArgs = mockFetch.mock.calls.find((c) => - String(c[0]).includes("/cp/orgs") - ); - expect(callArgs).toBeDefined(); - expect(callArgs![1]).toMatchObject({ credentials: "include" }); - expect(callArgs![1].signal).toBeInstanceOf(AbortSignal); - } finally { - vi.useFakeTimers(); - } + mockFetchSession.mockResolvedValue({ userId: "u-1" }); + mockFetch.mockResolvedValueOnce(okJson({ orgs: [] })); + render(); + await vi.advanceTimersByTimeAsync(50); + const callArgs = mockFetch.mock.calls.find((c) => + String(c[0]).includes("/cp/orgs") + ); + expect(callArgs).toBeDefined(); + expect(callArgs![1]).toMatchObject({ credentials: "include" }); + expect(callArgs![1].signal).toBeInstanceOf(AbortSignal); }); }); @@ -328,113 +285,98 @@ describe("/orgs — fetch includes credentials + timeout signal", () => { describe("/orgs — polling of in-flight orgs", () => { it("schedules a 5s refetch when at least one org is provisioning", async () => { - vi.useFakeTimers({ shouldAdvanceTime: true }); - try { - mockFetchSession.mockResolvedValue({ userId: "u-1" }); - // First /cp/orgs returns provisioning orgs so a poll is scheduled. - // Second returns running orgs to observe the state flip stop re-scheduling. - mockFetch.mockResolvedValueOnce( - okJson({ - orgs: [ - { - id: "o-1", - slug: "acme", - name: "Acme", - plan: "pro", - status: "provisioning", - created_at: "", - updated_at: "", - }, - ], - }) - ); - mockFetch.mockResolvedValueOnce( - okJson({ - orgs: [ - { - id: "o-1", - slug: "acme", - name: "Acme", - plan: "pro", - status: "running", - created_at: "", - updated_at: "", - }, - ], - }) - ); + // beforeEach already set up fake timers; advance time to fire the 5s poll. + mockFetchSession.mockResolvedValue({ userId: "u-1" }); + // First /cp/orgs returns provisioning orgs so a poll is scheduled. + // Second returns running orgs to observe the state flip stop re-scheduling. + mockFetch.mockResolvedValueOnce( + okJson({ + orgs: [ + { + id: "o-1", + slug: "acme", + name: "Acme", + plan: "pro", + status: "provisioning", + created_at: "", + updated_at: "", + }, + ], + }) + ); + mockFetch.mockResolvedValueOnce( + okJson({ + orgs: [ + { + id: "o-1", + slug: "acme", + name: "Acme", + plan: "pro", + status: "running", + created_at: "", + updated_at: "", + }, + ], + }) + ); - render(); - // Auto-advancing time fires the 5s poll while we await - await vi.advanceTimersByTimeAsync(5_100); - // First /cp/orgs + second poll /cp/orgs - expect(mockFetch).toHaveBeenCalledTimes(2); - } finally { - vi.useRealTimers(); - } + render(); + await vi.advanceTimersByTimeAsync(5_100); + // First /cp/orgs + second poll /cp/orgs + expect(mockFetch).toHaveBeenCalledTimes(2); }); it("does NOT schedule a refetch when all orgs are running", async () => { - vi.useFakeTimers({ shouldAdvanceTime: true }); - try { - mockFetchSession.mockResolvedValue({ userId: "u-1" }); - mockFetch.mockResolvedValueOnce( - okJson({ - orgs: [ - { - id: "o-1", - slug: "acme", - name: "Acme", - plan: "pro", - status: "running", - created_at: "", - updated_at: "", - }, - ], - }) - ); - render(); - // Auto-advancing time — no poll fires (stillMoving = false) - await vi.advanceTimersByTimeAsync(10_000); - // Only the initial /cp/orgs - expect(mockFetch).toHaveBeenCalledTimes(1); - } finally { - vi.useRealTimers(); - } + // beforeEach already set up fake timers. + mockFetchSession.mockResolvedValue({ userId: "u-1" }); + mockFetch.mockResolvedValueOnce( + okJson({ + orgs: [ + { + id: "o-1", + slug: "acme", + name: "Acme", + plan: "pro", + status: "running", + created_at: "", + updated_at: "", + }, + ], + }) + ); + render(); + await vi.advanceTimersByTimeAsync(10_000); + // Only the initial /cp/orgs — no poll fires (stillMoving = false) + expect(mockFetch).toHaveBeenCalledTimes(1); }); it("clears the poll timer on unmount — no fetch after unmount", async () => { - vi.useFakeTimers({ shouldAdvanceTime: true }); - try { - mockFetchSession.mockResolvedValue({ userId: "u-1" }); - mockFetch.mockResolvedValueOnce( - okJson({ - orgs: [ - { - id: "o-1", - slug: "acme", - name: "Acme", - plan: "pro", - status: "awaiting_payment", - created_at: "", - updated_at: "", - }, - ], - }) - ); - const { unmount } = render(); - // With shouldAdvanceTime, effects are deferred — flush microtasks first - // so the effect runs and schedules the 5s poll before we unmount. - await vi.advanceTimersByTimeAsync(0); - // Now the effect has run (scheduling the poll) but not the poll itself - expect(mockFetch).toHaveBeenCalledTimes(1); - // Tear down — cleanup must clear the 5s timer - unmount(); - // Advance timers — the cleanup cleared the 5s timer, so no poll fires - await vi.advanceTimersByTimeAsync(10_000); - expect(mockFetch).toHaveBeenCalledTimes(1); - } finally { - vi.useRealTimers(); - } + // beforeEach already set up fake timers. + mockFetchSession.mockResolvedValue({ userId: "u-1" }); + mockFetch.mockResolvedValueOnce( + okJson({ + orgs: [ + { + id: "o-1", + slug: "acme", + name: "Acme", + plan: "pro", + status: "awaiting_payment", + created_at: "", + updated_at: "", + }, + ], + }) + ); + const { unmount } = render(); + // Flush microtasks so the effect runs and schedules the 5s poll before we unmount. + await vi.advanceTimersByTimeAsync(0); + // Now the effect has run (scheduling the poll) but not the poll itself + expect(mockFetch).toHaveBeenCalledTimes(1); + // Tear down — cleanup must clear the 5s timer + unmount(); + // Advance timers — the cleanup cleared the 5s timer, so no poll fires + await vi.advanceTimersByTimeAsync(10_000); + expect(mockFetch).toHaveBeenCalledTimes(1); }); });