From 19df87dae664bd0a335ecd512ffeeb63cd637053 Mon Sep 17 00:00:00 2001 From: MalithaPrabhashana Date: Mon, 7 Sep 2026 02:52:27 +0530 Subject: [PATCH] test(byllm): a ratchet and a written contract for the test suite (#9002 phase 0) The byLLM suite is 10,361 lines holding 242 test blocks, and it grows by roughly one fixture per fix PR. The two consolidation PRs merged on 2026-09-04 (#8936, #8938) removed 280 lines; #8479 added 1,046 back the same day on the old seams, deleting the file #8936 had just migrated. Net for the effort so far is +766 lines. Consolidation alone does not hold. This adds the piece that does. test_guard.jac is a ratchet, not a wall. Every number in it is today's measured count: ceilings on the things being retired (fixture programs, MockLLM constructions, SimpleNamespace fakes, stdout scraping) and floors on the things being built (assert count, annex adoption). Cleaning up means editing a number down in the same PR, which is the intended direction and needs no explanation. Adding one of the retired patterns fails here, with the file, the delta, and what to use instead. The assert floor is the one that is not obvious. 66 of the suite's 842 assertions live inside fixture programs, guarded by only 10 sentinel greps, so deleting a fixture can drop a dozen checks while the test count does not move. A deleted-test to surviving-row mapping cannot see that; a total assert count can. TESTING.md is the contract the guard enforces: fake the model at model_call_* so make_model_params and parse_response are real code and the test can assert on the request; a fixture is a program under test with no model, entry block, prints or asserts; assert on values, never on stdout. The fixture section documents five things verified against the compiler beforehand, each of which costs an afternoon if missed: `by llm()` resolves its model global at call time, so a test can own the fake, but the fixture needs `glob llm: any = None;` or the assignment fails E1001; graph fixtures need a static import of their archetypes, because `root ++> g.Desk()` on a module from jac_import fails E1097; routing replies are generated handles, not indices; `Jac.jac_import` returns a cached module rather than re-executing it, which contradicts the comment above test_byllm.jac:1618; and per-test subgraphs stay isolated only if you spawn on your own node rather than from root. No existing test or fixture changes. The suite goes from 199 passed, 1 skipped to 207 passed, 1 skipped. --- jac/jaclang/byllm/tests/TESTING.md | 199 ++++++++++++++++ jac/jaclang/byllm/tests/test_guard.jac | 300 +++++++++++++++++++++++++ 2 files changed, 499 insertions(+) create mode 100644 jac/jaclang/byllm/tests/TESTING.md create mode 100644 jac/jaclang/byllm/tests/test_guard.jac diff --git a/jac/jaclang/byllm/tests/TESTING.md b/jac/jaclang/byllm/tests/TESTING.md new file mode 100644 index 00000000000..bbf44a7605d --- /dev/null +++ b/jac/jaclang/byllm/tests/TESTING.md @@ -0,0 +1,199 @@ +# Testing byLLM + +Read this before adding a test here. It is short on purpose. + +The suite is mid-refactor (jaseci-labs/jac#9002). New tests follow the rules below; +old tests that do not are being converted cluster by cluster. `test_guard.jac` holds +today's counts and fails a PR that moves one the wrong way, so you find out here rather +than in review. + +## Run it + +``` +JAC_TEST_JOBS=2 JAC_TEST_STRICT=1 jac test jac/jaclang/byllm/tests \ + --ignore jac/jaclang/byllm/tests/test_mtir_integration.jac +``` + +`test_mtir_integration.jac` runs in the sealed lane, from a copy outside the checkout; +`ci.yml` explains why. Read the pass count, not the exit code. + +## The one rule + +**Fake the model at `model_call_*`, never above it.** + +A `by llm()` call goes through these layers: + +``` + your function + MTRuntime: messages, resp_type, tools + BaseLLM.invoke: react loop, retries, compaction + dispatch_no_streaming + make_model_params ......... prompt, semstrings, schema, tool descriptions + model_call_* .............. the network hop <-- fake HERE + _parse_tool_calls, parse_response .................. typed output +``` + +`FakeLLM` replaces only the network hop, so everything above and below it is the real +code. That buys two things a higher fake cannot give you: the request is really built, +so you can assert on it, and the reply is really parsed, so a declared return type is +actually enforced. + +`MockLLM` replaces `dispatch_no_streaming`, four layers up. Under it the prompt is +built and thrown away and `parse_response` never runs. Two functions with opposite +`sem` strings return identical output, and `def f() -> int by llm()` hands back a +`str`. It is still exported for users, but do not reach for it in new tests here. + +## Writing a test + +```jac +import from support_tests { FakeLLM, say, call, finish, mk_run } + +glob emoji_llm = FakeLLM(replies=[say("")]); + +def get_emoji(text: str) -> str by emoji_llm(temperature=0.7); + +test "temperature is passed through and the input reaches the prompt" { + assert get_emoji("lets move to paris") == ""; + + sent = emoji_llm.seen[0]; + assert sent["temperature"] == 0.7; + assert "paris" in str(sent["messages"]).lower(); +} +``` + +The last two assertions are the point. Assert on what came back **and** on what went +out. A test that only checks the return value is checking that the reply you scripted +came back, which is true by construction. + +What the annex gives you, all in `support_tests.jac`: + +| | | +|---|---| +| `FakeLLM(replies=[...])` | a `BaseLLM` that serves scripted replies and records every outgoing params dict | +| `scripted(model, replies)` | same, but patched onto a real `Model` when you need provider behaviour | +| `say(text)` | a plain-text reply | +| `call(name, args)`, `calls([...])` | one or several tool calls | +| `finish(value)` | a `finish_tool` call | +| `fail(err, content, after)` | raise, optionally mid-stream | +| `mk_run(...)` | an `MTRuntime` with sane defaults | +| `response()`, `chunks()` | real `litellm` objects, for tests that drive dispatch directly | +| `load_fixture(name)` | import a fixture module | +| `run_fixture(name)` | import a fixture and capture its stdout (legacy; prefer values) | +| `llm.sent(key)`, `llm.seen`, `llm.exhausted()` | what was sent, and whether the script was drained | + +Add to the annex rather than defining a helper locally. If a helper already exists +under a different name, use it; if two exist, delete one. + +## Fixtures + +Most tests do not need one. A `by llm()` def belongs in the test file next to its test. + +A fixture is for a **program whose shape is the subject**, and it contains no model, no +entry block, no prints and no asserts. Only two kinds qualify: + +- **graph programs**: `node` / `walker` / `edge` with `visit ... by llm()` +- **compile targets**: files the MTIR, import and scope tests compile or analyze + +```jac +# fixtures/routing_graph.jac -- the program under test, nothing else +glob llm: any = None; # the test assigns its own fake + +edge Route { has priority: str = "low"; } +node Desk {} +node Agent { + has tag: str = "x"; + can handle with dispatcher entry { visitor.visited.append(self.tag); } +} +walker dispatcher { + has visited: list = []; + can route with Desk entry { visit [-->] by llm(select=1); } +} +``` + +```jac +# the test drives it +import from fixtures.routing_graph { Desk, Agent, Route, dispatcher } +import fixtures.routing_graph as rg; +import from support_tests { FakeLLM, say } + +test "routes along the edge attribute, not just the node" { + llm = FakeLLM(replies=[say('["Agent_beta"]')]); + rg.llm = llm; + + desk = root ++> Desk(); + desk +>: Route(priority="low") :+> Agent(tag="alpha"); + desk +>: Route(priority="high") :+> Agent(tag="beta"); + + w = dispatcher() spawn desk; + + assert w.visited == ["beta"]; + assert "priority='high'" in str(llm.sent("messages")[0]); +} +``` + +Five things about that example are load-bearing and were each found the hard way: + +1. **`glob llm: any = None;`**, not `glob llm = None;`. The bare form infers `NoneType` + and the test's assignment fails `jac check` with E1001. +2. **`by llm()` resolves the global at call time**, which is why the test can own the + fake and the fixture can stay pure. +3. **Static import of the archetypes.** `load_fixture()` returns the module as `any`, + and `root ++> g.Desk()` then fails `jac check` with + `E1097: Connection right operand must be a node instance`. Import the names directly; + use the module alias only to rebind `llm`. +4. **The routing reply is a handle, not an index.** The return type is + `list[enum[RouteChoice]]` and members are generated from the candidate set: two + `Agent` siblings give `Agent_alpha` / `Agent_beta`, a lone one gives `Agent`. An + index retries three times and then raises `OutputConversionError`. +5. **Spawn on the node you just made**, never from `root`. `root` accumulates across + tests in a file. Per-test subgraphs stay isolated as long as you do not walk from + the root, and you should not assert on `[root -->]`. + +Also: `Jac.jac_import` returns a **cached** module and does not re-execute it. State +carries over between calls, so every test must assign its own fake and must not rely on +fresh module state. + +## Assertions + +Assert on values. Do not grep stdout. + +| instead of | write | +|---|---| +| `assert "X_PASS" in stdout_value` | the fixture's own asserts, moved into the test, on returned values | +| `assert "Tool called with 12" in stdout` | `assert llm.sent("tools")[0][0]["function"]["name"] == "add"` and the tool's return value | +| slicing a dict out of a log line and `yaml.safe_load`ing it | `params = llm.seen[0]; assert "temperature" not in params` | + +A stdout match cannot tell a wrong answer from a missing `print`, it reports nothing +useful when it fails, and it cannot see the request at all. + +**Asserts inside a fixture are invisible to the runner.** 66 of this suite's assertions +still live in fixture files, guarded by 10 sentinel greps. If you delete such a fixture, +move every assert into the test first. `test_guard.jac` keeps a floor on the total count +because the test count alone will not notice. + +## Adding to the guard's budgets + +`test_guard.jac` is a ratchet, not a wall. Every number is today's measured count. + +- **Cleaning up?** Lower the ceiling or raise the floor in the same PR. That is the + intended direction and needs no explanation. +- **Genuinely need a new fixture?** Raise `FIXTURE_BUDGET` in the same PR and say why in + the description. +- **A number moved the wrong way and you did not mean it?** The failure message names + the file, the delta and the fix. + +## PR checklist + +Every PR that touches this directory carries this table: + +| | before | after | +|---|---|---| +| test blocks | | | +| table rows / cases | | | +| assert statements (test files) | | | +| assert statements (fixtures) | | | +| tests skipped | | | +| tests newly red, with issue number | | | + +Plus one line per deleted test naming the surviving row or assert, and one line per +dropped assert naming why. diff --git a/jac/jaclang/byllm/tests/test_guard.jac b/jac/jaclang/byllm/tests/test_guard.jac new file mode 100644 index 00000000000..1c6a1a17719 --- /dev/null +++ b/jac/jaclang/byllm/tests/test_guard.jac @@ -0,0 +1,300 @@ +"""Ratchet that keeps the byLLM suite from growing back the way it grew. + +Every number below is today's measured count, not a target. Ceilings may only fall and +floors may only rise; a PR that moves one the wrong way fails here with the reason and +the fix. Lowering a ceiling or raising a floor as you clean up is the point, and is a +one-line edit in the same PR. + +See TESTING.md for what the suite is converging on and why each rule exists. +""" + +import os; +import re; + +glob TESTS_DIR: str = os.path.dirname(os.path.abspath(__file__)), + FIXTURE_DIR: str = os.path.join(TESTS_DIR, "fixtures"), + ANNEX: str = "support_tests.jac"; + + +"""Every .jac under tests/, excluding fixtures/ and this file.""" +def test_files -> list[str] { + out: list[str] = []; + for name in sorted(os.listdir(TESTS_DIR)) { + if name.endswith(".jac") and name != "test_guard.jac" { + out.append(name); + } + } + return out; +} + + +"""Every fixture program, including the ones in subdirectories.""" +def fixture_files -> list[str] { + out: list[str] = []; + for (dirpath, _dirnames, filenames) in os.walk(FIXTURE_DIR) { + for name in filenames { + if name.endswith(".jac") { + full = os.path.join(dirpath, name); + out.append(os.path.relpath(full, FIXTURE_DIR)); + } + } + } + return sorted(out); +} + + +def read(path: str) -> str { + with open(path, "r", encoding="utf-8", errors="replace") as fh { + return fh.read(); + } +} + + +def read_test(name: str) -> str { + return read(os.path.join(TESTS_DIR, name)); +} + + +def read_fixture(rel: str) -> str { + return read(os.path.join(FIXTURE_DIR, rel)); +} + + +def count(pattern: str, text: str) -> int { + return len(re.findall(pattern, text)); +} + + +"""Compare measured counts to a baseline. `direction` is 'ceiling' or 'floor'.""" +def ratchet( + label: str, + measured: dict[str, int], + baseline: dict[str, int], + direction: str, + fix: str +) { + problems: list[str] = []; + for (key, limit) in baseline.items() { + got = measured.get(key, 0); + if direction == "ceiling" and got > limit { + problems.append(f" {key}: {got}, was {limit} (+{got - limit})"); + } + if direction == "floor" and got < limit { + problems.append(f" {key}: {got}, was {limit} ({got - limit})"); + } + } + for (key, got) in measured.items() { + if key not in baseline and got > 0 and direction == "ceiling" { + problems.append(f" {key}: {got}, not in the baseline at all"); + } + } + word = "rose" if direction == "ceiling" else "fell"; + assert not problems , ( + f"{label} {word}:\n" + + "\n".join(problems) + + f"\n\n{fix}\n\n" + + "If this is a deliberate cleanup, edit the baseline in test_guard.jac in " + + "the same PR. If it is new code, use the annex instead. See TESTING.md." + ); +} + + +# --------------------------------------------------------------------------- +# 1. Fixture programs. A new one has to be justified in the same PR. +# --------------------------------------------------------------------------- +glob FIXTURE_BUDGET: int = 66; + +test "fixture count does not grow" { + found = fixture_files(); + assert len(found) <= FIXTURE_BUDGET , ( + f"fixtures/ holds {len(found)} programs, budget is {FIXTURE_BUDGET}.\n\n" + + "Most tests do not need a fixture. A `by llm()` def belongs in the test " + + "file beside its test, scripted with FakeLLM. A fixture is only for a " + + "program whose SHAPE is the subject (node/walker/edge) or a compile " + + "target for the MTIR, import and scope tests.\n\n" + + "If this one really is a program under test, raise FIXTURE_BUDGET here in " + + "the same PR and say why in the PR description. See TESTING.md." + ); +} + + +# --------------------------------------------------------------------------- +# 2. A fixture is a program under test, not a test. +# Today 52 fixtures still print or assert; that number may only fall. +# --------------------------------------------------------------------------- +glob IMPURE_FIXTURE_BUDGET: int = 52; + +test "no new fixture asserts or prints its own result" { + impure: list[str] = []; + for rel in fixture_files() { + src = read_fixture(rel); + if count(r"(?m)^\s*assert\s", src) > 0 or count(r"print\s*\(", src) > 0 { + impure.append(rel); + } + } + assert len(impure) <= IMPURE_FIXTURE_BUDGET , ( + f"{len(impure)} fixtures assert or print, budget is " + + f"{IMPURE_FIXTURE_BUDGET}.\n\n" + + "A fixture that asserts hides its checks from the runner: the test greps a " + + "sentinel out of stdout and cannot report which assert failed. Put the " + + "assertions in the test and let it read values back directly.\n\n" + + f"Fixtures currently doing this: {len(impure)}. Lower the budget as you " + + "convert them." + ); +} + + +# --------------------------------------------------------------------------- +# 3. One way to fake the model. Every count here is a ceiling. +# --------------------------------------------------------------------------- +glob MOCKLLM_BASELINE: dict[str, int] = { + "test_compaction.jac": 14, + "test_usage.jac": 5 + }, + FIXTURE_MOCKLLM_BASELINE: int = 53, + SIMPLENAMESPACE_BASELINE: dict[str, int] = { + "test_byllm.jac": 10, + "test_compaction.jac": 4, + "test_telemetry_.jac": 8, + "test_tool_arg_streaming.jac": 7, + "test_usage.jac": 19 + }; + +test "MockLLM constructions in test files do not grow" { + measured: dict[str, int] = {}; + for name in test_files() { + if name == ANNEX { + continue; + } + n = count(r"MockLLM\s*\(", read_test(name)); + if n > 0 { + measured[name] = n; + } + } + ratchet( + "MockLLM constructions", + measured, + MOCKLLM_BASELINE, + "ceiling", + "Use FakeLLM from support_tests instead. MockLLM replaces " + + "dispatch_no_streaming, which sits above make_model_params and " + + "parse_response, so the prompt is built and discarded and the reply is " + + "never parsed: the test cannot assert on what was sent, and a typed " + + "return is not converted. FakeLLM replaces only model_call_*, so the " + + "real dispatch runs and llm.sent(...) shows the outgoing params." + ); +} + +test "fixtures reaching for MockLLM do not grow" { + n = 0; + for rel in fixture_files() { + if "MockLLM" in read_fixture(rel) { + n += 1; + } + } + assert n <= FIXTURE_MOCKLLM_BASELINE , ( + f"{n} fixtures construct a MockLLM, budget is " + + f"{FIXTURE_MOCKLLM_BASELINE}.\n\n" + + "A fixture should not own a model at all. Declare `glob llm: any = None;` " + + "and let the test assign its own FakeLLM: `by llm()` resolves the global " + + "at call time, so the fixture stays a pure program under test." + ); +} + +test "hand-rolled SimpleNamespace fakes do not grow" { + measured: dict[str, int] = {}; + for name in test_files() { + n = count(r"SimpleNamespace\s*\(", read_test(name)); + if n > 0 { + measured[name] = n; + } + } + ratchet( + "SimpleNamespace fakes", + measured, + SIMPLENAMESPACE_BASELINE, + "ceiling", + "support_tests gives you response(), chunks() and mk_run(), which build " + + "real litellm objects and a real MTRuntime. A SimpleNamespace stand-in " + + "accepts any shape, so it keeps passing after the real object's shape " + + "changes." + ); +} + + +# --------------------------------------------------------------------------- +# 4. Tests read values, not stdout. +# --------------------------------------------------------------------------- +glob STDOUT_GREP_BASELINE: dict[str, int] = { + "support_tests.jac": 2, + "test_byllm.jac": 95, + "test_mtir_integration.jac": 4, + "test_visit_routing.jac": 13 + }; + +test "stdout scraping does not grow" { + measured: dict[str, int] = {}; + for name in test_files() { + n = count(r"in stdout_value|getvalue\(\)", read_test(name)); + if n > 0 { + measured[name] = n; + } + } + ratchet( + "stdout scraping", + measured, + STDOUT_GREP_BASELINE, + "ceiling", + "Assert on the value the function returned and on llm.sent(\"messages\") / " + + "llm.sent(\"tools\"). A stdout match cannot tell a wrong answer from a " + + "missing print, and it cannot see the request at all." + ); +} + + +# --------------------------------------------------------------------------- +# 5. Floors. These may only rise. +# Assert count is here because a deleted fixture can take its checks with it +# without changing the test count: 66 of the suite's asserts live in fixtures, +# guarded by 10 sentinel greps. +# --------------------------------------------------------------------------- +glob ASSERT_FLOOR_TESTS: int = 776, + ASSERT_FLOOR_FIXTURES: int = 66, + ANNEX_IMPORT_FLOOR: int = 2; + +test "total assert count does not fall" { + in_tests = 0; + for name in test_files() { + in_tests += count(r"(?m)^\s*assert\s", read_test(name)); + } + in_fixtures = 0; + for rel in fixture_files() { + in_fixtures += count(r"(?m)^\s*assert\s", read_fixture(rel)); + } + total = in_tests + in_fixtures; + floor = ASSERT_FLOOR_TESTS + ASSERT_FLOOR_FIXTURES; + assert total >= floor , ( + f"assert statements fell to {total} ({in_tests} in test files, " + + f"{in_fixtures} in fixtures); the floor is {floor}.\n\n" + + "Deleting a fixture deletes its asserts, and the test count does not " + + "move, so this is the only check that notices. Move each assert into the " + + "test that owned the fixture before deleting it.\n\n" + + "If a check is genuinely redundant, lower the floor here and name the " + + "dropped assert in the PR description." + ); +} + +test "annex adoption does not fall" { + n = 0; + for name in test_files() { + if name != ANNEX and "import from support_tests" in read_test(name) { + n += 1; + } + } + assert n >= ANNEX_IMPORT_FLOOR , ( + f"{n} test files import support_tests, the floor is " + + f"{ANNEX_IMPORT_FLOOR}.\n\n" + + "The annex is the one place a fake, an MTRuntime or a response shape is " + + "defined. A file that stops importing it has grown its own copy." + ); +}