Repository navigation
chore(forge): store conformance fixtures compactly - #300
Conversation
Header on one line, one exchange per line: 3.8 MB -> 2.3 MB across both versions, still one reviewable line per exchange. Contents are unchanged (every file reloads to an identical structure); record writes the same format.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Important Review skippedWe couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Description Check |  |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title clearly and concisely describes the main change: storing Forge conformance fixtures in compact JSON format. |
| Docstring Coverage | |
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files. (28 skipped: 2… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files. (28 skipped: 28 unsupported.)
✨ Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Remove the generated Python bytecode
This is a generated CPython 3.11 cache rather than a source fixture; it embeds the developer's absolute source path, is interpreter-specific, and future executions can replace it with opaque binary diffs. Remove this file and ignore __pycache__//*.pyc so the compact-fixture change does not add a 216 KB machine-generated artifact.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| h = J.dumps(head, separators=(",", ":"))[:-1] | ||
| f.write(h + (',' if head else '') + '"exchanges":[\n') | ||
| f.write(",\n".join(J.dumps(e, separators=(",", ":")) for e in obj["exchanges"])) | ||
| f.write("\n]}\n") |
There was a problem hiding this comment.
This introduces a hand-built JSON serialization path without an automated test that round-trips its output or verifies the promised header/exchange line layout, including an empty exchange list. Add a focused test so malformed or semantically changed recordings are caught before replacing the conformance fixtures; the repository explicitly requires every new behavior to ship with a test. CLAUDE.mdL90-L92
Useful? React with 👍 / 👎.
…lftest The conversion left __pycache__/run.cpython-311.pyc in the previous commit. run.py selftest checks that dump_fixture round-trips awkward values and that every stored fixture is in canonical compact form; forge-check.sh (CI) runs it.
tonibergholm
left a comment
tonibergholm
left a comment
There was a problem hiding this comment.
Approved: required checks green on the final head; Codex code and security reviews found nothing; bot findings (stray bytecode, writer regression test) fixed.
Summary
run.py recordwrites this format through one helper (dump_fixture); the existing 48 fixture files were converted with the same helper.Evidence
fixtures/1.22.6andfixtures/1.26.2.After: 2.34 MB (0.62x). Most of what remains is real payload (diffs, base64, git output), so "roughly half" was optimistic.
run.py diffstill reports 334 differing exchanges between versions;showoutput unchanged;forge-check.shstill2/2 scenarios ok; 0 failed;cargo test -p scl-forge --test route_tablepasses.recordagainst Docker Gitea. The writer is the same helper the conversion used.Merge Danger
Door: two-way.
Blast Radius: fixtures and the recorder's writer only; one large mechanical diff (48 files), reviewable by exchange line.
Summary by CodeRabbit