feat(appkit): add createTestApp, a never-crash mock client, and app.close() - #540
feat(appkit): add createTestApp, a never-crash mock client, and app.close()#540IamGalymzhan wants to merge 63 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 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
📦 Bundle size reportCompared against
|
| dist | raw | gzip |
|---|---|---|
| JS (runtime) | 959 KB (+53 KB) | 335 KB (+18 KB) |
| Type declarations | 354 KB (+10 KB) | 125 KB (+4.8 KB) |
| Source maps | 1.9 MB (+107 KB) | 629 KB (+33 KB) |
| Other | 11 KB | 3.7 KB |
| Total | 3.2 MB (+170 KB) | 1.1 MB (+56 KB) |
Per-entry composition (own code — deps external (as shipped))
| Entry | Initial (gz) | Lazy (gz) | Total (gz) | node_modules (min) | Own code (min) |
|---|---|---|---|---|---|
. |
94 KB (+5.3 KB) | 2.5 KB | 96 KB (+5.3 KB) | external | 307 KB (+19 KB) |
./beta |
55 KB (+5.0 KB) | 457 B | 56 KB (+5.0 KB) | external | 166 KB (+19 KB) |
./testing |
38 KB (+21 KB) | 29 KB (+29 KB) | 66 KB (+50 KB) | external | 193 KB (+143 KB) |
./tsdown |
520 B | 0 B | 520 B | external | 813 B |
./type-generator |
21 KB | 0 B | 21 KB | external | 61 KB |
Chunks:
| Entry | Chunk | Load | Size (gz) |
|---|---|---|---|
. |
index.js |
initial | 90 KB |
. |
utils.js |
initial | 4.0 KB |
. |
remote-tunnel-manager.js |
lazy | 2.5 KB |
./beta |
beta.js |
initial | 39 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 |
manifest.js |
initial | 25 KB |
./testing |
index.js |
initial | 9.7 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 |
./tsdown |
index.js |
initial | 520 B |
./type-generator |
index.js |
initial | 21 KB |
@databricks/appkit-ui
npm tarball (packed): 348 KB (+5 B) — gzipped download (dist + bin; excludes release-only docs/NOTICE).
| dist | raw | gzip |
|---|---|---|
| JS (runtime) | 394 KB | 132 KB |
| Type declarations | 228 KB (+32 B) | 83 KB (+8 B) |
| Source maps | 764 KB | 252 KB (+1 B) |
| CSS | 16 KB | 3.2 KB |
| Total | 1.4 MB (+32 B) | 471 KB (+9 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 | 481 KB | 1.3 MB | 177 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 |
⚠️ Over budget: a package's shipped tarball, or a browser entry's consumer bundle (deps included), grew by more than 5% (and >10 KB). This check will fail — reduce the size, or acknowledge the increase by updatingbundle-size-baseline.json.
🤖 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 32739683359 -R databricks/appkit -n appkit-template-0.64.0-pr.2ee3070-feat-testing-kit-harness-540 -D appkit-pr-540 \
&& unzip -o "appkit-pr-540/appkit-template-0.64.0-pr.2ee3070-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.
| * 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>; |
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.
…ards
Nine of the seventeen review comments, the ones with no open decision.
**The singleton leak (P2, both instances).** On a `close()` timeout the release
was skipped deliberately — the phases still own those instances — but nothing
released it afterward either, so the refcount never returned to zero and every
later boot skipped its reset and inherited a half-closed app. Now the release is
attached to the still-running teardown, so it happens once the phases settle.
This also fixes the boot-failure report: that path calls `app.close()`, so it
routed through the same gap.
My own test was pinning the bug. It asserted `releaseCoreSingletons` was never
called on the timeout path, when the contract is "not yet, but once teardown
settles". Corrected, and verified by reverting the fix.
**Renames, while the pre-release window is open** (both symbols are absent from
0.62.0, so this is free now and breaking later): `getMockFn` -> `getMock`, since
it returns vitest's `Mock`; `resetAppKitSingletons` -> `resetGlobalState`, since
vitest's own resets are `reset<Scope>` with no product prefix.
**`getListeningPort` was `@internal` and exported**, which is a contradiction
either way. Made public: three integration suites already consume it through the
public entry, and it is a reasonable helper for anyone hand-rolling a server.
**The reserved-name list is now exhaustive by construction.** It was a
hand-kept `Set(["close"])` with nothing tying it to `AppHandle`, so a future
named method there would be silently shadowable by a plugin. A
`Record<Exclude<keyof AppHandle, keyof PluginMap | symbol>, true>` makes adding
one a compile error until it is reserved — verified by adding a `restart()` and
watching tsc fail.
**Passing both `client` and `responses` now throws** instead of silently
ignoring the responses, matching the existing `server: false` conflict.
**The two proxy traps share one guard.** `toLegacyWorkspaceClient` had copied
the symbol + PASSTHROUGH_DENY logic, so a key added to the set would have
covered one trap and missed the other — and missing `then` there is what makes
`await client` hang.
**Coverage gap closed:** the boot-failure branch that runs *after* `createApp`
succeeded had no test. Added one via a plugin that boots and then throws when
the harness reaches for its socket.
**Comment de-fragilised:** the canned-defaults note named a count ("13 suites")
and a position ("the first three"). It now names the three entries.
4521 tests pass; typecheck, lint, format, and docs build clean.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Half of Pawel's P2 on the diverging on-behalf-of fakes. The other half is
deferred on semver grounds, recorded in internal/testing-kit/README.md.
`stubUserContext` set `tokenFingerprint: test-${userId}` — keyed on the *user*,
so constant across tokens. Lakebase rotates its pool by comparing that value
(`pool-manager.ts:102`), which means rotation could never fire under
`createTestApp` while production rotates on every new token. A test exercising
pool rotation through the harness would have passed against a code path that
cannot run. It now derives `sha256(token)[0:16]`, exactly as the real
`createUserContext` does.
The rejection also matched only in spirit: a bare `Error` where production throws
`AuthenticationError.missingToken`. A handler that catches that class took a
different branch under the harness. Now the same class.
Two tests, both verified by reintroducing the old behaviour: the fingerprint is
stable per token and differs across tokens (a user-keyed value fails it), and the
missing-token rejection is asserted on the error class, not the message — a plain
`Error` whose message still says "token" fails it.
Not touched: `createUserContextSpy` in fixtures.ts, which neither throws nor sets
the field. It shipped in 0.62.0, so tightening it would break consumers' existing
tests on a minor upgrade; it needs its own PR and a changelog line. The end state
is one shared builder, worth extracting once that half is free to move.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
# Conflicts: # packages/appkit/src/plugins/agents/tests/route-handler-errors.test.ts
Completes Pawel's P2. Three fakes of `ServiceContext.createUserContext` disagreed and none matched production; they now share `fakeUserContext` in fixtures.ts — production's rejection (`AuthenticationError.missingToken`), production's `sha256(token)[0:16]` fingerprint, and `userEmail`. What the divergence cost: `pool-manager.ts:101` treats a missing `tokenFingerprint` as "not stale", so the old fixtures spy — which discarded the token outright — made Lakebase's drain-and-recreate branch unreachable. A test written to verify pool rotation would have passed while exercising nothing. I deferred this half earlier on semver grounds, arguing that `mockServiceContext` shipped in 0.62.0 so tightening a published test double would break consumers on a minor upgrade. Wrong: `./testing` was not published at all before 0.62.0 (`publishConfig.exports` lists no `./testing` in 0.61.0 or 0.61.1), and 0.62.0 landed four days ago with two releases since. There is no population to break. Checking the API's actual age beats reasoning about semver in the abstract. Harmless in-repo too — the full suite passed with the fake tightened, so nothing here relied on the looseness either. Six tests pin it, three per fake, all verified by restoring the loose version: the fingerprint is stable per token and differs across tokens, a missing token throws production's error class, and userEmail is carried through. 4579 tests pass; typecheck, lint and format clean. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Pawel's P1 on the guide's voice, plus the two smaller docs points. **Voice.** Ran the repo's doc-deslop pass, then a targeted follow-up. No vocabulary slop or hedging to remove — the divergence from sibling pages was punctuation and register: paired em-dashes injecting lists mid-sentence (the textbook tell), British `behaviour` against the siblings' American spelling, and cutesy editorial phrasing. `:::caution The honest catch` is now `:::caution Undeclared methods return undefined`, matching how siblings title admonitions. Worth recording how the "how florid is it" question actually resolved, because my first two measurements were wrong. Raw em-dash counts said this page was 2.7x denser than `analytics.md`. But that counted comments inside code blocks and the `**term** — definition` bullet pattern the siblings use too. Excluding both: 0.286 dashes per prose line here against 0.252 in `analytics.md`, its closest sibling by length and depth. The page was already at parity, so the remaining dashes were left alone rather than purged below the house norm. Four where a dash was doing a colon's job are now colons. **Teardown now leads with `await using`.** It previously said "use try/finally, or let the runtime do it" and then showed only the `await using` example — naming the weaker option first and never demonstrating it. `await using` comes first with the reason (it closes on a thrown error too), and `try/finally` follows as the form you need when the app outlives a block. Both are supported: TypeScript 5.9.3, Node 24, ES2022, and three suites already rely on it. **Sidebar.** `sidebar_position: 8` collided with `jobs.md` and `manifest.md`; `_category_.json` uses `autogenerated`, so frontmatter really does drive order. Moved to 10, which is unused. The pairs at 2, 5, 6, 7, 8 and 9 predate this work and are left for a separate tidy-up. Also documents two things the code now does but the page didn't say: passing `client` and `responses` together is refused rather than ignored, and `getListeningPort` is public API. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Unrelated to the testing kit. `styles.gen.css` is generated by the docs build and main's committed copy is stale: it lacks the `.collapse` utility. Any `pnpm docs:build` reproduces this diff, so it otherwise sits dirty in every working tree. Verified deterministic — reverted, rebuilt, and it came back. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Pawel's P3 on undeclared paths resolving `undefined`. I first declined this as a
feature deserving its own PR; the actual change is eight lines of production code
and additive, so that was over-caution.
`createMockWorkspaceClient({ strict: true })` throws when a path with no declared
response is *called*, naming the path. Off by default — the never-crash floor is
what lets a plugin touch services a test does not care about — so nothing about
existing behaviour moves.
Three details that needed care:
- **Throws on call, not on mint.** `getMock(client, path)` mints so a test can
hold a handle before the code under test runs; blowing up there would break the
normal assertion pattern. Pinned by a test.
- **The canned defaults count as declared**, so a harness boot still works — it
reads `currentUser.me` through this client. With `defaults: false` they are
undeclared again, which is also pinned.
- **Plumbed through `createTestApp`.** Without that the option was unreachable
from the recommended entry point: the harness builds its own mock, and passing
a hand-built client now refuses `responses`. The conflict guard covers `strict`
too.
This does not overlap with the typed-`responses`-keys idea, contrary to the
review's framing that they are two answers to the same problem: typed keys catch
a *typo* at compile time, `strict` catches an *omission* at runtime. Typed keys
cannot know you forgot to declare a path you actually call.
Six tests, and the guide documents it beside the caution that describes the
failure mode. 4585 tests pass; typecheck, lint, format and docs build clean.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
pkosiec
left a comment
pkosiec
left a comment
There was a problem hiding this comment.
Thanks for fixing previous comments! Here's another pass with a couple more comments:
| * | ||
| * Mirrors the `createUserContextSpy` in `fixtures.ts`; returns its restore. | ||
| */ | ||
| function stubUserContext(client: WorkspaceClient): () => void { |
There was a problem hiding this comment.
P1 — If two test apps are open at the same time, closing one breaks the fake login for the other. Every createTestApp installs one shared spy on ServiceContext.createUserContext. close() calls spy.mockRestore(), which puts back the real function — not the other app's spy. So after one app closes, the other app's OBO calls run the real Databricks SDK and try to reach the network.
Why it matters: the kit promises "no workspace, no network." This quietly breaks it. The two tests that open two apps at once (create-test-app.test.ts:223 and :540) only check ports and env, so they never catch it.
Fix: make the harness one app at a time — throw if a second app boots while one is still open (see the related comment on app.client). That makes this impossible.
|
|
||
| // 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 }); |
There was a problem hiding this comment.
P1 — If a second app opens while the first is still open, the second app's client and responses are silently ignored. When one app is already live the shared state isn't rebuilt, so ServiceContext.initialize hands back the first app's instance (service-context.ts:56) and CacheManager.getInstance does the same (cache/index.ts:124). The second app's handlers end up using the first app's client. But app.client still returns the second mock, so getMock(app.client, "jobs.getRun") shows 0 calls even though the handler did call it — on the other client.
Why it matters: the docs say app.client is exactly what the handler uses (testing.md:78). That isn't true after the first app, so a test can pass or fail for the wrong reason.
Fix: allow only one app at a time. Add a module-level harnessAppLive flag (one per test file), throw a clear error at the top of createTestApp if it's already set, and clear it when the app closes and on the boot-failure paths. Change the two concurrent-app tests (create-test-app.test.ts:223 and :540) to open apps one after another, and add one that checks a second open throws. Also document that the data plane is shared (see the docs comment).
| // app. But release once they settle, or the refcount never returns to | ||
| // zero and every later boot skips its reset. | ||
| void this.runOnce() | ||
| .finally(() => releaseCoreSingletons()) |
There was a problem hiding this comment.
P2 — On a close timeout, the cleanup is put off: void this.runOnce().finally(() => releaseCoreSingletons()). So close() returns while the app still holds its slot. If the next app opens before the old cleanup finishes, it reuses the closing app's shared state — then the old cleanup reaches phase 5, closes the shared InMemoryStorage, and wipes the new app's cache mid-test.
Why it matters: one slow close quietly corrupts a later test in the same file — the exact bug the refcount is meant to stop.
Fix: release right away on timeout, like the normal path (releaseCoreSingletons(); return;). It's safe: runPhases already saved its own copies of the cache and telemetry (capturedCache/capturedTelemetry) and never reads the shared state again, so the background cleanup doesn't need it. It's also less code. Update the comment and the test that checks the delayed release.
| @@ -0,0 +1,90 @@ | |||
| import { | |||
There was a problem hiding this comment.
P2 — This is the only CI test that imports from @databricks/appkit/testing, but it only pulls in createTestApp, expectStream, and getMock. Everything else — the main createTestPluginContext, all the fixtures, all the types — could be deleted from index.ts and this test would still pass.
Why it matters: the name says it guards the public surface, but a headline export could quietly disappear from the package with CI still green. That's false confidence.
Fix: do import * as testing from "@databricks/appkit/testing" and check each expected name is there, or at least call createTestPluginContext through the entry.
| @@ -1,5 +1,5 @@ | |||
| import { Plugin, type PluginManifest } from '@databricks/appkit'; | |||
| import { expectStream, createTestPluginContext } from '@databricks/appkit/testing'; | |||
| import { Plugin, type PluginManifest, toPlugin } from '@databricks/appkit'; | |||
There was a problem hiding this comment.
P3 — No CI runs this file. template/ isn't in pnpm-workspace.yaml, the root vitest config skips **/template/**, and the CI template job only checks that manifests and deps are in sync — it never runs the test.
Why it matters: the example can quietly rot. If the kit's API changes, nothing fails, and someone who scaffolds a new app gets a broken example. Together with the published-surface test above, nothing checks that createTestPluginContext stays exported.
Fix: either run the scaffolded template's vitest run in CI after npm install, or move a createTestPluginContext-through-the-entry check into the published-surface test.
| import { createMockWorkspaceClient } from "./mock-workspace-client"; | ||
| import { claimAppKitSingletons, releaseAppKitSingletons } from "./reset"; | ||
|
|
||
| type Any = any; |
There was a problem hiding this comment.
P3 — type Any = any is copied into three kit files, but only fixtures.ts explains why (repo-wide noExplicitAny is off, so the alias flags the intent). The other two are bare.
Why it matters: someone reading the bare Any has no idea what it means or why it isn't just any.
Fix: put the alias (with its comment) in one shared kit file and import it, or just use any.
|
|
||
| // `host` is read as a string and throws if falsy, so it cannot be a mock. | ||
| const configTarget: Record<string, Any> = { | ||
| host: "https://test.databricks.com", |
There was a problem hiding this comment.
P3 — Seeding an apiClient.* value through responses wraps it in vi.fn().mockResolvedValue(value), so responses: { "apiClient.userAgent": "x" } returns a Promise. That's the very thing the sync default avoids — a Promise turns into "[object Promise]" inside a Header. The config.* path sets values directly and doesn't have this problem, so the two paths behave differently.
Why it matters: rare input, but a wrong header value is hard to track down.
Fix: for apiClient.* seeds, keep a plain value synchronous like the config.* path does, or note that only async members can be seeded this way.
| export { | ||
| createMockWorkspaceClient, | ||
| type CreateMockWorkspaceClientOptions, | ||
| getMock, |
There was a problem hiding this comment.
P3 — The kit mixes two words for test doubles: helpers say "Mock" (createMockWorkspaceClient, mockServiceContext) but these types say "Fake" (FakeProvider, FakeProviders, FakeToolResponse). Fake* shipped in 0.64.0, so renaming would break people.
Why it matters: two words for the same idea makes a user wonder if they differ, and searching for "mock" misses the Fake types.
Fix: add one doc line on the difference (a fake stands in and works; a mock records calls), or export Mock* aliases alongside. Don't rename.
| // Nothing was booted, so nothing else will drop the claim taken above. | ||
| releaseAppKitSingletons(); | ||
| } | ||
| restoreUserContext?.(); |
There was a problem hiding this comment.
P3 — The failure-path cleanup calls plain await app.close(), which ignores the closeTimeoutMs the caller passed — the normal close path does pass it through.
Why it matters: a slow shutdown on the failure path can run past the timeout the caller set.
Fix: pass the same closeTimeoutMs here.
| bootPlugins.push(serverPlugin({ port: 0, host: "127.0.0.1" })); | ||
| } | ||
|
|
||
| // Both extras are load-bearing: without explicit storage the cache builds its |
There was a problem hiding this comment.
P3 — The new kit code is densely commented (about a quarter to a third of the lines), and many comments are written like a story. A few examples: "load-bearing" (here and mock-workspace-client.ts:66), "pull the singletons out from under a still-live sibling app" (create-test-app.ts:314 and :398), "broke the second app" (lifecycle-manager.ts:144), "hand the next boot a half-released app" (lifecycle-manager.ts:166).
Why it matters: round 1 desloped the docs for the same reason. Much of this "why" is worth keeping — it explains real, non-obvious singleton and proxy behavior — but the story voice and the length make the code hard to skim.
Fix: do a deslop pass on the kit's code comments, like the docs pass. Keep the load-bearing why, cut it to one plain line, and drop any comment that just restates the code.
pkosiec
left a comment
pkosiec
left a comment
There was a problem hiding this comment.
Thanks for fixing previous comments! Here's another pass with a couple more comments:
| * | ||
| * Mirrors the `createUserContextSpy` in `fixtures.ts`; returns its restore. | ||
| */ | ||
| function stubUserContext(client: WorkspaceClient): () => void { |
There was a problem hiding this comment.
P1 — If two test apps are open at the same time, closing one breaks the fake login for the other. Every createTestApp installs one shared spy on ServiceContext.createUserContext. close() calls spy.mockRestore(), which puts back the real function — not the other app's spy. So after one app closes, the other app's OBO calls run the real Databricks SDK and try to reach the network.
Why it matters: the kit promises "no workspace, no network." This quietly breaks it. The two tests that open two apps at once (create-test-app.test.ts:223 and :540) only check ports and env, so they never catch it.
Fix: make the harness one app at a time — throw if a second app boots while one is still open (see the related comment on app.client). That makes this impossible.
|
|
||
| // 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 }); |
There was a problem hiding this comment.
P1 — If a second app opens while the first is still open, the second app's client and responses are silently ignored. When one app is already live the shared state isn't rebuilt, so ServiceContext.initialize hands back the first app's instance (service-context.ts:56) and CacheManager.getInstance does the same (cache/index.ts:124). The second app's handlers end up using the first app's client. But app.client still returns the second mock, so getMock(app.client, "jobs.getRun") shows 0 calls even though the handler did call it — on the other client.
Why it matters: the docs say app.client is exactly what the handler uses (testing.md:78). That isn't true after the first app, so a test can pass or fail for the wrong reason.
Fix: allow only one app at a time. Add a module-level harnessAppLive flag (one per test file), throw a clear error at the top of createTestApp if it's already set, and clear it when the app closes and on the boot-failure paths. Change the two concurrent-app tests (create-test-app.test.ts:223 and :540) to open apps one after another, and add one that checks a second open throws. Also document that the data plane is shared (see the docs comment).
| // app. But release once they settle, or the refcount never returns to | ||
| // zero and every later boot skips its reset. | ||
| void this.runOnce() | ||
| .finally(() => releaseCoreSingletons()) |
There was a problem hiding this comment.
P2 — On a close timeout, the cleanup is put off: void this.runOnce().finally(() => releaseCoreSingletons()). So close() returns while the app still holds its slot. If the next app opens before the old cleanup finishes, it reuses the closing app's shared state — then the old cleanup reaches phase 5, closes the shared InMemoryStorage, and wipes the new app's cache mid-test.
Why it matters: one slow close quietly corrupts a later test in the same file — the exact bug the refcount is meant to stop.
Fix: release right away on timeout, like the normal path (releaseCoreSingletons(); return;). It's safe: runPhases already saved its own copies of the cache and telemetry (capturedCache/capturedTelemetry) and never reads the shared state again, so the background cleanup doesn't need it. It's also less code. Update the comment and the test that checks the delayed release.
| @@ -0,0 +1,90 @@ | |||
| import { | |||
There was a problem hiding this comment.
P2 — This is the only CI test that imports from @databricks/appkit/testing, but it only pulls in createTestApp, expectStream, and getMock. Everything else — the main createTestPluginContext, all the fixtures, all the types — could be deleted from index.ts and this test would still pass.
Why it matters: the name says it guards the public surface, but a headline export could quietly disappear from the package with CI still green. That's false confidence.
Fix: do import * as testing from "@databricks/appkit/testing" and check each expected name is there, or at least call createTestPluginContext through the entry.
| @@ -1,5 +1,5 @@ | |||
| import { Plugin, type PluginManifest } from '@databricks/appkit'; | |||
| import { expectStream, createTestPluginContext } from '@databricks/appkit/testing'; | |||
| import { Plugin, type PluginManifest, toPlugin } from '@databricks/appkit'; | |||
There was a problem hiding this comment.
P3 — No CI runs this file. template/ isn't in pnpm-workspace.yaml, the root vitest config skips **/template/**, and the CI template job only checks that manifests and deps are in sync — it never runs the test.
Why it matters: the example can quietly rot. If the kit's API changes, nothing fails, and someone who scaffolds a new app gets a broken example. Together with the published-surface test above, nothing checks that createTestPluginContext stays exported.
Fix: either run the scaffolded template's vitest run in CI after npm install, or move a createTestPluginContext-through-the-entry check into the published-surface test.
|
|
||
| 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. | ||
|
|
||
| `app.client` is the very object your handler resolves at runtime — reached inside a plugin via `getExecutionContext().client` — so you can assert calls on it: |
There was a problem hiding this comment.
P3 — This says app.client "is the very object your handler resolves at runtime," and only later (line 187) says the cache is shared across the process. The client and the OBO createUserContext spy are shared the same way, but that isn't said.
Why it matters: that missing half is exactly what makes the two concurrency issues above surprising.
Fix: extend the "shared across the process" note to include the client and the OBO spy, and limit the app.client line to the one open app.
| }, | ||
| }; | ||
|
|
||
| /** The seven generically-proxied services; `config`/`apiClient` are seeded below. */ |
There was a problem hiding this comment.
P3 — "The seven generically-proxied services" writes a count that goes stale the moment the list changes. Same in testing.md:317 — "AppKit owns that 9-member interface." The exact number is easy to get wrong (9 or 10, depending on whether you count toLegacyWorkspaceClient), and it drifts if the interface changes.
Why it matters: a number in a comment is wrong as soon as the code changes, and misleads the next reader. The point ("it's a closed set, so a typo is a compile error") holds without it.
Fix: describe the rule without the count.
| import { createMockWorkspaceClient } from "./mock-workspace-client"; | ||
| import { claimAppKitSingletons, releaseAppKitSingletons } from "./reset"; | ||
|
|
||
| type Any = any; |
There was a problem hiding this comment.
P3 — type Any = any is copied into three kit files, but only fixtures.ts explains why (repo-wide noExplicitAny is off, so the alias flags the intent). The other two are bare.
Why it matters: someone reading the bare Any has no idea what it means or why it isn't just any.
Fix: put the alias (with its comment) in one shared kit file and import it, or just use any.
| export { | ||
| createMockWorkspaceClient, | ||
| type CreateMockWorkspaceClientOptions, | ||
| getMock, |
There was a problem hiding this comment.
P3 — The kit mixes two words for test doubles: helpers say "Mock" (createMockWorkspaceClient, mockServiceContext) but these types say "Fake" (FakeProvider, FakeProviders, FakeToolResponse). Fake* shipped in 0.64.0, so renaming would break people.
Why it matters: two words for the same idea makes a user wonder if they differ, and searching for "mock" misses the Fake types.
Fix: add one doc line on the difference (a fake stands in and works; a mock records calls), or export Mock* aliases alongside. Don't rename.
| bootPlugins.push(serverPlugin({ port: 0, host: "127.0.0.1" })); | ||
| } | ||
|
|
||
| // Both extras are load-bearing: without explicit storage the cache builds its |
There was a problem hiding this comment.
P3 — The new kit code is densely commented (about a quarter to a third of the lines), and many comments are written like a story. A few examples: "load-bearing" (here and mock-workspace-client.ts:66), "pull the singletons out from under a still-live sibling app" (create-test-app.ts:314 and :398), "broke the second app" (lifecycle-manager.ts:144), "hand the next boot a half-released app" (lifecycle-manager.ts:166).
Why it matters: round 1 desloped the docs for the same reason. Much of this "why" is worth keeping — it explains real, non-obvious singleton and proxy behavior — but the story voice and the length make the code hard to skim.
Fix: do a deslop pass on the kit's code comments, like the docs pass. Keep the load-bearing why, cut it to one plain line, and drop any comment that just restates the code.
| userAgent: vi.fn().mockReturnValue("appkit-test/1.0"), | ||
| request: vi.fn().mockResolvedValue({}), | ||
| }; | ||
| for (const [key, value] of Object.entries(seededOverrides("apiClient"))) { |
There was a problem hiding this comment.
P3 — Seeding an apiClient.* value through responses wraps it in vi.fn().mockResolvedValue(value), so responses: { "apiClient.userAgent": "x" } returns a Promise. That's the very thing the sync default avoids — a Promise turns into "[object Promise]" inside a Header. The config.* path sets values directly and doesn't have this problem, so the two paths behave differently.
Why it matters: rare input, but a wrong header value is hard to track down.
Fix: for apiClient.* seeds, keep a plain value synchronous like the config.* path does, or note that only async members can be seeded this way.
| // 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(); |
There was a problem hiding this comment.
P3 — The failure-path cleanup calls plain await app.close(), which ignores the closeTimeoutMs the caller passed — the normal close path does pass it through.
Why it matters: a slow shutdown on the failure path can run past the timeout the caller set.
Fix: pass the same closeTimeoutMs here.
Nine of Pawel's thirteen round-2 comments, all verified true first. The two P1s,
the template CI wiring, and the code-comment deslop are left: they need the
concurrency decision or a wider change. Recorded in internal/testing-kit/review-log.md.
**The timeout release is now immediate, reversing last round's fix.** His first
pass said to defer it with `.finally()`; his second says that is wrong, and he is
right. Holding the refcount makes the next boot skip its reset, so it inherits
this app's `CacheManager` — which the orphaned teardown then closes in phase 5,
mid-test. Releasing at once is safe because `runPhases` captures its own cache and
telemetry before the first await and never re-reads the shared slots. Shorter and
safer. My test asserted the deferred behaviour, so it was pinning the wrong
contract — the second time a test of mine has done that on this exact code.
**A seeded `apiClient.userAgent` no longer returns a Promise.** Every non-function
seed was wrapped in `mockResolvedValue`, so `responses: { "apiClient.userAgent":
"x" }` produced `"[object Promise]"` in a Headers value — precisely what the
synchronous default exists to prevent. Seeds now match each member's own shape;
`request` stays async. The `config.*` path never had this because it assigns
values directly.
**The published-surface test now guards the published surface.** It imported three
symbols, so every other export could vanish from the barrel with this file still
green. It now asserts twenty names are reachable through the entry — verified by
dropping `createTestPluginContext` and watching it fail by name. `tsc` would also
catch that via other suites, which softens his "CI still green" wording, but a
test claiming to guard the surface should do it.
Also: dead `createHash`/`AuthenticationError` imports removed (left by the
`fakeUserContext` move); the boot-failure path now honours `closeTimeoutMs`; the
two bare `type Any = any` aliases carry the explanation the third one had; the
"seven services" count is gone from a comment; the guide now says the client and
the OBO stub are process-wide, not per app, and bounds its `app.client` claim to a
single open app; and it distinguishes mock from fake in one line.
Finally the `nodeEnv` JSDoc records what the option actually changes beyond
refusing `development` — `errorHandlerMiddleware` redacts 5xx bodies only under
`production`. That is the evidence his round-1 comment proposing to delete the
option overlooked, and it was undocumented, which is a fair reading of why he
missed it.
4587 tests pass; typecheck, lint, format and docs build clean.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
2 participants
@databricks/appkit/testing