Skip to content

feat(appkit): add testing-kit helpers for env, errors, context, and cache - #555

Open
IamGalymzhan wants to merge 19 commits into
feat/testing-kit-harnessfrom
feat/testing-kit-helpers
Open

feat(appkit): add testing-kit helpers for env, errors, context, and cache#555
IamGalymzhan wants to merge 19 commits into
feat/testing-kit-harnessfrom
feat/testing-kit-helpers

Conversation

@IamGalymzhan

@IamGalymzhan IamGalymzhan commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

Stack

Each PR targets the one above it, so the diff here is only the delta on top of #540. Review in order.


Summary

Adds customer-facing helpers to @databricks/appkit/testing so plugin tests assert business logic instead of hand-rolled boilerplate, then adopts them across the existing suites.

New helpers

  • withEnv(vars, fn) — set env for the duration of fn, then restore each key's prior state (deleting keys that were unset before). Sync and async forms; nested calls restore LIFO. Replaces the process.env.X = …; try/finally; delete pattern, which wrongly drops pre-existing env.
  • createTestPluginContext(fakes, { responses, env, strict }) — a synchronous options parameter that composes a plugin unit-test in one call: a mock workspace client from responses, an auto-restored service context wired to it, and scoped env. Non-breaking — calls with no second argument are unchanged.
  • createApiError({ statusCode, message, errorCode }) — a genuine ApiError (so error instanceof ApiError holds) for error-path tests.
  • useTestCache() — boots AppKit's real in-memory cache, clears it before each test, and returns the real CacheManager, so a plugin's caching path runs through production's own getOrExecute/generateKey instead of a mocked cache module. CacheManager stays a process singleton.

Adoption

  • The analytics, metric, and ai-search suites drop their vi.mock("cache") fakes and assert against the real cache via useTestCache().
  • 17 passthrough suites share a new internal createCacheMock() in place of a copy-pasted fake.
  • withEnv replaces 11 hand-rolled env blocks, and createMockRouter is surfaced in the testing guide.

The internal-mock migration and the singleton-reset changes stay deferred to a later PR.

Stacked on #540 (feat/testing-kit-harness) — these helpers extend testing-kit code on that branch, so this targets it rather than main.

withEnv(vars, fn) sets env for the duration of fn and restores each key's
prior state on exit — including deleting keys that were previously unset,
rather than the blanket delete the current test pattern uses. Sync and async
forms; nested calls restore LIFO. Exported from @databricks/appkit/testing.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
createApiError({ statusCode, message, errorCode }) returns a genuine ApiError
instance, so error-path tests can assert real instanceof ApiError checks
instead of hand-rolled look-alikes that pass name but fail instanceof.
Exported from @databricks/appkit/testing.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
An optional second parameter { responses, env, strict } composes a plugin
unit-test in one call: a mock workspace client seeded from responses, an
installed service context wired to it, and scoped env — all auto-restored via
an afterEach hook, with an idempotent restore() escape hatch. Synchronous and
non-breaking: no-options callers are unchanged. Exports TestPluginContextOptions.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Replaces 11 hand-rolled process.env set/try/finally/delete blocks with withEnv
across the files, jobs, and ai-search test suites. Behavior-preserving; the
vi.mock/vi.hoisted preludes are untouched (the broader internal-mock migration
is deferred).

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Documents withEnv, createApiError, and the createTestPluginContext options
parameter in the testing guide, and surfaces the already-shipped createMockRouter
fixture. The composition example is shown synchronously (no await on the factory).

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
…tests

Removes the unused ServiceContextMock import and three side-effect-only mock
bindings, and stops capturing an unused priorInitialized — clearing the lint
warnings the composition-options work introduced. No behavior change.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
…aths

Addresses two code-review findings. Extracts a shared applyEnv() helper so
withEnv and the createTestPluginContext options path no longer duplicate the
env capture/restore loop. Guards withEnv's error paths so a failing restore
can no longer mask the caller's original error (a restore failure still
surfaces on the success path). Behavior-preserving for normal use; all tests
pass.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Boots AppKit's real in-memory cache singleton and clears it before each test,
exposing the real CacheManager so plugin tests assert cache-key behaviour
through production's generateKey instead of mocking the internal cache module.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Shared, unexported factory for the passthrough cache instance the
non-behavioural suites mock; adopted across those suites in a follow-up unit.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Drops the hand-rolled functional cache fake; the suite now runs against the
real in-memory cache. Reworks the abort-fallback test to model a real client
disconnect (response close aborts the handler signal) rather than spying the
signal executeStatement happens to receive.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Runs against the real in-memory cache; the cache-key-equality test now spies
the real getOrExecute to capture the composed key parts instead of a fake.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Runs against the real in-memory cache. Completes the telemetry mock span
(adds end/addEvent/etc.) since the real getOrExecute opens a span the old
functional cache fake never did.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Replaces the copy-pasted passthrough cache fake with createCacheMock(),
pulled in via an async vi.hoisted + dynamic import (require can't resolve the
TS helper inside a hoisted block).

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
…uites

Replaces the copy-pasted passthrough cache fake in 16 suites (files/*, genie,
jobs, serving, lakebase, connectors/lakebase, analytics.readonly) with
createCacheMock(). server.test.ts keeps its own mock (close(), not a
passthrough getOrExecute).

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
The fake omitted the third (userKey) argument the real getOrExecute takes, so
calling it with three args failed typecheck.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Removes reasoning-trace and over-explained wording across the testing guide
(content from #530/#540/#555) and the reserved-name callout, matching the
terser house style. No facts or examples changed.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
@IamGalymzhan IamGalymzhan changed the title feat(appkit): add withEnv, createTestPluginContext options, and createApiError test helpers feat(appkit): add testing-kit helpers for env, errors, context, and cache Aug 28, 2026
The kit shipped two hook-wiring helpers — useServiceContextMock and, in this
stack, useTestCache — and none for the app itself, so a suite needing an app
per test hand-wired beforeEach/afterEach and had to remember close(). That gap
widened when useTestCache adopted the pattern for the cache and skipped the
app.

It matters more since the harness began allowing one open app at a time. A
forgotten close() used to leak a listener quietly; now the next boot throws and
takes the rest of the suite with it. await using is still shorter for a single
test, but it cannot carry an app from a beforeEach into the test body, and a
describe holding one in beforeAll cannot contain a test that boots its own — so
beforeEach/afterEach is the remaining pattern for per-test apps, and it was the
one without a helper.

Mirrors useTestCache: same { current } accessor, same call-it-in-a-describe
rule, same self-explaining error when read outside a test. The afterEach clears
the handle before awaiting close, so a close that throws cannot leave a stale
app readable by the next test.

The one-app-at-a-time guard makes the tests discriminating: verified by
removing the close, which fails four of them with "a harness app is already
open" rather than passing quietly. Removing the barrel export fails the
published-surface assertion by name.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
…ocks

Fourteen suites hand-rolled a workspace client and an ApiError look-alike, then
patched `../../workspace-client` and `../../context` so production code would
pick them up. None of that expressed anything the shipped kit cannot: the client
is `createMockWorkspaceClient`, the error is `createApiError`, and injecting the
client through `mockServiceContext` makes `getWorkspaceClient()` resolve to it
via the real ServiceContext — so nineteen of twenty-four module mocks go away.

`setupTestEnv` in the files suite now takes the client and passes it through,
which is what let nine files drop their `context` mock at once.

Strictness is preserved deliberately, not incidentally. A hand-rolled object
literal threw a TypeError on any undeclared call; `strict: true` keeps that
loudness with a sentence instead of a stack trace. Verified by counting rather
than asserting: 306 tests before and after, 558 expects before and 559 after.

One class of assertion gets stronger. `connectors/files` asserted
`toBeInstanceOf(MockApiError)` — circular, since it could only pass while the
module was patched to install that fake. It now asserts the real `ApiError`, and
a mutation that throws a plain Error fails it, which the old form could not
catch.

Two mocks are kept on purpose. jobs keeps a three-line `Context: vi.fn()` — the
SDK cancellation-token class, an unrelated concern the old mock also served. The
nine files that mock `workspace-client` to stop a real client being built have no
injection seam (they test ServiceContext, CacheManager, or the type-generator),
so module mocking is correct there.

Not migrated: files/plugin.test.ts (3978 lines, 49 client refs, 7 local
getRouteHandler copies) wants a reviewed diff rather than a scripted one, and
ai-search pins getCurrentUserId constant on purpose so cache-key scoping is
driven only by executorKey — the real path would change the keys its tests
assert on.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant