diff --git a/.gitignore b/.gitignore index a623d8837..b70e0c2e8 100644 --- a/.gitignore +++ b/.gitignore @@ -20,3 +20,5 @@ coverage internal .isaac/ + +.codex-tmp/ diff --git a/docs/docs/plugins/custom-plugins.md b/docs/docs/plugins/custom-plugins.md index 343734c97..2eeeec166 100644 --- a/docs/docs/plugins/custom-plugins.md +++ b/docs/docs/plugins/custom-plugins.md @@ -79,11 +79,11 @@ export const myPlugin = toPlugin(MyPlugin); JSON is the canonical authoring surface — it is what `appkit plugin sync` reads when aggregating manifests for templates. For the full v2.0 manifest contract (resources, discovery descriptors, scaffolding rules), see [Plugin manifest](./manifest.md). :::note Reserved plugin names -`close` cannot be used as a plugin `name`. Plugin exports are installed as own -properties on the object `createApp()` returns, and an own property shadows a -prototype method — so a plugin named `close` would silently replace the app -handle's own `close()` and break teardown. `createApp()` rejects it with a -`ConfigurationError` naming the plugin instead of failing quietly at shutdown. +`close` cannot be used as a plugin `name`. Plugin exports become own properties +on the object `createApp()` returns, so a plugin named `close` would shadow the +app handle's own `close()` method and break teardown. `createApp()` rejects it +with a `ConfigurationError` naming the plugin, rather than failing quietly at +shutdown. ::: ## Config-dependent resources diff --git a/docs/docs/plugins/testing.md b/docs/docs/plugins/testing.md index 29f85ac3c..300ef7dc5 100644 --- a/docs/docs/plugins/testing.md +++ b/docs/docs/plugins/testing.md @@ -17,7 +17,7 @@ The kit has three entry points plus a set of fixture helpers: - **`expectStream(...).toEmit(...)`** — assert the ordered event types a stream emits. - **Fixtures** — `createMockRequest`, `createMockResponse`, `createMockWorkspaceClient`, `mockServiceContext`, and SQL response builders. -The kit uses [Vitest](https://vitest.dev)'s `vi` for its mocks, so `vitest` is an **optional peer dependency**: you already have it (you're writing Vitest tests), and the kit resolves to your copy rather than bundling a second one. Because it's optional, it is not installed into apps that never import `@databricks/appkit/testing` — production installs stay free of the test framework. Any Vitest v3 or v4 works. +`vitest` is an **optional peer dependency**: the kit's mocks use its `vi` and resolve against your installed copy. Apps that never import `@databricks/appkit/testing` don't install it, so production stays free of the test framework. Any Vitest v3 or v4 works. ## Testing your plugin @@ -71,7 +71,7 @@ const app = await createTestApp({ }); ``` -A function value receives the call arguments, so you can script per-argument behavior or reject to test an error path. `responses` configures the built-in mock, so passing it alongside your own `client` is rejected rather than silently ignored — configure the responses on that client instead. Any path you **don't** declare resolves `undefined` rather than crashing — see [Mocking Databricks services](#mocking-databricks-services) for the trade-off it makes. +A function value receives the call arguments, so you can script per-argument behavior or reject to test an error path. `responses` configures the built-in mock, so passing it alongside your own `client` is rejected rather than silently ignored — configure the responses on that client instead. Any path you **don't** declare resolves `undefined` rather than crashing — see [Mocking Databricks services](#mocking-databricks-services). For the response *shapes*, follow the service types on the Databricks SDK. The kit doesn't validate them, so a wrong shape fails in your plugin, not in the fake. @@ -83,7 +83,7 @@ import { getMock } from "@databricks/appkit/testing"; expect(getMock(app.client, "jobs.getRun")).toHaveBeenCalledWith({ run_id: 42 }); ``` -`getMock` exists because facade accessors are typed against the SDK, so `expect(app.client.jobs.getRun).toHaveBeenCalled()` won't typecheck. +Facade accessors are typed against the SDK, so `expect(app.client.jobs.getRun).toHaveBeenCalled()` won't typecheck — `getMock` reaches the underlying spy. ### Requests @@ -118,6 +118,23 @@ try { Miss the close and each boot leaks a listener; Node warns at about six. +For a suite where **every** test needs its own app, `useTestApp()` wires both hooks for you — a fresh app before each test, closed after — so there is no `close()` to forget: + +```ts +import { useTestApp } from "@databricks/appkit/testing"; + +describe("my plugin over HTTP", () => { + const app = useTestApp({ plugins: [myPlugin()] }); + + test("answers a request", async () => { + const res = await app.current.post("/api/my-plugin/run", { body: { id: 1 } }); + expect(res.status).toBe(200); + }); +}); +``` + +Call it at the top of a `describe`, not inside a test — Vitest registers `beforeEach`/`afterEach` during collection. Read `.current` from within a test; outside one it throws rather than handing back a closed app. `await using` stays the shorter choice for a single test, but it cannot carry an app from a `beforeEach` into the test body. + ### Satisfying declared resources The harness runs the real validator with a strict posture, so a plugin whose manifest requires a resource fails the boot unless its env var is set. Supply it with `env`: @@ -141,7 +158,7 @@ The harness validates that required resources' **environment variables are prese - `server: false` — no socket. Plugin setup, validation, and teardown still run; the request methods throw if called. Useful when you only care that a plugin boots. - `client` — supply your own workspace client instead of the built-in fake. You then own its `currentUser.me()`: AppKit reads `currentUser.id` during boot and can't start without it. - `nodeEnv` — defaults to `"test"`. `"development"` is **refused**: dev mode routes the harness's ephemeral port through `get-port`, which throws on port `0`, and it also boots a real Vite server and relaxes validation. -- `cache` — defaults to in-memory. Overriding it is what would let the cache reach the network, so leave it alone unless that's the point of the test. +- `cache` — defaults to in-memory. Override it only when reaching the network is the point of the test. - `closeTimeoutMs` — teardown budget. ## `createTestPluginContext()` @@ -154,7 +171,7 @@ The harness validates that required resources' **environment variables are prese | Tool providers | Fakes registered through the real `registerToolProvider`, keyed by plugin then tool name. | | Routes | The real `addRoute`/`addMiddleware` are wrapped to record what a plugin registers. | -Because the context is real, `executeTool` still resolves the user scope via `asUser(req)` and still composes the abort signal from your timeout — so those paths are genuinely under test. +The context is real, so `executeTool` runs the actual user-scope (`asUser(req)`) and timeout-composition paths — not stubs of them. ### Registering fake tool responses @@ -184,9 +201,34 @@ await mock.attach(plugin); Instantiate the plugin **class** directly (`new MyAgentPlugin(...)`). The `analytics()` / `agents()` factories you pass to `createApp` return a descriptor for the app to construct — for a unit test you want the instance. -The workspace client and the on-behalf-of stub are process-wide too, not per app: `ServiceContext` holds one client, and the `createUserContext` fake is a single spy. Because of that, **`createTestApp` allows one open app at a time** and throws if you boot a second before closing the first — with two open, the second one's `client` and `responses` would not reach the handlers, and closing either would remove the shared OBO fake from the other. Vitest isolates test *files* in separate workers, so this only constrains apps within a single file. One consequence worth knowing: a `describe` that holds an app open in `beforeAll` cannot contain a test that boots its own. +### Seeding with workspace responses and environment + +`createTestPluginContext` accepts a second `options` parameter to control the faked workspace client and environment: + +```ts +const mock = createTestPluginContext({}, { + responses: { + "jobs.getRun": { state: "TERMINATED" }, + "servingEndpoints.query": (args, signal) => runFakeQuery(args), + }, + env: { MY_VAR: "test-value" }, + strict: true, +}); + +// The factory call is synchronous; attach is the async part. +await mock.attach(plugin); +``` + +`options` is: +- `responses` — seed the mock workspace client with responses keyed by dotted path (`"jobs.getRun"`, `"genie.getMessage"`). A value can be static or a function of call arguments and the abort signal. +- `env` — set environment variables scoped to the test; they are restored on plugin detach. +- `strict` — throw if a handler calls an undeclared workspace-client path (instead of silently resolving `undefined`). The built-in defaults still count as declared. + +The context installs a test-scoped service context via `beforeEach` and restores it on `afterEach`, so it survives across tests in the same suite. Call the returned `.restore()` explicitly if you need to clear it mid-test. + +The workspace client and on-behalf-of stub are process-wide too: `ServiceContext` holds one client, and the `createUserContext` fake is a single spy. So **`createTestApp` allows one open app at a time** and throws if you boot a second before closing the first. Vitest isolates test *files* in separate workers, so this constrains only apps within one file — and a `describe` holding an app open in `beforeAll` can't contain a test that boots its own. -The cache `attach()` seeds is a process-wide singleton: `CacheManager` is initialized once per test process and reused. Vitest isolates test *files* in separate workers, so caches never leak across files, but tests **within one file** share it. If a test populates the cache and a later test in the same file must not see it, clear it between tests with `resetTestCache()`: +The cache is a process-wide singleton too — initialized once per test process and shared by tests **within one file** (it never leaks across files). If one test populates it and a later one must not see that, clear between tests with `resetTestCache()`: ```ts import { resetTestCache } from "@databricks/appkit/testing"; @@ -198,6 +240,32 @@ beforeEach(async () => { It also helps *within* a single test — clear the cache to force a miss, then assert the following call is a hit. +### Asserting cache behaviour + +When a plugin caches its work (like `analytics` caching query results), test the caching *itself* — a second identical call is a hit, different users get different keys — with `useTestCache()`. It boots the real in-memory cache, clears it before each test, and hands back the real `CacheManager`, so you assert against production's own `getOrExecute` and `generateKey` rather than mocking the internal `cache` module: + +```ts +import { useTestCache } from "@databricks/appkit/testing"; + +describe("my plugin caches", () => { + const testCache = useTestCache(); + + test("a second identical request is served from cache", async () => { + const plugin = new MyPlugin(config); + // ...drive the same request twice against a mocked downstream call... + expect(downstreamMock).toHaveBeenCalledTimes(1); + }); + + test("scopes the cache key per user", () => { + const a = testCache.current.generateKey(["query", sql], "user-1"); + const b = testCache.current.generateKey(["query", sql], "user-2"); + expect(a).not.toBe(b); + }); +}); +``` + +Call it at the top of a `describe` (or module top-level), not inside a test — Vitest registers its `beforeEach`/`afterEach` at collection time. It boots the cache before each test, so a plugin you construct binds `this.cache` to the real cache and runs its actual caching path. Use `resetTestCache()` (above) instead when you only need to clear the cache, not a handle to it. + ### Inspecting what happened The returned object exposes live views you read after the action under test runs: @@ -224,7 +292,7 @@ expect(mock.telemetry.getTracer().startActiveSpan).toHaveBeenCalled(); `mock.telemetry` is injected into the `PluginContext`, so it captures the spans the *context* opens (notably `executeTool`). It is **not** the plugin's own telemetry: `attachContext` rebuilds `this.telemetry` from the real `TelemetryManager`, so spans a plugin opens internally do not land on `mock.telemetry`. -`RecordedToolCall.asUser` is the field to assert for cross-plugin calls: because the fake `asUser` enforces the same token precondition as the real `Plugin.asUser`, a dispatch that records `asUser: true` (with `userId` set) genuinely resolved the caller's user scope, and a request missing `x-forwarded-access-token` **rejects** instead — the OBO distinction that silent `{ executeTool }` stubs cannot verify. Assert both directions: a well-formed request records the expected `userId`, and a token-less one throws. +Assert cross-plugin on-behalf-of through `RecordedToolCall.asUser`. The fake `asUser` enforces the real `Plugin.asUser`'s token precondition: a request carrying a forwarded token records `asUser: true` with the resolved `userId`, and one missing `x-forwarded-access-token` **rejects**. Assert both directions — a well-formed request records the expected `userId`, a token-less one throws. A silent `{ executeTool }` stub verifies neither. The fake replicates `asUser`'s **token precondition**, not its internal dev-mode telemetry marker: in `NODE_ENV=development` the real `Plugin.asUser` skips impersonation and sets an OTel `isDevOboFallback()` flag, which the fake does not reproduce. Assert OBO through the recorded `asUser`/`userId` fields rather than `isDevOboFallback()`. @@ -273,11 +341,12 @@ await expectStream(handler.stream(req), { timeout: 1000 }).toEmit("result"); AppKit has two contexts, and they're faked by different tools. `PluginContext` is the mediator between plugins, handling routes, tool dispatch, and user scoping; `createTestPluginContext()` gives you the real thing with faked edges. `ServiceContext` is the **data plane**: it resolves the workspace client, the service principal, and the warehouse ID that plugins reach through `getWorkspaceClient()`. -The kit now covers both. `createTestApp` fakes the data plane for you by injecting a mock workspace client at the real seam; below that, `mockServiceContext` spies the singleton directly, and `createMockWorkspaceClient` builds the client either of them installs. +The kit covers both. `createTestApp` fakes the data plane by injecting a mock workspace client at the real seam; below that, `mockServiceContext` spies the singleton directly, and `createMockWorkspaceClient` builds the client either of them installs. The kit re-exports the request/response/context fixtures AppKit uses internally: - `createMockRequest(overrides?)` / `createMockResponse()` — Express request/response doubles, including the streaming flags (`headersSent`, `writableEnded`). Pass `obo: true` (or `obo: { userId, token, email }`) to set the forwarded identity headers `asUser` requires, instead of hand-adding them. `createMockResponse()` also captures everything a handler writes; pass it to `expectStream` (or call `sseResponse()`) to assert a streaming route's SSE. (Plugins resolve the workspace client through `getWorkspaceClient()`, not the request — use `mockServiceContext` to control it.) +- `createMockRouter()` — build a mock Express-style router for testing route-registration wiring. - `mockServiceContext(options?)` — spy the `ServiceContext` singleton so code that resolves the service principal or a user context gets test doubles. Call in `beforeEach`, and call the returned `restore()` in `afterEach`. - `useServiceContextMock(options?)` — the same, in one line: it registers the `beforeEach` install and `afterEach` restore for you. Call it at the top of a `describe` block (not inside a test), and read the live `.current` handle from within a test: ```ts @@ -291,6 +360,17 @@ The kit re-exports the request/response/context fixtures AppKit uses internally: ``` - `createSuccessfulSQLResponse(rows, columns)` / `createFailedSQLResponse(message)` — build SQL Warehouse statement responses. - `setupDatabricksEnv(overrides?)` — set `DATABRICKS_HOST` / `DATABRICKS_WAREHOUSE_ID` to test values. +- `withEnv(vars, fn)` — set environment variables for the duration of a sync or async function, restoring each key's prior state (or deleting it if it was previously unset). Unlike a bare `process.env.X = ...` followed by `delete`, nested calls restore LIFO and don't accidentally leave prior values in place. + ```ts + // Before: process.env.X = "test"; try { /* code */ } finally { delete process.env.X } + // After: + await withEnv({ X: "test" }, async () => { /* code */ }); + ``` +- `createApiError({ statusCode, message, errorCode })` — create a genuine `ApiError` instance for testing error paths. Returns an instance where `error instanceof ApiError` holds, so your error handling resolves the right type. + ```ts + const error = createApiError({ statusCode: 404, message: "Not found", errorCode: "NOT_FOUND" }); + expect(error instanceof ApiError).toBe(true); + ``` - `resetTestCache()` — clear the shared cache singleton between (or within) tests; no-ops if the cache isn't initialized yet. - `resetGlobalState()` — drop AppKit's process-wide singletons so a later `createApp` builds fresh ones. `createTestApp`'s `close()` already does this; you need it only if you call `createApp` yourself. Close first, then reset — it drops pointers, it doesn't release resources. The kit uses both words deliberately: a **mock** records calls so you can assert on them (`createMockWorkspaceClient`, `mockServiceContext`), while a **fake** stands in and simply works (`FakeProvider`, `FakeToolResponse`). @@ -333,9 +413,9 @@ const app = await createTestApp({ plugins: [myPlugin()], strict: true }); // a handler calling an undeclared path now fails the request ``` -TypeScript covers more of this than you might expect: because each accessor is typed against the SDK's own service class, both a misspelled **service** (`client.jbos`) and a misspelled **method** (`client.jobs.getRunz`) are compile errors. The gap is a *real* method with no declared response — and any call that bypasses the types with a cast. +TypeScript catches more than the obvious: each accessor is typed against the SDK's own service class, so both a misspelled **service** (`client.jbos`) and a misspelled **method** (`client.jobs.getRunz`) are compile errors. The gap is a *real* method with no declared response — and any call that bypasses the types with a cast. -One more divergence: a service's methods are minted on access, so they are **callable but not enumerable**. `typeof client.jobs.getRun` is `"function"`, but `'getRun' in client.jobs` is `false` and `Object.keys(client.jobs)` is `[]`. Plugin code that feature-detects with `in` or reflects over a service will therefore take a different branch than it does in production. This is deliberate: reporting those keys would make `util.inspect` probe each one, minting a mock per probe, which is the runaway recursion the default traps avoid. +One more divergence: a service's methods are minted on access, so they are **callable but not enumerable**. `typeof client.jobs.getRun` is `"function"`, but `'getRun' in client.jobs` is `false` and `Object.keys(client.jobs)` is `[]`. Plugin code that feature-detects with `in` or reflects over a service will therefore take a different branch than in production. That's deliberate — reporting the keys would make `util.inspect` probe each one, minting a mock per probe. Separately, `createLakebasePool({ workspaceClient })` will build a pool whose password callback resolves to a mock: the pool exists but cannot connect. A Lakebase test needs a real database or a purpose-built fake pool, not this. ::: diff --git a/packages/appkit/src/connectors/files/tests/client.test.ts b/packages/appkit/src/connectors/files/tests/client.test.ts index 31ebeb489..09be16e7c 100644 --- a/packages/appkit/src/connectors/files/tests/client.test.ts +++ b/packages/appkit/src/connectors/files/tests/client.test.ts @@ -1,55 +1,38 @@ import { createMockTelemetry } from "@tools/test-helpers"; import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; +import { createApiError } from "../../../testing"; import type { WorkspaceClient } from "../../../workspace-client"; +import { ApiError } from "../../../workspace-client"; import { FilesConnector } from "../client"; import { streamFromChunks, streamFromString } from "./utils"; -const { mockFilesApi, mockConfig, mockClient, MockApiError } = vi.hoisted( - () => { - const mockFilesApi = { - listDirectoryContents: vi.fn(), - download: vi.fn(), - getMetadata: vi.fn(), - upload: vi.fn(), - createDirectory: vi.fn(), - delete: vi.fn(), - }; - - const mockConfig = { - host: "https://test.databricks.com", - authenticate: vi.fn(), - }; - - const mockApiClient = { - userAgent: vi.fn(() => "@databricks/appkit/9.9.9"), - }; - const mockClient = { - files: mockFilesApi, - config: mockConfig, - apiClient: mockApiClient, - } as unknown as WorkspaceClient; - - class MockApiError extends Error { - errorCode: string; - statusCode: number; - constructor( - message: string, - errorCode: string, - statusCode: number, - _response?: any, - _details?: any[], - ) { - super(message); - this.name = "ApiError"; - this.errorCode = errorCode; - this.statusCode = statusCode; - } - } +const { mockFilesApi, mockConfig, mockClient } = vi.hoisted(() => { + const mockFilesApi = { + listDirectoryContents: vi.fn(), + download: vi.fn(), + getMetadata: vi.fn(), + upload: vi.fn(), + createDirectory: vi.fn(), + delete: vi.fn(), + }; - return { mockFilesApi, mockConfig, mockClient, MockApiError }; - }, -); + const mockConfig = { + host: "https://test.databricks.com", + authenticate: vi.fn(), + }; + + const mockApiClient = { + userAgent: vi.fn(() => "@databricks/appkit/9.9.9"), + }; + const mockClient = { + files: mockFilesApi, + config: mockConfig, + apiClient: mockApiClient, + } as unknown as WorkspaceClient; + + return { mockFilesApi, mockConfig, mockClient }; +}); vi.mock("../../../workspace-client", async (importOriginal) => { const actual = @@ -57,7 +40,6 @@ vi.mock("../../../workspace-client", async (importOriginal) => { return { ...actual, createWorkspaceClient: () => mockClient, - ApiError: MockApiError, }; }); @@ -375,7 +357,11 @@ describe("FilesConnector", () => { test("returns false on 404 ApiError", async () => { mockFilesApi.getMetadata.mockRejectedValue( - new MockApiError("Not found", "NOT_FOUND", 404), + createApiError({ + message: "Not found", + errorCode: "NOT_FOUND", + statusCode: 404, + }), ); const result = await connector.exists(mockClient, "missing.txt"); @@ -385,7 +371,11 @@ describe("FilesConnector", () => { test("rethrows non-404 ApiError", async () => { mockFilesApi.getMetadata.mockRejectedValue( - new MockApiError("Server error", "SERVER_ERROR", 500), + createApiError({ + message: "Server error", + errorCode: "SERVER_ERROR", + statusCode: 500, + }), ); await expect(connector.exists(mockClient, "file.txt")).rejects.toThrow( @@ -579,7 +569,7 @@ describe("FilesConnector", () => { try { await connector.upload(mockClient, "file.txt", "data"); } catch (error) { - expect(error).toBeInstanceOf(MockApiError); + expect(error).toBeInstanceOf(ApiError); expect((error as any).statusCode).toBe(403); } }); diff --git a/packages/appkit/src/connectors/lakebase/tests/pool-manager.test.ts b/packages/appkit/src/connectors/lakebase/tests/pool-manager.test.ts index 9def8f43e..8462d83fa 100644 --- a/packages/appkit/src/connectors/lakebase/tests/pool-manager.test.ts +++ b/packages/appkit/src/connectors/lakebase/tests/pool-manager.test.ts @@ -1,21 +1,6 @@ import type { Pool } from "pg"; import { afterEach, describe, expect, test, vi } from "vitest"; -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => ({ - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (_k: unknown[], fn: (signal?: AbortSignal) => Promise) => - fn(), - ), - generateKey: vi.fn(() => "test-key"), - })), - }, -})); - const mockPools: Pool[] = []; vi.mock("../index", () => ({ diff --git a/packages/appkit/src/connectors/lakebase/tests/routing-pool.test.ts b/packages/appkit/src/connectors/lakebase/tests/routing-pool.test.ts index c19f7c15e..4ccccd86d 100644 --- a/packages/appkit/src/connectors/lakebase/tests/routing-pool.test.ts +++ b/packages/appkit/src/connectors/lakebase/tests/routing-pool.test.ts @@ -3,21 +3,6 @@ import { describe, expect, test, vi } from "vitest"; import { RoutingPool } from "../routing-pool"; -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => ({ - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (_k: unknown[], fn: (signal?: AbortSignal) => Promise) => - fn(), - ), - generateKey: vi.fn(() => "test-key"), - })), - }, -})); - function makeMockPool(label: string) { return { query: vi.fn(async () => ({ rows: [{ source: label }] })), diff --git a/packages/appkit/src/plugin/tests/asUser-proxy.test.ts b/packages/appkit/src/plugin/tests/asUser-proxy.test.ts index 2566f2869..f9cdada9e 100644 --- a/packages/appkit/src/plugin/tests/asUser-proxy.test.ts +++ b/packages/appkit/src/plugin/tests/asUser-proxy.test.ts @@ -42,17 +42,6 @@ import type { ITelemetry, TelemetryProvider } from "../../telemetry"; import { TelemetryManager } from "../../telemetry"; import { isDevOboFallback, Plugin } from "../plugin"; -vi.mock("../../workspace-client", async (importOriginal) => { - const actual = - await importOriginal(); - return { - ...actual, - ApiError: class extends Error { - statusCode = 500; - }, - }; -}); - vi.mock("../../app"); vi.mock("../../cache", () => ({ CacheManager: { getInstanceSync: vi.fn() }, diff --git a/packages/appkit/src/plugin/tests/plugin.test.ts b/packages/appkit/src/plugin/tests/plugin.test.ts index 12108f7c4..dfee008ba 100644 --- a/packages/appkit/src/plugin/tests/plugin.test.ts +++ b/packages/appkit/src/plugin/tests/plugin.test.ts @@ -35,30 +35,10 @@ import { import { StreamManager } from "../../stream"; import type { ITelemetry, TelemetryProvider } from "../../telemetry"; import { TelemetryManager } from "../../telemetry"; +import { createApiError } from "../../testing"; import type { InterceptorContext } from "../interceptors/types"; import { isDevOboFallback, Plugin } from "../plugin"; -const { MockApiError } = vi.hoisted(() => { - class MockApiError extends Error { - statusCode: number; - constructor(message: string, statusCode: number) { - super(message); - this.name = "ApiError"; - this.statusCode = statusCode; - } - } - return { MockApiError }; -}); - -vi.mock("../../workspace-client", async (importOriginal) => { - const actual = - await importOriginal(); - return { - ...actual, - ApiError: MockApiError, - }; -}); - // Mock all dependencies vi.mock("../../app"); vi.mock("../../cache", () => ({ @@ -428,7 +408,11 @@ describe("Plugin", () => { test("should preserve 404 statusCode from ApiError (non-AppKitError)", async () => { const plugin = new TestPlugin(config); - const apiError = new MockApiError("Not found", 404); + const apiError = createApiError({ + statusCode: 404, + message: "Not found", + errorCode: "ERROR", + }); const mockFn = vi.fn().mockRejectedValue(apiError); const result = await (plugin as any).execute( @@ -445,7 +429,11 @@ describe("Plugin", () => { test("should preserve 401 statusCode from ApiError (non-AppKitError)", async () => { const plugin = new TestPlugin(config); - const apiError = new MockApiError("Unauthorized", 401); + const apiError = createApiError({ + statusCode: 401, + message: "Unauthorized", + errorCode: "ERROR", + }); const mockFn = vi.fn().mockRejectedValue(apiError); const result = await (plugin as any).execute( @@ -462,7 +450,11 @@ describe("Plugin", () => { test("should preserve 403 statusCode from ApiError (non-AppKitError)", async () => { const plugin = new TestPlugin(config); - const apiError = new MockApiError("Forbidden", 403); + const apiError = createApiError({ + statusCode: 403, + message: "Forbidden", + errorCode: "ERROR", + }); const mockFn = vi.fn().mockRejectedValue(apiError); const result = await (plugin as any).execute( @@ -479,7 +471,11 @@ describe("Plugin", () => { test("should preserve 502 statusCode from non-AppKitError", async () => { const plugin = new TestPlugin(config); - const apiError = new MockApiError("Bad gateway", 502); + const apiError = createApiError({ + statusCode: 502, + message: "Bad gateway", + errorCode: "ERROR", + }); const mockFn = vi.fn().mockRejectedValue(apiError); const result = await (plugin as any).execute( @@ -499,7 +495,11 @@ describe("Plugin", () => { process.env.NODE_ENV = "production"; try { const plugin = new TestPlugin(config); - const apiError = new MockApiError("Internal upstream detail", 502); + const apiError = createApiError({ + statusCode: 502, + message: "Internal upstream detail", + errorCode: "ERROR", + }); const mockFn = vi.fn().mockRejectedValue(apiError); const result = await (plugin as any).execute( @@ -522,7 +522,11 @@ describe("Plugin", () => { process.env.NODE_ENV = "production"; try { const plugin = new TestPlugin(config); - const apiError = new MockApiError("Forbidden", 403); + const apiError = createApiError({ + statusCode: 403, + message: "Forbidden", + errorCode: "ERROR", + }); const mockFn = vi.fn().mockRejectedValue(apiError); const result = await (plugin as any).execute( diff --git a/packages/appkit/src/plugins/ai-search/tests/ai-search.test.ts b/packages/appkit/src/plugins/ai-search/tests/ai-search.test.ts index 25ad528c9..9f8cf3d10 100644 --- a/packages/appkit/src/plugins/ai-search/tests/ai-search.test.ts +++ b/packages/appkit/src/plugins/ai-search/tests/ai-search.test.ts @@ -5,6 +5,8 @@ import { } from "@tools/test-helpers"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { withEnv } from "../../../testing"; +import { useTestCache } from "../../../testing/test-cache"; import { Context } from "../../../workspace-client"; vi.mock("../../../context", () => ({ @@ -52,8 +54,16 @@ vi.mock("../../../telemetry", () => ({ ) => fn({ setAttribute: vi.fn(), + setAttributes: vi.fn(), setStatus: vi.fn(), recordException: vi.fn(), + addEvent: vi.fn(), + addLink: vi.fn(), + addLinks: vi.fn(), + updateName: vi.fn(), + isRecording: vi.fn().mockReturnValue(false), + spanContext: vi.fn(), + end: vi.fn(), }), ), }), @@ -63,37 +73,9 @@ vi.mock("../../../telemetry", () => ({ normalizeTelemetryOptions: () => ({ traces: false, metrics: false }), })); -// In-memory cache keyed like the real CacheManager.generateKey, so tests -// exercise real key composition. Never stores rejections. -const { mockCacheStore } = vi.hoisted(() => ({ - mockCacheStore: new Map(), -})); - -vi.mock("../../../cache", () => { - const keyOf = (parts: unknown[], userKey: string) => - JSON.stringify([userKey, ...parts]); - return { - CacheManager: { - getInstanceSync: () => ({ - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - generateKey: keyOf, - getOrExecute: async ( - key: unknown[], - fn: (signal?: AbortSignal) => Promise, - userKey: string, - ) => { - const k = keyOf(key, userKey); - if (mockCacheStore.has(k)) return mockCacheStore.get(k); - const result = await fn(); - mockCacheStore.set(k, result); - return result; - }, - }), - }, - }; -}); +// Real in-memory cache so the plugin's caching path runs under test — no mock +// of the internal cache module. Boots and clears the cache before each test. +useTestCache(); vi.mock("../../../app", () => ({ AppManager: vi.fn().mockImplementation(() => ({})), @@ -139,7 +121,6 @@ describe("AiSearchPlugin", () => { beforeEach(() => { mockRequest.mockClear(); mockRequest.mockResolvedValue(validVsResponse); - mockCacheStore.clear(); }); describe("setup()", () => { @@ -206,31 +187,23 @@ describe("AiSearchPlugin", () => { }); it("throws outside development when an index has no columns", async () => { - const originalNodeEnv = process.env.NODE_ENV; - process.env.NODE_ENV = "production"; - try { + await withEnv({ NODE_ENV: "production" }, async () => { const plugin = new AiSearchPlugin({ indexes: { docs: { indexName: "cat.sch.idx" } }, }); await expect(plugin.setup()).rejects.toThrow( 'Index "docs" has no columns configured', ); - } finally { - process.env.NODE_ENV = originalNodeEnv; - } + }); }); it("does not throw outside development when columns are configured", async () => { - const originalNodeEnv = process.env.NODE_ENV; - process.env.NODE_ENV = "production"; - try { + await withEnv({ NODE_ENV: "production" }, async () => { const plugin = new AiSearchPlugin({ indexes: { docs: { indexName: "cat.sch.idx", columns: ["id"] } }, }); await expect(plugin.setup()).resolves.not.toThrow(); - } finally { - process.env.NODE_ENV = originalNodeEnv; - } + }); }); }); diff --git a/packages/appkit/src/plugins/analytics/tests/analytics.readonly.test.ts b/packages/appkit/src/plugins/analytics/tests/analytics.readonly.test.ts index 68b6b94d7..758ac62f0 100644 --- a/packages/appkit/src/plugins/analytics/tests/analytics.readonly.test.ts +++ b/packages/appkit/src/plugins/analytics/tests/analytics.readonly.test.ts @@ -1,22 +1,11 @@ import { describe, expect, test, vi } from "vitest"; -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => ({ - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (_k: unknown[], fn: (signal?: AbortSignal) => Promise) => - fn(), - ), - generateKey: vi.fn(() => "test-key"), - })), - }, -})); - +import { useTestCache } from "../../../testing/test-cache"; import { AnalyticsPlugin } from "../analytics"; +// Boots AppKit's real in-memory cache (no cache-module mock needed). +useTestCache(); + /** * Tests the read-only SQL enforcement on the analytics agent tool. * diff --git a/packages/appkit/src/plugins/analytics/tests/analytics.test.ts b/packages/appkit/src/plugins/analytics/tests/analytics.test.ts index 2151103eb..9cb9b2036 100644 --- a/packages/appkit/src/plugins/analytics/tests/analytics.test.ts +++ b/packages/appkit/src/plugins/analytics/tests/analytics.test.ts @@ -22,48 +22,13 @@ import { sql } from "shared"; import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; import { ServiceContext } from "../../../context/service-context"; +import { useTestCache } from "../../../testing/test-cache"; import { AnalyticsPlugin, analytics, writeChunk } from "../analytics"; import type { IAnalyticsConfig } from "../types"; -// Mock CacheManager singleton with actual caching behavior -const { mockCacheStore, mockCacheInstance } = vi.hoisted(() => { - const store = new Map(); - - const generateKey = (parts: unknown[], userKey: string): string => { - const { createHash } = require("node:crypto"); - const allParts = [userKey, ...parts]; - const serialized = JSON.stringify(allParts); - return createHash("sha256").update(serialized).digest("hex"); - }; - - const instance = { - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (key: unknown[], fn: () => Promise, userKey: string) => { - const cacheKey = generateKey(key, userKey); - if (store.has(cacheKey)) { - return store.get(cacheKey); - } - const result = await fn(); - store.set(cacheKey, result); - return result; - }, - ), - generateKey: vi.fn((parts: unknown[], userKey: string) => - generateKey(parts, userKey), - ), - }; - - return { mockCacheStore: store, mockCacheInstance: instance }; -}); - -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => mockCacheInstance), - }, -})); +// Real in-memory cache so the plugin's caching path runs under test — no mock +// of the internal cache module. Boots and clears the cache before each test. +useTestCache(); describe("Analytics Plugin", () => { let config: IAnalyticsConfig; @@ -72,7 +37,6 @@ describe("Analytics Plugin", () => { beforeEach(async () => { config = { timeout: 5000 }; setupDatabricksEnv(); - mockCacheStore.clear(); ServiceContext.reset(); serviceContextMock = await mockServiceContext(); }); @@ -1577,14 +1541,17 @@ describe("Analytics Plugin", () => { isAsUser: false, }); - const executeMock = vi.fn().mockImplementation((_wc, _opts, signal) => { - // Simulate a signal that becomes aborted before the failure surfaces — - // e.g. the client cancelled the SSE stream mid-query. Use vitest's - // getter spy rather than Object.defineProperty so we don't try to - // override the native non-configurable AbortSignal.aborted getter. - if (signal) { - vi.spyOn(signal, "aborted", "get").mockReturnValue(true); - } + const mockRes = createMockResponse(); + + const executeMock = vi.fn().mockImplementation(() => { + // Simulate the client cancelling mid-query: firing the response's + // "close" event aborts the handler's own AbortController (see + // `onClose` in `_handleArrowStreamQuery`), exactly as a real disconnect + // would. Modelling the abort on the handler signal — the one the + // fallback guard actually checks — rather than on whatever signal + // reaches executeStatement keeps this independent of how the cache + // threads its shared signal into the inner fn. + mockRes.end(); return Promise.reject( new Error( "INVALID_PARAMETER_VALUE: ARROW_STREAM not supported with INLINE disposition", @@ -1600,12 +1567,11 @@ describe("Analytics Plugin", () => { params: { query_key: "test_query" }, body: { parameters: {}, format: "ARROW_STREAM" }, }); - const mockRes = createMockResponse(); await handler(mockReq, mockRes); - // Even though the error message would normally trigger fallback, the - // aborted signal should short-circuit and prevent a second statement. + // The aborted request short-circuits the INLINE→EXTERNAL_LINKS fallback, + // so exactly one statement runs. expect(executeMock).toHaveBeenCalledTimes(1); }); diff --git a/packages/appkit/src/plugins/analytics/tests/metric.test.ts b/packages/appkit/src/plugins/analytics/tests/metric.test.ts index bf721feee..111ee0a95 100644 --- a/packages/appkit/src/plugins/analytics/tests/metric.test.ts +++ b/packages/appkit/src/plugins/analytics/tests/metric.test.ts @@ -15,6 +15,7 @@ import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; import { AppManager } from "../../../app"; import { ServiceContext } from "../../../context/service-context"; import { AuthenticationError } from "../../../errors"; +import { useTestCache } from "../../../testing/test-cache"; import { AnalyticsPlugin } from "../analytics"; import { buildMetricSql, @@ -31,40 +32,9 @@ import type { MetricRegistration, } from "../types"; -// Mirror the analytics.test.ts CacheManager mock so the inner `execute`'s -// cache interceptor is a no-op pass-through (each request re-executes). -const { mockCacheStore, mockCacheInstance } = vi.hoisted(() => { - const store = new Map(); - const generateKey = (parts: unknown[], userKey: string): string => { - const { createHash } = require("node:crypto"); - const serialized = JSON.stringify([userKey, ...parts]); - return createHash("sha256").update(serialized).digest("hex"); - }; - const instance = { - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (key: unknown[], fn: () => Promise, userKey: string) => { - const cacheKey = generateKey(key, userKey); - if (store.has(cacheKey)) return store.get(cacheKey); - const result = await fn(); - store.set(cacheKey, result); - return result; - }, - ), - generateKey: vi.fn((parts: unknown[], userKey: string) => - generateKey(parts, userKey), - ), - }; - return { mockCacheStore: store, mockCacheInstance: instance }; -}); - -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => mockCacheInstance), - }, -})); +// Real in-memory cache so the metric route's cache interceptor runs under test +// — no mock of the internal cache module. Boots and clears it before each test. +const testCache = useTestCache(); // Temp dirs created by `registryDir` / `writeRegistry`, cleaned up after each // test. Using real files (pointing the plugin's `AppManager` at the dir, see @@ -146,7 +116,6 @@ describe("analytics metric route", () => { beforeEach(async () => { config = { timeout: 5000 }; setupDatabricksEnv(); - mockCacheStore.clear(); ServiceContext.reset(); serviceContextMock = await mockServiceContext(); }); @@ -1064,8 +1033,12 @@ describe("analytics metric route", () => { // Capture the composed cache key the inner `execute` hands to the shared // CacheManager mock — the same key whether or not metadata is injected. + // Spy the real cache's getOrExecute to capture the composed key parts the + // metric route's cache interceptor passes — the same whether or not + // metadata is injected. + const getOrExecuteSpy = vi.spyOn(testCache.current, "getOrExecute"); const cacheKeyFor = async (mvMeta?: MetricViewsMetadata) => { - mockCacheInstance.getOrExecute.mockClear(); + getOrExecuteSpy.mockClear(); const plugin = pluginForDir( { ...config, metricViewsMetadata: mvMeta }, registryDir(registry), @@ -1079,12 +1052,13 @@ describe("analytics metric route", () => { createMockResponse(), ); // First getOrExecute call is the SQL execution's cache interceptor. - const call = mockCacheInstance.getOrExecute.mock.calls[0]; + const call = getOrExecuteSpy.mock.calls[0]; return { cacheKey: call[0], userKey: call[2] }; }; const withMeta = await cacheKeyFor(REVENUE_METADATA); const withoutMeta = await cacheKeyFor(undefined); + getOrExecuteSpy.mockRestore(); expect(withMeta.cacheKey).toEqual(withoutMeta.cacheKey); expect(withMeta.userKey).toEqual(withoutMeta.userKey); @@ -2489,7 +2463,6 @@ describe("metric — filter translator", () => { beforeEach(async () => { config = { timeout: 5000 }; setupDatabricksEnv(); - mockCacheStore.clear(); ServiceContext.reset(); serviceContextMock = await mockServiceContext(); }); @@ -2960,7 +2933,6 @@ describe("metric route — lane dispatch", () => { beforeEach(async () => { config = { timeout: 5000 }; setupDatabricksEnv(); - mockCacheStore.clear(); ServiceContext.reset(); serviceContextMock = await mockServiceContext(); }); diff --git a/packages/appkit/src/plugins/files/tests/_test-helpers.ts b/packages/appkit/src/plugins/files/tests/_test-helpers.ts index 1531ce1a4..b559e2722 100644 --- a/packages/appkit/src/plugins/files/tests/_test-helpers.ts +++ b/packages/appkit/src/plugins/files/tests/_test-helpers.ts @@ -134,13 +134,19 @@ export function makeStreamResponse(content: string) { return { contents: stream }; } -export async function setupTestEnv() { +export async function setupTestEnv(client?: unknown) { vi.clearAllMocks(); setupDatabricksEnv(); ServiceContext.reset(); process.env.DATABRICKS_VOLUME_UPLOADS = "/Volumes/catalog/schema/uploads"; process.env.DATABRICKS_VOLUME_EXPORTS = "/Volumes/catalog/schema/exports"; - return mockServiceContext(); + // Injecting a client makes `getWorkspaceClient()` resolve to it through the + // real ServiceContext, so a suite needs no vi.mock of `../../../context`. + return mockServiceContext( + client + ? { serviceDatabricksClient: client, userDatabricksClient: client } + : {}, + ); } export function teardownTestEnv( diff --git a/packages/appkit/src/plugins/files/tests/delete.test.ts b/packages/appkit/src/plugins/files/tests/delete.test.ts index 1cdaa7e5d..9ef00f942 100644 --- a/packages/appkit/src/plugins/files/tests/delete.test.ts +++ b/packages/appkit/src/plugins/files/tests/delete.test.ts @@ -1,5 +1,11 @@ import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; +import { + createApiError, + createMockWorkspaceClient, + getMock, + useTestCache, +} from "../../../testing"; import { FilesPlugin } from "../plugin"; import { getRouteHandler, @@ -10,73 +16,28 @@ import { VOLUMES_CONFIG, } from "./_test-helpers"; -const { mockClient, MockApiError, mockCacheInstance } = vi.hoisted(() => { - const mockFilesApi = { - listDirectoryContents: vi.fn(), - download: vi.fn(), - getMetadata: vi.fn(), - upload: vi.fn(), - createDirectory: vi.fn(), - delete: vi.fn(), - }; - const mockClient = { - files: mockFilesApi, - config: { - host: "https://test.databricks.com", - authenticate: vi.fn(), - }, - }; - class MockApiError extends Error { - statusCode: number; - constructor(message: string, statusCode: number) { - super(message); - this.name = "ApiError"; - this.statusCode = statusCode; - } - } - const mockCacheInstance = { - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (_key: unknown[], fn: (signal?: AbortSignal) => Promise) => - fn(), - ), - generateKey: vi.fn((...args: unknown[]) => JSON.stringify(args)), - }; - return { mockClient, MockApiError, mockCacheInstance }; -}); - -vi.mock("../../../workspace-client", async (importOriginal) => { - const actual = - await importOriginal(); - return { - ...actual, - createWorkspaceClient: (..._args: unknown[]) => mockClient, - ApiError: MockApiError, - }; -}); - -vi.mock("../../../context", async (importOriginal) => { - const actual = await importOriginal(); - return { - ...actual, - getWorkspaceClient: vi.fn(() => mockClient), - isInUserContext: vi.fn(() => true), - }; -}); - -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => mockCacheInstance), - }, -})); +// Real in-memory cache; spy on `testCache.current` to assert the plugin's +// cache-invalidation calls. +const testCache = useTestCache(); describe("FilesPlugin delete", () => { let serviceContextMock: Awaited>; + let client: ReturnType; + beforeEach(async () => { - serviceContextMock = await setupTestEnv(); + client = createMockWorkspaceClient({ + strict: true, + responses: { + "files.listDirectoryContents": undefined, + "files.download": undefined, + "files.getMetadata": undefined, + "files.upload": undefined, + "files.createDirectory": undefined, + "files.delete": undefined, + }, + }); + serviceContextMock = await setupTestEnv(client); }); afterEach(() => { @@ -88,7 +49,10 @@ describe("FilesPlugin delete", () => { const handler = getRouteHandler(plugin, "delete", ""); const res = mockRes(); - mockClient.files.delete.mockResolvedValue(undefined); + const generateKey = vi.spyOn(testCache.current, "generateKey"); + const del = vi.spyOn(testCache.current, "delete"); + + getMock(client, "files.delete").mockResolvedValue(undefined); await handler( mockReq("uploads", { @@ -100,8 +64,8 @@ describe("FilesPlugin delete", () => { expect(res.json).toHaveBeenCalledWith( expect.objectContaining({ success: true }), ); - expect(mockCacheInstance.generateKey).toHaveBeenCalled(); - expect(mockCacheInstance.delete).toHaveBeenCalled(); + expect(generateKey).toHaveBeenCalled(); + expect(del).toHaveBeenCalled(); }); test("delete without path returns 400", async () => { @@ -122,8 +86,12 @@ describe("FilesPlugin delete", () => { const handler = getRouteHandler(plugin, "delete", ""); const res = mockRes(); - mockClient.files.delete.mockRejectedValue( - new MockApiError("Not found", 404), + getMock(client, "files.delete").mockRejectedValue( + createApiError({ + statusCode: 404, + message: "Not found", + errorCode: "ERROR", + }), ); await handler( diff --git a/packages/appkit/src/plugins/files/tests/download-endpoint.test.ts b/packages/appkit/src/plugins/files/tests/download-endpoint.test.ts index e96470e1b..622aa702d 100644 --- a/packages/appkit/src/plugins/files/tests/download-endpoint.test.ts +++ b/packages/appkit/src/plugins/files/tests/download-endpoint.test.ts @@ -1,5 +1,10 @@ -import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, test } from "vitest"; +import { + createMockWorkspaceClient, + getMock, + useTestCache, +} from "../../../testing"; import { FilesPlugin } from "../plugin"; import { getRouteHandler, @@ -11,73 +16,27 @@ import { VOLUMES_CONFIG, } from "./_test-helpers"; -const { mockClient, MockApiError, mockCacheInstance } = vi.hoisted(() => { - const mockFilesApi = { - listDirectoryContents: vi.fn(), - download: vi.fn(), - getMetadata: vi.fn(), - upload: vi.fn(), - createDirectory: vi.fn(), - delete: vi.fn(), - }; - const mockClient = { - files: mockFilesApi, - config: { - host: "https://test.databricks.com", - authenticate: vi.fn(), - }, - }; - class MockApiError extends Error { - statusCode: number; - constructor(message: string, statusCode: number) { - super(message); - this.name = "ApiError"; - this.statusCode = statusCode; - } - } - const mockCacheInstance = { - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (_key: unknown[], fn: (signal?: AbortSignal) => Promise) => - fn(), - ), - generateKey: vi.fn((...args: unknown[]) => JSON.stringify(args)), - }; - return { mockClient, MockApiError, mockCacheInstance }; -}); - -vi.mock("../../../workspace-client", async (importOriginal) => { - const actual = - await importOriginal(); - return { - ...actual, - createWorkspaceClient: (..._args: unknown[]) => mockClient, - ApiError: MockApiError, - }; -}); - -vi.mock("../../../context", async (importOriginal) => { - const actual = await importOriginal(); - return { - ...actual, - getWorkspaceClient: vi.fn(() => mockClient), - isInUserContext: vi.fn(() => true), - }; -}); - -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => mockCacheInstance), - }, -})); +// Boots AppKit's real in-memory cache (no cache-module mock needed). +useTestCache(); describe("FilesPlugin download endpoint Content-Disposition", () => { let serviceContextMock: Awaited>; + let client: ReturnType; + beforeEach(async () => { - serviceContextMock = await setupTestEnv(); + client = createMockWorkspaceClient({ + strict: true, + responses: { + "files.listDirectoryContents": undefined, + "files.download": undefined, + "files.getMetadata": undefined, + "files.upload": undefined, + "files.createDirectory": undefined, + "files.delete": undefined, + }, + }); + serviceContextMock = await setupTestEnv(client); }); afterEach(() => { @@ -89,7 +48,7 @@ describe("FilesPlugin download endpoint Content-Disposition", () => { const handler = getRouteHandler(plugin, "get", "/download"); const res = mockRes(); - mockClient.files.download.mockResolvedValue( + getMock(client, "files.download").mockResolvedValue( makeStreamResponse("file data"), ); @@ -111,7 +70,9 @@ describe("FilesPlugin download endpoint Content-Disposition", () => { const handler = getRouteHandler(plugin, "get", "/download"); const res = mockRes(); - mockClient.files.download.mockResolvedValue(makeStreamResponse("data")); + getMock(client, "files.download").mockResolvedValue( + makeStreamResponse("data"), + ); await handler( mockReq("uploads", { @@ -131,7 +92,9 @@ describe("FilesPlugin download endpoint Content-Disposition", () => { const handler = getRouteHandler(plugin, "get", "/download"); const res = mockRes(); - mockClient.files.download.mockResolvedValue(makeStreamResponse("{}")); + getMock(client, "files.download").mockResolvedValue( + makeStreamResponse("{}"), + ); await handler( mockReq("uploads", { @@ -169,7 +132,7 @@ describe("FilesPlugin download endpoint Content-Disposition", () => { const res = mockRes(); // Response with no contents field (empty file) - mockClient.files.download.mockResolvedValue({}); + getMock(client, "files.download").mockResolvedValue({}); await handler( mockReq("uploads", { diff --git a/packages/appkit/src/plugins/files/tests/error-handling.test.ts b/packages/appkit/src/plugins/files/tests/error-handling.test.ts index f02fb8683..296b6f2b8 100644 --- a/packages/appkit/src/plugins/files/tests/error-handling.test.ts +++ b/packages/appkit/src/plugins/files/tests/error-handling.test.ts @@ -1,6 +1,12 @@ -import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, test } from "vitest"; import { AuthenticationError } from "../../../errors"; +import { + createApiError, + createMockWorkspaceClient, + useTestCache, +} from "../../../testing"; +import { withEnv } from "../../../testing"; import { FilesPlugin } from "../plugin"; import { getRouteHandler, @@ -10,73 +16,27 @@ import { VOLUMES_CONFIG, } from "./_test-helpers"; -const { mockClient, MockApiError, mockCacheInstance } = vi.hoisted(() => { - const mockFilesApi = { - listDirectoryContents: vi.fn(), - download: vi.fn(), - getMetadata: vi.fn(), - upload: vi.fn(), - createDirectory: vi.fn(), - delete: vi.fn(), - }; - const mockClient = { - files: mockFilesApi, - config: { - host: "https://test.databricks.com", - authenticate: vi.fn(), - }, - }; - class MockApiError extends Error { - statusCode: number; - constructor(message: string, statusCode: number) { - super(message); - this.name = "ApiError"; - this.statusCode = statusCode; - } - } - const mockCacheInstance = { - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (_key: unknown[], fn: (signal?: AbortSignal) => Promise) => - fn(), - ), - generateKey: vi.fn((...args: unknown[]) => JSON.stringify(args)), - }; - return { mockClient, MockApiError, mockCacheInstance }; -}); - -vi.mock("../../../workspace-client", async (importOriginal) => { - const actual = - await importOriginal(); - return { - ...actual, - createWorkspaceClient: (..._args: unknown[]) => mockClient, - ApiError: MockApiError, - }; -}); - -vi.mock("../../../context", async (importOriginal) => { - const actual = await importOriginal(); - return { - ...actual, - getWorkspaceClient: vi.fn(() => mockClient), - isInUserContext: vi.fn(() => true), - }; -}); - -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => mockCacheInstance), - }, -})); +// Boots AppKit's real in-memory cache (no cache-module mock needed). +useTestCache(); describe("FilesPlugin error handling", () => { let serviceContextMock: Awaited>; + let client: ReturnType; + beforeEach(async () => { - serviceContextMock = await setupTestEnv(); + client = createMockWorkspaceClient({ + strict: true, + responses: { + "files.listDirectoryContents": undefined, + "files.download": undefined, + "files.getMetadata": undefined, + "files.upload": undefined, + "files.createDirectory": undefined, + "files.delete": undefined, + }, + }); + serviceContextMock = await setupTestEnv(client); }); afterEach(() => { @@ -107,7 +67,11 @@ describe("FilesPlugin error handling", () => { (plugin as any)._handleApiError( res, - new MockApiError("Forbidden", 403), + createApiError({ + statusCode: 403, + message: "Forbidden", + errorCode: "ERROR", + }), "fallback msg", ); @@ -125,7 +89,11 @@ describe("FilesPlugin error handling", () => { (plugin as any)._handleApiError( res, - new MockApiError("Not found", 404), + createApiError({ + statusCode: 404, + message: "Not found", + errorCode: "ERROR", + }), "fallback msg", ); @@ -143,7 +111,11 @@ describe("FilesPlugin error handling", () => { (plugin as any)._handleApiError( res, - new MockApiError("Conflict", 409), + createApiError({ + statusCode: 409, + message: "Conflict", + errorCode: "ERROR", + }), "fallback msg", ); @@ -161,7 +133,11 @@ describe("FilesPlugin error handling", () => { (plugin as any)._handleApiError( res, - new MockApiError("Bad Gateway", 502), + createApiError({ + statusCode: 502, + message: "Bad Gateway", + errorCode: "ERROR", + }), "Operation failed", ); @@ -178,7 +154,11 @@ describe("FilesPlugin error handling", () => { (plugin as any)._handleApiError( res, - new MockApiError("Internal error", 500), + createApiError({ + statusCode: 500, + message: "Internal error", + errorCode: "ERROR", + }), "Fallback", ); @@ -220,38 +200,37 @@ describe("FilesPlugin error handling", () => { }); test("AuthenticationError via route returns generic 401 on OBO volume without token", async () => { - process.env.DATABRICKS_VOLUME_OBO = "/Volumes/catalog/schema/obo"; - const plugin = new FilesPlugin({ - volumes: { - obo: { auth: "on-behalf-of-user", policy: () => true }, + await withEnv( + { + DATABRICKS_VOLUME_OBO: "/Volumes/catalog/schema/obo", + NODE_ENV: "production", }, - }); - const handler = getRouteHandler(plugin, "get", "/list"); - const res = mockRes(); - - const originalEnv = process.env.NODE_ENV; - process.env.NODE_ENV = "production"; - - try { - await handler( - { - params: { volumeKey: "obo" }, - query: {}, - headers: {}, - header: () => undefined, - }, - res, - ); - - expect(res.status).toHaveBeenCalledWith(401); - expect(res.json).toHaveBeenCalledWith({ - error: "Unauthorized", - plugin: "files", - }); - } finally { - process.env.NODE_ENV = originalEnv; - delete process.env.DATABRICKS_VOLUME_OBO; - } + async () => { + const plugin = new FilesPlugin({ + volumes: { + obo: { auth: "on-behalf-of-user", policy: () => true }, + }, + }); + const handler = getRouteHandler(plugin, "get", "/list"); + const res = mockRes(); + + await handler( + { + params: { volumeKey: "obo" }, + query: {}, + headers: {}, + header: () => undefined, + }, + res, + ); + + expect(res.status).toHaveBeenCalledWith(401); + expect(res.json).toHaveBeenCalledWith({ + error: "Unauthorized", + plugin: "files", + }); + }, + ); }); }); diff --git a/packages/appkit/src/plugins/files/tests/mkdir.test.ts b/packages/appkit/src/plugins/files/tests/mkdir.test.ts index 00623bef5..e2ecdee10 100644 --- a/packages/appkit/src/plugins/files/tests/mkdir.test.ts +++ b/packages/appkit/src/plugins/files/tests/mkdir.test.ts @@ -1,5 +1,11 @@ import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; +import { + createApiError, + createMockWorkspaceClient, + getMock, + useTestCache, +} from "../../../testing"; import { FilesPlugin } from "../plugin"; import { getRouteHandler, @@ -10,73 +16,21 @@ import { VOLUMES_CONFIG, } from "./_test-helpers"; -const { mockClient, MockApiError, mockCacheInstance } = vi.hoisted(() => { - const mockFilesApi = { - listDirectoryContents: vi.fn(), - download: vi.fn(), - getMetadata: vi.fn(), - upload: vi.fn(), - createDirectory: vi.fn(), - delete: vi.fn(), - }; - const mockClient = { - files: mockFilesApi, - config: { - host: "https://test.databricks.com", - authenticate: vi.fn(), - }, - }; - class MockApiError extends Error { - statusCode: number; - constructor(message: string, statusCode: number) { - super(message); - this.name = "ApiError"; - this.statusCode = statusCode; - } - } - const mockCacheInstance = { - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (_key: unknown[], fn: (signal?: AbortSignal) => Promise) => - fn(), - ), - generateKey: vi.fn((...args: unknown[]) => JSON.stringify(args)), - }; - return { mockClient, MockApiError, mockCacheInstance }; -}); - -vi.mock("../../../workspace-client", async (importOriginal) => { - const actual = - await importOriginal(); - return { - ...actual, - createWorkspaceClient: (..._args: unknown[]) => mockClient, - ApiError: MockApiError, - }; -}); - -vi.mock("../../../context", async (importOriginal) => { - const actual = await importOriginal(); - return { - ...actual, - getWorkspaceClient: vi.fn(() => mockClient), - isInUserContext: vi.fn(() => true), - }; -}); - -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => mockCacheInstance), - }, -})); +// Real in-memory cache; spy on `testCache.current` to assert invalidation. +const testCache = useTestCache(); describe("FilesPlugin mkdir", () => { let serviceContextMock: Awaited>; + let client: ReturnType; beforeEach(async () => { - serviceContextMock = await setupTestEnv(); + // strict: true keeps the loudness the hand-rolled literal had by accident — + // an undeclared data-plane call throws instead of resolving undefined. + client = createMockWorkspaceClient({ + strict: true, + responses: { "files.createDirectory": undefined }, + }); + serviceContextMock = await setupTestEnv(client); }); afterEach(() => { @@ -88,7 +42,8 @@ describe("FilesPlugin mkdir", () => { const handler = getRouteHandler(plugin, "post", "/mkdir"); const res = mockRes(); - mockClient.files.createDirectory.mockResolvedValue(undefined); + const generateKey = vi.spyOn(testCache.current, "generateKey"); + const del = vi.spyOn(testCache.current, "delete"); await handler( mockReq("uploads", { @@ -100,8 +55,9 @@ describe("FilesPlugin mkdir", () => { expect(res.json).toHaveBeenCalledWith( expect.objectContaining({ success: true }), ); - expect(mockCacheInstance.generateKey).toHaveBeenCalled(); - expect(mockCacheInstance.delete).toHaveBeenCalled(); + expect(getMock(client, "files.createDirectory")).toHaveBeenCalled(); + expect(generateKey).toHaveBeenCalled(); + expect(del).toHaveBeenCalled(); }); test("mkdir without path returns 400", async () => { @@ -122,8 +78,12 @@ describe("FilesPlugin mkdir", () => { const handler = getRouteHandler(plugin, "post", "/mkdir"); const res = mockRes(); - mockClient.files.createDirectory.mockRejectedValue( - new MockApiError("Conflict", 409), + getMock(client, "files.createDirectory").mockRejectedValue( + createApiError({ + statusCode: 409, + message: "Conflict", + errorCode: "ALREADY_EXISTS", + }), ); await handler( diff --git a/packages/appkit/src/plugins/files/tests/path-validation.test.ts b/packages/appkit/src/plugins/files/tests/path-validation.test.ts index 7705b4778..42e21b883 100644 --- a/packages/appkit/src/plugins/files/tests/path-validation.test.ts +++ b/packages/appkit/src/plugins/files/tests/path-validation.test.ts @@ -1,5 +1,10 @@ -import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, test } from "vitest"; +import { + createMockWorkspaceClient, + getMock, + useTestCache, +} from "../../../testing"; import { FilesPlugin } from "../plugin"; import { getRouteHandler, @@ -11,73 +16,27 @@ import { VOLUMES_CONFIG, } from "./_test-helpers"; -const { mockClient, MockApiError, mockCacheInstance } = vi.hoisted(() => { - const mockFilesApi = { - listDirectoryContents: vi.fn(), - download: vi.fn(), - getMetadata: vi.fn(), - upload: vi.fn(), - createDirectory: vi.fn(), - delete: vi.fn(), - }; - const mockClient = { - files: mockFilesApi, - config: { - host: "https://test.databricks.com", - authenticate: vi.fn(), - }, - }; - class MockApiError extends Error { - statusCode: number; - constructor(message: string, statusCode: number) { - super(message); - this.name = "ApiError"; - this.statusCode = statusCode; - } - } - const mockCacheInstance = { - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (_key: unknown[], fn: (signal?: AbortSignal) => Promise) => - fn(), - ), - generateKey: vi.fn((...args: unknown[]) => JSON.stringify(args)), - }; - return { mockClient, MockApiError, mockCacheInstance }; -}); - -vi.mock("../../../workspace-client", async (importOriginal) => { - const actual = - await importOriginal(); - return { - ...actual, - createWorkspaceClient: (..._args: unknown[]) => mockClient, - ApiError: MockApiError, - }; -}); - -vi.mock("../../../context", async (importOriginal) => { - const actual = await importOriginal(); - return { - ...actual, - getWorkspaceClient: vi.fn(() => mockClient), - isInUserContext: vi.fn(() => true), - }; -}); - -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => mockCacheInstance), - }, -})); +// Boots AppKit's real in-memory cache (no cache-module mock needed). +useTestCache(); describe("FilesPlugin path validation", () => { let serviceContextMock: Awaited>; + let client: ReturnType; + beforeEach(async () => { - serviceContextMock = await setupTestEnv(); + client = createMockWorkspaceClient({ + strict: true, + responses: { + "files.listDirectoryContents": undefined, + "files.download": undefined, + "files.getMetadata": undefined, + "files.upload": undefined, + "files.createDirectory": undefined, + "files.delete": undefined, + }, + }); + serviceContextMock = await setupTestEnv(client); }); afterEach(() => { @@ -87,12 +46,14 @@ describe("FilesPlugin path validation", () => { // Defends against regressions where the handler calls the SDK and *also* // returns 400 — the status assertion alone wouldn't catch that. function expectNoSdkCall() { - expect(mockClient.files.download).not.toHaveBeenCalled(); - expect(mockClient.files.upload).not.toHaveBeenCalled(); - expect(mockClient.files.delete).not.toHaveBeenCalled(); - expect(mockClient.files.createDirectory).not.toHaveBeenCalled(); - expect(mockClient.files.getMetadata).not.toHaveBeenCalled(); - expect(mockClient.files.listDirectoryContents).not.toHaveBeenCalled(); + expect(getMock(client, "files.download")).not.toHaveBeenCalled(); + expect(getMock(client, "files.upload")).not.toHaveBeenCalled(); + expect(getMock(client, "files.delete")).not.toHaveBeenCalled(); + expect(getMock(client, "files.createDirectory")).not.toHaveBeenCalled(); + expect(getMock(client, "files.getMetadata")).not.toHaveBeenCalled(); + expect( + getMock(client, "files.listDirectoryContents"), + ).not.toHaveBeenCalled(); } test("path with null bytes returns 400", async () => { diff --git a/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts b/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts index 0134c09e6..2fd955297 100644 --- a/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts +++ b/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts @@ -17,11 +17,12 @@ import { import { ServiceContext } from "../../../context/service-context"; import { createApp } from "../../../core"; +import { createApiError } from "../../../testing"; import { server as serverPlugin } from "../../server"; import { files } from "../index"; import { streamFromString } from "./utils"; -const { mockFilesApi, mockSdkClient, MockApiError } = vi.hoisted(() => { +const { mockFilesApi, mockSdkClient } = vi.hoisted(() => { const mockFilesApi = { listDirectoryContents: vi.fn(), download: vi.fn(), @@ -42,25 +43,7 @@ const { mockFilesApi, mockSdkClient, MockApiError } = vi.hoisted(() => { }, }; - class MockApiError extends Error { - statusCode: number; - constructor(message: string, statusCode: number) { - super(message); - this.name = "ApiError"; - this.statusCode = statusCode; - } - } - - return { mockFilesApi, mockSdkClient, MockApiError }; -}); - -vi.mock("../../../workspace-client", async (importOriginal) => { - const actual = - await importOriginal(); - return { - ...actual, - ApiError: MockApiError, - }; + return { mockFilesApi, mockSdkClient }; }); const MOCK_AUTH_HEADERS = { @@ -246,7 +229,11 @@ describe("Files Plugin Integration", () => { test(`GET /api/files/${VOL}/exists returns { exists: false } on 404`, async () => { mockFilesApi.getMetadata.mockRejectedValue( - new MockApiError("Not found", 404), + createApiError({ + statusCode: 404, + message: "Not found", + errorCode: "ERROR", + }), ); const response = await fetch( @@ -677,7 +664,11 @@ describe("Files Plugin Integration", () => { test("ApiError 404 preserves upstream status code", async () => { mockFilesApi.getMetadata.mockRejectedValue( - new MockApiError("Not found", 404), + createApiError({ + statusCode: 404, + message: "Not found", + errorCode: "ERROR", + }), ); const response = await fetch( @@ -696,7 +687,11 @@ describe("Files Plugin Integration", () => { test("ApiError 409 preserves upstream status code", async () => { mockFilesApi.getMetadata.mockRejectedValue( - new MockApiError("Conflict", 409), + createApiError({ + statusCode: 409, + message: "Conflict", + errorCode: "ERROR", + }), ); const response = await fetch( diff --git a/packages/appkit/src/plugins/files/tests/plugin.test.ts b/packages/appkit/src/plugins/files/tests/plugin.test.ts index a9612d47e..a6736d307 100644 --- a/packages/appkit/src/plugins/files/tests/plugin.test.ts +++ b/packages/appkit/src/plugins/files/tests/plugin.test.ts @@ -7,6 +7,7 @@ import { ServiceContext } from "../../../context/service-context"; import { createApp } from "../../../core"; import { AuthenticationError } from "../../../errors"; import { ResourceType } from "../../../registry"; +import { useTestCache, withEnv } from "../../../testing"; import { FILES_DOWNLOAD_DEFAULTS, FILES_READ_DEFAULTS, @@ -15,7 +16,7 @@ import { import { FilesPlugin, files } from "../plugin"; import { PolicyDeniedError, policy } from "../policy"; -const { mockClient, MockApiError, mockCacheInstance } = vi.hoisted(() => { +const { mockClient, MockApiError } = await vi.hoisted(async () => { const mockFilesApi = { listDirectoryContents: vi.fn(), download: vi.fn(), @@ -42,18 +43,7 @@ const { mockClient, MockApiError, mockCacheInstance } = vi.hoisted(() => { } } - const mockCacheInstance = { - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (_key: unknown[], fn: (signal?: AbortSignal) => Promise) => - fn(), - ), - generateKey: vi.fn(), - }; - - return { mockFilesApi, mockClient, MockApiError, mockCacheInstance }; + return { mockFilesApi, mockClient, MockApiError }; }); vi.mock("../../../workspace-client", async (importOriginal) => { @@ -75,13 +65,6 @@ vi.mock("../../../context", async (importOriginal) => { }; }); -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => mockCacheInstance), - getInstance: vi.fn(async () => mockCacheInstance), - }, -})); - const VOLUMES_CONFIG = { volumes: { uploads: { maxUploadSize: 100_000_000, policy: policy.allowAll() }, @@ -89,6 +72,10 @@ const VOLUMES_CONFIG = { }, }; +// Boots AppKit's real in-memory cache; spy on `testCache.current` to assert +// cache behaviour. No mock of the internal cache module. +const testCache = useTestCache(); + describe("FilesPlugin", () => { let serviceContextMock: Awaited>; @@ -137,33 +124,27 @@ describe("FilesPlugin", () => { }); test("skips bare DATABRICKS_VOLUME_ prefix (no suffix)", () => { - process.env.DATABRICKS_VOLUME_ = "/Volumes/bare"; - try { + withEnv({ DATABRICKS_VOLUME_: "/Volumes/bare" }, () => { const volumes = FilesPlugin.discoverVolumes({}); expect(Object.keys(volumes)).not.toContain(""); - } finally { - delete process.env.DATABRICKS_VOLUME_; - } + }); }); test("skips empty env var values", () => { - process.env.DATABRICKS_VOLUME_EMPTY = ""; - try { + withEnv({ DATABRICKS_VOLUME_EMPTY: "" }, () => { const volumes = FilesPlugin.discoverVolumes({}); expect(volumes).not.toHaveProperty("empty"); - } finally { - delete process.env.DATABRICKS_VOLUME_EMPTY; - } + }); }); test("lowercases env var suffix", () => { - process.env.DATABRICKS_VOLUME_MY_DATA = "/Volumes/catalog/schema/data"; - try { - const volumes = FilesPlugin.discoverVolumes({}); - expect(volumes).toHaveProperty("my_data"); - } finally { - delete process.env.DATABRICKS_VOLUME_MY_DATA; - } + withEnv( + { DATABRICKS_VOLUME_MY_DATA: "/Volumes/catalog/schema/data" }, + () => { + const volumes = FilesPlugin.discoverVolumes({}); + expect(volumes).toHaveProperty("my_data"); + }, + ); }); test("returns only explicit volumes when no env vars match", () => { @@ -2448,6 +2429,8 @@ describe("FilesPlugin", () => { }, ); + const getOrExecute = vi.spyOn(testCache.current, "getOrExecute"); + // Alice's request. await handler( mockReq("obo_vol", { @@ -2468,7 +2451,7 @@ describe("FilesPlugin", () => { // Cache is disabled on OBO: `getOrExecute` is bypassed. The SDK // must execute on every request — no cross-user staleness possible. - expect(mockCacheInstance.getOrExecute).not.toHaveBeenCalled(); + expect(getOrExecute).not.toHaveBeenCalled(); expect(mockClient.files.listDirectoryContents).toHaveBeenCalledTimes(2); }); @@ -2494,6 +2477,8 @@ describe("FilesPlugin", () => { }, ); + const getOrExecute = vi.spyOn(testCache.current, "getOrExecute"); + // SP volume request — must consult the cache (cache enabled). await listHandler( mockReq("uploads", { @@ -2512,7 +2497,7 @@ describe("FilesPlugin", () => { mockRes(), ); - const calls = mockCacheInstance.getOrExecute.mock.calls; + const calls = getOrExecute.mock.calls; // Exactly one cache consultation — the SP volume's. The OBO request // bypassed the cache entirely. expect(calls).toHaveLength(1); @@ -2994,18 +2979,9 @@ describe("FilesPlugin", () => { mockClient.files.createDirectory.mockResolvedValue(undefined); - // Track which (parts, userKey) pairs go through generateKey so we - // can match the invalidation segment exactly. - const generateKeyCalls: Array<{ - parts: (string | number | object)[]; - userKey: string; - }> = []; - mockCacheInstance.generateKey.mockImplementation( - (parts: (string | number | object)[], userKey: string) => { - generateKeyCalls.push({ parts, userKey }); - return "stub-key"; - }, - ); + // Track which (parts, userKey) pairs go through the real generateKey + // so we can match the invalidation segment exactly. + const generateKey = vi.spyOn(testCache.current, "generateKey"); await mkdirHandler( mockReq("sp_vol", {}, { body: { path: "/Volumes/c/s/sp/foo/bar" } }), @@ -3013,9 +2989,11 @@ describe("FilesPlugin", () => { ); // Exactly one list-cache invalidation key was constructed. - const listInvalidations = generateKeyCalls.filter( - (c) => Array.isArray(c.parts) && c.parts[0] === "files:sp_vol:list", - ); + const listInvalidations = generateKey.mock.calls + .map((c) => ({ parts: c[0], userKey: c[1] })) + .filter( + (c) => Array.isArray(c.parts) && c.parts[0] === "files:sp_vol:list", + ); expect(listInvalidations).toHaveLength(1); // The path-segment is the PARENT directory (resolved), not the @@ -3067,25 +3045,19 @@ describe("FilesPlugin", () => { mockClient.files.createDirectory.mockResolvedValue(undefined); - const generateKeyCalls: Array<{ - parts: (string | number | object)[]; - userKey: string; - }> = []; - mockCacheInstance.generateKey.mockImplementation( - (parts: (string | number | object)[], userKey: string) => { - generateKeyCalls.push({ parts, userKey }); - return "stub-key"; - }, - ); + const generateKey = vi.spyOn(testCache.current, "generateKey"); await mkdirHandler( mockReq("uploads", {}, { body: { path: writePath } }), mockRes(), ); - const listInvalidations = generateKeyCalls.filter( - (c) => Array.isArray(c.parts) && c.parts[0] === "files:uploads:list", - ); + const listInvalidations = generateKey.mock.calls + .map((c) => ({ parts: c[0], userKey: c[1] })) + .filter( + (c) => + Array.isArray(c.parts) && c.parts[0] === "files:uploads:list", + ); const segments = listInvalidations.map((c) => c.parts[1]); expect(segments).toEqual( expect.arrayContaining([ @@ -3144,7 +3116,6 @@ describe("FilesPlugin", () => { const mkdirHandler = getRouteHandler(plugin, "post", "/mkdir"); mockClient.files.createDirectory.mockResolvedValue(undefined); - mockCacheInstance.generateKey.mockReturnValue("stub-key"); // Deferred promise that gates the cache delete. The handler must // await this before writing the success response. @@ -3152,9 +3123,9 @@ describe("FilesPlugin", () => { const deletePending = new Promise((resolve) => { releaseDelete = resolve; }); - mockCacheInstance.delete.mockImplementation( - async () => await deletePending, - ); + const del = vi + .spyOn(testCache.current, "delete") + .mockImplementation(async () => await deletePending); const res = mockRes(); @@ -3172,15 +3143,12 @@ describe("FilesPlugin", () => { // Use setImmediate to also drain macrotask queue items (telemetry/ // timeout interceptors may use setTimeout under the hood). const deadline = Date.now() + 1000; - while ( - mockCacheInstance.delete.mock.calls.length === 0 && - Date.now() < deadline - ) { + while (del.mock.calls.length === 0 && Date.now() < deadline) { await new Promise((resolve) => setImmediate(resolve)); } expect(mockClient.files.createDirectory).toHaveBeenCalledTimes(1); - expect(mockCacheInstance.delete).toHaveBeenCalledTimes(1); + expect(del).toHaveBeenCalledTimes(1); // Critical assertion: drain plenty of microtasks AND macrotasks // while `cache.delete` is still parked on the deferred. If the diff --git a/packages/appkit/src/plugins/files/tests/raw-endpoint.test.ts b/packages/appkit/src/plugins/files/tests/raw-endpoint.test.ts index 5614e8b37..9a2e1587a 100644 --- a/packages/appkit/src/plugins/files/tests/raw-endpoint.test.ts +++ b/packages/appkit/src/plugins/files/tests/raw-endpoint.test.ts @@ -1,5 +1,10 @@ -import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, test } from "vitest"; +import { + createMockWorkspaceClient, + getMock, + useTestCache, +} from "../../../testing"; import { FilesPlugin } from "../plugin"; import { getRouteHandler, @@ -11,73 +16,27 @@ import { VOLUMES_CONFIG, } from "./_test-helpers"; -const { mockClient, MockApiError, mockCacheInstance } = vi.hoisted(() => { - const mockFilesApi = { - listDirectoryContents: vi.fn(), - download: vi.fn(), - getMetadata: vi.fn(), - upload: vi.fn(), - createDirectory: vi.fn(), - delete: vi.fn(), - }; - const mockClient = { - files: mockFilesApi, - config: { - host: "https://test.databricks.com", - authenticate: vi.fn(), - }, - }; - class MockApiError extends Error { - statusCode: number; - constructor(message: string, statusCode: number) { - super(message); - this.name = "ApiError"; - this.statusCode = statusCode; - } - } - const mockCacheInstance = { - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (_key: unknown[], fn: (signal?: AbortSignal) => Promise) => - fn(), - ), - generateKey: vi.fn((...args: unknown[]) => JSON.stringify(args)), - }; - return { mockClient, MockApiError, mockCacheInstance }; -}); - -vi.mock("../../../workspace-client", async (importOriginal) => { - const actual = - await importOriginal(); - return { - ...actual, - createWorkspaceClient: (..._args: unknown[]) => mockClient, - ApiError: MockApiError, - }; -}); - -vi.mock("../../../context", async (importOriginal) => { - const actual = await importOriginal(); - return { - ...actual, - getWorkspaceClient: vi.fn(() => mockClient), - isInUserContext: vi.fn(() => true), - }; -}); - -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => mockCacheInstance), - }, -})); +// Boots AppKit's real in-memory cache (no cache-module mock needed). +useTestCache(); describe("FilesPlugin raw endpoint security headers", () => { let serviceContextMock: Awaited>; + let client: ReturnType; + beforeEach(async () => { - serviceContextMock = await setupTestEnv(); + client = createMockWorkspaceClient({ + strict: true, + responses: { + "files.listDirectoryContents": undefined, + "files.download": undefined, + "files.getMetadata": undefined, + "files.upload": undefined, + "files.createDirectory": undefined, + "files.delete": undefined, + }, + }); + serviceContextMock = await setupTestEnv(client); }); afterEach(() => { @@ -89,7 +48,9 @@ describe("FilesPlugin raw endpoint security headers", () => { const handler = getRouteHandler(plugin, "get", "/raw"); const res = mockRes(); - mockClient.files.download.mockResolvedValue(makeStreamResponse("data")); + getMock(client, "files.download").mockResolvedValue( + makeStreamResponse("data"), + ); await handler( mockReq("uploads", { @@ -109,7 +70,9 @@ describe("FilesPlugin raw endpoint security headers", () => { const handler = getRouteHandler(plugin, "get", "/raw"); const res = mockRes(); - mockClient.files.download.mockResolvedValue(makeStreamResponse("PNG data")); + getMock(client, "files.download").mockResolvedValue( + makeStreamResponse("PNG data"), + ); await handler( mockReq("uploads", { @@ -135,7 +98,7 @@ describe("FilesPlugin raw endpoint security headers", () => { const handler = getRouteHandler(plugin, "get", "/raw"); const res = mockRes(); - mockClient.files.download.mockResolvedValue( + getMock(client, "files.download").mockResolvedValue( makeStreamResponse(""), ); @@ -162,7 +125,7 @@ describe("FilesPlugin raw endpoint security headers", () => { const handler = getRouteHandler(plugin, "get", "/raw"); const res = mockRes(); - mockClient.files.download.mockResolvedValue( + getMock(client, "files.download").mockResolvedValue( makeStreamResponse(""), ); @@ -184,7 +147,9 @@ describe("FilesPlugin raw endpoint security headers", () => { const handler = getRouteHandler(plugin, "get", "/raw"); const res = mockRes(); - mockClient.files.download.mockResolvedValue(makeStreamResponse("content")); + getMock(client, "files.download").mockResolvedValue( + makeStreamResponse("content"), + ); await handler( mockReq("uploads", { diff --git a/packages/appkit/src/plugins/files/tests/shutdown.test.ts b/packages/appkit/src/plugins/files/tests/shutdown.test.ts index 239d92cff..31edbb8d0 100644 --- a/packages/appkit/src/plugins/files/tests/shutdown.test.ts +++ b/packages/appkit/src/plugins/files/tests/shutdown.test.ts @@ -1,75 +1,30 @@ import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; +import { createMockWorkspaceClient, useTestCache } from "../../../testing"; import { FilesPlugin } from "../plugin"; import { setupTestEnv, teardownTestEnv, VOLUMES_CONFIG } from "./_test-helpers"; -const { mockClient, MockApiError, mockCacheInstance } = vi.hoisted(() => { - const mockFilesApi = { - listDirectoryContents: vi.fn(), - download: vi.fn(), - getMetadata: vi.fn(), - upload: vi.fn(), - createDirectory: vi.fn(), - delete: vi.fn(), - }; - const mockClient = { - files: mockFilesApi, - config: { - host: "https://test.databricks.com", - authenticate: vi.fn(), - }, - }; - class MockApiError extends Error { - statusCode: number; - constructor(message: string, statusCode: number) { - super(message); - this.name = "ApiError"; - this.statusCode = statusCode; - } - } - const mockCacheInstance = { - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (_key: unknown[], fn: (signal?: AbortSignal) => Promise) => - fn(), - ), - generateKey: vi.fn((...args: unknown[]) => JSON.stringify(args)), - }; - return { mockClient, MockApiError, mockCacheInstance }; -}); - -vi.mock("../../../workspace-client", async (importOriginal) => { - const actual = - await importOriginal(); - return { - ...actual, - createWorkspaceClient: (..._args: unknown[]) => mockClient, - ApiError: MockApiError, - }; -}); - -vi.mock("../../../context", async (importOriginal) => { - const actual = await importOriginal(); - return { - ...actual, - getWorkspaceClient: vi.fn(() => mockClient), - isInUserContext: vi.fn(() => true), - }; -}); - -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => mockCacheInstance), - }, -})); +// Boots AppKit's real in-memory cache (no cache-module mock needed). +useTestCache(); describe("FilesPlugin shutdown and trackWrite", () => { let serviceContextMock: Awaited>; + let client: ReturnType; + beforeEach(async () => { - serviceContextMock = await setupTestEnv(); + client = createMockWorkspaceClient({ + strict: true, + responses: { + "files.listDirectoryContents": undefined, + "files.download": undefined, + "files.getMetadata": undefined, + "files.upload": undefined, + "files.createDirectory": undefined, + "files.delete": undefined, + }, + }); + serviceContextMock = await setupTestEnv(client); vi.useFakeTimers(); }); diff --git a/packages/appkit/src/plugins/files/tests/upload.test.ts b/packages/appkit/src/plugins/files/tests/upload.test.ts index 0e893cd59..9aa0c6357 100644 --- a/packages/appkit/src/plugins/files/tests/upload.test.ts +++ b/packages/appkit/src/plugins/files/tests/upload.test.ts @@ -1,5 +1,6 @@ import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; +import { createMockWorkspaceClient, useTestCache } from "../../../testing"; import { FilesPlugin } from "../plugin"; import { policy } from "../policy"; import { @@ -11,73 +12,27 @@ import { VOLUMES_CONFIG, } from "./_test-helpers"; -const { mockClient, MockApiError, mockCacheInstance } = vi.hoisted(() => { - const mockFilesApi = { - listDirectoryContents: vi.fn(), - download: vi.fn(), - getMetadata: vi.fn(), - upload: vi.fn(), - createDirectory: vi.fn(), - delete: vi.fn(), - }; - const mockClient = { - files: mockFilesApi, - config: { - host: "https://test.databricks.com", - authenticate: vi.fn(), - }, - }; - class MockApiError extends Error { - statusCode: number; - constructor(message: string, statusCode: number) { - super(message); - this.name = "ApiError"; - this.statusCode = statusCode; - } - } - const mockCacheInstance = { - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (_key: unknown[], fn: (signal?: AbortSignal) => Promise) => - fn(), - ), - generateKey: vi.fn((...args: unknown[]) => JSON.stringify(args)), - }; - return { mockClient, MockApiError, mockCacheInstance }; -}); - -vi.mock("../../../workspace-client", async (importOriginal) => { - const actual = - await importOriginal(); - return { - ...actual, - createWorkspaceClient: (..._args: unknown[]) => mockClient, - ApiError: MockApiError, - }; -}); - -vi.mock("../../../context", async (importOriginal) => { - const actual = await importOriginal(); - return { - ...actual, - getWorkspaceClient: vi.fn(() => mockClient), - isInUserContext: vi.fn(() => true), - }; -}); - -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => mockCacheInstance), - }, -})); +// Real in-memory cache; spy on `testCache.current` to assert invalidation. +const testCache = useTestCache(); describe("FilesPlugin upload", () => { let serviceContextMock: Awaited>; + let client: ReturnType; + beforeEach(async () => { - serviceContextMock = await setupTestEnv(); + client = createMockWorkspaceClient({ + strict: true, + responses: { + "files.listDirectoryContents": undefined, + "files.download": undefined, + "files.getMetadata": undefined, + "files.upload": undefined, + "files.createDirectory": undefined, + "files.delete": undefined, + }, + }); + serviceContextMock = await setupTestEnv(client); }); afterEach(() => { @@ -202,6 +157,9 @@ describe("FilesPlugin upload", () => { const handler = getRouteHandler(plugin, "post", "/upload"); const res = mockRes(); + const generateKey = vi.spyOn(testCache.current, "generateKey"); + const del = vi.spyOn(testCache.current, "delete"); + const req = mockUploadReq("uploads", [Buffer.from("file content")], { query: { path: "/Volumes/catalog/schema/uploads/dir/file.txt" }, }); @@ -223,8 +181,8 @@ describe("FilesPlugin upload", () => { expect(res.json).toHaveBeenCalledWith( expect.objectContaining({ success: true }), ); - expect(mockCacheInstance.generateKey).toHaveBeenCalled(); - expect(mockCacheInstance.delete).toHaveBeenCalled(); + expect(generateKey).toHaveBeenCalled(); + expect(del).toHaveBeenCalled(); }); }); }); diff --git a/packages/appkit/src/plugins/files/tests/volume-config.test.ts b/packages/appkit/src/plugins/files/tests/volume-config.test.ts index 121bdbe39..8ddba4b88 100644 --- a/packages/appkit/src/plugins/files/tests/volume-config.test.ts +++ b/packages/appkit/src/plugins/files/tests/volume-config.test.ts @@ -1,75 +1,31 @@ -import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, test } from "vitest"; +import { createMockWorkspaceClient, useTestCache } from "../../../testing"; +import { withEnv } from "../../../testing"; import { FilesPlugin } from "../plugin"; import { setupTestEnv, teardownTestEnv, VOLUMES_CONFIG } from "./_test-helpers"; -const { mockClient, MockApiError, mockCacheInstance } = vi.hoisted(() => { - const mockFilesApi = { - listDirectoryContents: vi.fn(), - download: vi.fn(), - getMetadata: vi.fn(), - upload: vi.fn(), - createDirectory: vi.fn(), - delete: vi.fn(), - }; - const mockClient = { - files: mockFilesApi, - config: { - host: "https://test.databricks.com", - authenticate: vi.fn(), - }, - }; - class MockApiError extends Error { - statusCode: number; - constructor(message: string, statusCode: number) { - super(message); - this.name = "ApiError"; - this.statusCode = statusCode; - } - } - const mockCacheInstance = { - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (_key: unknown[], fn: (signal?: AbortSignal) => Promise) => - fn(), - ), - generateKey: vi.fn((...args: unknown[]) => JSON.stringify(args)), - }; - return { mockClient, MockApiError, mockCacheInstance }; -}); - -vi.mock("../../../workspace-client", async (importOriginal) => { - const actual = - await importOriginal(); - return { - ...actual, - createWorkspaceClient: (..._args: unknown[]) => mockClient, - ApiError: MockApiError, - }; -}); - -vi.mock("../../../context", async (importOriginal) => { - const actual = await importOriginal(); - return { - ...actual, - getWorkspaceClient: vi.fn(() => mockClient), - isInUserContext: vi.fn(() => true), - }; -}); - -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => mockCacheInstance), - }, -})); +// Boots AppKit's real in-memory cache (no cache-module mock needed). +useTestCache(); describe("FilesPlugin volume config surface", () => { let serviceContextMock: Awaited>; + let client: ReturnType; + beforeEach(async () => { - serviceContextMock = await setupTestEnv(); + client = createMockWorkspaceClient({ + strict: true, + responses: { + "files.listDirectoryContents": undefined, + "files.download": undefined, + "files.getMetadata": undefined, + "files.upload": undefined, + "files.createDirectory": undefined, + "files.delete": undefined, + }, + }); + serviceContextMock = await setupTestEnv(client); }); afterEach(() => { @@ -94,14 +50,13 @@ describe("FilesPlugin volume config surface", () => { }); test("discovered volumes get empty config objects", () => { - process.env.DATABRICKS_VOLUME_DATA = "/Volumes/catalog/schema/data"; - - try { - const volumes = FilesPlugin.discoverVolumes({}); - expect(volumes.data).toEqual({}); - } finally { - delete process.env.DATABRICKS_VOLUME_DATA; - } + withEnv( + { DATABRICKS_VOLUME_DATA: "/Volumes/catalog/schema/data" }, + () => { + const volumes = FilesPlugin.discoverVolumes({}); + expect(volumes.data).toEqual({}); + }, + ); }); test("explicit volumes without env vars still appear", () => { @@ -119,20 +74,19 @@ describe("FilesPlugin volume config surface", () => { }); test("env var volume is not added when explicit config has the same key", () => { - process.env.DATABRICKS_VOLUME_SPECIAL = "/Volumes/catalog/schema/special"; - - try { - const volumes = FilesPlugin.discoverVolumes({ - volumes: { - special: { maxUploadSize: 500 }, - }, - }); - - // Explicit wins; should not be overwritten with {} - expect(volumes.special).toEqual({ maxUploadSize: 500 }); - } finally { - delete process.env.DATABRICKS_VOLUME_SPECIAL; - } + withEnv( + { DATABRICKS_VOLUME_SPECIAL: "/Volumes/catalog/schema/special" }, + () => { + const volumes = FilesPlugin.discoverVolumes({ + volumes: { + special: { maxUploadSize: 500 }, + }, + }); + + // Explicit wins; should not be overwritten with {} + expect(volumes.special).toEqual({ maxUploadSize: 500 }); + }, + ); }); }); diff --git a/packages/appkit/src/plugins/genie/tests/genie.test.ts b/packages/appkit/src/plugins/genie/tests/genie.test.ts index 2ada2ef3d..5cd03a348 100644 --- a/packages/appkit/src/plugins/genie/tests/genie.test.ts +++ b/packages/appkit/src/plugins/genie/tests/genie.test.ts @@ -11,36 +11,12 @@ import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; import { genieConnectorDefaults } from "../../../connectors/genie/defaults"; import { ServiceContext } from "../../../context/service-context"; import { Plugin } from "../../../plugin"; +import { useTestCache } from "../../../testing/test-cache"; import { GeniePlugin, genie } from "../genie"; import type { IGenieConfig } from "../types"; -// Mock CacheManager singleton -const { mockCacheInstance } = vi.hoisted(() => { - const instance = { - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi - .fn() - .mockImplementation( - async ( - _key: unknown[], - fn: (signal?: AbortSignal) => Promise, - ) => { - return await fn(); - }, - ), - generateKey: vi.fn((...args: unknown[]) => JSON.stringify(args)), - }; - - return { mockCacheInstance: instance }; -}); - -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => mockCacheInstance), - }, -})); +// Boots AppKit's real in-memory cache (no cache-module mock needed). +useTestCache(); function createMockGenieService() { const getMessageAttachmentQueryResult = vi.fn(); diff --git a/packages/appkit/src/plugins/jobs/tests/plugin.test.ts b/packages/appkit/src/plugins/jobs/tests/plugin.test.ts index 8933d9eed..84b44ed0c 100644 --- a/packages/appkit/src/plugins/jobs/tests/plugin.test.ts +++ b/packages/appkit/src/plugins/jobs/tests/plugin.test.ts @@ -4,6 +4,7 @@ import { z } from "zod"; import { ServiceContext } from "../../../context/service-context"; import { ResourceType } from "../../../registry"; +import { createApiError, useTestCache, withEnv } from "../../../testing"; import { JOBS_READ_DEFAULTS, JOBS_STREAM_DEFAULTS, @@ -12,72 +13,42 @@ import { import { mapParams } from "../params"; import { JobsPlugin, jobs } from "../plugin"; -const { mockClient, jobsApi, mockCacheInstance } = await vi.hoisted( - async () => { - // The testing kit's fake, not a hand-rolled literal: the seven jobs methods, - // `config.host` as a real string, and `config.authenticate` all come for free, - // and any *other* service this plugin grows into resolves instead of throwing. - // Imported inside the hoisted factory because the factory runs before the - // file's own imports are evaluated. - const { createMockWorkspaceClient, getMock } = - await import("../../../testing/mock-workspace-client"); - - const mockClient = createMockWorkspaceClient(); - - // Facade accessors are typed against the legacy SDK, so `.mockResolvedValue` - // on them would not typecheck. `getMock` is the typed handle; it mints - // idempotently, so these are the very functions the plugin will call. - const jobsApi = { - runNow: getMock(mockClient, "jobs.runNow"), - submit: getMock(mockClient, "jobs.submit"), - getRun: getMock(mockClient, "jobs.getRun"), - getRunOutput: getMock(mockClient, "jobs.getRunOutput"), - cancelRun: getMock(mockClient, "jobs.cancelRun"), - listRuns: getMock(mockClient, "jobs.listRuns"), - get: getMock(mockClient, "jobs.get"), - }; - - const mockCacheInstance = { - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async ( - _key: unknown[], - fn: (signal?: AbortSignal) => Promise, - ) => fn(), - ), - generateKey: vi.fn(), - }; +const { mockClient, jobsApi } = await vi.hoisted(async () => { + // The testing kit's fake, not a hand-rolled literal: the seven jobs methods, + // `config.host` as a real string, and `config.authenticate` all come for free, + // and any *other* service this plugin grows into resolves instead of throwing. + // Imported inside the hoisted factory because the factory runs before the + // file's own imports are evaluated. + const { createMockWorkspaceClient, getMock } = + await import("../../../testing/mock-workspace-client"); + + const mockClient = createMockWorkspaceClient(); + + // Facade accessors are typed against the legacy SDK, so `.mockResolvedValue` + // on them would not typecheck. `getMock` is the typed handle; it mints + // idempotently, so these are the very functions the plugin will call. + const jobsApi = { + runNow: getMock(mockClient, "jobs.runNow"), + submit: getMock(mockClient, "jobs.submit"), + getRun: getMock(mockClient, "jobs.getRun"), + getRunOutput: getMock(mockClient, "jobs.getRunOutput"), + cancelRun: getMock(mockClient, "jobs.cancelRun"), + listRuns: getMock(mockClient, "jobs.listRuns"), + get: getMock(mockClient, "jobs.get"), + }; - return { mockClient, jobsApi, mockCacheInstance }; - }, -); + return { mockClient, jobsApi }; +}); vi.mock("../../../workspace-client", async (importOriginal) => { const actual = await importOriginal(); - return { - ...actual, - createWorkspaceClient: () => mockClient, - Context: vi.fn(), - }; -}); - -vi.mock("../../../context", async (importOriginal) => { - const actual = await importOriginal(); - return { - ...actual, - getWorkspaceClient: vi.fn(() => mockClient), - isInUserContext: vi.fn(() => true), - }; + // Only `Context` — the client itself is injected through ServiceContext. + return { ...actual, Context: vi.fn() }; }); -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => mockCacheInstance), - }, -})); +// Boots AppKit's real in-memory cache (no cache-module mock needed). +useTestCache(); describe("JobsPlugin", () => { let serviceContextMock: Awaited>; @@ -86,7 +57,10 @@ describe("JobsPlugin", () => { vi.clearAllMocks(); setupDatabricksEnv(); ServiceContext.reset(); - serviceContextMock = await mockServiceContext(); + serviceContextMock = await mockServiceContext({ + serviceDatabricksClient: mockClient, + userDatabricksClient: mockClient, + }); }); afterEach(() => { @@ -163,33 +137,24 @@ describe("JobsPlugin", () => { }); test("skips bare DATABRICKS_JOB_ prefix (no suffix)", () => { - process.env.DATABRICKS_JOB_ = "999"; - try { + withEnv({ DATABRICKS_JOB_: "999" }, () => { const jobs = JobsPlugin.discoverJobs({}); expect(Object.keys(jobs)).not.toContain(""); - } finally { - delete process.env.DATABRICKS_JOB_; - } + }); }); test("skips empty env var values", () => { - process.env.DATABRICKS_JOB_EMPTY = ""; - try { + withEnv({ DATABRICKS_JOB_EMPTY: "" }, () => { const jobs = JobsPlugin.discoverJobs({}); expect(jobs).not.toHaveProperty("empty"); - } finally { - delete process.env.DATABRICKS_JOB_EMPTY; - } + }); }); test("lowercases env var suffix", () => { - process.env.DATABRICKS_JOB_MY_PIPELINE = "111"; - try { + withEnv({ DATABRICKS_JOB_MY_PIPELINE: "111" }, () => { const jobs = JobsPlugin.discoverJobs({}); expect(jobs).toHaveProperty("my_pipeline"); - } finally { - delete process.env.DATABRICKS_JOB_MY_PIPELINE; - } + }); }); test("returns only explicit jobs when no env vars match", () => { @@ -597,9 +562,15 @@ describe("JobsPlugin", () => { test("error result preserves upstream HTTP status code", async () => { process.env.DATABRICKS_JOB_ETL = "123"; - const error = new Error("Detailed internal failure: db connection reset"); - (error as any).statusCode = 403; - jobsApi.getRun.mockRejectedValue(error); + // A genuine ApiError (as the SDK throws): the real cache preserves an + // ApiError's status but wraps a plain Error into a 500. + jobsApi.getRun.mockRejectedValue( + createApiError({ + statusCode: 403, + message: "Detailed internal failure: db connection reset", + errorCode: "PERMISSION_DENIED", + }), + ); const plugin = new JobsPlugin({}); const handle = plugin.exports()("etl"); @@ -934,7 +905,10 @@ describe("injectRoutes", () => { vi.clearAllMocks(); setupDatabricksEnv(); ServiceContext.reset(); - serviceContextMock = await mockServiceContext(); + serviceContextMock = await mockServiceContext({ + serviceDatabricksClient: mockClient, + userDatabricksClient: mockClient, + }); }); afterEach(() => { @@ -1852,10 +1826,12 @@ describe("injectRoutes", () => { test("GET /:jobKey/runs returns upstream status on failure", async () => { process.env.DATABRICKS_JOB_ETL = "123"; - const error = new Error("Unauthorized"); - (error as any).statusCode = 401; jobsApi.listRuns.mockImplementation(() => { - throw error; + throw createApiError({ + statusCode: 401, + message: "Unauthorized", + errorCode: "UNAUTHENTICATED", + }); }); const plugin = new JobsPlugin({}); diff --git a/packages/appkit/src/plugins/lakebase/tests/lakebase-agent-tool.test.ts b/packages/appkit/src/plugins/lakebase/tests/lakebase-agent-tool.test.ts index 7e035bdea..89aaab9c7 100644 --- a/packages/appkit/src/plugins/lakebase/tests/lakebase-agent-tool.test.ts +++ b/packages/appkit/src/plugins/lakebase/tests/lakebase-agent-tool.test.ts @@ -1,5 +1,4 @@ import { beforeEach, describe, expect, test, vi } from "vitest"; - /** * Tests the agent-tool surface of the Lakebase plugin. * @@ -9,20 +8,11 @@ import { beforeEach, describe, expect, test, vi } from "vitest"; * (SP or per-user via RoutingPool). */ -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => ({ - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (_k: unknown[], fn: (signal?: AbortSignal) => Promise) => - fn(), - ), - generateKey: vi.fn(() => "test-key"), - })), - }, -})); +import { useTestCache } from "../../../testing/test-cache"; + +// Boot AppKit's real in-memory cache so the base Plugin constructor's +// getInstanceSync() resolves instead of throwing. +useTestCache(); // Client calls recorded by the read-only-statement test. The `connect()` // mock returns a fresh client whose `query` pushes to this array so tests diff --git a/packages/appkit/src/plugins/serving/tests/serving.test.ts b/packages/appkit/src/plugins/serving/tests/serving.test.ts index bca2f091a..2906b13ca 100644 --- a/packages/appkit/src/plugins/serving/tests/serving.test.ts +++ b/packages/appkit/src/plugins/serving/tests/serving.test.ts @@ -10,35 +10,12 @@ import { import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; import { ServiceContext } from "../../../context/service-context"; +import { useTestCache } from "../../../testing/test-cache"; import { ServingPlugin, serving } from "../serving"; import type { IServingConfig } from "../types"; -// Mock CacheManager singleton -const { mockCacheInstance } = vi.hoisted(() => { - const instance = { - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi - .fn() - .mockImplementation( - async ( - _key: unknown[], - fn: (signal?: AbortSignal) => Promise, - ) => { - return await fn(); - }, - ), - generateKey: vi.fn((...args: unknown[]) => JSON.stringify(args)), - }; - return { mockCacheInstance: instance }; -}); - -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => mockCacheInstance), - }, -})); +// Boots AppKit's real in-memory cache (no cache-module mock needed). +useTestCache(); // Mock the serving connector const mockInvoke = vi.fn(); diff --git a/packages/appkit/src/testing/fixtures.ts b/packages/appkit/src/testing/fixtures.ts index 13f79b5d7..81c164696 100644 --- a/packages/appkit/src/testing/fixtures.ts +++ b/packages/appkit/src/testing/fixtures.ts @@ -9,6 +9,7 @@ import type { ServiceContextState } from "../context/service-context"; import { ServiceContext } from "../context/service-context"; import { AuthenticationError } from "../errors"; import type { InstrumentConfig, ITelemetry } from "../telemetry/types"; +import { ApiError } from "../workspace-client"; import { createMockWorkspaceClient } from "./mock-workspace-client"; // Test fixtures intentionally use loose shapes; `noExplicitAny` is disabled @@ -340,6 +341,102 @@ export function setupDatabricksEnv(overrides: Record = {}) { Object.assign(process.env, overrides); } +/** + * Sets environment variables for the duration of `fn`, then restores them to + * their prior state. Each key's prior value (or "was absent") is captured on + * entry; on exit, the prior value is restored, or the key is deleted only if + * it was previously unset. + * + * Supports both sync and async `fn`. If `fn` returns a thenable, `withEnv` + * returns that promise and restores in `.finally()`. Otherwise, it restores + * in a synchronous `finally` and returns the callback's return value. + * Restoration runs even if `fn` throws. Nested calls restore in LIFO order. + * + * @example + * ```ts + * // Sync: restores synchronously after fn + * withEnv({ MY_VAR: "test" }, () => { + * console.log(process.env.MY_VAR); // "test" + * }); + * console.log(process.env.MY_VAR); // prior value (or undefined) + * + * // Async: restores after promise settles + * await withEnv({ MY_VAR: "test" }, async () => { + * await fetch(...); + * }); + * ``` + */ +/** + * Set environment variables and return a function that restores each key to its + * prior state — the prior value, or a delete when the key was previously unset. + * Shared capture/restore behind {@link withEnv} and the + * {@link createTestPluginContext} options path; not part of the public surface. + */ +export function applyEnv(vars: Record): () => void { + const prior = new Map(); + for (const key of Object.keys(vars)) { + prior.set(key, process.env[key]); + } + Object.assign(process.env, vars); + return () => { + for (const [key, value] of prior) { + if (value === undefined) { + delete process.env[key]; + } else { + process.env[key] = value; + } + } + }; +} + +/** + * Run a restore on an error path, suppressing any failure it throws so it + * cannot replace the caller's original error. + */ +function restoreQuietly(restore: () => void): void { + try { + restore(); + } catch (restoreError) { + // A failed env restore must not mask the caller's original error. + void restoreError; + } +} + +export function withEnv( + vars: Record, + fn: () => T | Promise, +): T | Promise { + const restore = applyEnv(vars); + + let result: T | Promise; + try { + result = fn(); + } catch (err) { + // Sync throw: restore, but never let a restore failure mask `err`. + restoreQuietly(restore); + throw err; + } + + // Async: restore after the promise settles. On rejection, guard the restore + // so it cannot replace the caller's error; on success, let a genuine restore + // failure surface. + if (result != null && typeof (result as Any).then === "function") { + return (result as Promise).then( + (value) => { + restore(); + return value; + }, + (err: unknown) => { + restoreQuietly(restore); + throw err; + }, + ); + } + + restore(); + return result; +} + /** * Clears AppKit's process-wide cache singleton so cached values don't leak * between tests in the same file. @@ -563,3 +660,36 @@ export function createFailedSQLResponse(errorMessage: string) { statement_id: `stmt-${Date.now()}`, }; } + +/** + * Creates a genuine `ApiError` instance for testing error paths. Returns a real + * instance (where `error instanceof ApiError` holds), suitable for testing + * `instanceof` checks and `.statusCode` / `.errorCode` / `.message` accessors. + * + * @param options Error details: `statusCode`, `message`, and `errorCode`. All required. + * @returns A genuine `ApiError` instance. + * + * @example + * ```ts + * const error = createApiError({ + * statusCode: 404, + * message: "File not found", + * errorCode: "NOT_FOUND", + * }); + * expect(error).toBeInstanceOf(ApiError); + * expect(error.statusCode).toBe(404); + * ``` + */ +export function createApiError(options: { + statusCode: number; + message: string; + errorCode: string; +}): ApiError { + return new ApiError( + options.message, + options.errorCode, + options.statusCode, + undefined, // response: sensible default for testing + [], // details: empty array + ); +} diff --git a/packages/appkit/src/testing/index.ts b/packages/appkit/src/testing/index.ts index 66b7b5e98..7a8b04900 100644 --- a/packages/appkit/src/testing/index.ts +++ b/packages/appkit/src/testing/index.ts @@ -60,6 +60,7 @@ export { type StreamSource, } from "./expect-stream"; export { + createApiError, createFailedSQLResponse, createMockRequest, createMockResponse, @@ -74,6 +75,7 @@ export { setupDatabricksEnv, type TestContextOptions, useServiceContextMock, + withEnv, } from "./fixtures"; export { createMockWorkspaceClient, @@ -83,6 +85,8 @@ export { } from "./mock-workspace-client"; export { createTestPlugin } from "./create-test-plugin"; export { resetGlobalState } from "./reset"; +export { type TestAppHandle, useTestApp } from "./test-app"; +export { type TestCacheHandle, useTestCache } from "./test-cache"; export { createTestPluginContext, type FakeProvider, @@ -91,4 +95,5 @@ export { type RecordedRoute, type RecordedToolCall, type TestPluginContext, + type TestPluginContextOptions, } from "./test-plugin-context"; diff --git a/packages/appkit/src/testing/test-app.ts b/packages/appkit/src/testing/test-app.ts new file mode 100644 index 000000000..d726cd30d --- /dev/null +++ b/packages/appkit/src/testing/test-app.ts @@ -0,0 +1,83 @@ +import type { PluginConstructor, PluginData } from "shared"; +import { afterEach, beforeEach } from "vitest"; + +import type { CreateTestAppOptions, TestApp } from "./create-test-app"; +import { createTestApp } from "./create-test-app"; + +/** Mirrors `create-test-app.ts`'s own constraint; not part of the public surface. */ +type Plugins = PluginData[]; + +/** + * The handle {@link useTestApp} returns: a live accessor for the harness app + * booted for the current test. + */ +export interface TestAppHandle { + /** + * The app booted for the current test. Read it inside a test body — each + * `beforeEach` boots a fresh app and each `afterEach` closes it. + */ + readonly current: TestApp; +} + +/** + * Boot a harness app before each test and close it after, so a suite that needs + * an app per test never hand-wires the hooks or risks a forgotten `close()`. + * + * Mirrors {@link useServiceContextMock} and {@link useTestCache}: call it at the + * top of a `describe` block (or module top-level), NOT inside a test — Vitest + * registers `beforeEach`/`afterEach` during collection. + * + * Reach for this when the app must outlive a single expression. `await using` + * covers one test more concisely, but it cannot carry an app from a `beforeEach` + * into the test body, and the harness allows only one open app at a time — so a + * `describe` that holds one in `beforeAll` cannot contain a test that boots its + * own. + * + * @example + * ```ts + * describe("my plugin over HTTP", () => { + * const app = useTestApp({ + * plugins: [myPlugin()], + * responses: { "jobs.getRun": { state: "TERMINATED" } }, + * }); + * + * test("answers a request", async () => { + * const res = await app.current.post("/api/my-plugin/run", { body: { id: 1 } }); + * expect(res.status).toBe(200); + * }); + * }); + * ``` + * + * @param options - Passed to {@link createTestApp} unchanged, for every boot. + * @returns `{ current }` — the app booted for the current test. + */ +export function useTestApp( + options: CreateTestAppOptions = {}, +): TestAppHandle { + let app: TestApp | undefined; + + beforeEach(async () => { + app = await createTestApp(options); + }); + + afterEach(async () => { + const booted = app; + // Cleared before the await so a close that throws cannot leave a stale + // handle readable by the next test. + app = undefined; + await booted?.close(); + }); + + return { + get current(): TestApp { + if (!app) { + throw new Error( + "useTestApp: no active app. Call useTestApp() at the top of a " + + "describe block (not inside a test), and read `.current` from " + + "within a test.", + ); + } + return app; + }, + }; +} diff --git a/packages/appkit/src/testing/test-cache.ts b/packages/appkit/src/testing/test-cache.ts new file mode 100644 index 000000000..6a1559010 --- /dev/null +++ b/packages/appkit/src/testing/test-cache.ts @@ -0,0 +1,90 @@ +import { afterEach, beforeEach } from "vitest"; + +import { CacheManager } from "../cache"; +import { InMemoryStorage } from "../cache/storage"; +import { resetTestCache } from "./fixtures"; + +/** + * The handle {@link useTestCache} returns: a live accessor for the real, + * in-memory {@link CacheManager} active in the current test. + */ +export interface TestCacheHandle { + /** + * The real (in-memory) cache for the current test. Each test's `beforeEach` + * seeds and clears the singleton, so reading this always sees a fresh cache — + * call `generateKey`, `get`, `has`, or `vi.spyOn(handle.current, "getOrExecute")` + * to assert real caching behaviour. + */ + readonly current: CacheManager; +} + +/** + * Stand up AppKit's real in-memory cache for a test file and clear it before + * each test, so a plugin's real caching path runs under test with no mock of + * the internal `cache` module. + * + * Boots the process-wide {@link CacheManager} singleton backed by + * {@link InMemoryStorage} (idempotent — an already-initialized singleton is + * reused and the storage argument ignored), then clears it via + * {@link resetTestCache} in `beforeEach` so each test starts empty. The + * singleton is left in place: this clears the cache's contents, never the + * pointer. + * + * Because the cache is booted before the test body runs, a plugin constructed + * in the test — whose constructor reads `CacheManager.getInstanceSync()` — + * binds `this.cache` to this real cache. So `getOrExecute` genuinely caches and + * the key is production's real `generateKey`, not a re-implemented fake. + * + * Call it at the top of a `describe` block (or module top-level), NOT inside a + * test: Vitest's `beforeEach`/`afterEach` only register during collection. + * + * @example + * ```ts + * describe("my plugin caches", () => { + * const testCache = useTestCache(); + * + * test("second identical request is a cache hit", async () => { + * const plugin = new MyPlugin(config); + * // ...drive the same request twice... + * expect(downstreamMock).toHaveBeenCalledTimes(1); + * }); + * + * test("metadata does not change the cache key", () => { + * const key = testCache.current.generateKey(["query", "SELECT 1"], "svc"); + * expect(key).toBe(testCache.current.generateKey(["query", "SELECT 1"], "svc")); + * }); + * }); + * ``` + * + * @returns `{ current }` — the active real {@link CacheManager} for the test. + */ +export function useTestCache(): TestCacheHandle { + let cache: CacheManager | undefined; + + beforeEach(async () => { + // Idempotent: reuses an existing singleton (ignoring the storage arg) or + // stands up a fresh in-memory one. Keeps the singleton either way. + cache = await CacheManager.getInstance({ + storage: new InMemoryStorage({}), + }); + // Fresh contents per test — clears storage without dropping the singleton. + await resetTestCache(); + }); + + afterEach(() => { + cache = undefined; + }); + + return { + get current(): CacheManager { + if (!cache) { + throw new Error( + "useTestCache: no active cache. Call useTestCache() at the top of a " + + "describe block (not inside a test), and read `.current` from " + + "within a test.", + ); + } + return cache; + }, + }; +} diff --git a/packages/appkit/src/testing/test-plugin-context.ts b/packages/appkit/src/testing/test-plugin-context.ts index 721e3cdbe..037235c2b 100644 --- a/packages/appkit/src/testing/test-plugin-context.ts +++ b/packages/appkit/src/testing/test-plugin-context.ts @@ -5,6 +5,7 @@ import type { IAppRequest, ToolProvider, } from "shared"; +import { afterEach, onTestFinished } from "vitest"; import { CacheManager } from "../cache"; import { InMemoryStorage } from "../cache/storage"; @@ -12,7 +13,8 @@ import { isToolProvider, PluginContext } from "../core/plugin-context"; import { AuthenticationError } from "../errors"; import type { Plugin } from "../plugin"; import type { ITelemetry } from "../telemetry"; -import { createMockTelemetry } from "./fixtures"; +import { applyEnv, createMockTelemetry, mockServiceContext } from "./fixtures"; +import { createMockWorkspaceClient } from "./mock-workspace-client"; /** * A concrete (non-function) fake tool response — returned as-is. Covers the @@ -52,6 +54,30 @@ export type FakeToolResponse = */ export type FakeProviders = Record>; +/** + * Options for {@link createTestPluginContext} when called with a second parameter. + * When provided, `createTestPluginContext` installs a service context seeded + * from a mock workspace client, plus optional environment variables. + */ +export interface TestPluginContextOptions { + /** + * Responses keyed by dotted path (`"jobs.getRun"`) for the mocked workspace + * client. Passed directly to {@link createMockWorkspaceClient}. + */ + responses?: Record; + /** + * Environment variables to set for the test. Captured on entry, restored + * (or deleted if they were unset) on exit via an `afterEach` hook and/or + * explicit {@link TestPluginContext.restore}. + */ + env?: Record; + /** + * If `true`, throw when a workspace client path with no declared response is + * called, instead of resolving `undefined`. Defaults to `false` (never crash). + */ + strict?: boolean; +} + /** A single dispatch observed by a fake provider. */ export interface RecordedToolCall { /** Registered plugin name (the key in {@link FakeProviders}). */ @@ -144,6 +170,14 @@ export interface TestPluginContext { * gate on `isReady`. Returns the same plugin for chaining. */ attach

(plugin: P): Promise

; + /** + * Restore the service context and environment variables to their pre-test state. + * Called automatically via `afterEach` when options were provided to + * `createTestPluginContext`. Can also be called explicitly for escape hatches + * (e.g., cleanup inside a test body). Idempotent — safe to call multiple times. + * Only present if the context was created with options. + */ + restore?: () => void; } /** @@ -164,18 +198,39 @@ export interface TestPluginContext { * Nothing about `PluginContext` is reimplemented. * * @param fakes - Canned tool responses keyed by plugin then tool name. + * @param options - When provided, installs a mock workspace client seeded from + * `responses`, mocks the service context, and sets `env`. Omit it for the + * original behavior. * * @example * ```ts + * // No options * const mock = createTestPluginContext({ analytics: { query: fixtureRows } }); * await mock.attach(agentsPlugin); * // ...exercise a handler that dispatches analytics.query... * expect(mock.toolCalls[0]).toMatchObject({ plugin: "analytics", asUser: true }); + * + * // With options — installs service context + seeded client + * const mock = createTestPluginContext( + * {}, + * { responses: { "jobs.getRun": { state: "DONE" } } }, + * ); * ``` */ export function createTestPluginContext( fakes: FakeProviders = {}, + options?: TestPluginContextOptions, ): TestPluginContext { + // No options: original behavior. + if (!options) { + return createTestPluginContextSync(fakes); + } + + // Options provided: install the seeded client, service context, and scoped env. + return createTestPluginContextWithOptions(fakes, options); +} + +function createTestPluginContextSync(fakes: FakeProviders): TestPluginContext { const telemetry = createMockTelemetry(); const ctx = new PluginContext({ telemetry }); @@ -351,6 +406,61 @@ export function createTestPluginContext( }; } +function createTestPluginContextWithOptions( + fakes: FakeProviders, + options: TestPluginContextOptions, +): TestPluginContext { + const { responses = {}, env: envVars = {}, strict = false } = options; + + // Build a mock workspace client seeded from responses + const client = createMockWorkspaceClient({ + responses, + strict, + }); + + // Install the mock service context with the seeded client + const serviceContextMock = mockServiceContext({ + serviceDatabricksClient: client, + }); + + // Set env (captured for restore) via the shared helper. + const restoreEnv = applyEnv(envVars); + + // Create the base context (without options this time, since we're handling everything) + const base = createTestPluginContextSync(fakes); + + // Restore function: restores env and service context (idempotent) + let hasRestored = false; + const restore = () => { + if (hasRestored) return; + hasRestored = true; + restoreEnv(); + serviceContextMock.restore(); + }; + + // Auto-restore after the current test. This helper is documented and used + // from inside a test body, where a runtime-registered `afterEach` does NOT run + // for that test (Vitest only collects `afterEach` before the test runs) — the + // reason the old `afterEach` here silently leaked. `onTestFinished` is the hook + // built for runtime registration and fires after the creating test. If called + // at collection scope instead, it throws, so fall back to `afterEach` there. + try { + onTestFinished(() => { + restore(); + }); + } catch { + afterEach(() => { + restore(); + }); + } + + // Return the context with the restore method + return { + ...base, + restore, + }; +} + function cacheReady(): boolean { try { CacheManager.getInstanceSync(); diff --git a/packages/appkit/src/testing/tests/fixtures.test.ts b/packages/appkit/src/testing/tests/fixtures.test.ts index 599fb8151..9f7a906f8 100644 --- a/packages/appkit/src/testing/tests/fixtures.test.ts +++ b/packages/appkit/src/testing/tests/fixtures.test.ts @@ -4,11 +4,14 @@ import { CacheManager } from "../../cache"; import { InMemoryStorage } from "../../cache/storage"; import { ServiceContext } from "../../context"; import { AuthenticationError } from "../../errors"; +import { ApiError } from "../../workspace-client"; import { + createApiError, createMockRequest, mockServiceContext, resetTestCache, useServiceContextMock, + withEnv, } from "../fixtures"; describe("createMockRequest — obo option", () => { @@ -184,3 +187,184 @@ describe("useServiceContextMock — restores after the block", () => { expect(vi.isMockFunction(ServiceContext.isInitialized)).toBe(false); }); }); + +describe("withEnv — environment variable restoration", () => { + test("sets a var inside fn, restores it to its prior value afterward", () => { + const original = process.env.TEST_VAR; + process.env.TEST_VAR = "original"; + + const result = withEnv({ TEST_VAR: "modified" }, () => { + expect(process.env.TEST_VAR).toBe("modified"); + return "done"; + }); + + expect(result).toBe("done"); + expect(process.env.TEST_VAR).toBe("original"); + + // Cleanup + if (original === undefined) { + delete process.env.TEST_VAR; + } else { + process.env.TEST_VAR = original; + } + }); + + test("a key that was UNSET before is deleted (not left set) after fn returns", () => { + if (process.env.NEVER_SET_VAR !== undefined) { + delete process.env.NEVER_SET_VAR; + } + + withEnv({ NEVER_SET_VAR: "temp" }, () => { + expect(process.env.NEVER_SET_VAR).toBe("temp"); + }); + + expect(process.env.NEVER_SET_VAR).toBeUndefined(); + }); + + test("a key that PRE-EXISTED is restored to its original value, not deleted", () => { + process.env.PRE_EXISTING = "before"; + + withEnv({ PRE_EXISTING: "changed" }, () => { + expect(process.env.PRE_EXISTING).toBe("changed"); + }); + + expect(process.env.PRE_EXISTING).toBe("before"); + + // Cleanup + delete process.env.PRE_EXISTING; + }); + + test("restores even when fn throws", () => { + process.env.THROW_TEST = "before"; + + expect(() => { + withEnv({ THROW_TEST: "during" }, () => { + expect(process.env.THROW_TEST).toBe("during"); + throw new Error("test error"); + }); + }).toThrow("test error"); + + expect(process.env.THROW_TEST).toBe("before"); + + // Cleanup + delete process.env.THROW_TEST; + }); + + test("async form: await withEnv({...}, async () => …) restores after the promise settles", async () => { + process.env.ASYNC_TEST = "before"; + + await withEnv({ ASYNC_TEST: "during" }, async () => { + expect(process.env.ASYNC_TEST).toBe("during"); + await new Promise((resolve) => setTimeout(resolve, 10)); + }); + + expect(process.env.ASYNC_TEST).toBe("before"); + + // Cleanup + delete process.env.ASYNC_TEST; + }); + + test("async form restores even when the promise rejects", async () => { + process.env.ASYNC_REJECT_TEST = "before"; + + await expect( + withEnv({ ASYNC_REJECT_TEST: "during" }, async () => { + expect(process.env.ASYNC_REJECT_TEST).toBe("during"); + throw new Error("async error"); + }), + ).rejects.toThrow("async error"); + + expect(process.env.ASYNC_REJECT_TEST).toBe("before"); + + // Cleanup + delete process.env.ASYNC_REJECT_TEST; + }); + + test("nested withEnv calls restore in reverse order (LIFO)", () => { + process.env.NESTED_VAR = "original"; + const log: string[] = []; + + withEnv({ NESTED_VAR: "level1" }, () => { + log.push(`L1-during: ${process.env.NESTED_VAR}`); + + withEnv({ NESTED_VAR: "level2" }, () => { + log.push(`L2-during: ${process.env.NESTED_VAR}`); + }); + + log.push(`L1-after: ${process.env.NESTED_VAR}`); + }); + + log.push(`outside: ${process.env.NESTED_VAR}`); + + expect(log).toEqual([ + "L1-during: level1", + "L2-during: level2", + "L1-after: level1", + "outside: original", + ]); + + // Cleanup + delete process.env.NESTED_VAR; + }); +}); + +describe("createApiError — genuine ApiError factory", () => { + test("returns a genuine ApiError instance", () => { + const error = createApiError({ + statusCode: 404, + message: "Not found", + errorCode: "NOT_FOUND", + }); + expect(error).toBeInstanceOf(ApiError); + }); + + test("preserves statusCode", () => { + const error = createApiError({ + statusCode: 500, + message: "Server error", + errorCode: "INTERNAL_ERROR", + }); + expect(error.statusCode).toBe(500); + }); + + test("preserves message", () => { + const error = createApiError({ + statusCode: 400, + message: "Bad request input", + errorCode: "INVALID_ARGUMENT", + }); + expect(error.message).toBe("Bad request input"); + }); + + test("preserves errorCode", () => { + const error = createApiError({ + statusCode: 403, + message: "Access denied", + errorCode: "PERMISSION_DENIED", + }); + expect(error.errorCode).toBe("PERMISSION_DENIED"); + }); + + test("works in production-shaped instanceof checks", () => { + const error = createApiError({ + statusCode: 404, + message: "Not found", + errorCode: "NOT_FOUND", + }); + // This is the production pattern from files/client.ts + const isNotFoundError = + error instanceof ApiError && error.statusCode === 404; + expect(isNotFoundError).toBe(true); + }); + + test("has sensible defaults for optional fields", () => { + const error = createApiError({ + statusCode: 400, + message: "Bad request", + errorCode: "BAD_REQUEST", + }); + // Should have response (undefined or null) and details (array) + expect(error).toHaveProperty("response"); + expect(error).toHaveProperty("errorInfoType"); + }); +}); diff --git a/packages/appkit/src/testing/tests/published-surface.integration.test.ts b/packages/appkit/src/testing/tests/published-surface.integration.test.ts index 6e5fef16e..d26526330 100644 --- a/packages/appkit/src/testing/tests/published-surface.integration.test.ts +++ b/packages/appkit/src/testing/tests/published-surface.integration.test.ts @@ -78,11 +78,15 @@ describe("@databricks/appkit/testing as a standalone surface", () => { "createMockTelemetry", "createSuccessfulSQLResponse", "createFailedSQLResponse", + "createApiError", "parseSSEResponse", "resetTestCache", "runWithRequestContext", "setupDatabricksEnv", "useServiceContextMock", + "useTestApp", + "useTestCache", + "withEnv", ]; const missing = expected.filter( (name) => diff --git a/packages/appkit/src/testing/tests/test-app.test.ts b/packages/appkit/src/testing/tests/test-app.test.ts new file mode 100644 index 000000000..0bd75d0e1 --- /dev/null +++ b/packages/appkit/src/testing/tests/test-app.test.ts @@ -0,0 +1,87 @@ +import type { PluginManifest } from "shared"; +import { describe, expect, test } from "vitest"; + +import { Plugin, toPlugin } from "../../plugin"; +import { createTestApp } from "../create-test-app"; +import { useTestApp } from "../test-app"; + +/** + * The behaviour that matters is the hook wiring: a fresh app per test, closed + * after each, with no `close()` for the caller to forget. The harness allows one + * open app at a time, so "the previous test's app was really closed" is + * observable — a leak makes the next boot throw. + */ + +class ProbePlugin extends Plugin { + static manifest = { + name: "probe", + displayName: "Probe", + version: "0.0.0", + description: "useTestApp probe", + resources: { required: [], optional: [] }, + } as unknown as PluginManifest; + + injectRoutes(router: never): void { + this.route(router, { + name: "ping", + method: "get", + path: "/ping", + handler: async (_req, res) => { + res.json({ pong: true }); + }, + }); + } +} +const probe = toPlugin(ProbePlugin); + +describe("useTestApp", () => { + const app = useTestApp({ plugins: [probe()] }); + + test("boots an app and serves a route inside a test", async () => { + const res = await app.current.get("/api/probe/ping"); + expect(res.status).toBe(200); + await expect(res.json()).resolves.toEqual({ pong: true }); + }); + + test("the previous test's app was closed, so this boot succeeded", async () => { + // If afterEach had not closed it, the one-app-at-a-time guard would have + // thrown during this test's beforeEach and never reached the body. + expect(app.current.port).toBeGreaterThan(0); + }); + + test("hands out a different app than the previous test", async () => { + const res = await app.current.get("/api/probe/ping"); + expect(res.status).toBe(200); + }); +}); + +describe("useTestApp passes options through", () => { + const app = useTestApp({ + plugins: [probe()], + env: { USE_TEST_APP_PROBE: "set" }, + }); + + test("env reaches the boot", () => { + expect(process.env.USE_TEST_APP_PROBE).toBe("set"); + }); +}); + +describe("useTestApp cleans up after the file's suites", () => { + test("env from the previous suite was restored on close", () => { + expect(process.env.USE_TEST_APP_PROBE).toBeUndefined(); + }); + + test("no app is held open, so a manual boot is allowed", async () => { + // The guard makes this the discriminating check: it only passes if every + // app useTestApp booted above was actually closed. + await using manual = await createTestApp({ plugins: [probe()] }); + expect(manual.port).toBeGreaterThan(0); + }); +}); + +describe("useTestApp misuse", () => { + test("reading .current outside a registered test explains itself", () => { + const stray = useTestApp({ plugins: [probe()] }); + expect(() => stray.current).toThrow(/no active app/); + }); +}); diff --git a/packages/appkit/src/testing/tests/test-cache.test.ts b/packages/appkit/src/testing/tests/test-cache.test.ts new file mode 100644 index 000000000..d0c3732fd --- /dev/null +++ b/packages/appkit/src/testing/tests/test-cache.test.ts @@ -0,0 +1,73 @@ +import { describe, expect, test, vi } from "vitest"; + +import { CacheManager } from "../../cache"; +import { useTestCache } from "../test-cache"; + +describe("useTestCache", () => { + const testCache = useTestCache(); + + test("boots the cache and exposes it inside a test", () => { + expect(testCache.current).toBeInstanceOf(CacheManager); + }); + + test("generateKey is production's key fn: stable for equal parts, differs by userKey", () => { + const a = testCache.current.generateKey(["op", 1], "user-1"); + const b = testCache.current.generateKey(["op", 1], "user-1"); + const c = testCache.current.generateKey(["op", 1], "user-2"); + expect(a).toBe(b); + expect(a).not.toBe(c); + }); + + test("getOrExecute caches: same key runs fn once, different keys run it twice", async () => { + const fn = vi.fn(async () => "value"); + await testCache.current.getOrExecute(["op", 1], fn, "user-1"); + await testCache.current.getOrExecute(["op", 1], fn, "user-1"); + expect(fn).toHaveBeenCalledTimes(1); + + const fn2 = vi.fn(async () => "value2"); + await testCache.current.getOrExecute(["op", 2], fn2, "user-1"); + await testCache.current.getOrExecute(["op", 3], fn2, "user-1"); + expect(fn2).toHaveBeenCalledTimes(2); + }); + + test("is spy-able: getOrExecute records the key parts a caller passes", async () => { + const spy = vi.spyOn(testCache.current, "getOrExecute"); + await testCache.current.getOrExecute( + ["listing", "/a"], + async () => 1, + "svc", + ); + expect(spy).toHaveBeenCalledWith( + ["listing", "/a"], + expect.any(Function), + "svc", + ); + }); +}); + +// Proves the per-test clear: these two tests run in source order (Vitest's +// default within a file), and the second must not see the first's write. +describe("useTestCache clears between tests", () => { + const testCache = useTestCache(); + const parts = ["shared"]; + const user = "u"; + + test("writes a value", async () => { + const key = testCache.current.generateKey(parts, user); + await testCache.current.set(key, "first"); + expect(await testCache.current.get(key)).toBe("first"); + }); + + test("does not see the previous test's value", async () => { + const key = testCache.current.generateKey(parts, user); + expect(await testCache.current.get(key)).toBeNull(); + }); +}); + +describe("useTestCache keeps the singleton", () => { + useTestCache(); + + test("getInstanceSync still returns an instance during a test", () => { + expect(() => CacheManager.getInstanceSync()).not.toThrow(); + }); +}); diff --git a/packages/appkit/src/testing/tests/test-plugin-context.test.ts b/packages/appkit/src/testing/tests/test-plugin-context.test.ts index d84a52937..fe276a90b 100644 --- a/packages/appkit/src/testing/tests/test-plugin-context.test.ts +++ b/packages/appkit/src/testing/tests/test-plugin-context.test.ts @@ -1,5 +1,5 @@ import type express from "express"; -import { describe, expect, test } from "vitest"; +import { describe, expect, test, vi } from "vitest"; import { PluginContext } from "../../core/plugin-context"; import { Plugin } from "../../plugin"; @@ -324,3 +324,213 @@ describe("createTestPluginContext — attach()", () => { expect(result).toBe("fake"); }); }); + +describe("createTestPluginContext — optional second parameter (options overload)", () => { + test("returns synchronously in both forms (not a promise)", () => { + const noOptions = createTestPluginContext(); + expect(noOptions).not.toBeInstanceOf(Promise); + expect(noOptions.ctx).toBeInstanceOf(PluginContext); + + const withOptions = createTestPluginContext( + {}, + { responses: { "jobs.getRun": { state: "DONE" } } }, + ); + expect(withOptions).not.toBeInstanceOf(Promise); + expect(withOptions.ctx).toBeInstanceOf(PluginContext); + withOptions.restore?.(); + }); + + test("no-options call behaves exactly as before (non-breaking)", async () => { + const mock = createTestPluginContext({ + analytics: { query: [{ id: 1 }] }, + }); + + const result = await mock.ctx.executeTool( + mockReq(), + "analytics", + "query", + {}, + ); + + expect(result).toEqual([{ id: 1 }]); + expect(mock.toolCalls).toHaveLength(1); + }); + + test("with options and responses, installs a mock workspace client seeded from responses", async () => { + createTestPluginContext( + {}, + { + responses: { + "jobs.getRun": { job_id: 42, state: "RUNNING" }, + }, + }, + ); + + // The service context is installed, so getWorkspaceClient() returns the mocked client. + const { getWorkspaceClient } = await import("../../context"); + const run = await getWorkspaceClient().jobs.getRun({ run_id: 42 } as never); + expect(run).toEqual({ job_id: 42, state: "RUNNING" }); + }); + + test("with env in options, sets env vars during the test", async () => { + const prior = process.env.TEST_VAR_ABC; + delete process.env.TEST_VAR_ABC; + + createTestPluginContext( + {}, + { + env: { TEST_VAR_ABC: "test-value" }, + }, + ); + + expect(process.env.TEST_VAR_ABC).toBe("test-value"); + + // Cleanup + delete process.env.TEST_VAR_ABC; + if (prior !== undefined) { + process.env.TEST_VAR_ABC = prior; + } + }); + + test("env is restored after the test", async () => { + const prior = process.env.TEST_VAR_XYZ; + delete process.env.TEST_VAR_XYZ; + + const mock = createTestPluginContext( + {}, + { + env: { TEST_VAR_XYZ: "value1" }, + }, + ); + + expect(process.env.TEST_VAR_XYZ).toBe("value1"); + + // Call restore explicitly + if ("restore" in mock && typeof mock.restore === "function") { + mock.restore(); + } + + // After explicit restore, env should be gone (it was unset before) + expect(process.env.TEST_VAR_XYZ).toBeUndefined(); + + // Cleanup + if (prior !== undefined) { + process.env.TEST_VAR_XYZ = prior; + } + }); + + test("restore() is idempotent", async () => { + const prior = process.env.TEST_VAR_IDEMPOTENT; + process.env.TEST_VAR_IDEMPOTENT = "prior"; + + const mock = createTestPluginContext( + {}, + { + env: { TEST_VAR_IDEMPOTENT: "changed" }, + }, + ); + + expect(process.env.TEST_VAR_IDEMPOTENT).toBe("changed"); + + if ("restore" in mock && typeof mock.restore === "function") { + mock.restore(); + } + + expect(process.env.TEST_VAR_IDEMPOTENT).toBe("prior"); + + // Calling restore again should not error + if ("restore" in mock && typeof mock.restore === "function") { + mock.restore(); + } + + expect(process.env.TEST_VAR_IDEMPOTENT).toBe("prior"); + + // Cleanup + if (prior !== undefined) { + process.env.TEST_VAR_IDEMPOTENT = prior; + } else { + delete process.env.TEST_VAR_IDEMPOTENT; + } + }); + + test("strict: true passes through to the mock client", async () => { + createTestPluginContext( + {}, + { + responses: { "jobs.getRun": { state: "DONE" } }, + strict: true, + }, + ); + + const { getWorkspaceClient } = await import("../../context"); + const client = getWorkspaceClient(); + + // Declared response should work + const run = await client.jobs.getRun({ run_id: 42 } as never); + expect(run).toEqual({ state: "DONE" }); + + // Undeclared path should throw when called with strict: true + // Access the method through the facade (which returns a service proxy) + const undeclaredFn = (client as any).warehouses.undeclaredMethod; + try { + await undeclaredFn({ foo: "bar" }); + expect.fail("Should have thrown"); + } catch (err) { + expect((err as Error).message).toContain("no declared response"); + } + }); + + // Proves the options overload auto-restores the service-context spies after + // the creating test WITHOUT a manual restore(). Split across two ordered tests + // because the cleanup fires between them: the second test would fail if the + // hook did not run for the first (the runtime-`afterEach` bug this replaced). + describe("service context auto-restore (no manual restore)", () => { + test("the mock is active inside the test that created it", async () => { + const { ServiceContext } = await import("../../context/service-context"); + + // Intentionally NO manual restore() — auto-cleanup must handle it. + createTestPluginContext( + {}, + { responses: { "jobs.getRun": { state: "DONE" } } }, + ); + + expect(ServiceContext.isInitialized()).toBe(true); + expect(vi.isMockFunction(ServiceContext.isInitialized)).toBe(true); + }); + + test("the previous test's service-context spies were auto-restored", async () => { + const { ServiceContext } = await import("../../context/service-context"); + + // If auto-restore fired after the test above, the spy is gone and the real + // static method is back. + expect(vi.isMockFunction(ServiceContext.isInitialized)).toBe(false); + }); + }); + + test("combines fakes and responses in a single call", async () => { + const mock = createTestPluginContext( + { + analytics: { query: [{ result: "fake" }] }, + }, + { + responses: { + "jobs.getRun": { state: "DONE" }, + }, + }, + ); + + // Fakes work + const fakeResult = await mock.ctx.executeTool( + mockReq(), + "analytics", + "query", + {}, + ); + expect(fakeResult).toEqual([{ result: "fake" }]); + + // Responses work (via seeded mock client in service context) + const { getWorkspaceClient } = await import("../../context"); + const run = await getWorkspaceClient().jobs.getRun({ run_id: 42 } as never); + expect(run).toEqual({ state: "DONE" }); + }); +});