test: drop the filePrefix assertion its exact pin already subsumes - #99
Conversation
`testFilePrefixNamesNoScript` asserted the prefix holds no `.sol`. `testFilePrefixExact` pins the whole prefix, so exactly one string passes it and every string naming a `.sol` file fails it. `testBytesToHexHasNoPrefix` asserted no output byte is `x`. `testBytesToHexCharset` asserts every output byte is in `[0-9a-f]`, an alphabet that excludes `x`, and both fuzz the same `bytes` domain for 2048 runs. Each survivor carries the removed intent in its docstring. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 57 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 |
|
@coderabbitai review |
|
Two fuzz tests over one domain are not one test. `x` is outside `[0-9a-f]`, so on any single output `testBytesToHexCharset` implies `testBytesToHexHasNoPrefix` — but `[fuzz] runs = 2048` with no pinned seed means each of them draws its own 2048 inputs, and the survivor's coverage of `bytes memory` is its own draws. Deleting the weaker sibling cost 2048 draws of the property, which is coverage rather than redundancy. The `testFilePrefixNamesNoScript` deletion stands. `filePrefix()` takes no arguments and is `pure`, so `testFilePrefixExact` pins the only input there is and subsumption is total. Drops the docstring paragraph added to `testBytesToHexCharset` to stand in for the deleted test, since the test is back. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
thedavidmeister
changed the title
The automated merge at 5c68149 adds no commit this branch lacks - it is the same origin/main@c8440fb merged with a different resolution. Two reasons this tree wins: - 5c68149 restores testFilePrefixNamesNoScript, which #99 deliberately removed as subsumed by testFilePrefixExact. filePrefix() is nullary and pure, so the exact pin is a real subsumption and the name assertion is dead weight. - 5c68149 predates the reorder of testBuildFileForContractCarriesTheCallers- LicenceAndCopyright to read then clean up then assert, and the constant docstrings that went with it.
2 participants
Closes #72
Deletes
testFilePrefixNamesNoScript, and folds its intent into the docstring oftestFilePrefixExact, the sibling that already subsumes it.This PR previously deleted a second test,
testBytesToHexHasNoPrefix. Thatdeletion is reverted — the test is restored, and
test/lib/LibHexString.bytesToHex.t.solis now byte-identical tomain. Issue#72 bundled both deletions under one rationale; the rationale holds for one of
them. Correction posted on the issue:
#72
Why the correction
The brief this PR was written under said every fix ships a test and that
verify-by-hand is never acceptable, with no exemption for changes that have no
behaviour. That was wrong and is corrected. Here it pushed a deletion argument
past what the evidence supported: a per-input implication between two fuzz tests
was read as one test subsuming the other.
filePrefix()takes no arguments and ispure, so its domain is a single point.testFilePrefixExactassertEqs the whole return value, so exactly one stringpasses it — every string the deleted test rejected is rejected there too, and so
is every other string. One input, one implication, total subsumption.
bytesToHexis not that. On any given output, "every byte is in[0-9a-f]"does imply "no byte is
x" —xis0x78, outside the alphabet. ButtestBytesToHexHasNoPrefixandtestBytesToHexCharsetare two fuzz tests overbytes memory, not two assertions on one input.foundry.tomlsets[fuzz] runs = 2048and pins noseed, so each runs its own independent2048-draw campaign and each test's coverage of the domain is its own draws.
Deleting the weaker sibling cost 2048 draws of the property over the same domain.
That is coverage, not redundancy.
A mutation matrix cannot rescue that half either. Both tests dying on the same
mutant is exactly what a per-input implication predicts; it cannot show the
survivor would catch a defect only some of the deleted test's own draws reach.
Mutation matrix — the deletion that stands
Measured on the pre-change tree —
origin/mainatb422d97, where bothtests are still present, so they can be compared on the same mutant in the same
run. Whole
--match-pathfile every time, never a name filter that could matchnothing.
src/lib/LibCodeGen.solrestored withgit checkoutafter each mutantand confirmed clean with
git status --short.Unmutated first, so a red below is the mutant and not a broken harness:
src/lib/LibCodeGen.solAUTOGENERATED BY script/Build.soltestFilePrefixNamesNoScriptFAIL —prefix names a scripttestFilePrefixExactFAIL^0.8.25→^0.8.26testFilePrefixExactFAILM1 is the subsumption proof — the mutant the deleted test caught, the survivor
caught too. M1b is the converse, and it is why this is a deletion rather than a
swap: the survivor is strictly stronger, killing a mutant the deleted test
sails through.
M1, verbatim
M1b, verbatim — the strictly-stronger direction
Post-deletion re-check
M1 re-applied to this branch's tree (
36a640b), where the deleted test is gone,to confirm the behaviour is still caught without it:
The prefix literal is in fact pinned in a third place —
test/lib/LibFs.buildFileForContract.t.sol:51rebuilds it from the literal text —so the
.sol-free property has more behind it than the pair the issue named.Nothing here is left unguarded, and nothing moves to rainix#317
Stated explicitly because the sweep that produced this correction asks every PR in
it to say plainly what it stops guarding.
This PR stops guarding nothing. The one deletion it keeps is over a single-point
domain where the surviving pin is provably stronger, so there is no input on which
testFilePrefixExactpasses and the deleted test would have failed — M1b is thatasymmetry measured. No check is being rehomed, so
rainlanguage/rainix#317is notthis PR's business.
QA
origin/main(b422d97) into the branch — no rebase, no force-push. Thebranch predated merged PRs Unlink a symlink with no target before writing the generated file #110 and Declare every tooling builder
viewso an implementation can read state #124; the merge was clean.origin/mainis one file,test/lib/LibCodeGen.filePrefix.t.sol, −9 / +6.nix develop -c forge testonorigin/main(b422d97):Ran 18 test suites: 142 tests passed, 0 failed, 0 skipped (142 total tests).nix develop -c forge teston this branch:Ran 18 test suites: 141 tests passed, 0 failed, 0 skipped (141 total tests),exit 0. The count drops by exactly 1 — the one deleted test.
[PASS] testBytesToHexHasNoPrefix(bytes) (runs: 2048, μ: 24199, ~: 19406)and[PASS] testBytesToHexCharset(bytes) (runs: 2048, μ: 29504, ~: 22485)— therestored test is running, not merely compiling.
nix develop -c forge fmt --check→ exit 0.testFilePrefixExactspells the prefix out as a string literal rather thancalling
filePrefix(). Consumers hold that literal committed in their own repos,which is what makes the literal the oracle.
src/behaviour changed: the diff is test-only.Note on the TDD order
The remaining finding is redundant coverage, so there is no failing-test-first
step to run: nothing was broken to begin with, and the change removes an assertion
rather than adding behaviour. The mutation matrix is the evidence in its place —
the deleted test was shown to catch a real mutant, and its survivor to catch that
one and one more.
🤖 Generated with Claude Code
Post-
Build.sol-removal sweep (2026-08-17)main(959d527) merged in, no conflict. Unaffected by #138's removal —nothing cut. The removal does not weaken the deletion argument; it strengthens
the docstring the intent was folded into, since after #138 this repo names no
build script at all and "each consumer names its own" is the whole rule.
Re-verified against
mainrather than restated:src/lib/LibCodeGen.sol:73—filePrefix()is stillinternal purewith noparameters, so its domain is still a single point.
testFilePrefixExactstillassertEqs the entire return value, so exactly onestring passes it and the subsumption is total.
test/lib/LibHexString.bytesToHex.t.solon this branch is still byte-identicalto
main—testBytesToHexHasNoPrefixandtestBytesToHexCharsetare bothpresent at
main's lines 91 and 106. The withdrawn half oftestFilePrefixNamesNoScriptandtestBytesToHexHasNoPrefixassert only what a sibling in the same file already implies #72 stayswithdrawn, and this branch no longer touches that file, so it no longer
overlaps test: construct the conforming half of the bytesToHex Vm-output property #102 or Check the hex charset in LibHexString.bytesToHex #108, which both do.
Suite on the merge commit:
Ran 19 test suites: 144 tests passed, 0 failed, 0 skipped—main's 145 minus the one deleted test, nothing else moved.forge fmt --checkclean,git statusclean after the run.forge coverage --no-match-coverage "test|script"still 100% lines / statements / branches /funcs on all three
src/libfiles (79/79, 92/92, 10/10, 18/18), so the deletioncosts no coverage.