test(byllm): graph fixtures become pure programs (#9002 PR B) - #9049
Open
MalithaPrabhashana wants to merge 40 commits into
Open
test(byllm): graph fixtures become pure programs (#9002 PR B)#9049MalithaPrabhashana wants to merge 40 commits into
MalithaPrabhashana wants to merge 40 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.
…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.
…abs#9002 PR B) Five routing fixtures shared 47% of their lines verbatim: each declared its own graph, its own MockLLM subclass, and its own copy of a parser that reads the Candidates block out of the routing prompt. They become one fixtures/routing_graph.jac holding only shape, and test_routing.jac holding the five subjects. The parser moves to the annex as routing_candidates(), with RoutingLLM(pick=) on top of it: a FakeLLM that reads the prompt and answers with the handles its pick function chooses. Scripting handles by hand would be brittle, because they are generated from the candidate set (a lone Agent stays Agent; two siblings become Agent_alpha and Agent_beta), and it would discard the property these tests exist for, that the prompt carries enough for a chooser to decide. Two assertions get stronger. The param-leak test captured mt_run.call_params, an intermediate, and printed LEAKED:select; it now asserts on the outgoing params dict, which is what a real provider would receive. The edge-choice test now asserts both successors are offered and that the edge attribute is rendered in the candidate line, not only that the right node was visited.
…seci-labs#9002 PR B) The five routing_*.jac fixtures and test_visit_routing.jac are replaced by fixtures/routing_graph.jac and test_routing.jac, landed in the previous commit. Ledger: fixtures 27 to 22. Tests 246, unchanged: the five stdout-driven tests became five tests asserting on values and on the routing prompt. Suite 203 passed, 1 skipped.
…jaseci-labs#9002 PR B) Three more, and with them the last test in the suite that read its result out of a fixture's stdout. a bound method can be a tool, and runs against its own receiver method_tool.jac declared a `node` but tested nothing graph-shaped: the subject is that `self.add` can be a tool. It is now an obj beside its test, and the test asserts the tool ran against the receiver it was bound to, which the printed "Calculator.add called with 12, 34" only implied. a supervisor routes to the agent whose speciality matches the request math_poem_agents.jac becomes fixtures/agent_graph.jac. One run makes two model calls of different kinds through one model, so RoutingLLM answers the routing call from the prompt and falls through to the scripted replies for the agent's own `by llm()`. a streaming routing decision reaches the handler and still selects an edge streaming_visit_by.jac folds into routing_graph.jac as stream_router. RoutingLLM gained the streaming path for it. Three things worth writing down, each of which failed first: The supervisor walker triggers on a Team node, not Root. `root` accumulates across tests in a file, so a Root-triggered walker routed over every other test's leftover nodes. Its picker reads the request out of the prompt rather than matching the whole thing. Handles are derived from node type names, so a bare `"poem" in prompt` is true for the math request too, because PoemAgent is on the candidate list. RoutingLLM's streaming override yields rather than returning: a Jac generator cannot return a value, so the non-routing fall-through yields from super instead. Ledger: fixtures 22 to 19. Tests 246, unchanged. Zero tests now read a fixture's stdout. Suite 203 passed, 1 skipped.
…aseci-labs#9002 PR B) Mutation testing found this: flipping visit_routing's routing-key filter to `if True`, so select/intent/incl_info survive into the call, left the suite green. The test meant to catch that asserted they were absent from the outgoing params dict, and make_model_params assembles that dict from a fixed set of keys, so a routing key could never appear there whatever the filter did. The assertion was vacuously true. It now captures the MTRuntime the routing call built, through the same Jac.call_llm seam the dispatch test uses, and asserts there. Re-run against the same mutation: caught. This was a regression I introduced in the conversion. The original fixture asserted on mt_run.call_params and was right to; I moved the assertion to what looked like a stronger layer and it was a weaker one.
…PR B) Comment-only. Measured first: 121 docstring lines and 37 comment lines across 1,168, a 14% share, so a narrow pass over the over-length ones rather than a sweep. Five lose a narrative clause and keep their contract. The pattern is the same as in PR A: a rule with its justification attached, where the justification is what the old shape used to do. "Reading the prompt rather than scripting a reply is what keeps these tests honest" becomes nothing; the sentence above it already says what pick() takes and returns. What stays, because a caller who has not read it writes a broken test: routing_candidates() on how handles are generated and that they must be read rather than written; RoutingLLM on the pick signature and the fall-through for prompts with no candidates; the annex's usage examples; typed() on the envelope; prompt_text() on when to reach for it instead of roles()/user_blocks(). Untouched on purpose: docstrings on `by llm()` defs and on anything passed in tools=[] (add, get_data, get_live_wind_speed, get_mood, get_speed_unit, lookup), because byLLM sends those to the model as prompt and tool description; also every sem statement and every test name. Verification: triple-quote count identical per file, so no docstring was added, deleted, split or merged. Docstrings stripped and the remainder compared per file: zero files with code changes, zero with comment changes. Suite 203 passed, 1 skipped.
12 tasks
MalithaPrabhashana
added
the
skip-release-notes-check
Collaborator
Author
|
Added There is also a stacked-PR artifact behind the red check: because this branch is based on #9014, its diff against |
…nd weak (jaseci-labs#9002 PR B) Re-ran mutation testing over `visit_routing.jac` with only the converted routing tests running. Three assertions passed under a mutant they claimed to guard: incl_info dropped the walker's own repr already carries `user_input=` into every routing prompt, so a bare "Agentic AI" substring matched with incl_info gone. Now asserts the rendered `User input = ...` entry. ability filter removed `Agent` carries one ability per walker type and the prompt lists only the one that fires. Nothing read that line. Now asserts the firing ability is named and the other three are not. select=1 not truncating the picker returned one candidate anyway, so the truncation never ran. The picker now returns both. Also corrects the dedup test's rationale, inherited from the fixture it replaced: node archetypes do not define value equality (probed: two Agents with identical fields are `!=`), so value-based dedup is indistinguishable from identity-based dedup and the old comment described a guard that does not exist. Mutations: 11 run, 8 caught, 2 equivalent, 1 dead branch (the "in" edge direction, which no test on this branch or on main builds). Tests 232 -> 232; asserts in test_routing.jac 17 -> 19.
…test/byllm-graph-fixtures
christianwilkins
left a comment
christianwilkins
left a comment
There was a problem hiding this comment.
Reviewed 781adb777225bae708ef9daf0229a81cfc2da57f, including the full patch against main, the shared #9014 changes, all 15 additional graph-test file changes, the routing implementation, and existing discussion. No additional blocking finding in the graph refactor. The 063552a90 and 629944fd0 follow-ups make the assertions observe routing parameters at the runtime boundary, force select=1 to truncate a two-choice response, and distinguish the explicit incl_info entry from the walker's repr.
Independent local validation, with CI-matching byLLM dependencies installed in a temporary project site: test_routing.jac plus test_tools.jac passes all 11 tests. Removing only chosen = chosen[:1] from the real routing implementation makes the arbitrary-node-list test fail: it visits ['alpha', 'beta'] where one visit is required. The original source was restored and the worktree is clean. The initial run without those dependencies skipped both files and is not counted as validation. git diff --check passes.
Current-head CI is green and the PR is conflict-free. I checked the logs: the package lane records 203 passed, 1 skipped; the separate sealed MTIR lane records 29 passed. No fixes were made during this review.
Status: the graph increment has no new author fix requested, but this remains stacked on the still-open #9014. Land/reconcile that dependency first, then recheck this PR's final diff and checks. I am leaving a comment review while that dependency is outstanding; no merge was performed.
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. Stacked on #9014 (PR A), which must land first: this branch is based on it and shares the annex it built.
What this does
PR A moved every def-only fixture into its test. This one takes the graph fixtures, the ones whose
node/walker/edgeshape genuinely is the subject. They stay files, but stop being tests: no model, no entry block, no prints, no asserts. The test assigns the model, builds its graph and asserts.print(orassertrun_fixture/load_fixturecalls15 files, +465 / -579, 5 commits. Suite 203 passed, 1 skipped, unchanged throughout.
No test in the suite now reads its result out of a fixture's stdout.
The routing cluster
Five
routing_*.jacfixtures shared 47% of their lines verbatim. Each declared its own graph, its ownMockLLMsubclass, and its own copy of a parser that reads theCandidatesblock back out of the routing prompt. They become onefixtures/routing_graph.jacholding only shape, andtest_routing.jacholding the seven subjects.The parser moves to the annex, with a model on top of it:
Scripting handles by hand would be brittle, because they are generated from the candidate set: a lone
AgentstaysAgent, two siblings becomeAgent_alphaandAgent_beta. Reading them also preserves the property these tests exist for, that the prompt carries enough for a chooser to decide.RoutingLLMfalls through to the scripted replies when a prompt has no candidates, because a graph whose nodes also make their ownby llm()calls sends both kinds through one model.Mutation testing
Same treatment as #9014. Four mutations to
visit_routing.jac, each applied alone, suite run, reverted.select,intent,incl_info) leak into the callJac.call_llmand callsmodel.invokeR1 found a regression I introduced. The test meant to catch it asserted the routing keys were absent from the outgoing params dict.
make_model_paramsassembles that dict from a fixed set of keys, so a routing key could never appear there whatever the filter did: the assertion was vacuously true. The original fixture asserted onmt_run.call_paramsand was right to. The test now captures the MTRuntime through the sameJac.call_llmseam the dispatch test uses. Re-run against the same mutation: caught.R2 is an equivalent mutant, not a coverage gap. Jac nodes never compare by value:
Plain() == Plain()isFalseeven with no fields. So identity-dedup and value-dedup behave identically and the mutation changes nothing observable.Three things that failed first, and are now relied on
A walker triggered on
Rootroutes over every other test's leftovers.rootaccumulates across tests in a file, so the supervisor was choosing among the previous tests' hubs. It triggers on aTeamnode the test builds itself.A picker matching the whole prompt matches the handles. Handles are derived from node type names, so
"poem" in promptis true for the math request too, becausePoemAgentis on the candidate list. The picker reads the request out of the prompt instead.A Jac generator cannot
returna value.RoutingLLM's streaming override yields from super for the non-routing fall-through rather than returning it.Docstrings
Comment-only, own commit. Measured first: 121 docstring lines and 37 comment lines across 1,168, a 14% share, so a narrow pass. Five lose a narrative clause and keep their contract.
Untouched on purpose: docstrings on
by llm()defs and on anything passed intools=[], since byLLM sends those to the model as prompt and tool description; also everysemand every test name.Verified: triple-quote count identical per file, so nothing was added, split or merged; docstrings stripped and the remainder compared gives zero code changes, zero comment changes.
What is left as a fixture
20 programs, and every one earns it:
routing_graph,agent_graph):node/walker/edgeshape, no model, no entry blocktest_mtir_integration.jac, never runsystem_prompt_override/: byLLM resolves[byllm] system_promptfrom the module's own location, so its defs sit beside thatjac.tomlbuiltin_llm_no_override.jac: exists precisely to be a module with noglob llmasync_by_llm.jac: compiled byJacProgramto check codegen, never runThe 4 fixtures still containing a
print(orassertare compile targets whose content is the thing being compiled.Next
PR C in #9002:
test_usage,test_parallel,test_compaction,test_telemetry_andtest_mtir_integrationonto the annex, renamesupport_tests.jactosupport.jacto match the repo convention, and landTESTING.md.