From b023d86512da4a3df0eac16ac4f01a31e90089c1 Mon Sep 17 00:00:00 2001 From: Kris McGinnes Date: Mon, 21 Sep 2026 17:20:37 -0500 Subject: [PATCH 1/4] Stop asserting transient query state in useSchemaSync test "should fetch schema when no active schema exists" asserted that schemaDiscoveryQuery.data was undefined after flushing pending atom updates. FakeExplorer.fetchSchema resolves immediately, so whether the data has landed by then depends on microtask timing, and under CI load it had already landed (#2205 CI run). Assert the absence of initialData before settling, then wait for the fetched data, and drop the now-redundant duplicate in the staleTime block. --- .../src/hooks/useSchemaSync.test.ts | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/packages/graph-explorer/src/hooks/useSchemaSync.test.ts b/packages/graph-explorer/src/hooks/useSchemaSync.test.ts index f4267f4298..4ef2281736 100644 --- a/packages/graph-explorer/src/hooks/useSchemaSync.test.ts +++ b/packages/graph-explorer/src/hooks/useSchemaSync.test.ts @@ -120,9 +120,14 @@ describe("useSchemaSync", () => { const fetchSchemaSpy = vi.spyOn(explorer, "fetchSchema"); const { result } = renderHookWithState(() => useSchemaSync(), state); - await flushPendingAtomUpdates(); + // Asserted before settling because there is no initialData to seed the + // query. Waiting first would race the fetch, which resolves immediately. expect(result.current.schemaDiscoveryQuery.data).toBeUndefined(); + + await waitFor(() => { + expect(result.current.schemaDiscoveryQuery.data).toBeDefined(); + }); expect(fetchSchemaSpy).toHaveBeenCalled(); }); }); @@ -355,17 +360,6 @@ describe("useSchemaSync", () => { expect(fetchSchemaSpy).not.toHaveBeenCalled(); }); - it("should fetch schema when no active schema exists", async () => { - const state = new DbState(explorer).withNoActiveSchema(); - - const fetchSchemaSpy = vi.spyOn(explorer, "fetchSchema"); - - renderHookWithState(() => useSchemaSync(), state); - await flushPendingAtomUpdates(); - - expect(fetchSchemaSpy).toHaveBeenCalled(); - }); - it("should allow manual refresh when initialData exists", async () => { const state = new DbState(explorer); state.activeSchema.vertices = [createRandomVertexTypeConfig()]; From a79f145549f0404e0f671f3f703c22feedd0b94d Mon Sep 17 00:00:00 2001 From: Kris McGinnes Date: Mon, 21 Sep 2026 17:20:49 -0500 Subject: [PATCH 2/4] Select proxy fetch calls by URL instead of by index An abandoned query fires cancelQuery from the res "close" handler, and that fetch can be recorded after the test that triggered it, landing in mock.calls[0] for whichever test runs next (#2190). Cancellation is asynchronous and outlives an aborted request, so no server-side change prevents it; the assertions have to stop depending on call order. Select outbound calls by URL, so a cancellation to /sparql/status can never be read as the /sparql call under test, and scope the disallowed-origin check to the blocked host for the same reason. --- .../src/app.test.ts | 53 +++++++++++++------ 1 file changed, 37 insertions(+), 16 deletions(-) diff --git a/packages/graph-explorer-proxy-server/src/app.test.ts b/packages/graph-explorer-proxy-server/src/app.test.ts index 3ca11d821f..50d7bae156 100644 --- a/packages/graph-explorer-proxy-server/src/app.test.ts +++ b/packages/graph-explorer-proxy-server/src/app.test.ts @@ -67,6 +67,24 @@ function mockFetchOnce(body = "ok", status = 200, headers = {}) { ); } +/** + * Returns the request options of the single outbound fetch to the given path. + * + * Selected by URL rather than by call index because a query cancellation from + * an abandoned request can be recorded after the test that triggered it, + * landing in `mock.calls[0]` for whichever test runs next. + */ +function fetchOptionsFor(endpointPath: string): any { + const url = `${graphDbUrl}/${endpointPath}`; + const calls = mockFetch.mock.calls.filter( + ([calledUrl]) => + typeof calledUrl === "string" && + (calledUrl === url || calledUrl.startsWith(`${url}?`)), + ); + expect(calls).toHaveLength(1); + return calls[0][1]; +} + describe("createApp", () => { beforeEach(() => { mockFetch.mockReset(); @@ -366,7 +384,7 @@ describe("createApp", () => { const query = "SELECT * WHERE { ?s ?p ?o }"; await request(app).post("/sparql").set(dbHeaders()).send({ query }); - const fetchOptions = mockFetch.mock.calls[0][1] as any; + const fetchOptions = fetchOptionsFor("sparql"); expect(fetchOptions.headers["content-type"]).toBe( "application/x-www-form-urlencoded", ); @@ -382,7 +400,7 @@ describe("createApp", () => { .set(dbHeaders({ queryid: "q-123" })) .send({ query: "SELECT 1" }); - const fetchOptions = mockFetch.mock.calls[0][1] as any; + const fetchOptions = fetchOptionsFor("sparql"); expect(fetchOptions.body).toContain( `queryId=${encodeURIComponent("q-123")}`, ); @@ -447,7 +465,7 @@ describe("createApp", () => { const query = "g.V().limit(1)"; await request(app).post("/gremlin").set(dbHeaders()).send({ query }); - const fetchOptions = mockFetch.mock.calls[0][1] as any; + const fetchOptions = fetchOptionsFor("gremlin"); const body = JSON.parse(fetchOptions.body); expect(body.gremlin).toBe(query); }); @@ -461,7 +479,7 @@ describe("createApp", () => { .set(dbHeaders({ queryid: "q-456" })) .send({ query: "g.V()" }); - const fetchOptions = mockFetch.mock.calls[0][1] as any; + const fetchOptions = fetchOptionsFor("gremlin"); const body = JSON.parse(fetchOptions.body); expect(body.queryId).toBe("q-456"); }); @@ -507,7 +525,7 @@ describe("createApp", () => { const query = "MATCH (n) RETURN n"; await request(app).post("/openCypher").set(dbHeaders()).send({ query }); - const fetchOptions = mockFetch.mock.calls[0][1] as any; + const fetchOptions = fetchOptionsFor("openCypher"); expect(fetchOptions.headers["content-type"]).toBe( "application/x-www-form-urlencoded", ); @@ -602,7 +620,7 @@ describe("createApp", () => { .send({ query: "SELECT 1" }); // SigV4 signing adds the authorization and x-amz-* headers to the options - const fetchOptions = mockFetch.mock.calls[0][1] as any; + const fetchOptions = fetchOptionsFor("sparql"); expect(fetchOptions.headers).toHaveProperty("authorization"); }); @@ -620,7 +638,7 @@ describe("createApp", () => { ) .send({ query: "SELECT 1" }); - const fetchOptions = mockFetch.mock.calls[0][1] as any; + const fetchOptions = fetchOptionsFor("sparql"); const authHeader: string = fetchOptions.headers["authorization"]; // The Authorization header must reference our fake credential, proving // the mock intercepted the credential provider chain. @@ -643,7 +661,7 @@ describe("createApp", () => { .set(dbHeaders()) .send({ query: "SELECT 1" }); - const fetchOptions = mockFetch.mock.calls[0][1] as any; + const fetchOptions = fetchOptionsFor("sparql"); expect(fetchOptions.headers).not.toHaveProperty("authorization"); }); }); @@ -710,7 +728,7 @@ describe("createApp", () => { ) .send({ query: "SELECT 1" }); - const fetchOptions = mockFetch.mock.calls[0][1] as any; + const fetchOptions = fetchOptionsFor("sparql"); // The service appears in the SigV4 credential scope, not on the request. expect(fetchOptions.headers["authorization"]).toContain( "/us-east-1/neptune-db/aws4_request", @@ -731,7 +749,7 @@ describe("createApp", () => { ) .send({ query: "SELECT 1" }); - const fetchOptions = mockFetch.mock.calls[0][1] as any; + const fetchOptions = fetchOptionsFor("sparql"); expect(fetchOptions.headers["authorization"]).toContain( "/us-east-1/neptune-graph/aws4_request", ); @@ -750,7 +768,7 @@ describe("createApp", () => { .set(dbHeaders()) .send({ query: "SELECT 1" }); - const fetchOptions = mockFetch.mock.calls[0][1] as any; + const fetchOptions = fetchOptionsFor("sparql"); expect(fetchOptions.headers["User-Agent"]).toBe( `graph-explorer/${testVersion}`, ); @@ -778,7 +796,7 @@ describe("createApp", () => { .set(dbHeaders()) .send({ query: "SELECT 1" }); - const fetchOptions = mockFetch.mock.calls[0][1] as any; + const fetchOptions = fetchOptionsFor("sparql"); expect(fetchOptions.headers["User-Agent"]).toBe("graph-explorer"); }); @@ -796,7 +814,7 @@ describe("createApp", () => { ) .send({ query: "SELECT 1" }); - const fetchOptions = mockFetch.mock.calls[0][1] as any; + const fetchOptions = fetchOptionsFor("sparql"); expect(fetchOptions.headers["User-Agent"]).toBe( `graph-explorer/${testVersion}`, ); @@ -816,7 +834,7 @@ describe("createApp", () => { ) .send({ query: "SELECT 1" }); - const fetchOptions = mockFetch.mock.calls[0][1] as any; + const fetchOptions = fetchOptionsFor("sparql"); expect(fetchOptions.headers["content-type"]).toBe( "application/x-www-form-urlencoded", ); @@ -947,7 +965,7 @@ describe("createApp", () => { .set(dbHeaders()) .send({ query: "SELECT 1" }); - const fetchOptions = mockFetch.mock.calls[0][1] as any; + const fetchOptions = fetchOptionsFor("sparql"); expect(fetchOptions.redirect).toBe("error"); }); }); @@ -1003,7 +1021,10 @@ describe("createApp", () => { expect(response.status).toBe(403); expect(response.body.error.message).toContain("allowed origins list"); - expect(mockFetch).not.toHaveBeenCalled(); + expect(mockFetch).not.toHaveBeenCalledWith( + expect.stringContaining("https://blocked:8182"), + expect.anything(), + ); }, ); }); From 602977666e48157856f948219a6a5ac294abddd0 Mon Sep 17 00:00:00 2001 From: Kris McGinnes Date: Tue, 22 Sep 2026 12:18:50 -0500 Subject: [PATCH 3/4] Name the disallowed-origin test after what it asserts The title promised "without calling fetch" while the body checks only that the blocked host was never contacted. Keep the scoped assertion, since whether some other host was contacted is not this test's concern and is the part that depended on call history, and make the title say so. --- packages/graph-explorer-proxy-server/src/app.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/graph-explorer-proxy-server/src/app.test.ts b/packages/graph-explorer-proxy-server/src/app.test.ts index 50d7bae156..0af643c699 100644 --- a/packages/graph-explorer-proxy-server/src/app.test.ts +++ b/packages/graph-explorer-proxy-server/src/app.test.ts @@ -1009,7 +1009,7 @@ describe("createApp", () => { { method: "get", route: "/pg/statistics/summary", body: undefined }, { method: "get", route: "/rdf/statistics/summary", body: undefined }, ] as const)( - "$method $route returns 403 for disallowed origin without calling fetch", + "$method $route returns 403 without fetching the disallowed origin", async ({ method, route, body }) => { const app = createTestApp(".", undefined, allowedOrigins); const req = request(app) From 9639f65501a1f7b3ad677bc8122c501423d16794 Mon Sep 17 00:00:00 2001 From: Kris McGinnes Date: Tue, 22 Sep 2026 12:20:27 -0500 Subject: [PATCH 4/4] Write down the by-URL fetch assertion rule Nothing told the next author why mock.calls[0] is unsafe in the proxy package, and the helper's failure message named neither the endpoint it wanted nor the calls it saw. - Add a proxy section to docs/agents/testing.md covering the rule, why a cancellation can be recorded after the test that caused it, and the scoping a negative assertion needs. - Say in the helper's doc comment that the length assertion is what makes it stricter than indexing, and report the expected URL alongside the calls actually made when it fails. --- docs/agents/testing.md | 13 +++++++++++ .../src/app.test.ts | 22 ++++++++++++++----- 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/docs/agents/testing.md b/docs/agents/testing.md index 57b9cd7417..429a5ca0d8 100644 --- a/docs/agents/testing.md +++ b/docs/agents/testing.md @@ -77,6 +77,19 @@ Commands are in AGENTS.md. - **jsdom/happy-dom layout**: jsdom and happy-dom never lay out elements, so `offsetHeight`/`offsetWidth` are always `0`. A component that measures its own size (e.g. a virtualizer deciding which rows are visible) will render as empty, and the failure looks like a component bug rather than an environment limitation. Call `mockVirtualizedLayout` in a `beforeEach` — it mocks `offsetHeight`/`offsetWidth` to read the element's own inline style, falling back to a fixed size, so real measurements are distinguishable from unmeasured ones. - **Errors**: assert the full error, not just that one was thrown. `expect(() => fn()).toThrow(new FooError(a, b))` — or `await expect(fn()).rejects.toThrow(new FooError(a, b))` for a rejected promise — deep-compares every property, so a wrong field fails the test. Prefer this over `toThrow(FooError)` (type only) or `toThrow("message")` (message only), which pass even when the code built the error with the wrong data. No need to catch the error and assert fields separately — the instance form already covers them. +## Proxy server: asserting outbound fetches + +`packages/graph-explorer-proxy-server` mocks `node-fetch` globally and resets it in `beforeEach`. Assert what the proxy sent by matching the request URL and expecting exactly one call, never by call index: + +```ts +const fetchOptions = fetchOptionsFor("sparql"); // filters mock.calls by URL +expect(fetchOptions.headers["User-Agent"]).toBe("graph-explorer/1.2.3"); +``` + +`mock.calls[0]` is wrong here even though the reset makes it look safe. When a client abandons a request the proxy cancels the query from the `res` close handler, and that fetch is dispatched by socket death rather than by the request finishing, so it can be recorded after the test that caused it. It then lands in slot 0 for whichever test runs next. Positive `toHaveBeenCalledWith(url, …)` assertions are already immune. A negative assertion needs scoping to the URL it cares about, because a bare `not.toHaveBeenCalled()` fails on any stray call. + +A test that deliberately abandons a request should await the cancellation before it ends, rather than leaving it in flight for the next test. + ## Backward compatibility for persisted data Anything persisted to IndexedDB via localForage/Jotai may be reloaded in an older shape after a type change, silently breaking logic that assumes the new shape. So: **when you change the shape of a persisted type, add tests that exercise the old shape alongside the new** — old shape loads without error, consuming logic produces correct results for both, and old/new can coexist in a collection. diff --git a/packages/graph-explorer-proxy-server/src/app.test.ts b/packages/graph-explorer-proxy-server/src/app.test.ts index 0af643c699..ee9541781e 100644 --- a/packages/graph-explorer-proxy-server/src/app.test.ts +++ b/packages/graph-explorer-proxy-server/src/app.test.ts @@ -73,16 +73,26 @@ function mockFetchOnce(body = "ok", status = 200, headers = {}) { * Selected by URL rather than by call index because a query cancellation from * an abandoned request can be recorded after the test that triggered it, * landing in `mock.calls[0]` for whichever test runs next. + * + * The length assertion is what makes this stricter than the indexing it + * replaced: it fails on a missing call and on an unexpected second call to the + * same path, where reading an index silently accepted both. */ function fetchOptionsFor(endpointPath: string): any { const url = `${graphDbUrl}/${endpointPath}`; - const calls = mockFetch.mock.calls.filter( - ([calledUrl]) => - typeof calledUrl === "string" && - (calledUrl === url || calledUrl.startsWith(`${url}?`)), + // The proxy always passes url.href, so anything else is a test setup mistake + const calledUrls = mockFetch.mock.calls.map(([calledUrl]) => + typeof calledUrl === "string" ? calledUrl : "(non-string url)", + ); + const matches = mockFetch.mock.calls.filter( + (_, index) => + calledUrls[index] === url || calledUrls[index].startsWith(`${url}?`), ); - expect(calls).toHaveLength(1); - return calls[0][1]; + expect( + matches, + `expected exactly one fetch to ${url}, called: ${calledUrls.join(", ") || "(none)"}`, + ).toHaveLength(1); + return matches[0][1]; } describe("createApp", () => {