test(byllm): def-only fixtures move into their tests (#9002 PR A) - #9014
Open
MalithaPrabhashana wants to merge 34 commits into
Open
test(byllm): def-only fixtures move into their tests (#9002 PR A)#9014MalithaPrabhashana wants to merge 34 commits into
MalithaPrabhashana wants to merge 34 commits into
Conversation
…bs#9002 PR A) Five def-only fixtures existed to host one `by llm()` function each, print the result, and let the test grep it back out of stdout. They become inline defs beside their tests, scripted with FakeLLM. The point is not the five deleted files. It is what the tests can now check. MockLLM replaces dispatch_no_streaming, four layers above make_model_params, so the prompt was built and thrown away; FakeLLM replaces only model_call_*, so the outgoing params are real and readable. Every converted test gained an assertion that was impossible before: incl_info examples and temperature actually reach the prompt a method's receiver fields reach the prompt a sem string reaches the prompt, which is the whole subject of that test an image argument is inlined as a data URL a call argument reaches the prompt The sem-string test is the sharpest case. Its fixture carried a comment saying that returning any number other than 120597 would mean "the semstring is not working correctly", but the mock was scripted with "120597", so the assertion could never fail. It now asserts the sem text is in the outgoing prompt. Three things this surfaced, all now load-bearing: `sem` is module-level, so those two defs and their model sit beside the test rather than inside it. Every non-str return needs a `{"schema_object_wrapper": <value>}` envelope, scalars included: a bare "120597" raises OutputConversionError for a `-> int`. That is what `typed()` in the annex is for, added here with its first callers. Under MockLLM the `-> int` in llm_semstrings had been returning a str, and nothing noticed. A `-> Image` parameter annotation resolves against module globals, so the import cannot live inside the test block. Ledger: fixtures 66 to 61. Asserts in test_byllm.jac 249 to 258; no fixture-side asserts existed in these five, so the suite-wide count rises by 9. Tests 89 passed, 1 skipped, unchanged.
…9002 PR A) Four more def-only fixtures inlined. Each keeps what it checked and gains an assertion on the outgoing request. a nested method's incl_info and receiver state reach the prompt was: "Personality.INTROVERT" appears in stdout now: the enum member is returned AND the diary entries passed through incl_info, plus the argument, are in the prompt the deprecated method= parameter still runs and warns once also asserts tools= is still sent alongside the deprecated method= an enum return is reconstructed as the member was: "POSITIVE" or "positive" appears anywhere in stdout now: the value is the member, is an instance of the enum, and the input reached the prompt a verbose model logs the call without leaking the api key now runs through a scripted real Model, so no key resolution is skipped, and it restores the loguru handler it adds instead of calling bare logger.remove() and leaving the logger stripped for whatever runs next An enum reply carries the member's VALUE, not its name, and a bare `enum { A, B }` auto-numbers from 1. Scripting `typed(Member.value)` keeps that out of the tests as a magic number. Worth flagging separately: the schema a bare enum generates asks a real model for 1 or 2, which no mock-backed test could ever have noticed. Ledger: fixtures 61 to 57. Asserts in test_byllm.jac 258 to 264. No fixture-side asserts existed in these four. Tests 89 passed, 1 skipped, unchanged.
…bs#9002 PR A) Image, webp and video. The image test gains the most: it used to set show_params on the mock, let the fixture log the whole params dict, and grep substrings out of that log line, including one assertion that only looked at the first 500 characters of stdout. It now reads the message blocks directly. an image parameter is sent as a system prompt plus a data url was: "'role': 'system'" and "{'type': 'text', 'text': 'solve_math_question" appear somewhere in a logged dict now: messages[0] is the system message, the first user block is text and introduces the caller, and the last is an image_url whose data url is a jpeg a webp image is accepted and its typed answer parsed was: two substrings of a printed Person repr now: the parsed object's three fields, plus proof the webp stayed webp rather than being transcoded a video parameter is sampled into image frames was: one long explanation string appears in stdout now: the frames actually reach the model, more than one, each an inlined image. The old test could not tell sampling from a no-op Two details worth keeping in mind for the rest of the conversion. A typed return appends schema guidance after the image, so an image block has to be selected by type rather than by position. And the video test still skips wherever opencv is absent, which is everywhere in CI: opencv appears in no dependency declaration in this repo, so this test and its 2 MB mp4 fixture have never run there. That is a separate decision, noted in jaseci-labs#9002. Ledger: fixtures 57 to 54. Asserts in test_byllm.jac 264 to 272. No fixture-side asserts existed in these three. Tests 89 passed, 1 skipped, unchanged.
…bs#9002 PR A) Five fixtures, one per routing strategy, each re-declaring two or three MockLLMs, a pool, a `by llm()` def, its own copy of `_as_any`, and its own hand-written router response. They become five tests over two shared helpers, `_pool(strategy, names)` and `_pool_reply(content)`. The seam moves with them. The fixtures pooled MockLLMs, which is the wrong object here: a pool never calls its members' dispatch, it dispatches through a litellm Router. The tests now pool real Models and patch router.completion, so the aliasing, the fallback chain and the async bridge are the real code. a fallback pool dispatches through its router a load-balancing pool shares one alias across every deployment asserts the alias directly instead of grepping "pool_alias:" out of a printed line a pool with every deployment exhausted raises rather than swallowing the fixture caught the exception itself and printed its class name, so the test only ever read a string the fixture chose. It now asserts the exception escapes the call cost-based routing dispatches through the async router path router init aliases deployments by strategy was seven substrings of eleven printed lines; now the model_list and fallbacks structures directly, including which deployment each alias points at, which the printed form did not carry Ledger: fixtures 54 to 49. Asserts in test_byllm.jac 272 to 274; the count moves little because five tests collapsed onto shared helpers while gaining checks. No fixture-side asserts existed in these five. Tests 89 passed, 1 skipped.
…bs#9002 PR A) Inlining a fixture moves its lines into the test file, so test_byllm.jac had grown 1,795 to 2,110 across the last four commits even as the suite shrank. That is the wrong direction for the one file that was already too big, and leaving the split until the end would have meant peaking near 2,400 and then tearing it apart in a separate review. The converted clusters move to files named after what they test, and every cluster from here lands in one of these rather than back in test_byllm.jac. test_prompting.jac what byLLM puts in the prompt and what it makes of the reply: sem strings, incl_info, temperature, receiver state, typed returns, enums, scalars test_media.jac image, webp and video arguments, asserted on the message blocks actually sent test_model_pool.jac pool aliasing, the fallback chain, the async bridge test_byllm.jac 2,110 to 1,561; keeps config, dispatch internals, tool and error-mapping tests until their own clusters convert Sizes: prompting 164, media 133, model_pool 126. Each carries a module docstring saying what belongs in it, so the next cluster has an obvious home. No test changed in this commit; blocks moved verbatim, with the five stale ModelPool docstrings dropped since the new test names carry that meaning and the module docstring carries the rest. Suite 199 passed, 1 skipped, matching the pre-refactor baseline exactly.
…PR A)
Reading test_media.jac back, the conversion had reproduced the thing it exists
to remove. The Pillow skip was pasted three times, the chain
llm.sent("messages")[0][1]["content"] three times, and the image-block filter
twice. Worse, two of its tests were near-duplicates: image_test and
with_llm_image had been separate fixtures, so both got converted instead of one
being recognised as subsuming the other.
Six helpers move to support_tests.jac, after FakeLLM and Script since they take
either. Asserting on the outgoing message is the point of this refactor, so the
vocabulary for it belongs in one place rather than being re-derived per file:
sent_messages(llm, n) the conversation as the model saw it
roles(llm, n) ["system", "user"], for shape assertions
user_blocks(llm, n) the user turn's content blocks, normalising the
text-only case to a single block so callers can
treat both alike
media_blocks(llm, n) just the inlined media, in order
data_url(block) the data url out of one media block
need(module, package) skip when an optional dependency is absent
test_media.jac then reads as three tests and nothing else: 133 lines to 115,
with no local helpers left.
The two image tests become one. It keeps both assertions of the weaker test in
stronger form, its stdout substring becoming a startswith on the extracted
block, and adds a roles() check the other lacked. Asserts in the file go 16 to
14; the two removed are the duplicated pair, not coverage.
Ledger: tests 199 to 198, one deliberate merge documented above. Asserts in
test_media.jac 16 to 14, in support_tests.jac 2 to 3. Fixtures unchanged at 49.
Suite 198 passed, 1 skipped.
…#9002 PR A) The same pass over the other two split files, for the same reason: both had re-derived what the annex should own. test_prompting.jac wrote `str(llm.sent("messages")[0])` six times. That becomes prompt_text(llm, n) in the annex, whose docstring says when to reach for it: use it when the question is whether something reached the model at all, and use roles(), user_blocks() or media_blocks() when the question is about structure. The two are easy to confuse and the weaker one reads fine at a glance, so the distinction is worth writing down where the helper lives. test_model_pool.jac had its own copy of _as_any and wrote `_as_any(pool._router)` seven times. ModelPool builds the router in postinit and never exposes it nominally, so that cast is unavoidable; having it once as router(pool) is not. The local _as_any is gone. Both files also regained the module docstring that jac fmt had relocated when they were first split out, so each one still says what belongs in it. No test changed. Suite 198 passed, 1 skipped.
…ixtures (jaseci-labs#9002 PR A) Four more, and the params pair is the clearest case in the suite for why this is worth doing. The old test read the outgoing request by slicing a dict out of captured stdout with find("{") and rfind("}"), then yaml.safe_load-ing a Python repr. `llm.seen[0]` is that dict. an outgoing request carries the model, messages, tools and response format the second turn replays the tool result back to the model temperature is sent when the caller sets one three tests off one shared _wind_report() setup, replacing two fixtures that differed only by temperature=0.3. They now also assert that the tool's sem string is its description, that the parameter signature reaches the model, and that finish_tool is appended alongside the caller's tool. None of that was checkable through a repr. a streaming call yields the answer in pieces, not all at once was: one sentence appears somewhere in stdout, which a non-streaming implementation would also satisfy. Now asserts the call hands back an iterator, that it arrives in more than one piece, and that the pieces reassemble into the answer. plain text instead of finish_tool is re-prompted, not accepted was: two printed lines. Now asserts the typed Chat, that there were exactly two turns, and that the script was drained. Two new files, since both subjects will take more tests as their clusters convert: test_streaming.jac and test_tools.jac. `report` is a Jac keyword and cannot name an ability, so the shared helper's inner def is wind_report. Ledger: fixtures 49 to 45. Tests 199, unchanged: four removed, five added. Asserts up across the four subjects from 24 to 33. Suite 199 passed, 1 skipped.
…9002 PR A) The first sentinel fixture, and the first time the fixture-side assert count moves. conversation_param.jac asserted twelve things about itself and then printed "CONV_PARAM_PASS"; the test greped for that string. Twelve real checks behind one boolean, with the runner unable to say which one failed. All twelve are now visible assertions in test_conversation.jac, across three tests named for what they establish: the caller's list is mutated in place, not replaced id(history) unchanged; a pre-seeded dict survives the round trip scaffolding and finish_tool are filtered out, a real tool result is not no system message; exactly one tool result, keeping its name and its tool_call_id; no finish_tool leaking under any role a second turn grows the same list and keeps every earlier message the list grows; three user messages across both turns; the pre-seeded and turn-one messages both survive Two assertions are new: the values the calls returned. The fixture printed a sentinel and never checked them, so a by-llm() that wrote history correctly and answered wrongly would have passed. Also here, because the branch picked up checker fix jaseci-labs#8314 from main: that made MTRuntime(messages=[Message(...)]) an E1053 and broke jac check on four pre-existing constructions in test_byllm.jac. They become mk_run() calls, which is where this refactor was taking them anyway. Verified those errors exist on the branch without this commit's changes. Two of the four passed resp_type=None. mk_run does not accept that, and widening it only moves the error inward: MTRuntime's own resp_type is annotated `type` while callers pass None at runtime. Both tests are about tool resolution and are indifferent to the response type, so they take the default. The annotation mismatch in MTRuntime is left alone and noted for jaseci-labs#9002. Ledger: fixtures 45 to 44. Fixture-side asserts 66 to 53. Tests 199 to 201, one sentinel test out and three real ones in. Inline MTRuntime constructions 4 to 0. Suite 201 passed, 1 skipped.
… PR A)
Comment-only. Measured first rather than assumed: the seven new and changed
files carry 111 docstring lines and 5 comment lines over 1,220, a 10% share, so
this is a narrow pass over the over-length ones, not a sweep.
Nine docstrings lose their narrative clause and keep their contract. The pattern
in each is the same: a rule with its history attached, where the history is what
the test used to be. "fails here instead of passing on a substring of a log
line" becomes "Assert on the message blocks byLLM actually sent, not on a
rendered string." A why turned into an imperative fits on the line and the
caller still cannot get it wrong.
What deliberately stays, whatever the length, because a caller who has not read
it writes a broken test:
typed() every non-str return needs the schema_object_wrapper envelope,
scalars included, and an enum takes the member's value not its
name. Both are things you only learn by hitting
OutputConversionError.
prompt_text() when to reach for it and when to use roles(), user_blocks() or
media_blocks() instead. The two are easy to confuse and the
weaker one reads fine at a glance.
user_blocks() that a text-only request is normalised to one block
fail() how a mid-stream drop is scripted
response() why usage is omitted rather than empty
the annex's module docstring and its two usage examples
Untouched on purpose: docstrings on `by llm()` defs and on any function passed
in tools=[], because byLLM sends those to the model as prompt and tool
description. Rewriting one changes behaviour. Off-limits list built from the
code: _wind_speed, get_mood, get_weather, lookup, plus the two `by llm()` defs
in test_streaming.jac and test_tools.jac. Also untouched: every `sem` statement,
and every test name.
Verification: triple-quote count identical per file on both sides, so no
docstring was added, deleted, split or merged and every one was rewritten in
place. Docstrings stripped and the remainder compared per file: zero files with
code changes, zero with comment changes. Zero adjacent docstrings. jac check
clean. Docstring lines 111 to 99. Suite 201 passed, 1 skipped.
12 tasks
…i-labs#9002 PR A) Three fixtures sharing one skeleton, holding 23 assertions between them behind three _PASS greps. All 23 are now visible in test_streaming.jac, and the in-fixture assert count drops 53 to 30. a logging stream with no tools yields chunks then one usage event the fixture listed the event types it did not want and asserted that list was empty. The test asserts the set of types is exactly {chunk, usage}, which also fails when a new event type appears. a logging stream with tools reports the call, the result, then the answer tool_call names the tool, tool_result carries it, steps_done fires, the answer reassembles, and the last tool event precedes the first chunk. a str answer that skips finish_tool still arrives on the chunk channel Four helpers move to the annex rather than living in the file, because four more streaming fixtures are still to convert and will want them: stream_events() drains a logging stream and checks every item is a StreamEvent, events_of() selects one type, event_types() gives the order, chunk_text() reassembles the chunk channel. Ledger: fixtures 44 to 41. In-fixture asserts 53 to 30. Tests 201, unchanged: three sentinel tests out, three real ones in. Suite 201 passed, 1 skipped.
…PR A) Checking this branch against the team tests policy, point 4 (table-driven tests for multiple inputs instead of one test per input) was the one it was failing. Five model-pool tests and two media tests were the same assertion over different inputs, written out one per input. jaseci-labs#8930's acceptance table asks for the pool one specifically. test_model_pool.jac, 5 tests to 3 fallback, simple-shuffle and cost-based-routing all assert "the pool dispatches through its router and answers", so they become one table over (strategy, deployments, expected alias, async path). The two that stay are different shapes: exhaustion raises, and router init inspects structure. test_media.jac, 3 tests to 3, but the pair that overlapped is now one table jpeg and webp were both "the file is inlined in its own format under a system prompt", now a table over (file, expected mime). The typed-answer parsing that was tangled into the webp test is its own test, since it asserts something else entirely. Video stays separate: it asserts frame sampling, not format. Writing the pool table surfaced a fact the five separate tests had encoded wrongly by omission: only "fallback" gives each deployment a unique alias. Every load-balancing strategy shares "pool-model", cost-based-routing included. The old cost-based test never checked its alias, so nothing said so. Ledger: tests 201 to 199, back to the baseline count, with the cases they cover unchanged and one alias assertion added. Fixtures unchanged at 41. Suite 199 passed, 1 skipped.
…r ships (jaseci-labs#9002 PR A) a structured return cannot stream without a handler to consume it a stream survives being consumed from a foreign context the two consumption paths, a copied contextvars Context and a thread-pool worker, assert the same thing over different inputs, so they are one table rather than two tests. Both also assert the invocation id does not leak into the caller's Context, which is what the original regression was. an image parameter carrying a sem still encodes, and the sem is dropped The third one found a product gap. `sem <fn>.<param>` beside an Image parameter never reaches the model: the user turn is the caller name and the image, and the sem text appears in no message, system or user. A function-level sem does reach the prompt, which test_prompting.jac asserts, so this is specific to parameter sems. The fixture declared such a sem and asserted only that the returned string contained "hot air balloon", so it could not have noticed. Its test was named for the crash it was guarding against, which is still guarded. Rather than write a passing test that implies the sem works, the test asserts what is true today and pins the gap with a failing-when-fixed assertion: the sem text must be absent, with a message telling whoever fixes it to invert the assertion. Reported on jaseci-labs#9002. Ledger: fixtures 41 to 38. In-fixture asserts 30 to 23. Tests 242, unchanged: three removed, two added plus one table covering the two paths the third had. Suite 199 passed, 1 skipped.
stream_retry (12 asserts) and streaming_conversation (6) were the last fixtures
hiding assertions behind a _PASS grep. In-fixture asserts drop 23 to 5.
a mid-stream drop is retried only when the error is transient
one table over three error kinds: a transient drop recovers on the same
model with the partial discarded and stream_reset surfaced; a
non-transient error is not retried; an unending transient one is bounded
at three calls and then propagates. The fixture wrote these as three
separate blocks with three sets of hand-rolled generators; the annex's
fail(error, content, after) scripts a mid-stream drop directly, so the
table needs no generators at all.
the inter-chunk read timeout is set for streaming only
a streamed answer is written back as dicts, whatever the history was seeded with
one table over empty, dict and Message seeds
a second streamed turn grows the same history and keeps the first answer
Two counts in the old fixtures were assertions nobody had made. An unending
transient error emits a stream_reset per retry, so that case has two, not zero;
the fixture asserted the call count there and never the resets. And a streamed
tool-using turn writes two assistant messages, the turn that called the tool and
the answer, so two turns give four.
Test count 242 to 244, and the increase is deliberate: each of these two
fixtures bundled two subjects. stream_retry tested retry behaviour and the
timeout parameter; streaming_conversation tested seed-type write-back and
multi-turn growth. Splitting by subject is the point, and the cases within each
subject are tables rather than one test per case.
Ledger: fixtures 38 to 36. In-fixture asserts 23 to 5. Tests 242 to 244,
justified above. Suite 201 passed, 1 skipped.
…eci-labs#9002 PR A) Three fixtures, two tables. react_max_iterations.jac and react_max_iterations_finish_tool.jac differ by one line: past max_react_iterations one model answers in plain text and the other returns the answer through a finish_tool call, which is what Anthropic does. jaseci-labs#8930 flags this pair specifically. One table over (delivery, final reply) asserts both yield the same answer, that neither re-runs the tools, and that the last turn really was the forced-answer prompt. The old pair asserted the tool counts by printing them and grepping "WIND_TOOL_CALLS: 1" out of stdout, and only one of the two checked the forced prompt at all. on_iteration_callback.jac ran two hooks past each other in one fixture, sharing a module-level counter that had to be reset between them by hand. ABORT and ABORT_WITH_SUMMARY differ only in whether a summary call follows, so they are one table over (action, extra replies, expected result), each with its own tool-call list rather than a shared counter. Ledger: fixtures 36 to 33. Tests 244 to 243: three tests out, two tables in. Suite 200 passed, 1 skipped.
… PR A)
with_llm_type.jac tested that a nested obj return (University.Department, which
nests a Person) comes back as real instances. Its test read three printed reprs
out of stdout, one of them as a negative: a 130-character literal of a codegen
artifact that must NOT appear. That assertion silently stops testing anything
the moment the artifact's formatting changes by a character.
The test now asserts what the negative was really guarding: both levels come
back as instances, isinstance on each, and the nested Person's field is right.
It also runs the call twice against the same list, which is what the old
"count the repr twice in stdout" was checking.
Two fixtures I had classified as def-only are not, and stay:
async_by_llm.jac test_byllm.jac compiles it with JacProgram to
check codegen emits await Jac.acall_llm; it is
never run
builtin_llm_no_override.jac exists precisely to be a module with no
`glob llm`, proving the name resolves as a
builtin. Inlining it into a file that declares
models would defeat the test
That leaves 9 def-only fixtures, not 11.
Ledger: fixtures 33 to 32. Tests 243, unchanged. Suite 200 passed, 1 skipped.
…ci-labs#9002) jac-check goes red on jac/jaclang/byllm/types.impl/media.impl.jac with 13 errors, all the same two: "Type ImageFile has no attribute format", "has no attribute save", and the fmt argument they feed. PIL's stubs type Image.open() as ImageFile.ImageFile. Both attributes exist at runtime, inherited from Image.Image, but the checker does not see through that inheritance, so every open_image() result and every PIL image passed in by a caller trips it. The file is not in .jacignore, so it is meant to pass. Five handles gain an `any` annotation and their fmt gains `str`, which is the escape this repo already uses for values whose type comes from a foreign stub. No behaviour changes: the same objects, the same calls. Product code in a test-refactor branch, so it is its own commit. It surfaced on this branch rather than on main because the errors only appear once PIL actually resolves, which happens in the jac-check lane where pillow is installed, and because the branch now annotates an Image parameter at module level rather than only inside test bodies. jac check on the file: 13 errors to 0. Suite 200 passed, 1 skipped.
…res (jaseci-labs#9002 PR A) a list-of-obj return parses, and each element feeds the next call llm_mail_summerize.jac scripted five Email objects and five summaries, and its test asserted the five summary strings appear in stdout. Those strings were typed into the fixture, so the assertions were true by construction. What the fixture was actually exercising is that a list[Email] return comes back as instances and that each one can be passed straight into a second by-llm() call. The test asserts that: two elements rather than five, since five identical iterations prove nothing two do not, each an Email instance, the fields intact, and the email passed in reaching the prompt of the call that summarises it. That last one is what the old assertions could not reach at all. a streaming react call runs its tools, then streams the answer was: "29-10-2025", "100" and "Test passed!" appear somewhere in stdout. "100" is satisfied by any 100 anywhere, including a model that made the number up without calling the tool. Now the tool results are asserted in the conversation the final turn was built from, so a tool that never ran fails. Also asserts the call hands back an iterator, that it arrives in more than one piece, and that the pieces reassemble. Ledger: fixtures 32 to 30. Tests 243, unchanged: two out, two in. Suite 200 passed, 1 skipped.
…#9002 PR A) every accepted image source becomes a usable url image_types.jac constructed eleven Images and printed a label before each one. The test asserted the eleven labels appear in stdout, so it proved the prints ran, not that any Image was usable: a constructor returning a broken url passed. One table over (source, expected url prefix) asserts what each source produces. A remote or data url passes through untouched and everything else is read and inlined, which is the distinction the labels never named. No model is involved; this was never an LLM test. the model can be any expression, and a bound call keeps its tools by_expr.jac used `by models["text-gen"]`, a subscript rather than a bare name, with the second entry already bound through a tools= call. Its test asserted eight strings from stdout, including the executed code echoed by the tool's own print. The test asserts the two calls return their answers, that the tool bound inside the expression really ran and with what, and that the unbound model sends no tools while the bound one sends its own. The tools payload was invisible to the old shape. Two checker findings while writing these, both kept rather than worked around. Calling a FakeLLM with tools= returns a BaseLLM, which carries no seen/sent, so each recorder is bound by name before it goes in the dict. And Image.url is a union whose members do not all have startswith, so the table coerces it. Ledger: fixtures 30 to 28. Tests 243, unchanged: two out, two in. Suite 200 passed, 1 skipped.
…PR A)
llm_typed_retry.jac was the best fixture in the set: it used MockRawResponse so
malformed text really reached parse_response, and it recorded prompts so the
tests could assert the corrective feedback. Converting it is mostly a port, and
FakeLLM needs no special reply type for it, since every reply already goes
through the parser.
broken json is regenerated, and the retry prompt shows what was wrong
sync and async in one table
retries are bounded, and the error reports how many were spent
default budget and an instance limit of 1, one table
only a conversion failure is retried
a ValueError propagates on the first attempt; a str return accepts even
an empty answer without retrying
the default retry budget comes from config
One case is deliberately dropped rather than ported. The fixture asserted that
a plain string for a `-> Person` return comes back as that string with no
retry, described as "MockLLM passthrough". That is not byLLM behaviour, it is
MockLLM bypassing parse_response: the fixture had encoded the bypass as the
expected result. Under real parsing the same input fails to convert and
retries, which the bounded-retries test above now proves directly, since it
drives the retry budget with exactly that shape of unparseable text.
Tests 243 to 245. The two removed covered eight scenarios between them in two
blocks; the four added split them by subject, with sync/async and the two retry
budgets as tables. One scenario is gone for the reason above.
Ledger: fixtures 28 to 27. Suite 202 passed, 1 skipped.
jaseci-labs#9002 PR A) The jac.toml here is the subject, not scaffolding, so this fixture keeps its directory. byLLM resolves `[byllm] system_prompt` from the module's own location, so the two defs have to sit beside that file; patching a config dict would skip the loader, which is half of what the test checks. What the fixture does lose is everything that was not the subject: the MockLLM with its show_params and verbose flags, the placeholder api_base and api_key, and the entry block that ran the calls and printed the results. It is now a `glob llm: any = None;` and two defs, and the test assigns the model. The assertions get sharper for it. The old test read four substrings out of a logged params dict, including the placeholder api_base, which only proved that a config value was echoed into a log line. The test now asserts the jac.toml system prompt is in the first call's system message, that the per-call system_prompt is NOT in it, and that the second call's system message carries both. That middle assertion is new and is the one that would catch a per-call prompt leaking backwards into earlier calls. Ledger: fixtures 27, unchanged, since this one legitimately stays a file. Tests 245, unchanged. Suite 202 passed, 1 skipped.
…ci-labs#9002 PR A) The last def-only fixture, and the clearest case of the shape this PR removes. model_pool_streaming_fallback.jac computed eight booleans, printed them as "name:True", and the test greped for the literal strings. The fixture did the asserting; the test checked that the fixture had printed the word True. Two tests, not a table: the scenarios have opposite outcomes. a pool that fails before streaming falls back to the next deployment the primary raises before yielding, so the fallback takes over and the output is clean. Three printed booleans (primary_tried, fallback_tried, order_correct) collapse into one assertion on the list of models tried, which carries membership and order together. a pool that fails mid-stream raises rather than splicing in a fallback once a chunk is delivered the caller has already seen partial output, so falling back would concatenate partial-A with a whole B. The yielded guard raises instead. Asserts the exception reaches the caller, that what was already streamed is still delivered, that the fallback's text never appears, and that the fallback was not tried at all. The seam stays at `router._completion`, the per-model call the fallback loop drives with fallbacks=[] so the Router cannot recurse into the primary. That is the thing under test, so it is the right place to fake. The pool holds real Models rather than MockLLMs, matching the other pool tests: a pool never calls its members' dispatch. Ledger: fixtures 27 to 26. Tests 245 to 246, one test out and two in, split because the scenarios assert opposite outcomes. Suite 203 passed, 1 skipped. PR A is complete: every def-only fixture is now inlined. The three tests still driving a fixture are the graph programs (method_tool, math_poem_agents, streaming_visit_by), which are PR B.
MalithaPrabhashana
marked this pull request as ready for review
September 7, 2026 13:44
Collaborator
Author
Mutation testing: does this suite actually catch breakage?Assert counts are a proxy. The real question is whether these tests go red when byLLM breaks, so I broke it. Eleven one-line mutations to byLLM product code, each applied on its own, the suite run, then reverted. Every mutation was also run against
Eleven of eleven caught here. The old suite missed two. What this does and does not showIt does not show the old suite was blind to prompt regressions. It catches a dropped argument and dropped media, because a stdout grep for a value does fail when that value never reaches the model. My earlier framing was too strong. The accurate statement is narrower: the old suite catches breakage that changes the answer, and misses breakage that only changes the request or the delivery. The two misses are exactly those shapes.
Method, and two corrections to itMutations were applied to product code only; the suite under test was swapped between this branch's version and Two things went wrong in the first attempt and are worth recording, because both produce a confident wrong answer:
Not coveredI did not mutate the react loop's iteration accounting, compaction, or telemetry emission. Those clusters are |
christianwilkins
left a comment
christianwilkins
left a comment
There was a problem hiding this comment.
Reviewed head 624ef70124093e80b5ecafde86e1452f75b884d7, including all removed fixtures, the replacement tests and support helpers, the remaining main test file, and the media typing changes.
P3 coverage suggestion inline: the two streamed conversation turns use identical answers, so the assertion that the first answer survives cannot distinguish it from a copy of the second answer. Use distinct scripted answers to preserve that part of the old regression coverage. I found no blocking production-code defect in the patch; no source changes made during review.
Local validation with the optional byLLM dependencies installed: 26 tests passed across model-pool, streaming, tools, conversation and typed-retry; the separate prompting file passed all 13 tests. Independent media/main-file validation remains incomplete: I stopped media compilation after excessive memory use, and a separate main-file attempt reached a sampled 38.9 GB footprint without finishing. I have not established that this behavior is caused by the PR. The earlier broad run without all optional dependencies is not counted as a successful validation of this refactor.
Current-head CI is green and the PR is conflict-free. I checked the package job log (203 passed, 1 skipped) and the separate sealed MTIR log (29 passed). Commenting rather than approving because of the local validation limit and the coverage suggestion; author next step is the distinct-answer test improvement, with maintainer review of the CI evidence for the remaining test scope.
|
|
||
| test "a second streamed turn grows the same history and keeps the first answer" { | ||
| history: list = []; | ||
| llm = FakeLLM(replies=_stream_replies() + _stream_replies()); |
There was a problem hiding this comment.
[P3] Give the two turns different scripted answers. _stream_replies() + _stream_replies() makes first == second, so the two assertions at lines 173–174 check the same content. Replacing the first stored answer with a copy of the second would still satisfy those assertions and the message counts. The removed fixture used different answers; retaining that distinction would actually test that the first answer survives the second turn.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #9002. Ready for review.
What this does
43 of byLLM's 66 fixtures were
def-only: a whole.jacprogram whose entire content was oneby llm()function, a model to answer it, awith entryto run it, and aprintso the test could grep the answer back out of stdout. Zeronode/walker/edgebetween them. This PR moves those into the test files, scripted with the annex'sFakeLLM.MockLLMrun_fixturecalls51 files, +1,855 / -2,811, across 27 commits. Suite goes 199 passed / 1 skipped to 203 passed / 1 skipped.
Why
MockLLMoverridesdispatch_no_streaming, four layers abovemake_model_paramsand aboveparse_response. Under it the prompt is built and thrown away, and the reply is never parsed. Two things reproduce what that costs:semstrings return identical output, so a regression dropping every semstring, everyincl_infoexample, or the whole JSON schema passed this suitellm_semstrings.jacdeclared-> intand had been returning astrin CI for as long as it existedFakeLLMreplaces onlymodel_call_*, the network hop, so both halves are the real code and the outgoing request is readable asllm.sent(...). Every converted test keeps the assertion it had and gains one it could not previously make.Shape
Before, two files:
After, one:
The last two assertions are new.
temperature=0.7and twoincl_infoexamples were set in the old fixture and checked by nothing; deleting either left the test green.What stays a fixture
routing_*,math_poem_agents,streaming_visit_by,method_tool):node/walker/edgewhere the shape is the subject. PR B.test_mtir_integration.jac, never run.system_prompt_override/: byLLM resolves[byllm] system_promptfrom the module's own location, so the defs have to sit beside thatjac.toml. It is now a pure program (no model, no entry block, no prints) and the test assigns the model.Files split as the work went
Inlining moves fixture lines into the test file, so
test_byllm.jacgrew before it shrank (1,795 to 2,110 at the peak). Rather than let it peak near 2,400 and tear it apart in a later review, each converted cluster lands in a file named for what it tests.test_prompting.jactest_media.jacImagesourcetest_model_pool.jactest_streaming.jacstream=Truecall yields,logging=TrueStreamEvents, mid-stream retry, foreign-context consumptiontest_tools.jacon_iterationhookstest_conversation.jacconversation=<list>writes backtest_typed_retry.jacCoverage
The risk in this PR is a fixture taking its assertions with it: 66 of the suite's assertions lived inside fixture files behind 10
_PASSgreps, and deleting one moves no test count. Every commit reports the assert count before and after, and every sentinel conversion writes its fixture-assert to new-home mapping into the commit message before the file is deleted.In-fixture asserts 66 to 2. Asserts in test files 776 to 844. Test blocks 242 to 246, with every increase justified in its commit: a fixture that bundled two subjects becomes two tests.
Per the team tests policy, same-shape cases are tables rather than one test per input. Six went in that way: the react delivery pair, the
on_iterationactions, the stream-retry error kinds, the conversation seed types, the pool strategies, and the image sources.One test was deliberately merged:
image_testandwith_llm_imagewere separate fixtures testing the same thing, so both got converted before the duplication was noticed. The survivor keeps both of the weaker one's assertions in stronger form and adds aroles()check neither had.Defects the conversion surfaced
Six, each of which the old shape structurally could not see. All recorded on #9002.
semnever reaches the model.sem fn.paramis accepted and silently dropped; a function-levelsemdoes reach the prompt.fixtures/image_with_sem.jacdeclared one and asserted only the returned string, which the mock had been scripted with. The replacement test asserts today's behaviour and fails when this is fixed, with a message saying to invert it.llm_typed_retry.jacassertedMockLLM'sparse_responsebypass as expected behaviour: a plain string returned for a-> Personwith no retry, described in the fixture as "MockLLM passthrough". Under real parsing the same input fails to convert and retries. That case is dropped rather than ported.cost-based-routingaliases aspool-model, notpool-primary. Onlyfallbackgives unique aliases. The old cost-based test never checked its alias.stream_resetper retry, two of them. The fixture asserted the call count and never the resets.MTRuntime.resp_typeis annotatedtypewhile callers passNoneat runtime.enum { A, B }generates a schema asking a real model to answer1or2. No mock-backed test could notice, since the mock returned the member object directly.Two commits are not fixture conversion
fix(byllm): PIL handles inmedia.impl.jac.jac-checkwent red with 13 errors: PIL typesImage.open()asImageFile, whose inherited.formatand.savethe checker cannot see. The file is not in.jacignore, so it is meant to pass. Five handles gain ananyannotation. No behaviour change. It surfaced on this branch because the errors only appear once PIL actually resolves, which needs thejac-checklane where pillow is installed, and because the branch now annotates anImageparameter at module level. Carries a release-note fragment.A docstring trim, comment-only, its own commit. Measured first: 111 docstring lines and 5 comment lines across 1,220, a 10% share, so a narrow pass over the over-length ones rather than a sweep. Nine lost a narrative clause and kept their contract. Untouched on purpose: docstrings on
by llm()defs and on any function passed intools=[], since byLLM sends those to the model as prompt and tool description; also everysemand every test name. Verified by identical triple-quote counts per file, and by stripping docstrings and comparing the remainder: zero files with code changes, zero with comment changes.CI
Green, 24 checks.
test-packages-and-docs, the lane that runs this suite, reports the count rather than only an exit code:Next
PR B converts the 6 graph fixtures, whose mechanics are already verified and written up in #9002 section 4. PR C moves the remaining test files onto the annex, renames
support_tests.jactosupport.jacto match the repo convention, and landsTESTING.md.