Relative import paths in test/src/lib - #142
Conversation
Bare project-root paths only resolve through foundry's root auto-remap, which points at the consumer's tree when the package is consumed. src/ already imports relatively. Closes #94
…elative-test-imports
|
Warning Review limit reached
Next review available in: 45 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 (35)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (37)
Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review. WalkthroughThe change replaces bare or remapped Solidity imports in test and harness files with repository-relative paths. Imported entities and test behavior remain unchanged. ChangesTest import path normalization
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to This PR only updates test import paths without changing production code or test behavior; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
…elative-test-imports Eight test files conflicted. Every conflict was the same shape: main changed which symbols are imported and from where, this branch changed only the shape of the path. Each is resolved by taking main's import list verbatim and re-applying this branch's rewrite, so the error consolidation from #140 and the new coverage from #133/#126/#127 land intact with relative paths. LibBytes.t.sol TruncateError -> OutOfBoundsTruncate, now from src/error/ErrTruncate.sol LibBytes32Array.truncate.t.sol OutOfBoundsTruncate moved off LibUint256Array.truncate.t.sol ErrUint256Array.sol onto ErrTruncate.sol LibStackSentinel.t.sol sentinel errors moved out of the library and into src/error/ErrStackSentinel.sol, and MissingSentinel/ZeroSentinelTupleSize added LibBytes32Matrix.flatten.t.sol main added imports for new tests LibUint256Matrix.flatten.t.sol LibBytes32Matrix.itemCount.t.sol LibUint256Matrix.itemCount.t.sol Imports only. The merged tree differs from main on import lines alone, and all 112 first-party imports resolve to the same files they do on main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
test/src/error/ErrTruncate.t.sol landed on main with #140 after this branch was cut, carrying four fresh bare src/ imports. Same situation as LibArray.aliasedTail.t.sol in 67ff20b, and converted the same way, so that the invariant this PR claims holds on the merge result: grep -rn 'from "\(src\|test\)/' src/ test/ now returns nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…elative-test-imports # Conflicts: # test/src/error/ErrTruncate.t.sol # test/src/lib/LibArray.aliasedTail.t.sol # test/src/lib/LibArray.selfExtend.t.sol # test/src/lib/LibBytes.t.sol # test/src/lib/LibBytes32Array.pointer.t.sol # test/src/lib/LibBytes32Array.reverse.t.sol # test/src/lib/LibBytes32Array.truncate.t.sol # test/src/lib/LibBytes32Matrix.flatten.t.sol # test/src/lib/LibBytes32Matrix.itemCount.t.sol # test/src/lib/LibBytes32Matrix.matrixFrom.t.sol # test/src/lib/LibBytes32Matrix.pointer.t.sol # test/src/lib/LibMatrix.flattenWrap.t.sol # test/src/lib/LibStackSentinel.tupleSizeWrap.t.sol # test/src/lib/LibUint256Array.pointer.t.sol # test/src/lib/LibUint256Array.reverse.t.sol # test/src/lib/LibUint256Array.truncate.t.sol # test/src/lib/LibUint256Matrix.flatten.t.sol # test/src/lib/LibUint256Matrix.itemCount.t.sol # test/src/lib/LibUint256Matrix.matrixFrom.t.sol # test/src/lib/LibUint256Matrix.pointer.t.sol
1 participant
Closes #94
Verified against main
Branched at
9a238f4,origin/mainmerged in at7620643.The claim holds and has grown.
grep -rn 'from "\(src\|test\)/' src/ test/returned 83 hits at9a238f4(the issue said 73), all intest/src/lib/, zero insrc/, which already imports relatively throughout.remappings.txtcarries onlyforge-std-1.16.1/=dependencies/forge-std-1.16.1/, so these resolve solely through foundry's project-root auto-remap — which points at the consumer's tree, not ours, whenever the package is consumed.The issue's second ask —
LibStackSentinel.t.solreachingLibUint256Arrayindirectly throughsrc/lib/LibStackPointer.sol— is already fixed on main:LibStackPointer.solno longer exists and the import is direct fromsrc/lib/LibUint256Array.sol. Only the bare-path conversion remained.The issue predicted the convention "gets copied into a new file". It did, during this branch's life:
test/src/lib/LibArray.aliasedTail.t.sol, landed on main by #121 after this branch was cut, arrived with two fresh baresrc/imports. They are converted here too, bringing the total to 85.Changed
Every import in
test/src/lib/**converted to relative, matchingsrc/:"src/lib/X.sol""../../../src/lib/X.sol""src/error/X.sol""../../../src/error/X.sol""test/lib/XSlow.sol""../../lib/XSlow.sol""test/src/lib/XHarness.sol""./XHarness.sol"Relative rather than the
rain-solmem/=src/self-remapping the issue offers as an alternative:.soldeerignoredropsremappings.txtfrom the package, so a remapping would split the repo's own convention betweensrc/(relative) andtest/(remapped) for no consumer-visible gain.Imports only. No source, no test logic, no assertion changed. The full suite passes before and after with the same counts.
QA
9a238f4and post-change are both349 passed, 0 failedacross 33 suites; after merging7620643in,353 passed, 0 failedacross 34 suites — the 4 extra tests areLibArray.aliasedTail.t.sol, which main added.src/lib/LibUint256Array.sol:399mstore(left, mload(right))→mstore(left, mload(left))../../../src/lib/binds to the real sourcetestReverse(uint256[]); suite ran 5 tests, 4 passed / 1 failedtest/lib/LibUint256ArraySlow.sol:165b[i] = a[a.length - i - 1]→b[i] = a[i]../../lib/binds to the real reference impltestReverse(uint256[]); suite ran 5 tests, 4 passed / 1 failedtest/src/lib/LibMatrixFlattenWrapHarness.sol:148canary[3] = CANARY→canary[3] = SCRIBBLE./binds to the sibling harnesstest/src/lib/LibMatrixFlattenWrapHarness.sol:49mstore(matrix, 2)→mstore(matrix, 0)./binds to the sibling harnesstestUint256ItemCountUncheckedSumUnderreportsTheTotal,testUint256ItemCountOverflowRevertsWithArithmeticPanic,testFlattenNonWrappingOversizedLengthFailsLoudly; 9 ran, 6 passed / 3 failedtest/src/lib/LibPointer.t.sol:7../../../src/lib/→../../src/lib/Error (6275): Source "test/src/lib/LibPointer.sol" not foundtest/src/lib/LibUint256Array.reverse.t.sol:8../../lib/→../lib/Error (6275): Source "test/src/lib/LibUint256ArraySlow.sol" not foundtest/src/lib/LibMatrix.flattenWrap.t.sol:6./→../Error (6275): Source "test/src/LibMatrixFlattenWrapHarness.sol" not foundM1–M3b break the target file and watch the suite notice, which is what proves each rewritten path still reaches the file it used to. M4–M6 break the path and watch resolution fail, which is the negative control the shape change needs: it proves the new paths resolve relatively and that no root auto-remap silently rescues a wrong one. Every run's own
Suite result/Ran N test suitesline is quoted, so no result rests on a filter that matched nothing.M3's survival is the documented design of
LibMatrix.flattenWrap.t.sol, not a gap this diff created. Its header states that every assertion lives in the success branch of atryso that any guard rejecting the forged count takes the emptycatchand passes. The #62 guard is on main, souint256FlattenThenWriteThroughResultreverts and the assertion never runs. Confirmed by plantingassertTrue(false, "catch taken")in all fourcatchbodies: all four report[FAIL: catch taken], 9 ran / 5 passed / 4 failed. These are regression sentinels for #62, so no mutation to that call path could have killed M3. M3b re-probes the same./binding throughitemCount, which the suite does observe.src/andtest/imports converted and (b)LibStackSentinel.t.sol's indirect import throughLibStackPointer.sol. (a) covers all 85 — the 73 that existed when the issue was filed, the 10 added since, and the 2 that landed on main mid-branch — andgrep -rn 'from "\(src\|test\)/' src/ test/now returns zero. (b) was already fixed on main before this branch.Not done here
Nothing stops the next new test file from arriving with bare paths again, as #121's did. A mechanical guard is the durable answer, but per the org convention that rainix owns shared CI, the lint belongs in the rainix reusable workflow rather than in this repo — a separate change against a separate repo.
Summary by CodeRabbit
Chores
Tests