feat(appkit): add createTestApp, a never-crash mock client, and app.close() - #540
Open
IamGalymzhan wants to merge 22 commits into
Open
feat(appkit): add createTestApp, a never-crash mock client, and app.close()#540IamGalymzhan wants to merge 22 commits into
IamGalymzhan wants to merge 22 commits into
Conversation
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>
…dogfood both
Audit first: the ./testing subpath was already in both exports maps and the
tsdown entry, vitest is already an optional peer dep, and attw/publint pass. The
real gap was proof that a test needs nothing else, so the integration suites
moved onto the public entry point — that migration *is* the audit. A new
acceptance suite imports only from @databricks/appkit/testing and boots,
requests, asserts a stream, and closes.
Self-referencing the package from inside it needed a tsconfig paths entry.
Resolving the package's own export map made tsc's project root ambiguous
(TS2209), and the alias mirrors how shared and @databricks/lakebase are already
mapped. It also makes source resolution deterministic rather than depending on
the "development" export condition — verified by marker: the subpath resolves to
src, not dist.
createTestPlugin(factory, config) closes the last dogfooding footgun. Reaching
through a descriptor with `new (genie({}).plugin)(config)` skips DEFAULT_CONFIG
and forgets `name`, so the instance under test is configured differently from
the one production builds. It mirrors createAndRegisterPlugin's merge order.
createTestApp does not subsume it: the harness takes descriptors and builds
instances itself, so the unit path needs its own ergonomics.
Dogfooding results, reported as measured rather than as hoped:
- analytics.integration.test.ts: 300 -> 216 lines. Setup/teardown went 104 -> 55,
against the plan's predicted ~30. Its local getListeningPort helper is gone and
its 12 mock handles now come from getMockFn. Same 6 tests, same assertions.
- getListeningPort is lifted into the kit, and files/plugin.integration.test.ts
imports it instead of carrying its own copy.
- server.integration.test.ts moves four of its five blocks to ephemeral ports.
The fifth keeps its fixed port deliberately, because it asserts the server
honours a configured one; a comment says so. The removed sleep-100ms waits are
replaced by getListeningPort, which waits on the listening event instead of
guessing.
One plan claim corrected: the hardcoded TEST_PORT = 9879 said to collide with
server.integration was already fixed on this branch — analytics had moved to
port: 0. The real fixed ports were the five in server.integration itself, which
is what this commit addresses instead.
Docs lead with createTestApp: a which-harness comparison table, the dotted-path
responses convention, the teardown contract, a "Mocking Databricks services"
section carrying the never-crash floor's honest catch (a misspelled *method*
returns undefined, and a Lakebase pool built on the fake cannot connect), and an
explicit callout that manifest.config.schema is not validated. The
PluginContext/ServiceContext boundary note now says the kit covers the data
plane. The template example gains a createTestApp test.
4519 tests pass (+8). pnpm docs:build is clean.
Co-authored-by: Isaac
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
…ings Verified the kit end to end the way the plan prescribes: pnpm pack:sdk, an app scaffolded by `databricks apps init` from this repo's template, the tarballs installed into it, and the suites run with no .env, no credentials, and every non-loopback socket connection hard-blocked. Nine customer-style tests plus the template's three pass, and the scaffolded app typechecks against the shipped .d.ts. That run corrected a claim this branch had been making. Both the plan's risk table and the docs said a misspelled *method* slips through the never-crash floor and only a misspelled *service* is caught. Not so: each facade accessor is typed against the SDK's own service class, so `client.jobs.getRunz` and `client.files.anything` are compile errors too. The compile-time block now asserts that for three services, and the docs say what the real gap is — a method that exists but has no declared response, or a call that bypasses the types with a cast. Also repointed one doc line that told readers to reach the client via `getWorkspaceClient()`. That is right inside this repo but wrong from the published entry, where the name currently resolves to Lakebase's unrelated `getWorkspaceClient(config)`. The docs now use `getExecutionContext().client`, which is exported and works. The mis-export itself is a main-entry defect, outside this branch's scope, and is left for a follow-up. 4519 tests pass. Build, docs:build, attw, and publint are clean, and the packed tarball carries dist/testing/*.js and .d.ts for every new module. Co-authored-by: Isaac Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Eight reviewers over the branch diff produced 24 findings; 14 were actionable.
The two real defects, both in code this branch added:
- **An orphaned teardown could tear down the *next* app's resources.**
`close()` races the shutdown phases against a 5s budget, then resets the core
singletons and resolves. The phases keep running. A plugin `shutdown()` hook
slower than that budget but inside its own 10s per-plugin budget — which the
files plugin's drain can be — left phase 5 re-reading the static slots, so it
either skipped draining this app's cache pool or closed the *following* app's
storage and shut down its OTEL SDK. The comment claiming the phases had
"already closed the cache storage" was only true when teardown finished in
budget. Phase 5 now uses the instances captured before the first await, and a
test drives the exact 5s-to-10s window (verified by reintroducing the bug).
- **`process.env` restore did not compose across overlapping boots.** Each app
snapshotted independently, so a second boot captured the first's mutations and
whichever closed last re-applied them, stranding the harness keys and the first
app's `env` entries after both apps were gone. Confirmed by probe, in-repo and
from the packed tarball. There is now one reference-counted baseline: the first
live app anchors it, the last one to close restores it, and the outcome no
longer depends on close order.
Also fixed: `server: false` alongside a caller-supplied server plugin is now
refused instead of half-honoured (the plugin still bound a socket while the
handle denied one existed); `AppHandle.close()` declares the `{ timeoutMs }` the
implementation accepts, so the harness no longer casts to reach it; the duplicate
`listeningPort` helper in the close integration suite is gone in favour of the
kit's (two reviewers flagged it); the analytics suite drops an `as never` that
erased `app.plugins` typing; the `clientFns` WeakMap moved above its users; and a
comment claiming "9 typed facade members" over a 7-element array is corrected.
Two of my own tests were weak and are now stronger: the `authenticate` test
wrapped its whole body in `if (mockFn)` and only asserted "was called" — it now
asserts the Authorization header it claims to set — and a close-after-signal test
proved ordering by counting microtask ticks, which cannot see through
`raceWithTimeout`; it uses the same sentinel the sibling test does. A new
compile-time assertion pins that `AppHandle` still satisfies a `PluginMap`
annotation, so a regression in the widening can't pass silently.
Documented rather than changed: a service's methods are callable but not
enumerable, so `'getRun' in client.jobs` is false and `Object.keys` is empty.
Reporting those keys would make `util.inspect` mint a mock per probe, which is
the recursion the default traps exist to avoid. Also documented why
`onPluginsReady` keeps the narrower `PluginMap<T>`.
One finding rejected as a false positive: project-standards reported CLAUDE.md
still documents Biome. It does not — main's own oxlint migration (9538d58)
updated it, and only the pre-merge copy said Biome.
Six findings were demoted to residual risks, chiefly the P1 claim that the mock
resolving `undefined` for undeclared paths lets a test pass while production is
broken. That is the deliberate, documented contract of the never-crash floor, not
a defect; an independent reviewer re-deriving it argues the existing caution
callout is warranted, not that the design changed.
4524 tests pass. Re-verified end to end from a repacked tarball in the
`databricks apps init` app with all non-loopback sockets blocked.
Co-authored-by: Isaac
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
`createApp()` rejects a plugin whose manifest name is `close`, because plugin exports are installed as own properties and an own property shadows a prototype method — so such a plugin would silently replace the app handle's teardown. The thrown ConfigurationError already names the offending plugin, but nothing told an author the constraint existed before they hit it. Noted beside where custom-plugins.md introduces `static manifest`. Landing this as part of the `feat:` framing for the branch rather than a BREAKING CHANGE footer: the failure is loud and at boot, not a silent runtime change, and no plugin in this repo is affected. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
These were added earlier on this branch to work around a locked agent worktree at .claude/worktrees/, which every tool saw as 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 that other branch's tree. The worktree has since been removed, so the ignores are treating a symptom that no longer exists and are out of scope for this branch. Verified after removal: `pnpm knip` and `pnpm check` both exit 0 at the repo root. .oxfmtrc.json and .oxlintrc.json are now byte-identical to origin/main. The one remaining knip.json difference — ignoreDependencies: ["vitest"] for packages/appkit — predates this work and is required because vitest is an optional peer dependency of the published testing subpath. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
The comments on this branch were far past the repo's own density: reset.ts was 85% comment (35 of 41 lines) for a one-line function, create-test-plugin.ts 63%, lifecycle-manager.ts 48%, mock-workspace-client.ts 45%. Much of it restated the code or ran to several paragraphs where a clause would do. Net 364 comment lines removed. Every file now sits at or below the repo baseline (main's own sources run 20-42%): mock-workspace-client 45% -> 21%, create-test-app 36% -> 25%, lifecycle-manager 48% -> 35%, reset.ts 41 -> 13 lines total, create-test-plugin 63 -> 27. What was kept is the non-obvious "why" that a maintainer would otherwise delete and reintroduce a bug: that `then` must stay in the deny-set or `await client.jobs` hangs; that `ownKeys` stays default or util.inspect mints a mock per probe; that config.host must be a real string; that the three canned defaults are byte-identical because 13 suites depend on them; that phase 5 captures its singletons before the first await; and the four boot hazards behind createTestApp's setup. Pre-existing comments in files this branch only touched (fixtures.ts, test-plugin-context.ts, the shutdown() phase list) are left alone — reverting other people's prose is not this change's business. Also dropped an unnecessary `as Any` cast in createTestPlugin: DEFAULT_CONFIG is already declared on PluginConstructor, so the type escape and its explanatory comment both went. 4524 tests pass; lint, format, and typecheck clean. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
The mock-workspace-client suite had 37 tests written against the plan's checklist rather than against behaviours, so eight asserted something a sibling already covered: getRun memoization twice, config.host being a string twice, the 9-member facade twice (one a strict subset of the other), a rejecting function response twice, getMockFn path resolution twice, function-valued responses twice, the canned defaults twice, and two config-option tests that fit in one. 29 tests now, with no assertion lost — where a dropped test had a unique claim it was folded into the survivor. Three describe blocks became empty and were removed; one had only a comment saying its subject could not be tested at runtime, which the compile-time contract block covers properly. Note for anyone reading this as a bundle-size fix: it is not one. Tests do not ship — the packed tarball contains zero test files — and dropping these eight moved the measured bundle by exactly 0 bytes. The comment trimming in the previous commit is what actually helped (+8.1% -> +6.9%), because JSDoc is preserved in the emitted .d.ts. 4516 tests pass. 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>
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>
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
1 participant
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.
Summary
Stacked on #530 — review that one first; this PR's base is
feat/testing-kit, so GitHub shows only the incremental diff (42 files, +3626/−445) and will retarget tomainautomatically once #530 merges.Where #530 gave plugin authors a fake
PluginContext, this PR lets them boot a real app over real HTTP with no workspace, no credentials, and no network.What's in it
createMockWorkspaceClient— a never-crash fake SDK client. Any service path resolves rather than throwing, so a plugin touching an unstubbed method fails on its own assertion instead ofCannot read properties of undefined.getMockFn(client, "jobs.getRun")gets the underlying spy for call assertions.configandapiClientare seeded as real values, not mocks, because production code reads them directly. It also replaces the older builder behindmockServiceContext, so the 13 suites already using that fixture now get a client that fakes both the service principal and the user.app.close(), split from signal shutdown.shutdown()remains the signal path that exits the process;close()is the programmatic path that never does.AppHandlealso implementsSymbol.asyncDispose, soawait using app = await createApp(...)works.closeis now a reserved plugin name.Re-bootable process-wide singletons.
ServiceContext,CacheManager, and the two telemetry singletons are refcounted: the first boot claims them, the last close drops them. Without this, booting a second app rebound the first app's context, and closing either left the survivor with none.createTestApp— the harness. Boots a real Express server on an ephemeral port, fakes the workspace client, stubs on-behalf-of user context, and tears everything down onclose(). Exported from@databricks/appkit/testingalongsidecreateTestPlugin.Notes for the reviewer
close()timeout, andprocess.envrestore didn't compose across overlapping boots.thenmust stay in the mock'sProxydeny-set, orawait client.jobshangs — the service looks thenable and the runtime tries to adopt it.ownKeysis deliberately left at its default soutil.inspectcan't mint a mock per probed property.bundle-size-baseline.json, which I've left as a deliberate decision rather than quietly bumping.Verification
4517 tests pass (up from 4413 on #530); typecheck, lint, and format clean. Also verified end-to-end against a packed local tarball in a fresh app scaffolded with
databricks apps init.This pull request and its description were written by Isaac.