Cover the generated directory being created, by making the directory injectable - #112
Conversation
Take the directory as a parameter on an overload of buildFileForContract so a test can drive the create against a directory it owns, and make the existing four argument function that overload applied to GENERATED_DIR. Closes #64 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The file cannot be read back at that path unless the directory it is in was created, so the assertion above the read does no work: removing it leaves the suite green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 8 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 (2)
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 |
# Conflicts: # src/lib/LibFs.sol
…injectable The library's vm.createDir(GENERATED_DIR, true) had no test: commenting it out on main leaves 145/145 passing, twice, including after rm -rf src/generated. Both test/lib/LibFs.buildFileForContract.t.sol and test/src/lib/LibFs.isPresent.t.sol create GENERATED_DIR in their own setUp, which masks the library creating it. Deleting GENERATED_DIR to make it missing is not an option: every generating test writes under it and forge runs them in parallel, which is why those setUp calls exist in the first place. So the directory becomes injectable. A private pathForContractIn(dir, name) and an internal buildFileForContract(vm, instance, dir, name, body) overload; the existing four-argument function is that overload applied to GENERATED_DIR and is otherwise untouched. The new test owns src/generated/LibFsBuildCreatesDir/nested, two missing levels deep, so a create that did not also make the missing parent fails too. Mutant proven dead: with vm.createDir(dir, true) removed the new test fails with vm.writeFile: ... No such file or directory. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Re-sites the unlink loop into #112's five-arg overload, which is where the unlink and the write now live, and keeps main's `vm.createDir(dir, true)`. The docstring change stays on the four-arg NatSpec, which is where the unlink paragraph lives and which the overload defers to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`pathForTaggedContract` is `pathForContractIn(dirForTag(tag), contractName)` and `buildFileForTaggedContract` is the five argument `buildFileForContract` applied to `dirForTag(tag)`, so the name check, the separator, the extension, the directory creation, the unlink guard and the write exist once. Argument evaluation runs `dirForTag` before the call, so the tag is still checked before the name and before any cheatcode. `LibFsBuildFileForTaggedContractTest` creates `GENERATED_DIR` in `setUp`: `testBuildFileForTaggedContractLeavesTheUntaggedFileAlone` writes its sentinel there directly, and `src/generated/` holds no committed file, so on a fresh clone that write has no parent directory unless another test in the contract happened to run first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Take #138's deletion of script/Build.sol and src/generated/CodeGennable.sol, and drop testBuildFileForContractCommittedArtifactIsCurrent with them: it read the deleted artifact. Re-site the licence and copyright parameters into #112's five-arg buildFileForContract overload, which is where the write now lives, and thread them through the wrapper. testBuildFileForContractCreatesTheDirectory, added on main, passes them too. Take main's testFilePrefixExact docstring, which subsumes the testFilePrefixNamesNoScript main deliberately dropped in 95a1e89.
Re-sites the orphaned-artifact refusal onto main's current `LibFs`: - `buildFileForContract` is now the six-arg call applied to `GENERATED_DIR` over a seven-arg `dir` overload (#112), builds the whole file content before touching disk (#100), and unlinks in a `while` loop (#127). The check goes into the shared body, after `vm.createDir` because it is a read of that directory, and before the unlink so a refusal leaves the existing artifact where it found it. - `requireNoOrphanedArtifact(vm, contractName)` is that check applied to `GENERATED_DIR`, over a private `requireNoOrphanedArtifactIn`, mirroring `pathForContract` / `pathForContractIn`. The overload reads the directory it writes into rather than always `GENERATED_DIR`. - The test moves from `test/lib/` to `test/src/lib/` (#56), and its calls carry the licence and copyright `filePrefix` now takes (#135). - `InvalidContractName` / `isContractNameSlow` are `InvalidIdentifier` / `isIdentifierSlow`, and forge-std is 1.16.2. - The README's "Generated paths" section anchors ahead of "Formatter requirements": the worked-example section it sat under is gone (#138) and the publish section it appended to was rewritten (#140). Drops the hand-set `[package].version = "0.2.0"` and the README paragraph that justified it. Autopublish owns the version. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes #64
This branch had been reduced to nothing, and is restored here
Read this first if you looked at this PR before today. Its previous head,
30478d4, was a merge ofmainthat resolved by takingmainwholesale anddiscarded both of the branch's own commits.
git diff b422d97 30478d4wasempty: the head tree was byte-identical to
mainatb422d97, which ispre-#138. Against today's
mainthat head was not a no-op but a revert — itwould have put
script/Build.sol,src/generated/CodeGennable.soland.github/workflows/build-pointers.yamlback.The work has been reconstructed on top of current
mainand the branch nowcarries a real diff again.
The gap is real, measured on
maintodaysrc/lib/LibFs.solcreates the generated directory for itself:Comment that line out on
mainand the suite reports145 tests passed, 0 failed. Re-run it afterrm -rf src/generated, to rule out the directorymerely being inherited from an earlier run, and it is
145 passedagain. Themutant survives, so the bootstrap behaviour — and the NatSpec claim that "the
first generation in a repo does not need it committed already" — is unprobed.
The masker is the suite's own scaffolding. Both
test/lib/LibFs.buildFileForContract.t.sol:40andtest/src/lib/LibFs.isPresent.t.sol:26callvm.createDir(GENERATED_DIR, true)in
setUp, so the directory is always already there by the time the librarywould have created it.
Those
setUpcalls are not the bug and are not removed here. They exist becauseevery generating test writes under
GENERATED_DIRandforgeruns testcontracts in parallel, so a test that deletes the directory to make it missing
races all of its siblings. That is the hazard issue #64 measured directly: 1
failure in 6 full-suite runs, 0 in 8 runs under
-j 1.What ships
Issue #64 offered two ways to land the test — serialize the suite, or make the
directory injectable. This takes the second, because the first taxes every run
forever to cover one line.
src/lib/LibFs.sol:pathForContractIn(string dir, string contractName), private. The namecheck is unchanged, so the path is still a direct child of
dirfor everyname accepted at all.
buildFileForContract(Vm, address, string dir, string contractName, string body), internal overload.diris what gets created when missing and whatthe file is written a direct child of.
The existing
pathForContractand four-argumentbuildFileForContractare noweach the new function applied to
GENERATED_DIRand are otherwise untouched, sono consumer sees a behaviour change.
diris interpolated verbatim and is notchecked — that is why
pathForContractInis private rather than internal, andit is stated in the NatSpec of both new functions.
One NatSpec line is corrected while it is being touched: the directory is created
"along with any missing parent of it", which
vm.createDir(dir, true)has alwaysdone and the docstring did not say.
test/lib/LibFs.buildFileForContract.t.sol:testBuildFileForContractCreatesTheDirectoryownssrc/generated/LibFsBuildCreatesDir/nestedand never touchesGENERATED_DIRitself, so it does not race a single sibling. Two levels are missing, so a
create that made the leaf but not the missing parent fails here too.
Mutant proven dead
With
vm.createDir(dir, true)removed from the new overload, on this branch:Exactly one test fails, and it is the new one. That is the same mutant that
survives 145/145 on
main.QA
origin/main=7394a42, merged in (merge, not rebase).nix develop -c forge test—Ran 19 test suites: 146 tests passed, 0 failed, 0 skipped (146 total tests).mainis 145; the one added test is the wholedifference.
nix develop -c forge fmt --check— clean, no diff.git status --porcelain— empty after a full run.