feat(appkit): add createTestApp, a never-crash mock client, and app.close() - #540
Open
IamGalymzhan wants to merge 55 commits into
Open
feat(appkit): add createTestApp, a never-crash mock client, and app.close()#540IamGalymzhan wants to merge 55 commits into
IamGalymzhan wants to merge 55 commits into
Conversation
The testing kit needs to construct a real PluginContext without a live OpenTelemetry pipeline. Add an optional constructor dependency for the telemetry provider, defaulting to the shared "plugin-context" provider so the production path is unchanged. This is the single production edit required to wrap the real class in tests rather than reimplementing it. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Wire the testing kit as a published subpath and prove it against the first
of the two hand-rolled context stubs (the design gate):
- Add ./testing to both exports maps (dev + publishConfig) following the
./type-generator shape, add src/testing/index.ts to the tsdown entry, and
declare vitest as an optional peerDependency. Build passes attw + publint;
dist/testing/{index,mock-plugin-context,expect-stream,fixtures}.{js,d.ts}
are emitted and vitest stays external to the main entry.
- Migrate dispatch-tool-call.test.ts: replace (plugin as any).context =
{ executeTool } with mockPluginContext. executeTool is now the REAL method,
so the forwarded toolCallTimeoutMs is asserted through actual signal
composition, the on-behalf-of (asUser) path is verified, and a new test
proves the forwarded timeout actually aborts a slow toolkit tool end-to-end.
This is the primary win from the plan: executeTool's OBO and timeout paths
gain real assertions instead of a stub that proved nothing.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
…Context
Replace the second and final hand-rolled stub — (plugin as any).context =
{ addRoute } — with the real PluginContext from mockPluginContext. The kit's
route recorder captures raw handlers, so the alias assertion (both
/invocations and /responses mount the same handler reference) holds against
the real class, where forwardAsyncErrors wrapping would otherwise break
reference identity.
Both context stubs the plan identified are now migrated.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
- Add docs/docs/development/testing.md covering mockPluginContext(), expectStream(), and the fixture helpers, with a full end-to-end example. Cross-links to local-development, custom-plugins, and execution-context. - Add template/server/example.test.ts: a self-contained, plugin-agnostic example that scaffolded apps ship with — it defines a tiny custom plugin and exercises both mockPluginContext (route recording) and expectStream (ordered event assertions), running with no workspace or network. Ships the kit to users, satisfying the plan's acceptance criteria that a docs page exists and the template carries at least one example test. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Validation by scaffolding a real app with `databricks apps init` surfaced
that the examples called the `analytics()`/`toPlugin()` factory and then
treated the result as a plugin instance — but a factory returns a
{ plugin, config, name } descriptor for createApp to construct, so
`.attachContext`/handler methods are absent.
Rewrite both the template example test and the docs "Full example" to
instantiate the plugin class directly (`new GreeterPlugin({})`), matching how
the migrated agents suites use the kit. The scaffolded app's `npm test` and
`tsc` both pass against the published `@databricks/appkit/testing` subpath
with no workspace or network.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
…pe error
Drop `undefined` from the static FakeToolValue union. `resolve()` treats an
undefined map entry as "unregistered tool" and throws, so allowing undefined
as a declared response made `{ query: undefined }` a confusing runtime error
instead of a compile error. A function returning undefined still works for the
rare "returns nothing" case. Add a test pinning that a null response is
returned as a value, not misread as a missing tool.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
…ting kit The plan's step 5 was to MOVE the fixtures into the package, not copy them. The shipped kit (src/testing/fixtures.ts) duplicated all 15 exports of tools/test-helpers.ts, which would drift over time. Collapse the original into a thin re-export of @databricks/appkit/testing so src/testing is the single source of truth while the 18 existing @tools/test-helpers importers keep working unchanged. The re-exported mockServiceContext is now synchronous; every call site either awaits it (no-op on a non-promise) or reads it through Awaited<ReturnType<...>>, so all suites pass unchanged (full appkit suite: 3117 passed, 1 pre-existing skip). Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
…ing docs Code review follow-ups: - expectStream's parseSSEBody split frames on \n\n, so a spec-compliant SSE stream delimited by \r\n\r\n (from a real server) collapsed into one event. AppKit's own writer uses \n\n so existing tests were unaffected, but expectStream is public API that accepts any Response. Normalize CRLF to LF before splitting; add a CRLF regression test. - Docs: instantiate the plugin CLASS in the attach() snippet (the factory returns a descriptor, not an instance), and note that the cache attach() seeds is a per-process singleton shared by tests within a file. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
CI's "Lint & Type Check" job runs `pnpm run check` over the whole repo, so a pre-existing lint error unrelated to this branch failed the build: - remote-tunnel-controller.test.ts had two `afterEach` hooks in one describe (lint/suspicious/noDuplicateTestHooks, error severity). Merge them into one — behavior preserved (env reset + console-spy clear both still run after each test). This file is byte-identical to main; the error predated the branch and only surfaced because CI lints the entire tree. Also drop two dead `biome-ignore lint/suspicious/noExplicitAny` suppressions in the testing kit (fixtures.ts, expect-stream.test.ts): `noExplicitAny` is turned off repo-wide in biome.json, so the comments had no effect (suppressions/unused warnings). The invalid-source test now casts through `unknown as never`. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Verified and fixed the findings from an independent code review: - #1 (correctness) expectStream dropped the wire `event:` name when the JSON payload carried its own `type` (spread ran after the assignment). Spread the payload first, then set `type = name ?? parsed.type`, so a frame like `event: error` + `data: {"type":"result"}` reports `error`. Regression test added. - #2 (contract) `@databricks/appkit/testing` eagerly loads vitest via fixtures even for `expectStream`, so vitest is a real requirement. Drop the "optional" peerDependenciesMeta and correct the docs sentence. - #6 (OBO fidelity) the fake `asUser` recorded `asUser: true` unconditionally. Enforce the real `Plugin.asUser` token precondition: a request without `x-forwarded-access-token` throws `missingToken` (missing user id throws too), and the resolved `userId` is recorded on each tool call. Tests now assert both directions (well-formed request vs token-less). - #3 (fidelity) attach() now mirrors AppKit core: registerPlugin plus registerToolProvider for real tool providers, without clobbering injected fakes. getPlugins()/getPluginNames()/hasPlugin() behave as in production. - #12 unknown-tool lookup used `tools[name] === undefined`, so a tool named "constructor"/"toString" hit Object.prototype. Use Object.hasOwn. - #5 drop data-less named SSE frames (real clients ignore them). - #7 re-export the PluginContext type from the testing barrel so MockPluginContext.ctx is nameable through the exports map. - #13 correct the docs: mock.telemetry captures the context's executeTool spans, not plugin-level spans (attachContext rebuilds the plugin's own telemetry). - #4 parseSSEResponse now delegates to the same parseSSEBody as expectStream — one parser, no divergence. All 3 analytics.integration call sites still pass. - #8 reformat template/server/example.test.ts with the template's Prettier so a scaffolded app's `npm run format` passes. - #10 fix the package-doc @example (agentsPlugin._handleStream does not exist). - #11 add kit tests that exercise attach() end-to-end (cache seed, isReady, registration, fake-not-clobbered). Build passes attw + publint; full appkit suite 3125 passed / 1 pre-existing skip. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
… dep With vitest declared as a (non-optional) peerDependency, knip recognizes it as used, so the earlier ignoreDependencies entry is unnecessary. This reverts knip.json to its original state. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
A required peerDependency has no per-subpath scope: it applied to the whole @databricks/appkit package, so every production consumer that never imports the testing kit got an unsatisfied peer (npm 7+ auto-installs vitest into their tree; pnpm warns) — a wider blast radius than the eager-import bug it was meant to fix. Follow appkit's own precedent instead: `vite` backs the ./type-generator subpath as a normal `dependency`, installed for everyone but loaded only by importers of that subpath. Do the same for `vitest` and ./testing. vitest is referenced solely by dist/testing/fixtures.js, never by the main/plugin/core entry, so a consumer importing createApp never loads it. Verified end-to-end: scaffolded an app whose own vitest (4.1.9) differs in major from appkit's dependency (3.2.4), forcing a nested second copy. The testing kit's vi.fn()/vi.spyOn() mocks and expect(...).toHaveBeenCalled() assertions work across the two instances (vi spies carry their own call state), and npm install emits no peer-dep warning. Build passes attw + publint. Also fold in the template example's Prettier formatting (template uses Prettier, not Biome) so a scaffolded app's `npm run format` passes. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
The helper builds the REAL PluginContext with faked edges — it does not mock the context — so the name was misleading. Rename to createTestPluginContext (and the MockPluginContext type to TestPluginContext), matching the create*-for-tests convention, and rename the files to test-plugin-context.ts. Pre-merge and unreleased, so no external consumers are affected. Also finish the #13 doc-accuracy fix in the shipped JSDoc (not just the docs page): the telemetry field comment now states it captures the context's spans (executeTool), not plugin-internal spans — attachContext rebuilds the plugin's this.telemetry from the real TelemetryManager. These comments ship in dist/testing/*.d.ts, so IntelliSense previously showed the unqualified claim. Build passes attw + publint; full appkit suite 3125 passed / 1 pre-existing skip. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Behavior-preserving cleanups in the testing kit: - createMockRequest reuses createMockWorkspaceClient() instead of an inline copy of the same mock client (verified identical). - createMockServiceContext / createMockUserContext / mockServiceContext inline the createMockWorkspaceClient() call into the `||` fallback, so the mock client is built only when the caller did not supply one. - The fake asUser view spreads `...base` and overrides executeAgentTool rather than re-declaring getAgentTools. - expectStream's isSubsequence breaks once the expected sequence is fully matched. No semantic change; typecheck clean and all kit + migrated tests pass. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
- #1 (P1) The docs called vitest a peer dependency, but the manifest ships it under `dependencies` (the decision we landed on, matching how appkit ships `vite` for ./type-generator). Correct the docs to match: appkit installs vitest for you, and it loads only when you import ./testing. Manifest and docs now agree. - #2 (P2) expectStream buffered the source eagerly with no bound, so a non-terminating stream hung until the runner's own timeout. Add an optional `{ timeout }` that fails fast with a clear, kit-specific error; document it and cover both directions with tests. - #3 (P2) The fake asUser replicates asUser's token precondition but not the real dev-mode `DEV_OBO_FALLBACK_KEY` OTel marker (a module-private telemetry detail). Narrow the docs and JSDoc to say so and point users at the recorded asUser/userId fields instead of isDevOboFallback(). Build passes attw + publint; full appkit suite 3141 passed / 1 pre-existing skip. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Exercise @databricks/appkit/testing against real core plugins to validate it beyond the two agent proof sites and produce usage references: - analytics.kit.test.ts: cross-plugin executeTool via createTestPluginContext — OBO identity (asUser/userId), token-precondition rejection, and per-call timeout abort. Needs only the kit (no workspace/ServiceContext). - genie.kit.test.ts: drives the real _handleSendMessage SSE stream and asserts event order with expectStream(...).toEmit(...). Both add genuinely new coverage (streamed SSE order + OBO dispatch identity were untested). Full appkit suite 3145 passed / 1 pre-existing skip. Developer-experience notes (kit wins + friction, e.g. createMockResponse doesn't compose with expectStream) captured in internal/ for the milestone review. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Resolve the eight review comments on the testing kit: - createMockResponse now captures written SSE bytes and exposes sseResponse(); expectStream reads a captured mock response directly, so streaming-route tests no longer need a hand-rolled bridge. - Ship vitest as an optional peer dependency (+ devDependency) instead of a plain runtime dependency, keeping the test framework out of production installs and deduping to the app's own copy. Ignore it in knip. - Add an obo option to createMockRequest so on-behalf-of tests set the forwarded identity headers with one flag. - Add resetTestCache() to clear the shared cache singleton between tests. - Use the documented attach() instead of an any-cast in the agents dispatch tests. - Drop the unused createMockServiceContext/createMockUserContext builders from the public surface; keep the service-context builder internal. - Pin the previously untested edges: the Object.hasOwn tool-lookup guard, the dev-mode asUser branch, and parseSSEBody's non-object data values. - Add useServiceContextMock() to register the mock lifecycle in one line, returning a live accessor. Dogfood the new helpers in the analytics, genie, and serving suites, and document them in the testing guide. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
The testing kit is entirely plugin-scoped (createTestPluginContext, attach(plugin), plugin route/tool/SSE assertions), and the page's own cross-links already pointed into plugins/. Move it next to custom-plugins and fix the relative links. Keep the heading as 'Testing'; the Plugins section supplies the context. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Address round-2 review: the kit should be the default way to test a plugin, not a parallel '*.kit.test.ts' track. - Fold the three cross-plugin executeTool OBO tests into analytics.test.ts and delete analytics.kit.test.ts. - Upgrade genie.test.ts's SSE test to assert event ORDER via expectStream on genie's real event names (message_start, status, message_result, query_result), replacing brittle write.mock.calls substring checks, and delete genie.kit.test.ts. - Trim the heavy comment narration from the folded-in tests. - Re-export createTestPluginContext and expectStream from the test-helpers shim. - Finish the testing-guide move under plugins/ (sidebar position + links). Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
The dogfood fold trimmed expect(mock.toolCalls).toHaveLength(1), so a double-dispatch would no longer fail the happy-path test — and it was inconsistent with the token-less sibling that kept toHaveLength(0). Restore it. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
The toEmit swap pinned event order but dropped the payload values the old substring checks covered (conversationId=new-conv-id, status=ASKING_AI), which aren't asserted elsewhere. Restore them structurally via collect() + toMatchObject — keeping the ordering guarantee without brittle substrings. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
…equest createMockRequest returned userWorkspaceClient, serviceWorkspaceClient, getWarehouseId and getWorkspaceId — fields no production code reads (plugins resolve those through getWorkspaceClient()/getWarehouseId() from src/context, which mockServiceContext stands in for). Publishing them via @databricks/appkit/testing would make four inert fields a permanent public promise. The two warehouse cold-start tests (analytics + metric) overrode mockReq.serviceWorkspaceClient.warehouses.get, which the route never reads — so they passed on the default RUNNING client without exercising the warehouse path at all. Route the warehouse client through mockServiceContext (the real seam) so the tests are live, and drop the 'mock WorkspaceClient' claim from the testing guide. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Brings feat/testing-kit up to v0.60.0. Three conflicts, all resolved in favour of this branch: - tools/test-helpers.ts — main only reformatted the old implementation; this branch replaced it with a re-export shim over packages/appkit/src/testing. - agents/tests/route-handler-errors.test.ts and dispatch-tool-call.test.ts — this branch migrated both onto createTestPluginContext, a superset of main's raw-stub versions (dispatch-tool-call keeps an extra timeout-abort test). Main migrated Biome -> oxlint+oxfmt, so this commit also reconciles the branch with the new toolchain: the 64 dead biome-ignore comments are dropped from the two conflicted test files, and the shipped testing-kit sources are reformatted under oxfmt's import grouping. Ignore **/.claude in knip, oxlint, and oxfmt. Agent worktrees live under .claude/worktrees/, so every tool was analysing a second full copy of the repo: knip reported hundreds of phantom unused exports and failed the pre-commit hook outright, and a repo-root `oxfmt` would have rewritten another branch's working tree. Co-authored-by: Isaac Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Every core plugin's actual work runs through getWorkspaceClient(), which the
testing kit did not fake — so a jobs/genie/serving/files plugin crashed on its
first client call and authors hand-rolled nested client literals instead.
createMockWorkspaceClient() fakes the whole facade in three layers:
- The 9 facade members are explicitly typed, so `client.jbos` is a compile
error. The facade is closed and AppKit-owned, so there is no per-service
fixture to maintain as the SDK grows.
- Each service is a Proxy minting one memoized vi.fn() per method name, keyed
by dotted path. `client.jobs.getRun === client.jobs.getRun`, so call
assertions work, and the legacy view shares the map so one `responses` entry
covers both — including un-faceted services like `legacy.clusters.list()`.
- `config` and `apiClient` are seeded objects rather than bare Proxies, because
three of their members must not be mocks: `config.host` is a real string that
production code builds URLs from and throws on when falsy,
`apiClient.userAgent()` must be synchronous (a Promise inside a Headers value
stringifies to "[object Promise]"), and `apiClient.request` resolves {} so
destructuring its result does not throw.
Two guards keep the Proxy safe. Symbol keys delegate to Reflect.get, and a
passthrough deny-set answers `undefined`. `then` is the load-bearing entry:
without it a service looks thenable, so `await client.jobs` either hangs or
resolves to a mock's return value. ownKeys is left at its default so
util.inspect and toEqual see {} instead of recursing forever.
The three historical canned defaults are byte-identical, because 13 test files
reach them implicitly through mockServiceContext. `currentUser.me` is additive
and load-bearing: ServiceContext.createContext reads `currentUser.id`, so an
unresolved me() is a TypeError and createApp({ client }) cannot boot without it.
getMockFn(client, "jobs.getRun") is the typed assertion path — facade accessors
are legacy-SDK-typed, so expect(client.jobs.getRun).toHaveBeenCalled() does not
typecheck. It mints idempotently, so the handle can be grabbed before the code
under test runs.
The compile-time block is enforced by tsc, not at runtime. It records one
correction to the plan: the SDK types `config.host` as `string | undefined`, so
the contract is that it narrows to a string, not that it is non-optional.
4451 tests pass (+38); the 667 tests reaching the default client indirectly
through mockServiceContext are unchanged.
Co-authored-by: Isaac
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
fixtures.ts had its own two-service createMockWorkspaceClient, so the shipped fixture and the new never-crash builder were near-duplicates. The fixture now re-exports the builder and the barrel points at its new home. The blast radius is entirely indirect. Nothing in src imports the exported fixture by name (connectors/genie/tests/client.test.ts defines its own local one), but buildServiceContextState calls it as the default client for mockServiceContext, which 13 test files use. The risk therefore lives in the default return value, which is why U1 kept the three canned defaults byte-identical — and why this commit adds the convergence guard that asserts both halves: jobs/genie now resolve instead of throwing "Cannot read properties of undefined", while the SQL path those 13 files depend on still succeeds. createConfigurableMockWorkspaceClient is left byte-for-byte unchanged and only gains a @deprecated notice. Its bare vi.fn()s return undefined *synchronously* whereas the new floor returns Promise<undefined>, and its one caller (analytics.integration.test.ts) can observe that difference; reimplementing it here would change behaviour for no benefit. It migrates with that suite later. The jobs suite drops its hand-rolled client literal — the seven method mocks plus the config.host/authenticate block — onto the builder, which is the proof the boilerplate actually goes away. Its 57 assertion sites move to a getMockFn handle because facade accessors are legacy-SDK-typed, so .mockResolvedValue on them does not typecheck. The factory needs `await vi.hoisted(async ...)` with a dynamic import, since a hoisted factory runs before the file's imports. 4454 tests pass (+3). Co-authored-by: Isaac Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
LifecycleManager's shutdown sequence was reachable only by killing the process, so nothing could release AppKit's sockets, timers, pools, cache, and telemetry and keep running. That is what blocks an app handle's close(), and with it any test that wants to boot more than once in a file. The sequence is now a phase runner that returns an exit code, a promise memo, and two thin callers: - shutdown() is the signal path, observably unchanged: it arms the same unref'd 15s force-exit backstop and still exits 0 on completion, 1 on an unexpected throw. The timer stays here deliberately — it is the one thing close() must not inherit, since a programmatic caller wants a logged error when teardown hangs, not a dead process. - close() is the programmatic path: it detaches signal handlers, runs the same phases under a shorter default budget (5s, not the production 15s), logs the phase that was in flight if the budget is spent, and never exits. Replacing the isShuttingDown boolean with a promise memo is a strict improvement. The boolean made a second caller return *immediately* while teardown was still running — harmless for a signal, since the first caller exits the process anyway, but for close() it would resolve before resources were released, which is the difference between a correct handle and a misleading one. The read and the assignment stay in one synchronous statement, preserving the invariant the boolean was there to protect. One production behaviour does shift: a second signal now awaits the first teardown. installSignalHandlers registered anonymous arrows that could never be removed. The [signal, handler] pairs are now retained and detached individually, never via removeAllListeners, so a host embedding AppKit keeps its own handlers. The tests assert that with two managers installed, a.close() leaves b's pair and an unrelated host listener intact, and that counts return to their pre-install baseline — which is what stops repeated boots tripping MaxListenersExceededWarning. The signal-mid-close race is documented rather than papered over: handlers come off before the first await, and if a signal still lands it joins the memo and exits, because it wanted the process dead. The idempotency test is verified by injection — it fails against the old return-immediately semantics and passes against the memo. Its first draft did not: it counted microtask ticks, which cannot distinguish an early return through close()'s raceWithTimeout wrapper. It now asserts that neither caller settles until the plugin hook has actually completed. 4463 tests pass (+9); the 14 pre-existing shutdown tests are untouched. Co-authored-by: Isaac Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
createApp acquired sockets, timers, and pools but returned no way to release them, so the only teardown was killing the process. The LifecycleManager built at the end of _createApp was constructed and immediately discarded; it is now retained on the instance and reachable through the handle. The return type widens from PluginMap<T> to AppHandle<T>, which is PluginMap<T> plus close() and Symbol.asyncDispose. Widening a return type is source-compatible for every existing caller, and the cast that produces the handle already hid instance methods, so close() rides along naturally. onPluginsReady deliberately keeps PluginMap<T>: it runs before the server starts, so handing it a close() would invite a footgun for no gain. The name collision is a real hazard, not a theoretical one. Plugin exports are installed with Object.defineProperty, and an own property shadows a prototype method — so a plugin named `close` would silently replace teardown rather than merely confuse the types. Three layers guard it: Symbol.asyncDispose is unreachable from a manifest name, so `await using` is always safe; createAndRegisterPlugin now throws a ConfigurationError naming the offending plugin; and no plugin in the repo is affected. Coverage is deliberately unmocked, because the claim is about real resources: a boot on an ephemeral port serves /health, close() runs the plugin's shutdown hook, the socket stops accepting, and the SIGTERM listener count returns to its pre-boot baseline. Also covered: idempotency at the app level, a server-less app closing cleanly, `await using` releasing at scope exit, and the reserved name being rejected. Verified by injection — with close() stubbed to a no-op and the reserved-name guard removed, 5 of the 6 fail. 4469 tests pass (+6). Co-authored-by: Isaac Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
close() released resources but left the singletons pointing at them, so close() followed by createApp() silently reused what the teardown had just torn down. This delivers the actual driver — boot, assert, close, repeat. CacheManager.reset() drops both `instance` and `initPromise`. Clearing only `instance` is insufficient because getInstance() returns `initPromise` when `instance` is null, so the next boot would await a promise resolving to the dead manager. Testing surfaced a third case the plan missed: clearing both is *still* not enough, because an initialization already in flight runs its continuation and re-publishes the very instance being discarded. A generation counter now invalidates that write. The covering test models PersistentStorage rather than using the default in-memory storage. This matters: InMemoryStorage.close() only clears a Map and stays usable, so an in-memory test passes whether or not the reset exists — which is precisely why the bug hid. Against storage whose close() is terminal, the way pool.end() is, the test shows the stale manager throwing "Cannot use a pool after calling end()" and the reset fixing it. One plan claim is corrected rather than implemented. The plan asserted that TelemetryManager's never-cleared `shutdownPromise` made a second shutdown() return a stale promise and skip flushing a re-initialized SDK. It does not: shutdown() only returns the memo after reassigning it for whatever SDK is currently live, so a stale resolved promise can be returned only when there is no SDK to flush. Verified twice — by mocking NodeSDK across three initialize/shutdown cycles, and by running the original implementation in isolation. An earlier draft of this commit added a generation counter here too; it has been reverted, since it fixed nothing and cost a field. What TelemetryManager did need, and now has, is the static reset() that drops the singleton. The resets are wired into close() only, never the signal path, where the process is dying and pointer drops are pure cost. Symmetry is the justification: core initializes all four in _createApp, so core drops all four. This is a semantic expansion, not purely a bug fix — a host that closes and then expects ServiceContext.get() to work will now get an InitializationError. resetAppKitSingletons() is published from @databricks/appkit/testing for tests that hand-roll createApp and would otherwise deep-import ../context/service-context to reach ServiceContext.reset(). Both it and LifecycleManager.close() delegate to one core-side implementation rather than duplicating the list. resetTestCache() is untouched — it calls clear() on the existing cache, a different and still-useful operation. 4480 tests pass (+11). Co-authored-by: Isaac Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
One call boots a real AppKit app with no workspace, no credentials, and no
network, and calls it over real HTTP:
const app = await createTestApp({ plugins: [myPlugin()] });
const res = await app.post("/api/my-plugin/thing", { body, obo: true });
await expectStream(res).toEmit("status", "result");
await app.close();
Four of the setup steps exist only because of hazards found by reading the boot
path, and each has a test that fails without it:
- NODE_ENV is pinned away from "development". Not tidiness: dev mode routes the
injected `port: 0` through get-port, where portNumbers(0, …) throws a
RangeError. "development" is refused outright with an explanation rather than
worked around, since dev mode also boots a real Vite server, downgrades
resource validation to a warning, and stops filtering dev-only plugins.
- DATABRICKS_WORKSPACE_ID is set, short-circuiting the SCIM probe in
getWorkspaceId, and internal telemetry is disabled. Both would otherwise fire
apiClient.request during boot. A canary test asserts zero calls after boot, so
either regression fails loudly.
- The cache gets explicit in-memory storage. Without it CacheManager builds its
own workspace client — ignoring the injected one — and probes Lakebase over
the network, so "no network" would be false.
- The server plugin is reached through a lazy `await import()`, because it runs
dotenv.config() at module load. A static import would mutate a consumer's
process.env merely by importing the testing entry point.
process.env is snapshotted wholesale rather than by whitelist, since plugins
read vars the harness cannot enumerate, and restored on close() — including
deleting keys the harness added and restoring a pre-existing DATABRICKS_HOST to
its own value rather than the test default. Teardown also runs from the
boot-failure path, or a plugin whose setup() throws would leak env mutations into
every later test in the file.
Plugin exports live under app.plugins rather than spread onto the handle: `get`
and `delete` are plausible plugin names and would collide with the request
methods.
The request methods return a native Response, so expectStream composes with no
bridge — the dogfooding report's top friction, avoided by construction. `obo`
reuses createMockRequest's OboOption rather than inventing a second convention.
Two corrections to the plan, both found by testing:
- A `strictValidation: false` opt-out was specified and has been dropped as a
false affordance. enforceValidation computes `shouldThrow = !isDevelopment ||
strict`, so with NODE_ENV pinned away from "development" validation always
throws and the flag cannot do anything. The env var is still set as
belt-and-braces, and a test pins the unconditional behaviour.
- The error-middleware test initially asserted a redacted body. It is not
redacted: errorHandlerMiddleware hides the message only under
NODE_ENV=production, and the harness pins "test". Useful for tests — an
assertion can name the failure — but it means that response is the dev shape,
which the test now says out loud.
The HTTP suite's probe plugin registers routes through `this.route()`, the way
real plugins do. Registered with raw `router.get()` a rejection escapes
forwardAsyncErrors and hangs the request — correct AppKit behaviour, and worth
having a representative test rather than a misleading one.
4511 tests pass (+31).
Co-authored-by: Isaac
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
The new suites were verbose in ways that cost a reviewer without buying coverage. 1837 lines across 8 files -> 1529 across 7. - mock-workspace-client.test.ts: 471 -> 253. The nine-accessor walk and the canned defaults become one test.each table; per-test client construction goes through a short factory alias. Verified by injection: dropping `then` from the deny-set, changing a canned default, and adding an ownKeys trap each still fail the suite, so the compaction did not gut it. - create-test-app-http.test.ts is folded into create-test-app.test.ts. Both tested one unit through two near-identical 100+ line plugin fixtures; there is now one probe plugin and one manifest helper. Six of the nine routes in the old echo fixture were dead — they duplicated the HTTP file's own plugin. - A withApp() helper absorbs the boot/try/finally/close block that appeared 13 times. It is generic over the plugin tuple so app.plugins stays typed. - The three /headers tests differed only in inputs and expectations, so they are one test.each. The cache reset suite's storage double loses its repeated ended-guard boilerplate. 4509 tests pass; typecheck, lint, and format clean. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
**Restore three upstream mlflow tests dropped by the merge.** Resolving the route-handler-errors conflict with `--ours` took the whole file from this branch, discarding main's non-conflicting additions from PR #477: the `vi.mock("../mlflow")` hoist, the linkTraceToRun/mockTraceId resets, the parameterised seedPlugin(adapter), the seedEchoPlugin/invoke helpers, and the three trace tests. 15 upstream tests, 12 here, and nothing failed to say so. Restored from 9538d58 alongside this branch's createTestPluginContext rewrite of the aliases test — the two changes are independent. **close() now memoizes itself, not just the phases.** runOnce() guaranteed the teardown body ran once, but resetCoreSingletons() sat outside it, so every call reset again: `await a.close(); createApp(); await a.close()` dropped the second app's singletons. The reset is also skipped when the budget expired, because the phases are still running and still own those instances. Only reachable through the raw AppHandle — createTestApp's wrapper memoizes, which is why the harness-level test could not see it and the regression test lives in app-close.integration. **Refcount singleton ownership.** The env baseline was already refcounted so overlapping harness apps compose, while the singleton layer reset on every boot and every close — so booting B rebound A's ServiceContext and CacheManager, and closing A while B was live left B with none at all. claimCoreSingletons/ releaseCoreSingletons now follow the same model as the env baseline: first boot claims, last close drops. **Fake the on-behalf-of client.** The kit promised "no workspace, no credentials, no network", but createApp({ client }) installs only the service principal; an `obo` request reached ServiceContext.createUserContext, which builds a real SDK client from process.env.DATABRICKS_HOST. The harness now stubs that for the app's lifetime and restores it on close, mirroring fixtures.ts's createUserContextSpy. Every fix has a test verified by reintroducing the bug. Two of those tests needed a second attempt: asserting the OBO client's host does not discriminate, since a real client carries the same DATABRICKS_HOST string — the test now asserts the harness's mock recorded the call. The probe plugin gained a route that calls the client under asUser, because the existing /as-user route only reads ctx.userId, which is how this escaped notice. 4517 tests pass; typecheck, lint, and format clean. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Swept into merge 45601bf from a dirty working tree; not part of this work. npm had run in apps/dev-playground/client during the local-tarball verification and pruned the `extraneous: true` entries for ../../../packages/appkit-ui. Both parents of that merge carry the same blob, so `git log -- <path>` reports nothing for this branch even with --full-history, which is why it went unnoticed. Restored to origin/main byte-for-byte. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
…g kit createTestApp's failure path released its singleton claim twice when the boot failed after createApp resolved: app.close() already drops the claim, and the catch block released it again, which could pull the singletons out from under a still-live sibling app. It now releases only when nothing was booted. Alongside it, a simplification pass over the branch: - LifecycleManager.closeOnce: drop the timedOut flag in favour of an early return from the catch. - createTestApp: drop the restoreEnv alias for releaseEnvBaseline. - Delete createConfigurableMockWorkspaceClient. Its last caller moved to createMockWorkspaceClient earlier in this branch, so the JSDoc rationale for keeping it byte-for-byte pointed at deleted code, and the ./testing subpath it would have shipped on is new here and unreleased. - Reuse the kit's createMockRequest instead of three local mockReq helpers that re-rolled the same forwarded-identity headers. - genie.test.ts: reuse one expectStream handle rather than parsing the same captured SSE body twice. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Both were swept into the oxlint merge from a dirty working tree and corrected later on the branch; folding those corrections in here keeps them out of the follow-up PR. The knip `.claude/**` entry and the `**/.claude` ignorePatterns in oxfmt/oxlint were never needed — nothing in the repo lints or formats that directory. The `packages/appkit` vitest ignoreDependencies entry stays: vitest is a real dependency of the testing entry. apps/dev-playground/client/package-lock.json is restored to origin/main byte-for-byte; npm had run in that directory and pruned its `extraneous: true` entries. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
IamGalymzhan
requested review from
MarioCadenas
and removed request for
a team
August 19, 2026 11:29
The 3-line `CacheManager.reset()` carried 30 lines of comment and 135 lines of test. Mutation testing showed what each test was actually worth: with `reset()` stubbed to a no-op, or clearing only `instance`, four tests fail; with the generation guard removed, exactly one does. - Dropped "reset is safe when the cache was never initialized". It survived all three mutations — a body of three assignments cannot throw, so the test could only ever pass. - Replaced the hand-rolled 23-line `CacheStorage` double with a 7-line `InMemoryStorage` subclass overriding just `close()` and `set()`. It also stops claiming `isPersistent() === true`, which had the manager's probabilistic cleanup eligible to fire against ended storage. - Cut the comments to the two facts a maintainer would otherwise remove and reintroduce the bug with: both fields must clear because `getInstance()` falls back to `initPromise`, and a reset is a pointer drop so callers close first. Same treatment for `reset-singletons.ts` and `testing/reset.ts`, which were at 47% and 52% comment lines against a repo baseline of 20-35%. Test file 135 -> 115 lines, production diff +38 -> +23. Mutation coverage is unchanged, re-verified against all three mutations. 4517 tests pass; typecheck, lint, and format clean. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
60% of this branch's additions to appkit.ts were comment: 41 lines over 22 lines of code. Trimmed to 35 with no fact dropped — audited claim by claim — and two of them moved somewhere they do more good. **The onPluginsReady note was on the wrong function.** It sat on the internal `_createApp`, which typedoc does not publish, while the public `createApp` that callers actually read carried the same parameter with no explanation. Moved, and the generated API page now renders it (see the Function.createApp.md diff) where before it reached nobody. While moving it, corrected the hazard it described. It said offering `close()` there "would invite tearing down a half-booted app" — but `#lifecycle` is not assigned until after the server starts, so `close()` at that point is a *no-op*, not a teardown. The narrow type and the `#lifecycle?.` optional chain guard against silently skipping cleanup, which is the opposite failure. **RESERVED_PLUGIN_NAMES gained the reasoning for its scope.** It reserves only `close`, which reads like an oversight: `bindExportMethods` and the other prototype methods are equally shadowable, since TS `private` is compile-time only. The distinction is that shadowing those throws `TypeError` on the next plugin's registration, while shadowing `close` fails silently — you call it, get no error, and leak every socket and pool. Only silent breakage needs a guard, and that is now written down. Also dropped the comment above the LifecycleManager construction, which restated the `#lifecycle` field's own JSDoc. 4517 tests pass; typecheck, format, and docs build clean. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Mutation-tested the close-handle suites (33 tests, 8 mutations). One mutation survived: deleting the early `return` in `closeOnce`, so a timed-out close() releases the core singletons while its own phases are still running and still own those instances. That is the exact hazard the code comment warns about, and it was the review fix with no coverage. The test that sounds like it covers this — "phase 5 still closes the app's own cache and telemetry, not the next app's" — cannot: it mocks CacheManager and TelemetryManager wholesale, so whether releaseCoreSingletons() ran is invisible to it. It guards the captured-singleton half of the fix, not the early return. Closed by mocking the one symbol lifecycle-manager imports from reset-singletons and extending two existing tests, so the count stays at 24: the clean-close test now asserts one release, and the hung-teardown test asserts none. Verified in both directions — dropping the early return fails the second, removing releaseCoreSingletons() entirely fails the first. Also corrected a comment in the phase-5 test that predated the review fix. It said close() "already dropped the singletons" on timeout, which is what the fix stopped it from doing. 4517 tests pass; typecheck and format clean. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
`createMockRequest` stored header keys exactly as given while `header()`
lowercased the lookup, so a mixed-case override was unreachable:
createMockRequest({ obo: { userId: "alice" }, headers: { "X-Forwarded-User": "bob" } });
// header("x-forwarded-user") === "alice"
Both keys were kept — ["x-forwarded-access-token", "x-forwarded-user",
"X-Forwarded-User"] — and the lowercase one obo seeded still answered, which
contradicted the "an explicit override wins" contract documented right above it.
Keys are now lowercased on the way in, matching what Node's parser hands
Express. Thanks @pkosiec.
The existing override test passed because it used a lowercase key, so it is now
parametrised over both casings, and the case-insensitivity test additionally
pins that every stored key is lowercase. Reverting the fix fails both.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Same root cause as the `createMockRequest` fix on the parent branch (#530, found by @pkosiec), and worse here. `Object.assign(headers, reqOptions.headers)` kept case variants as separate keys, and `Headers` **comma-joins** duplicates rather than replacing them: new Headers({ "x-forwarded-user": "alice", "X-Forwarded-User": "bob" }) // -> x-forwarded-user: "alice, bob" So a mixed-case override did not merely lose, it corrupted the value the server received — the "caller headers last" contract broken in a way that produces a plausible-looking string rather than an error. Keys are lowercased before assignment. The existing `/headers` table gained a mixed-case row rather than a new test; reverting the fix fails it. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
# Conflicts: # docs/docs/plugins/testing.md # packages/appkit/src/plugins/agents/tests/dispatch-tool-call.test.ts # packages/appkit/src/plugins/agents/tests/route-handler-errors.test.ts # packages/appkit/src/plugins/analytics/tests/analytics.test.ts # packages/appkit/src/plugins/genie/tests/genie.test.ts # packages/appkit/src/testing/fixtures.ts # packages/appkit/src/testing/index.ts # packages/appkit/src/testing/tests/test-plugin-context.test.ts # template/server/example.test.ts # tools/test-helpers.ts
Contributor
📦 Bundle size reportCompared against
|
| dist | raw | gzip |
|---|---|---|
| JS (runtime) | 914 KB (+21 KB) | 320 KB (+8.9 KB) |
| Type declarations | 348 KB (+7.9 KB) | 122 KB (+3.9 KB) |
| Source maps | 1.8 MB (+43 KB) | 603 KB (+17 KB) |
| Other | 11 KB | 3.7 KB |
| Total | 3.0 MB (+72 KB) | 1.0 MB (+29 KB) |
Per-entry composition (own code — deps external (as shipped))
| Entry | Initial (gz) | Lazy (gz) | Total (gz) | node_modules (min) | Own code (min) |
|---|---|---|---|---|---|
. |
89 KB (+426 B) | 2.5 KB | 91 KB (+426 B) | external | 290 KB (+1.5 KB) |
./beta |
49 KB (+43 B) | 457 B | 49 KB (+43 B) | external | 143 KB (+168 B) |
./testing |
33 KB (+16 KB) | 29 KB (+29 KB) | 61 KB (+45 KB) | external | 175 KB (+125 KB) |
./type-generator |
21 KB | 0 B | 21 KB | external | 61 KB |
Chunks:
| Entry | Chunk | Load | Size (gz) |
|---|---|---|---|
. |
index.js |
initial | 85 KB |
. |
utils.js |
initial | 4.0 KB |
. |
remote-tunnel-manager.js |
lazy | 2.5 KB |
./beta |
beta.js |
initial | 33 KB |
./beta |
stream-manager.js |
initial | 5.8 KB |
./beta |
wide-event-emitter.js |
initial | 3.2 KB |
./beta |
databricks.js |
initial | 3.0 KB |
./beta |
configuration.js |
initial | 2.1 KB |
./beta |
service-context.js |
initial | 1.3 KB |
./beta |
client.js |
initial | 434 B |
./beta |
client-options.js |
initial | 220 B |
./beta |
supervisor-api.js |
lazy | 192 B |
./beta |
databricks.js |
lazy | 142 B |
./beta |
index.js |
lazy | 123 B |
./testing |
stream-manager.js |
initial | 18 KB |
./testing |
index.js |
initial | 12 KB |
./testing |
wide-event-emitter.js |
initial | 2.9 KB |
./testing |
index.js |
lazy | 25 KB |
./testing |
remote-tunnel-manager.js |
lazy | 2.5 KB |
./testing |
utils.js |
lazy | 1.2 KB |
./type-generator |
index.js |
initial | 21 KB |
@databricks/appkit-ui
npm tarball (packed): 342 KB — gzipped download (dist + bin; excludes release-only docs/NOTICE).
| dist | raw | gzip |
|---|---|---|
| JS (runtime) | 390 KB | 130 KB |
| Type declarations | 228 KB | 83 KB |
| Source maps | 752 KB | 247 KB (+1 B) |
| CSS | 16 KB | 3.2 KB |
| Total | 1.4 MB | 464 KB (+1 B) |
Per-entry composition (consumer bundle — deps bundled, peerDeps external)
| Entry | Initial (gz) | Lazy (gz) | Total (gz) | node_modules (min) | Own code (min) |
|---|---|---|---|---|---|
./js |
5.3 KB | 49 KB | 55 KB | 208 KB | 14 KB |
./js/beta |
20 B | 0 B | 20 B | 0 B | 0 B |
./react |
432 KB | 49 KB | 480 KB | 1.3 MB | 175 KB |
./react/beta |
1.0 KB | 0 B | 1.0 KB | 0 B | 1.9 KB |
Chunks:
| Entry | Chunk | Load | Size (gz) |
|---|---|---|---|
./js |
index.js |
initial | 5.2 KB |
./js |
chunk |
initial | 120 B |
./js |
apache-arrow |
lazy | 49 KB |
./js/beta |
beta.js |
initial | 20 B |
./react |
index.js |
initial | 430 KB |
./react |
tslib |
initial | 2.1 KB |
./react |
apache-arrow |
lazy | 49 KB |
./react/beta |
beta.js |
initial | 1.0 KB |
Contributor
🤖 AppKit PR bot🔬 Run evalsStart an eval for this PR from the evals-monitor app: Go to Evals Monitor → 📦 Try this PR's app templateScaffolds a new app from this PR's SDK build. Run it in any folder (requires the GitHub CLI — gh run download 32368655535 -R databricks/appkit -n appkit-template-0.62.0-pr.35f78e4-feat-testing-kit-harness-540 -D appkit-pr-540 \
&& unzip -o "appkit-pr-540/appkit-template-0.62.0-pr.35f78e4-feat-testing-kit-harness-540.zip" -d "appkit-pr-540" \
&& databricks apps init --template "appkit-pr-540"The template pins |
…seline The bundle-size gate is the last red check on this PR. The growth is the feature, not slack: the tarball ships no test files at all, so deleting eight tests moved the measured size by exactly 0 bytes. Comment trimming did help, taking it from +8.1% to +6.9%, because JSDoc survives into the `.d.ts`. `@databricks/appkit` packed 860106 -> 919127 (+58 KB), which is the `./testing` entry: 61 KB gz of harness plus its type declarations. `@databricks/appkit-ui` moves -291 bytes, incidentally. Regenerated with `pnpm size:baseline` from a clean build — `rm -rf packages/*/dist packages/*/tmp` first, because tsdown runs with `clean: false` and a stale artifact would be baked into the baseline as real growth. `pnpm size:compare` now reports no change and exits 0. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
…n the baseline" This reverts commit 04839f0.
pkosiec
reviewed
Aug 20, 2026
| // Returning early leaves the singletons in place: the phases are still | ||
| // running and still own these instances, so dropping the pointers now | ||
| // would hand the next boot a half-released app. | ||
| return; |
Member
There was a problem hiding this comment.
P2 — On a close() timeout we skip releaseCoreSingletons() on purpose (phases still run) — but nothing releases it afterward either.
Why it matters: the owner count never returns to 0, so the next createTestApp() skips its reset and reuses the old app's half-closed singletons. Later tests in the file then break silently.
Fix: on timeout, attach .finally(() => releaseCoreSingletons()) to the still-running teardown so release runs once it finishes.
| // No release alongside this: close() drops the claim itself, and a second | ||
| // release would pull the singletons out from under a live sibling app. | ||
| try { | ||
| await app.close(); |
Member
There was a problem hiding this comment.
P2 — Same leak on the boot-failure path: if this close() times out, the singleton count is never released.
Why it matters: a later boot reuses stale singletons, so tests pass or fail for the wrong reason.
Fix: release the count once teardown settles (same as the timeout path above).
| } catch (err) { | ||
| // Teardown must run from the failure path too, or the boot leaks env | ||
| // mutations and singletons into every later test in the file. | ||
| if (app) { |
Member
There was a problem hiding this comment.
P3 — Only the "app was never built" branch is tested. This if (app) branch — app built, then a later step throws — is not.
Why it matters: the cleanup after a successful createApp is untested, so a leak here would go unnoticed.
Fix: add a test where a step after createApp throws, then assert env + singletons still restore.
| export { | ||
| createTestApp, | ||
| type CreateTestAppOptions, | ||
| getListeningPort, |
Member
There was a problem hiding this comment.
P1 — getListeningPort is marked @internal (see its definition) but exported here, so it ships as public API in the .d.ts.
Why it matters: the tag says "don't use me" while the export invites it. Someone depends on it, then a cleanup that trusts the tag breaks them. It hasn't shipped yet, so now is the free window.
Fix: drop it from the barrel (deep-import it in the @tools/test-helpers shim), or make it real public API and remove the @internal tag.
| * | ||
| * Mirrors the `createUserContextSpy` in `fixtures.ts`; returns its restore. | ||
| */ | ||
| function stubUserContext(client: WorkspaceClient): () => void { |
Member
There was a problem hiding this comment.
P2 — This stubUserContext and fixtures' createUserContextSpy fake the same thing but disagree — and neither matches the real ServiceContext.createUserContext. Real + this stub throw on a missing token and set tokenFingerprint/userEmail; the fixtures spy does neither.
Why it matters: tests using mockServiceContext miss missing-token bugs, and code that reads tokenFingerprint (e.g. Lakebase pool rotation) takes a different path than under createTestApp or in production. Same test, different result depending on the helper.
Fix: make both fakes agree with each other and the real shape, or share one builder. (Also: this stub's test-${userId} isn't the real sha256 fingerprint.)
| - `config.host` is a real **string** (not a mock), because AppKit builds URLs from it. `apiClient.userAgent()` is synchronous for the same reason, and `apiClient.request` resolves `{}` so destructuring its result doesn't throw. | ||
| - Sensible defaults are built in: SQL statements succeed, warehouses report `RUNNING`, and `currentUser.me()` returns a service user. Pass `defaults: false` to script everything yourself. | ||
|
|
||
| :::caution The honest catch |
Member
There was a problem hiding this comment.
P1 — The admonition title :::caution The honest catch reads as AI-generated — a cute label, not a descriptive one. Prefer a plain title, e.g. :::caution Undeclared methods return undefined.
Why it matters: the docs are the kit's UX; a florid label undercuts trust and makes the page harder to scan.
Fix: retitle it. Also, while here: the frontmatter sidebar_position: 8 collides with jobs.md and manifest.md (both 8), so the sidebar order is unstable — give it a unique position.
|
|
||
| // Boot runs ServiceContext.createContext for real, which reads | ||
| // currentUser.id — the mock's built-in default is what lets it through. | ||
| const client = suppliedClient ?? createMockWorkspaceClient({ responses }); |
Member
There was a problem hiding this comment.
P3 — responses only feeds the built-in mock (suppliedClient ?? createMockWorkspaceClient({ responses })). Passing both client and responses silently drops the responses (your passed client is used as-is — its own responses are untouched, but the top-level ones do nothing).
Why it matters: a silent no-op invites "why isn't my response taking effect?" The kit already throws on a similar conflict (server: false + a server plugin), so the same guard fits.
Fix: throw when both client and responses are passed, telling the caller to configure responses on their own client.
| * Responses keyed by dotted path (`"jobs.getRun"`). A function value is called | ||
| * with the arguments, so a test can script behaviour or reject. | ||
| */ | ||
| responses?: Record<string, Any>; |
Member
There was a problem hiding this comment.
P3 — Experiment worth a spike: type the responses keys instead of Record<string, any>. A template-literal union over the facade (`${service}.${method}`) would give autocomplete on the dotted paths and turn a typo'd key into a compile error.
Why it matters: it kills the whole stringly-typed seam (a typo'd key silently does nothing today), and the same idea helps the getMock path arg. Biggest DX win available on the mock.
Fix: prototype and measure — mapped/conditional types can hurt IDE perf and produce ugly errors, and it partly fights the "undeclared -> undefined" design, so gate it behind the experiment before committing.
| @@ -10,14 +10,129 @@ AppKit ships a testing kit at `@databricks/appkit/testing` so you can test a plu | |||
|
|
|||
| Exercise a plugin's real code paths — route registration, cross-plugin tool dispatch, user-scoped (on-behalf-of) execution, and per-call timeouts — against a real `PluginContext` with only its outer edges faked. Nothing about the context is reimplemented, so a test can't drift from production behavior. | |||
Member
There was a problem hiding this comment.
P1 — Do a deslop pass over this guide (and the kit's JSDoc). "The honest catch" is the clearest tell, but the AI-generated register shows up elsewhere too.
Why it matters: the docs are the kit's first impression; the florid voice reads as machine-written and undercuts trust.
Fix: run the repo's deslop skill over testing.md and the testing-kit JSDoc, then a human read for plain, direct phrasing.
| * exporters leak. `app.close()` does both, so this is only for tests that | ||
| * hand-roll `createApp`. | ||
| */ | ||
| export function resetAppKitSingletons(): void { |
Member
There was a problem hiding this comment.
P1 — Rename resetAppKitSingletons — "AppKit" is redundant inside @databricks/appkit/testing, and "Singletons" leaks the implementation.
Why it matters: the name should say what it does, not how it's built. vitest's own global resets are resetAllMocks/resetModules/unstubAllGlobals — always reset<Scope> with a plain scope noun, never product-prefixed. This symbol is new in this PR (not in 0.62.0), so a rename is still free.
Fix: rename to resetGlobalState(). Avoid dispose* — it drops pointers, it doesn't release resources. Update the doc reference too.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
@databricks/appkit/testing