Deduplicate LibHexStringExternal and NonConformingVm construction in the bytesToHex suite - #105
Conversation
The wrapper that forces the ABI boundary is deployed once in the constructor and held as an immutable. The stub `Vm` is built through a helper that names the return string each case is about. Closes #63 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 54 minutes Limit details: You’ve used all 1 included review currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
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 |
It asserts on vm.getNonce(address(this)), never on bytesToHex's output, so its only kills are mutants of the test file itself. It also couples two tests by calling one from inside the other and hardcodes one stub per revert test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
build-pointers failed in nix-quick-install-action before any repo step ran: `Fetching nix archives ... nix-2.24.12-x86_64-linux.tar.zstd` then `zstd: /*stdin*\: unexpected end of file` / `tar: Child returned status 1`. A truncated download of the installer tarball, not a defect in this branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes #63
What changed
test/lib/LibHexString.bytesToHex.t.solbuilt a freshLibHexStringExternal12 times and wrote
Vm(address(new NonConformingVm(...)))11 times. Two of theLibHexStringExternalsites sat inside 2048-run fuzz tests, so those were 2048deployments per test rather than one.
LibHexStringExternal internal immutable iExternal, built in the constructor,matching the
iExternalpattern already inLibFsTestandLibFsBuildFileForContractTest. It holds no state, so one instance serves thewhole suite.
function vmReturning(string memory toStringReturn) internal returns (Vm)builds the stub
Vm. The stub is still built per case, because the returnstring is what each case is about, but the
Vm(address(new ...))wrapping iswritten once.
testBytesToHexSurvivesAbiBoundaryno longer deploys, so it isexternal view. Leaving itexternalproduced a newWarning (2018): Function state mutability can be restricted to view.No test guards this change, and it can regress silently
This PR adds no test for what it changes. Nothing in the suite — before or after
— asserts how many contracts the test contract deploys, so the deduplication
can be undone without any test going red. Putting
new LibHexStringExternal()back inside
testBytesToHexSurvivesAbiBoundary, or adding a secondnew NonConformingVminsidevmReturning, leaves the suite fully green. Thosetwo edits are mutants M1 and M3 in the matrix below and both are recorded there
as SURVIVED, measured, not assumed. The only signal a regression would give is
suite runtime and gas.
An earlier revision of this PR carried
testBytesToHexDeploysOnlyStubVms, a16-line guard that counted deployments off
vm.getNonce(address(this)). It hasbeen removed and not replaced. It asserted on the test contract's nonce and
never on
bytesToHex's output, so the only mutants it killed were mutants of thetest file itself; it coupled two tests by calling one from inside the other; and
it hardcoded "one stub per revert test", so every new revert case would have had
to edit it.
The deployment count is not rehomed anywhere — there is no
rainlanguage/rainix#317rule for it and no follow-up issue. The gap above isthe accepted cost of this PR.
Where the issue's proposed fix was wrong
The issue proposed writing each revert case as
Arguments are evaluated after
expectRevertis armed, so thenew NonConformingVminside the helper is the next frame foundry sees and it consumesthe expectation. That form is mutant M4 below, and it is red:
[FAIL: next call did not revert as expected] testBytesToHexRevertsOnTruncatedVmOutput().The landed form builds the stub into a local first, then arms
expectRevert,then calls — the ordering the file already used.
The helper is named
vmReturning, notbadVm: two of its callers(
testBytesToHexAcceptsConformingVm,testBytesToHexAcceptsConformingVmForEmptyData) deliberately pass aconforming return string, and
badVmwould misname them.QA
Everything below was run via
nix develop -c, on2026-08-16-issue-63withorigin/main(b422d97, i.e. including merged #110 and #124) merged in — mergecommit
ee9f46d, no rebase, no force-push.forge teston the merge commit, before deleting the guard:Ran 18 test suites: 143 tests passed, 0 failed, 0 skipped (143 total tests)forge testat HEAD, after deleting the guard:Ran 18 test suites: 142 tests passed, 0 failed, 0 skipped (142 total tests)— a drop of exactly one test, the deleted guard, with nothing else changed.
forge fmt --check: exit 0, no diff.mainfor every test in the file. The expectedstrings come from the definition of
Vm.toString(bytes)("0x" plus two hexcharacters per byte), not from
LibHexString.LibHexStringExternalandone helper for the
NonConformingVmconstruction, across all 12 + 11 sites;both are covered at every site, and the proposed call ordering that breaks
vm.expectRevertis corrected.Mutation matrix
Run with
mutation-probeagainst the wholeforge testsuite. Baseline green at142 passed / 0 failed. Every mutant is an exact-string replacement asserted to
match its file exactly once, applied, run, then restored and re-verified
byte-exact, so a harness error cannot pass for "survived".
test/lib/LibHexString.bytesToHex.t.soltestBytesToHexSurvivesAbiBoundarydeploys its ownLibHexStringExternalper run againtest/lib/LibHexString.bytesToHex.t.solvmReturningreturns the realvminstead of the stub it builttestBytesToHexRevertsOnEmptyVmOutput,…OnEmptyVmOutputForEmptyData,…OnOneCharacterVmOutput,…OnOverlongVmOutput,…OnTruncatedVmOutputtest/lib/LibHexString.bytesToHex.t.solvmReturningdeploys a secondNonConformingVmbefore returningtest/lib/LibHexString.bytesToHex.t.solvm.expectReverttestBytesToHexRevertsOnTruncatedVmOutputsrc/lib/LibHexString.sol:44if eq(len, expectedLength)→if 1testBytesToHexRevertsOnOverlongVmOutput,testBytesToHexRevertsOnTruncatedVmOutputsrc/lib/LibHexString.sol:48if eq(shr(240, mload(add(hexString, 0x20))), 0x3078)→if 1testBytesToHexRevertsOnUnprefixedVmOutput,testBytesToHexRevertsOnWrongFirstPrefixCharacter,testBytesToHexRevertsOnWrongSecondPrefixCharacterThe "killed by" lists are the probe's reported killers and are capped at five
entries per mutant, so they name at least these tests, not necessarily only
these.
M5 and M6 mutate the library rather than the test. They are the check that the
refactor did not weaken what the suite proves about
bytesToHex— the revertcases still catch a
Vmreturn of the wrong length and aVmreturn without theprefix, through the shared
iExternalexactly as they did through their ownlocal one.
CodeRabbit
CodeRabbit has not reviewed this PR.
reviewThreadsandreviewsover GraphQLboth return zero at the time of writing; its earlier check reported
pass / Review rate limited. That green is an absence of review, not a passedone.