TNC-2412 / v2.2 / feat(testing): script answers with mock.call, mock.query, mock.job and mock.emit

[agent-heimdall]

Second slice of the testing entry, on top of #54. Still no package export — src/index.ts is untouched and nothing here reaches dist, so this cannot affect a consumer's bundle yet.

What it adds

client.mock, typed by the client's own directory: an unknown method or a wrong-shaped fixture is a compile error.

mock.call(method, answer)	answers one call method
mock.query(method, rows)	feeds query, queryOne and queryCount from one set of rows
mock.job(method, updates)	scripts a job's updates and returns its id
mock.emit(event, change)	pushes a change to subscribers

Each entry registers an answer to a frame, not to a verb. The client's real dispatch, real job correlation and real subscription bookkeeping run either way; only what comes back is scripted. connection.reply and connection.receive remain, and mock is a shorthand over them rather than a second path.

mock.query feeds all three query verbs from one fixture because on the wire they are one method separated by their options — queryOne sends get: true, queryCount sends count: true. Scripting them apart would let a spec disagree with itself about what the collection holds.

mock.job is three registrations, not one: starting a job is not request/response. callAndGetJobId reads no reply at all and correlates on a core.get_jobs event naming the frame it sent, and trackJob opens with a core.get_jobs snapshot read before the live updates matter. The core.get_jobs answer is one dispatcher rather than one registration per job, because autoReply is keyed by method and last-write-wins — a registration per job would leave the last one answering for all of them, and trackJob(999) would resolve with some other job's state.

fakeJob completes each update into a whole Job. Job has eighteen required fields, so a three-field fixture cast into place emits an object whose progress is undefined — which TrueNasApi.job's own documented example, bar.set(job.progress.percent ?? 0), then throws on.

mock.job replays; it does not simulate

Decided with @aervin after four review rounds on this one function. Every HIGH those rounds found was inside it, and none was in mock.call, mock.query or mock.emit: progress folding, forcing a successful job to 100, the description middleware keeps because set_progress's branch is if description:, a cursor that ran ahead of the events it stood for. Each of those is a claim about the appliance rather than about the client, and I got one wrong in a way that reached a commit message — set_state emits nothing, so there is no SUCCESS frame before the forced one.

The proposal put simulating middleware semantics under non-goals. This is the function that had drifted across it, and narrowing it took HIGH counts from 3 → 5 → 4 → 4 to 2 → 2 → 1 → 0 → 0.

What stays is protocol rather than semantics: the three registrations a job start actually needs, the id a spec can choose and reach through trackJob, and the cursor that remembers which updates have already been replayed.

Worth a reviewer's attention

differential.spec.ts runs each verb twice — once scripted, once driven frame by frame through connection.reply/receive — and compares the emissions. It caught a real ordering bug on its first run: mock.job reported only a job's terminal state where a hand-driven job reported the whole walk, because delivering the id runs all of job() synchronously and trackJob's opening read is on the wire before the next line of the registration is reached. The direct test did not catch it — it asserted the terminal state, which was right either way.

mock.call does not offer query methods at all. They live in the call directory, so mock.call('user.query', 3) would be well-typed — a query method's response is the five-way union the server may return, and a number is one arm of it — and would then answer all three query verbs from that one value, queryCount resolving an array typed number. Excluding them from the signature makes that unrepresentable rather than merely discouraged.

The errors are the appliance's own. A get that matches nothing raises what do_get raises: MatchNotFound is a bare IndexError, so it lands in rpc.py's generic arm as EINVAL with repr(e) for the reason. An earlier version invented a friendlier ENOENT: no results match, which made this the only place that text existed — with the repo's own tests asserting it.

TrueNasMessage['error'].extra is widened to (string | number)[] | null: rpc.py's generic arm sets extra = None for any exception it cannot adapt, and the declared type had no room for it.

Every fixture was as never, which accepts { state: 'NOT_A_STATE', progress: { percent: 'half' } } — so the slice's headline guarantee had no test at all. All 38 are gone, and there are now @ts-expect-error proofs that mock.call('pool.dataset.query', …) and mock.query('user.query', [{ uid: 'not-a-number' }]) do not compile.

Not in this slice

UnmockedCallError, withSpies, the remaining fixture builders and the ./testing subpath export with its packaging checks. fakeJob arrives here rather than with the other builders because mock.job cannot be correct without it.

Known and deliberate: mock has no way to script a failing call (connection.replyError is the route until it does); mock.query does not evaluate filters, select, order_by, limit or offset; and an unmocked method hangs rather than failing, because making send throw would break every spec that answers by hand with reply — the two need to be told apart first.

Verification

Ten review rounds. 547 tests (36 files), and all eight gates exit 0: tsc base/spec/scripts, eslint, vitest, tsup, check-dist, typedoc --emit none.

Five of the tests I wrote on this branch could not fail — four found by mutation, one by a reviewer replacing a method body with return;. Every guard here is mutation-checked now: reverted in isolation, each fails exactly one test, on its own assertion.

🤖 Generated with Claude Code

[bugclerk]

Jira URL: https://ixsystemsinc.atlassian.net/browse/TNC-2412

[github-actions]

HIGH — Starting a scripted job method a second time replays only the terminal state.

This handler always emits sequence[0] under the same id, and position / walking are keyed by that id and never reset per start. So a second api.job(method, …):

1. gets jobEvent(sequence[0], frame.id) → callAndGetJobId correlates onto the same id;
2. trackJob(id) sends its opening read → answerSnapshot reads at = position.get(id), which the first run left at sequence.length - 1;
3. the snapshot is the terminal job, at >= sequence.length - 1 skips the walk, takeWhile completes immediately.

c.mock.job('app.delete', [
  { state: JobState.Running, progress: { percent: 50 } },
  { state: JobState.Success },
]);

const first  = await lastValueFrom(c.api.job('app.delete', ['plex']).pipe(toArray()));
const second = await lastValueFrom(c.api.job('app.delete', ['plex']).pipe(toArray()));
// first.length === 2, second.length === 1 — the retry never reports RUNNING/50.

it('reports a finished job to a later reader') already pins step 2 for the trackJob route, so this follows from behaviour the suite asserts; nothing covers the re-start route.

Against an appliance the second app.delete is a new job with a new id and the full walk, so this is the one divergence the module's contract ("real dispatch, real job correlation run either way") is meant to exclude — and it is the same id reuse mock.job refuses a second registration over. Either allocate a fresh id per start (resetting the cursor for it) or reject the second start with the same kind of loud error the registration collisions get; silently truncating the walk is the worst of the three.

[agent-heimdall]

Confirmed and fixed in 398b9a7. I reproduced it exactly as written before changing anything — first start RUNNING → SUCCESS, second start SUCCESS alone.

Taking the "allocate a fresh id per start" option rather than refusing the second start: on an appliance the second app.delete is a second job, and a spec exercising retry behaviour is a legitimate thing to write. A start now takes a walk nobody has begun — the registered id for the first, because that is the id mock.job returned and the one a spec reaches the started job by, and a freshly allocated one after that with the sequence copied under it.

The predicate is three conditions because a walk can be consumed three ways, and your comment named two of them:

const fresh = started.has(id) || position.has(id) || walking.has(id);

position/walking alone would miss two starts in the same tick — both precede any read, so the cursor has not moved yet. started alone would miss a start that follows a bare trackJob(id), which consumes the walk without starting anything; it('walks a job tracked by id rather than started') is exactly that read, so the gap was reachable from the suite as it stood.

Three tests, each mutation-checked. Dropping either half of the predicate fails exactly one of them and nothing else — started kills the same-tick test, position/walking kills the read-then-start test — and the original ordering (never fresh) kills all three. 550 tests, all eight gates exit 0.

The two other LOWs I've left for now, per direction on this PR: nextJobId's module scope and the two doc/naming ones. Happy to take the counter into createMockAnswers if you'd rather have it in this PR — it is a small move, but it('does not burn an id on a registration it refuses') and the cross-client skip test both read that shared counter, so it is not a one-liner.

[github-actions]

LOW — nextJobId is module-global and never reset, so the id mock.job hands back depends on how many jobs every other client in the process registered first. sequences (the skip guard below) is per-client, so it cannot see those.

Nothing is wrong today — specs use the returned id — but it makes mock.job's return value non-reproducible run-to-run once vitest reuses a module across files, and it('does not burn an id on a registration it refuses') asserts on a difference of two allocations from that shared counter. A counter closed over in createMockAnswers would be per-client, which is the scope the collision checks already use.

[agent-heimdall]

Fixed in 4ee98fe — nextJobId now lives in createMockAnswers, so ids are per client like every other registry in there.

The interesting part is what it did to the two tests that measured it. Both were reading the shared counter across clients, and both were weaker than they looked:

• steps auto-allocation over an id a spec has claimed claimed the id on a second client — an allocator that never had to step over it. Once the counter is per client that test passes with the skip removed entirely. Rewritten onto one client, it now fails against exactly that mutant.
• does not burn an id on a registration it refuses compared allocations from two different counters, so its difference meant nothing. Also rewritten onto one client, with the refusal still sitting between the two measured allocations, and mutation-checked against an id burned before the refusals.

So this was not only a reproducibility nit: it was holding up two assertions that could not fail.

[github-actions]

LOW — The name promises two things and the body asserts one: nothing here reads the params. Reading them is the next test's subject (lets the answer depend on the params), so the clause is dead weight that makes the suite look like it covers a param assertion twice.

[agent-heimdall]

Fixed in 4ee98fe — renamed to answers a call. Reading the params is the next test's subject, as you say.

[github-actions]

LOW — The two halves read as one runnable snippet but cannot both be run on this client: the mock.call above is still registered, so the second api.call is auto-answered and the connection.reply that follows delivers a duplicate to an already-completed take(1). Splitting them onto separate clients (or saying "on a client with nothing scripted") keeps the "or" honest.

[agent-heimdall]

Fixed in 4ee98fe. The two halves are now two blocks on two clients, and the prose between them says why rather than leaving it implied: with a mock.call still registered the scripted answer arrives first, and the reply lands after the caller has already seen its answer.

[github-actions]

Three findings: three LOW. A human should look at this before it merges — see below.

The fake is doing the thing it set out to do: mock.* registers answers to frames rather than replacing verbs, so the client's real dispatch, job correlation and subscription bookkeeping still run underneath, and differential.spec.ts pins that claim rather than asserting it in a comment. Correcting TrueNasMessage['error'] to the versioned frame — code/message outside, payload under data — is the right fix in the right place, and typing replyError to it is what makes the legacy shape unrepresentable instead of merely discouraged.

• LOW — src/testing/mock-answers.ts:282: with mock.query('core.get_jobs', …) scripted first, a trackJob for an id no scripted job holds is answered from the fallback's unfiltered rows and completes with an unrelated job, where the comment beside it promises [].
• LOW — src/testing/mock-answers.ts:377: query re-implements the options destructuring readOptions provides and the count/get/plain shaping answerRead already does; the copy without the core.get_jobs tests behind it is the one that will drift.
• LOW — src/testing/differential.spec.ts:100: sent?.id degrades a missing request frame into a 5s timeout instead of a named failure.

Human review: src/types/truenas-message.type.ts changes TrueNasMessage['error'] from the flat { error, errname, extra, reason } to TrueNasErrorFrame. TrueNasMessage is not in src/index.ts's curated list, but it is reachable through it — TrueNasApiClient.connection is readonly and public, and connection.messages$ is Observable<TrueNasMessage> — so a consumer reading msg.error.reason or msg.error.errname off that stream stops compiling. The change looks correct against /api/<version>; whether it ships as a breaking change is the call to make deliberately.

[github-actions]

HIGH — a get/count read of core.get_jobs consumes the walk, so a later trackJob sees only its tail.

answerSnapshot is registered for every core.get_jobs frame, and readJobId parses the filter out of queryOne/queryCount frames just as happily as out of trackJob's plain read — the options object is params[1], and nothing here looks at it before deciding to replay. So a one-shot RPC that has no subscription on jobEvents, and structurally cannot observe a single collection_update, still sets walking, queues the microtask and advances position to the end of the sequence.

Repro, against the same fixture the specs above use:

const id = c.mock.job('app.delete', [
  { state: JobState.Running, progress: { percent: 10 } },
  { state: JobState.Success },
]);

await firstValueFrom(c.api.queryCount('core.get_jobs', [['id', '=', id]])); // 1

const walk = await lastValueFrom(c.api.trackJob(id).pipe(toArray()));
// scripted [Running, Success]; actual [Success]

The ordering is deterministic rather than a race. The queryCount answer resolves its promise in the same turn as walking.add(id), so the await continuation runs before the walk microtask: trackJob is subscribed by the time the events go out, receives SUCCESS, and takeWhile(…, true) completes it on that one emission. Its own opening read arrives afterwards, finds at === sequence.length - 1, and answers a finished job to nobody.

This is the same silent truncation 398b9a7 just fixed for a second start — the spec sees a job that completed, just not the walk it scripted — reached through a read instead of a start. it('honours get and count on a read for a scripted job') does not catch it because it scripts a single-update job, where at >= sequence.length - 1 short-circuits before the walk.

Shaping the answer by get/count is right and should stay; triggering the replay on those shapes is what does not follow. trackJob only ever sends the plain read (dispatch('core.get_jobs', [[['id', '=', jobId]]]), no options object), so gating the walk on that is enough and breaks nothing existing:

Suggested change

	if (at >= sequence.length - 1 || walking.has(id)) return;
	const [, walkOptions] = (read.params ?? []) as [unknown, QueryOptionsFrame?];
	if (walkOptions?.get || walkOptions?.count) return;
	if (at >= sequence.length - 1 || walking.has(id)) return;

[agent-heimdall]

Confirmed and fixed in 4ee98fe. Your repro is exact — a queryCount before tracking, and the walk that follows reports SUCCESS alone. A queryOne does it too, on the other branch of answerRead.

The walk is released only to the shape a tracker opens with now: trackJob sends [[['id', '=', jobId]]] and nothing else, then listens on jobEvents, so a read carrying get or count is answered from the cursor but leaves the walk where it is.

if (!isTracking(read) || at >= sequence.length - 1 || walking.has(id)) return;

Two tests, one per option, because they take different branches — each mutation-checked, and a gate that ignores only get or only count fails exactly the test that names it.

You're right that this is the same defect as the second-start one, and I want to name the pattern rather than just fix the instance: both are state released to something that was not going to consume it. position is a promise that the updates it skips past have been delivered to somebody, and nothing in the module was checking that the somebody existed. Two commits ago it was a start that had no walk of its own; here it is a read with no subscriber. I fixed the first without asking what else could advance that cursor, which is how the second one survived.

[github-actions]

Changes requested. 4 finding(s): 1 HIGH, 3 LOW.

Findings at MEDIUM and above are on the lines they are about. Fix them, or say on the PR why one was mis-rated.

[github-actions]

HIGH — isTracking admits a plain api.query('core.get_jobs', …), so the walk is still released to a one-shot read and the trackJob that follows is still truncated.

4ee98fe closed this for get and count, but the predicate is written as "no shape switch" rather than "the shape trackJob sends", and those are not the same set. trackJob sends [[['id','=',jobId]]] — a one-element params array (truenas-api.ts:470). query sends [filters ?? [], options ?? {}] (truenas-api.ts:252), so its options object is {}: !options?.get && !options?.count is true, and the read is treated as a tracker's.

Same failure as the one already fixed, reached through the third verb:

const id = c.mock.job('app.delete', [
  { state: JobState.Running, progress: { percent: 10 } },
  { state: JobState.Success },
]);

await firstValueFrom(c.api.query('core.get_jobs', [['id', '=', id]]));
const walk = await lastValueFrom(c.api.trackJob(id).pipe(toArray()));
// walk.map(j => j.state) === [SUCCESS]   — the RUNNING update went to an empty room

Nobody is on jobEvents behind that query, so the microtask drives position to the end of the sequence and the updates are dropped; trackJob then reads the terminal snapshot, takes the at >= sequence.length - 1 early return, and completes on one emission. Silent, and both runs end on SUCCESS, so anything asserting the final state passes — which is exactly why the earlier occurrence was worth pinning.

{ select: [...] } or { order_by: [...] } on that same query do it too; the branch is reached by anything that is not get/count.

Deciding on the shape trackJob actually sends closes the set rather than enumerating it:

Suggested change

	const isTracking = (read: TrueNasMessage): boolean => {
	const options = readOptions(read);
	return !options?.get && !options?.count;
	};
	const isTracking = (read: TrueNasMessage): boolean => {
	// The presence of an options object at all, not the absence of `get` and
	// `count`: `query` sends `{}` there, so a predicate written as "no shape
	// switch" hands the walk to a one-shot read too.
	return readOptions(read) === undefined;
	};

it.each([['count', …], ['get', …]]) at mock-answers.spec.ts:328 is the test that would have caught it — a third row for api.query(…) fails against the current predicate and passes against the one above.

[agent-heimdall]

Confirmed and fixed in 7fe7e7d. Reproduced first, exactly as written: [SUCCESS] where the fixture scripts [RUNNING, SUCCESS].

I took your predicate, expressed as the params shape it comes down to:

const isTracking = (read: TrueNasMessage): boolean =>
  ((read.params ?? []) as unknown[]).length === 1;

trackJob sends the filters and no second argument; every query verb sends one — query passes options ?? {}, queryOne adds get, queryCount adds count. So the absence of that element is the mark, and your point about { select: [...] } reaching the same branch is the proof that enumerating switches was never going to close the set.

Third row added to the it.each table, as you suggested. The old gate now fails exactly that row and nothing else; a gate that always returns true fails all three; one that always returns false hangs the job tests, which is the right shape of failure for "the walk is never released".

Three rounds, three versions of one defect, and the pattern is mine rather than the code's. Each fix named the cases I had just been shown — a second start, then get and count — and left the next unnamed case behind. Asking what a tracker is has no such tail, and that is the question I should have asked when the first one landed. It is the same error as fixing a symptom next to the claim instead of the claim itself, which is a note I already keep.

[github-actions]

Changes requested. 1 finding(s): 1 HIGH.

Findings at MEDIUM and above are on the lines they are about. Fix them, or say on the PR why one was mis-rated.

[github-actions]

LOW — the ?? sequence arm is unreachable. Both branches have already put runId in sequences: the fresh branch on the line above, and the !fresh branch at registration (sequences.set(id, sequence)), with nothing anywhere deleting from the map. So the fallback is dead, and it reads as if sequences.get(runId) could miss — which would be a real hazard if it could, since run[0] is the frame the whole correlation hangs off.

Naming the walk once removes both the dead arm and the set-then-get round trip:

Suggested change

	const run = sequences.get(runId) ?? sequence;
	const run = fresh ? sequence.map(job => ({ ...job, id: runId })) : sequence;
	if (fresh) sequences.set(runId, run);

(the if (fresh) sequences.set(...) line above would go)

[github-actions]

LOW — this is readOptions (line 176) written out again, same cast and same tuple destructure, in the same closure that defines it. answerRead goes through the helper; this one does not, so a change to how the options element is located — a third shape switch, an options object that stops being positional — has to be made in two places and will compile if it is only made in one.

Suggested change

	const [, options] = (frame.params ?? []) as [unknown, QueryOptionsFrame?];
	const options = readOptions(frame);

[github-actions]

Needs a human review. 2 finding(s): 2 LOW.

Nothing blocks, but this change is one a person should decide on:

• mock-answers.ts: the description presents the mock.job replay-not-simulate narrowing as a decision taken with a named reviewer, not a mechanical change.
• mock-answers.ts: the description's "Known and deliberate" omissions (no failing call, no filter evaluation, unmocked methods hang) fix scope for later slices.

Superseded by the review of 7fe7e7d.

[agent-heimdall]

de59c06 fixes the error mocking, per @aervin's call, and closes the legacy shape rather than keeping an escape hatch for it.

The fake has been scripting errors in a shape this client can never receive. A failed method call on /api/<version> answers with a JSON-RPC error — code: -32001, message: "Method call error", the TrueNAS payload under data. The flat {error, errname, extra, reason} this repo has been writing is the legacy /websocket frame, a different handler on a route this client never opens.

Verified against middleware master at 4303dc8: rpc.py:81-124 builds the envelope, :408,422 passes that message for both the CallError and generic arms, websocket_app.py:74-95 builds the flat one, main.py:553 routes every /api/{version} to the first, and jsonrpc.rst documents -32001.

Nothing broke on it, which is why three rounds of review went past it: getApiErrorMessage finds a reason at either depth and answers with the same string, so every assertion on a thrown message passed either way. A consumer branching on error.code or error.data.errname would have found it — against an appliance, not against us.

What changed

• TrueNasMessage['error'] declares the frame, with TrueNasErrorData for the payload beside ApiError in api-error.type.ts where a consumer can reach it. data is optional because the dataless codes are real: -32601, -32600, -32000.
• replyError takes that frame type and nothing else. It was the loose ApiError union, which admits both shapes. There is a @ts-expect-error proof that the flat shape no longer compiles, and widening the parameter again fails the build on an unused directive — so the hatch cannot be reopened quietly.
• notFound sends the envelope, and its test asserts the whole frame rather than the payload. Asserting the payload alone passes against the legacy shape, which is precisely how this survived.
• Two casts are gone: the one inside replyError, and the one in truenas-api.spec.ts, which was already writing a real dataless frame and casting past the type that disagreed with it.

A correction to this branch's own second commit. 5af28e8 widened extra to (string | number)[] | null and justified it with rpc.py's generic arm setting extra = None. The observation was right and the location was wrong — that is error.data.extra, not error.extra — and the element type was a guess. rpc.py types it list[Any], and a validation error puts the validation errors themselves there, so it is unknown[] | null.

554 tests, all eight gates exit 0.

[github-actions]

LOW — the chained order reopens, for unknown ids, the same silent-wrong-job failure the reverse order is refused for.

mock.query('core.get_jobs', …) does not apply filters — "the rows given are the rows answered" — so when it is the fallback, a read for an id no scripted job holds is answered with the whole row set, and trackJob takes jobs[0]:

c.mock.query('core.get_jobs', [fakeJob({ id: 999, state: JobState.Failed }) as …]);
c.mock.job('app.delete', { state: JobState.Success });

await lastValueFrom(c.api.trackJob(12345));   // emits job 999 and completes

The comment two lines above promises the opposite for this case — "with nothing behind it, an unknown id gets [], which is what middleware sends for an id it has reaped" — and that only holds when nothing was scripted first. refuseJobMethodTakeover exists precisely to stop job() "completing with a job it never started, silently"; this is the narrower version of it reached through trackJob.

Filtering the fallback's answer by readJobId before handing it back — or at least answering [] for a tracker's read the registry does not know — would close it without giving up the chaining.

[github-actions]

LOW — this is the third copy of "read the options element, then shape the answer by count / get". readOptions (line 176) already does the destructuring, and answerRead (line 201) already does the count → number, get → row-or-notFound, otherwise → array branch, including the notFound for an empty get.

query could be answerRead's caller rather than its twin:

const [, options] = …;                // -> readOptions(frame)
if (options?.count) return answer(frame, rows.length);   // -> answerRead's count arm

Worth collapsing because the two copies encode the same wire fact: if a fourth shape switch ever lands, or get's empty-result frame changes, only one of the two is going to be updated — and the core.get_jobs path is the one with the tests, so the mock.query copy is the one that would drift unnoticed.

[github-actions]

LOW — sent?.id turns the one precondition this half of the differential rests on into a timeout.

If the app.delete frame is not on the wire when this runs, messageId is undefined, the event goes out with no message_ids, callAndGetJobId's filter never matches, and pending never settles — so the failure surfaces as a 5s vitest timeout on the whole test rather than as "the request was never sent". That is the slowest and least legible way for this file to report a regression in dispatch/callAndGetJobId send timing, which is exactly what a differential spec is here to catch.

An assertion on the frame first (expect(sent).toBeDefined(), or a non-null read) keeps the failure pointed at the cause.

[github-actions]

Needs a human review. 3 finding(s): 3 LOW.

Nothing blocks, but this change is one a person should decide on:

• truenas-message.type.ts narrows TrueNasMessage['error'] to TrueNasErrorFrame; reachable via exported TrueNasApiClient.connection.messages$, so consumers reading msg.error.reason break.

[github-actions]

🎉 This PR is included in version 6.0.1 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀