test InsufficientFlowOutputs revert and boundary#442
Conversation
Pins the lower-bound guard that rejects flowOutputs < MIN_FLOW_SENTINELS returned by the deployer. Two tests: - Negative: any flowOutputs in [0, MIN_FLOW_SENTINELS) reverts with InsufficientFlowOutputs. - Boundary: flowOutputs == MIN_FLOW_SENTINELS is accepted (lowest valid). Mutation verified: inverting the check (`<` → `>=`) makes both tests fail; reverting passes. Closes #311 #321. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds test coverage for the ChangesFlow Construction Test Coverage
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/src/concrete/Flow.construction.t.sol`:
- Around line 42-45: Add an explicit fuzz-run pin for the boundary test by
configuring the test to run a fixed number of fuzz iterations (e.g., 2048) so
the sweep is stable and reproducible; update the test harness or metadata for
the function testFlowConstructionAcceptsFlowOutputsAtMin to include the
forge-config / fuzz-runs directive (matching the pattern used in the revert
test), ensuring the test is executed with the pinned run count every time.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 35a821b2-2973-436d-b85a-7756f2d5038f
📒 Files selected for processing (1)
test/src/concrete/Flow.construction.t.sol
| /// `flowOutputs == MIN_FLOW_SENTINELS` is the lowest accepted value. | ||
| /// Pinning this boundary against regression complements the negative | ||
| /// test above. | ||
| function testFlowConstructionAcceptsFlowOutputsAtMin( |
There was a problem hiding this comment.
Add explicit fuzz-run pin for the boundary test.
Line 45 introduces the boundary test, but unlike the revert test it does not pin fuzz runs. If the intent is the stated 2048-run boundary sweep, add an explicit forge-config line so this is stable and reproducible.
Suggested patch
/// `flowOutputs == MIN_FLOW_SENTINELS` is the lowest accepted value.
/// Pinning this boundary against regression complements the negative
/// test above.
+ /// forge-config: default.fuzz.runs = 2048
function testFlowConstructionAcceptsFlowOutputsAtMin(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// `flowOutputs == MIN_FLOW_SENTINELS` is the lowest accepted value. | |
| /// Pinning this boundary against regression complements the negative | |
| /// test above. | |
| function testFlowConstructionAcceptsFlowOutputsAtMin( | |
| /// `flowOutputs == MIN_FLOW_SENTINELS` is the lowest accepted value. | |
| /// Pinning this boundary against regression complements the negative | |
| /// test above. | |
| /// forge-config: default.fuzz.runs = 2048 | |
| function testFlowConstructionAcceptsFlowOutputsAtMin( |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/src/concrete/Flow.construction.t.sol` around lines 42 - 45, Add an
explicit fuzz-run pin for the boundary test by configuring the test to run a
fixed number of fuzz iterations (e.g., 2048) so the sweep is stable and
reproducible; update the test harness or metadata for the function
testFlowConstructionAcceptsFlowOutputsAtMin to include the forge-config /
fuzz-runs directive (matching the pattern used in the revert test), ensuring the
test is executed with the pinned run count every time.
There was a problem hiding this comment.
Rejecting. The pattern you reference doesn't exist:
- Revert test pins
forge-config: default.fuzz.runs = 100— an override down from the project default to keep the slowvm.assumesweep cheap. - Accept test has no override and inherits
foundry.toml'sfuzz.runs = 2048already.
Adding default.fuzz.runs = 2048 to the accept test is a no-op duplicate of the project default, not "matching the pattern". The boundary test is already running 2048 iterations.
There was a problem hiding this comment.
@thedavidmeister, you're right — I misread the intent. The forge-config directive on the revert test is a deliberate downward override to offset the cost of vm.assume rejection sampling, while the accept test correctly relies on the foundry.toml project default of 2048 runs. Adding the directive there would be a no-op. Acknowledged and sorry for the noise.
✏️ Learnings added
Learnt from: thedavidmeister
Repo: rainlanguage/flow PR: 442
File: test/src/concrete/Flow.construction.t.sol:42-45
Timestamp: 2026-05-07T12:52:45.226Z
Learning: In the `rainlanguage/flow` repository (Solidity/Foundry), `forge-config: default.fuzz.runs = N` in test NatDoc comments is used as a **downward** override from the project default (set to 2048 in `foundry.toml`). Tests with a heavy `vm.assume` rejection loop (e.g., `vm.assume(x < MIN_FLOW_SENTINELS)`) pin a lower run count (e.g., 100) to keep CI cheap. Tests without the directive intentionally inherit the 2048-run project default — adding `forge-config: default.fuzz.runs = 2048` to such tests is a no-op duplicate and should not be suggested.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
…321-insufficient-flow-outputs-test # Conflicts: # test/src/concrete/Flow.construction.t.sol
1 participant
Summary
Pins the
flowInitguard that rejects deployer-returnedflowOutputs < MIN_FLOW_SENTINELS(= 3). Two tests:flowOutputsin[0, MIN_FLOW_SENTINELS)reverts withInsufficientFlowOutputs.flowOutputs == MIN_FLOW_SENTINELSis accepted (lowest valid value).Mutation verified: inverting the check (
<→>=) makes both tests fail; reverting passes.Closes #311 #321.
Test plan
testFlowConstructionRevertsOnInsufficientFlowOutputs— 100 fuzz runstestFlowConstructionAcceptsFlowOutputsAtMin— 2048 fuzz runsFlow.sol:206→ both tests fail🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes