chore(tests): share the --json purity helper and de-hardcode the command count - #206
Conversation
…and count Enabling work for #196: the three follow-up PRs (decisions, clean/cleanup, validate) would otherwise all conflict on these same lines. Move `_machine_json` from tests/test_cli.py to tests/conftest.py as `machine_json`, so test_probe.py and test_cleanup.py can reach it too. Two keyword-only parameters, both defaulting to the strict form: - `rc` — validate --json will legitimately exit 1 while still owing the caller a complete document. Stdout purity is what is being asserted, not success. - `err_contains` — probe-adapter --json routes its human `ok:` trailer to stderr by design, so `err == ""` is false for it. The default stays empty- stderr; the opt-in is a different assertion (the substring must be present), not a waiver, so a trailer moving back to stdout still fails. Adopt it in test_probe.py, which re-implemented the same stdout assertions inline. Its sorted-keys check moves to the renderer as `test_render_json_sorts_keys_at_every_depth`: key order survives only in the raw bytes, and the shared helper returns a parsed dict that has already lost it — feeding it a re-serialized `json.dumps(..., sort_keys=True)` would sort the keys on the way in and assert nothing. Asserted against render_json's own return value instead, where the property lives; that also reaches nested objects, which the stdout-based check only covered at the top level. Reword machine.py and docs/FEATURES.md so neither hardcodes "all four" commands — three more are about to take the flag. The #195 reference and the no-exception meaning are unchanged. Sub-issues filed per #196's request: #203, #204, #205. No --json flag is added in this phase.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughThe ChangesJSON contract and test coverage
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Enabling work for #196. No
--jsonflag is added here — this only touches thelines the three follow-up PRs (#203
decisions --list, #204clean/cleanup,#205
validate) would all otherwise conflict on.The shared purity helper moves to
conftest.py_machine_jsonlived intests/test_cli.py, wheretest_probe.pyandtest_cleanup.pycould not reach it. It is nowmachine_jsonintests/conftest.py— the moduletest_cleanup.pyalready imports from — withtwo keyword-only parameters, both defaulting to the strict form:
rc—validate --json(Adopt --json on validate (pure-document contract) #205) will legitimately exit 1 while stillowing the caller a complete document. What the helper asserts is stdout
purity, not success.
err_contains— see below.Parsing the whole of stdout is still the purity assertion, and the docstring
still says why.
err_contains, and a wrong premise in the planThe plan called for
test_probe.pyto adopt the helper on the grounds that it"re-implements the same assertions inline". It does not — it asserts the
opposite on stderr:
That is deliberate.
probe-adapter --jsonroutes itsok:trailer to stderrprecisely so stdout stays the document alone (
cli.py:1945-1947), whichmachine.pydocuments as legitimate ("with only a confirmation on stderr").So
err == ""is simply false for probe.Rather than relax the default,
err_containsopts a call site into adifferent assertion. Default
Nonekeepserr == ""forstatus,listand
diagnose; probe passeserr_contains="ok:", which still fails if thattrailer ever moves back to stdout. Same shape as
rc: strict by default,explicit at the one site that differs.
The sorted-keys check moves to the renderer
_assert_keys_sortedneeds the raw string — onlyobject_pairs_hookseeskey order, and
machine_jsonreturns a parsed dict that has already lost it.Handing it a re-serialized
json.dumps(doc, sort_keys=True)would sort thekeys on the way in and assert nothing at all; the docstring now says so, since
that is the obvious wrong repair.
It is now
test_render_json_sorts_keys_at_every_depth, asserting againstprobe.render_json's own return value. That is where the property lives — theCLI just prints what the renderer returns — and it is strictly stronger:
dropping
sort_keys=Truefromrender_jsonfails it on a nested object(
['SessionStart', 'Stop', 'SessionEnd', 'PreCompact']), which the oldstdout-based check only reached at the top level. Verified by making that edit
and watching it fail, then reverting.
Docs
machine.pyanddocs/FEATURES.mdno longer hardcode "all four" commands —three more are about to take the flag. The #195 reference and the
no-exception meaning are unchanged.
machine.pynow also says the contract isthe flag's meaning, not a style the first four happen to share.
README.md:257("preflights all four upstream skills") is left alone — that isDEV_BASE_SKILLS, unrelated.Verification
uv run pytest -q— 2548 passed, 1 skippedtrunk check— clean (it reformatted one argument list intest_probe.py)Closes nothing; unblocks #203, #204, #205 under #196.
Summary by CodeRabbit
Documentation
--jsonfollows the same machine-readable output contract.status,list,diagnose, andprobe-adapter.Tests