feat/simulation readme#352
Conversation
Signed-off-by: Stevan A <stevana@users.noreply.github.com>
Signed-off-by: Stevan A <stevana@users.noreply.github.com>
Signed-off-by: Stevan A <stevana@users.noreply.github.com>
Document all parameters in the read me. Signed-off-by: Stevan A <stevana@users.noreply.github.com>
Signed-off-by: Stevan A <stevana@users.noreply.github.com>
Signed-off-by: Stevan A <stevana@users.noreply.github.com>
Signed-off-by: Stevan A <stevana@users.noreply.github.com>
WalkthroughThis update introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Simulator
participant Shrinker
User->>CLI: Run simulation with --disable_shrinking flag
CLI->>Simulator: Pass disable_shrinking to SimulateConfig
Simulator->>Simulator: Execute test on generated entries
alt Test fails
Simulator->>Simulator: Check disable_shrinking
alt Shrinking disabled
Simulator->>Simulator: Display failure (no shrinking)
else Shrinking enabled
Simulator->>Shrinker: Shrink failing input
Shrinker-->>Simulator: Return minimized input
Simulator->>Simulator: Display failure with shrunk input
end
else Test passes
Simulator->>Simulator: Continue to next test
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
simulation/amaru-sim/README.md (2)
40-50: Environment variables documentation is aceThe usage section now clearly documents all the environment variables with their defaults. It's like having a proper game manual instead of just button-mashing until something works!
However, the static analysis tool has a point about the code block language specifier.
-``` +```bash AMARU_NUMBER_OF_TESTS=50 # Set the number of test cases to generate. \ AMARU_NUMBER_OF_NODES=1 # Set the number of nodes in a simulation. \ AMARU_NUMBER_OF_UPSTREAM_PEERS=2 # Set the number of upstream peers.
52-101: Debugging section is absolutely brilliant! 🎮This new debugging section is like having a strategy guide for when things go pear-shaped. The detailed breakdown of failure output components - minimized input, history, error message, saved schedule, and seed - is incredibly valuable for developers. It's like having a debug console in a video game that actually tells you useful stuff!
The example output makes it crystal clear what each part means and how to use it.
One tiny improvement for the static analysis tool:
-``` +```text Minimised input (0 shrinks):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
simulation/amaru-sim/README.md(1 hunks)simulation/amaru-sim/src/simulator/mod.rs(4 hunks)simulation/amaru-sim/src/simulator/simulate.rs(5 hunks)simulation/amaru-sim/tests/simulation.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: stevana
PR: pragma-org/amaru#210
File: simulation/amaru-sim/src/simulator/simulate.rs:264-277
Timestamp: 2025-05-12T14:21:27.470Z
Learning: The team plans to replace the out-of-process test in `simulation/amaru-sim/src/simulator/simulate.rs` with an in-process NodeHandle implementation in the future, eliminating the need for hard-coded binary paths (`../../target/debug/echo`) and making tests more reliable.
simulation/amaru-sim/README.md (5)
Learnt from: stevana
PR: #210
File: simulation/amaru-sim/src/simulator/simulate.rs:264-277
Timestamp: 2025-05-12T14:21:27.470Z
Learning: The team plans to replace the out-of-process test in simulation/amaru-sim/src/simulator/simulate.rs with an in-process NodeHandle implementation in the future, eliminating the need for hard-coded binary paths (../../target/debug/echo) and making tests more reliable.
Learnt from: abailly
PR: #250
File: simulation/amaru-sim/tests/data/stake-distribution.json:2-18
Timestamp: 2025-06-13T09:06:16.025Z
Learning: The file simulation/amaru-sim/tests/data/stake-distribution.json contains fully generated, non-sensitive private key values used solely for testing; they are considered safe to keep in the repository despite resembling real secrets.
Learnt from: abailly
PR: #228
File: crates/amaru-stores/src/rocksdb/consensus.rs:89-128
Timestamp: 2025-05-21T18:58:48.631Z
Learning: The InMemConsensusStore implementation in crates/amaru-stores/src/rocksdb/consensus.rs will be fleshed out incrementally on a by-need basis, driven by test requirements rather than implementing all functionality upfront.
Learnt from: rkuhn
PR: #206
File: crates/pure-stage/src/simulation/running.rs:240-242
Timestamp: 2025-05-09T13:09:47.915Z
Learning: Cloning messages in the pure-stage crate should be avoided for performance reasons. The current implementation in SimulationRunning deliberately avoids duplicating message data structures.
Learnt from: abailly
PR: #75
File: crates/amaru/src/consensus/mod.rs:164-165
Timestamp: 2025-02-03T11:15:22.640Z
Learning: In the Amaru project, chain selection operations (roll_forward and rollback) should use separate result types to leverage the type system for preventing impossible states, rather than using runtime checks or panics.
simulation/amaru-sim/tests/simulation.rs (5)
Learnt from: stevana
PR: #210
File: simulation/amaru-sim/src/simulator/simulate.rs:264-277
Timestamp: 2025-05-12T14:21:27.470Z
Learning: The team plans to replace the out-of-process test in simulation/amaru-sim/src/simulator/simulate.rs with an in-process NodeHandle implementation in the future, eliminating the need for hard-coded binary paths (../../target/debug/echo) and making tests more reliable.
Learnt from: abailly
PR: #250
File: simulation/amaru-sim/tests/data/stake-distribution.json:2-18
Timestamp: 2025-06-13T09:06:16.025Z
Learning: The file simulation/amaru-sim/tests/data/stake-distribution.json contains fully generated, non-sensitive private key values used solely for testing; they are considered safe to keep in the repository despite resembling real secrets.
Learnt from: rkuhn
PR: #149
File: crates/amaru/src/stages/consensus/chain_forward/test_infra.rs:0-0
Timestamp: 2025-04-20T17:56:39.223Z
Learning: For mpsc::channel in Tokio-based test code, use buffer sizes larger than 1 (e.g., 8) to avoid potential deadlocks when producers send multiple messages before consumers can process them.
Learnt from: abailly
PR: #228
File: crates/amaru-stores/src/rocksdb/consensus.rs:89-128
Timestamp: 2025-05-21T18:58:48.631Z
Learning: The InMemConsensusStore implementation in crates/amaru-stores/src/rocksdb/consensus.rs will be fleshed out incrementally on a by-need basis, driven by test requirements rather than implementing all functionality upfront.
Learnt from: rkuhn
PR: #149
File: crates/amaru/src/stages/consensus/chain_forward/test_infra.rs:272-285
Timestamp: 2025-04-20T17:57:23.233Z
Learning: In test infrastructure code, rkuhn prefers explicit panics (using .unwrap() or similar) over returning Result types, as test failures should be immediate and obvious.
simulation/amaru-sim/src/simulator/mod.rs (3)
Learnt from: stevana
PR: #210
File: simulation/amaru-sim/src/simulator/simulate.rs:264-277
Timestamp: 2025-05-12T14:21:27.470Z
Learning: The team plans to replace the out-of-process test in simulation/amaru-sim/src/simulator/simulate.rs with an in-process NodeHandle implementation in the future, eliminating the need for hard-coded binary paths (../../target/debug/echo) and making tests more reliable.
Learnt from: rkuhn
PR: #149
File: crates/amaru/src/stages/consensus/chain_forward/test_infra.rs:0-0
Timestamp: 2025-04-20T17:56:39.223Z
Learning: For mpsc::channel in Tokio-based test code, use buffer sizes larger than 1 (e.g., 8) to avoid potential deadlocks when producers send multiple messages before consumers can process them.
Learnt from: jeluard
PR: #69
File: crates/amaru/src/ledger/state/diff_epoch_reg.rs:112-117
Timestamp: 2025-01-21T15:32:17.911Z
Learning: When suggesting code changes in Rust, always verify that the types align correctly, especially when dealing with references and Options. The Fold::Registered variant in diff_epoch_reg.rs expects a reference &'a V, so unwrapping an Option<&V> requires only a single .expect().
simulation/amaru-sim/src/simulator/simulate.rs (2)
Learnt from: stevana
PR: #210
File: simulation/amaru-sim/src/simulator/simulate.rs:264-277
Timestamp: 2025-05-12T14:21:27.470Z
Learning: The team plans to replace the out-of-process test in simulation/amaru-sim/src/simulator/simulate.rs with an in-process NodeHandle implementation in the future, eliminating the need for hard-coded binary paths (../../target/debug/echo) and making tests more reliable.
Learnt from: rkuhn
PR: #149
File: crates/amaru/src/stages/consensus/chain_forward/test_infra.rs:272-285
Timestamp: 2025-04-20T17:57:23.233Z
Learning: In test infrastructure code, rkuhn prefers explicit panics (using .unwrap() or similar) over returning Result types, as test failures should be immediate and obvious.
🪛 markdownlint-cli2 (0.17.2)
simulation/amaru-sim/README.md
40-40: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
56-56: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (12)
simulation/amaru-sim/src/simulator/mod.rs (4)
98-99: G'day mate! Clean CLI argument addition 🎮The
disable_shrinkingboolean flag is properly defined using clap's derive API. Simple and effective - like choosing the right weapon in Dark Souls, sometimes you just need a straightforward approach!
358-358: Flag extraction looks spot onExtracting the
disable_shrinkingvalue from the args is clean and follows the same pattern as the other config values. No worries here, cobber!
378-378: Threading the flag through to SimulateConfigPerfect! The
disable_shrinkingflag is properly passed into theSimulateConfigstruct, maintaining consistency with the rest of the configuration pattern. Like a well-orchestrated heist in Ocean's Eleven - every piece falls into place!
413-414: Nice touch on the error formatting, legend!Adding proper indentation and newlines to the error message makes it much more readable when comparing expected vs actual chain tips. It's like the difference between reading subtitles in a Tarantino film versus having them all jumbled together - presentation matters!
simulation/amaru-sim/tests/simulation.rs (3)
15-26: Brilliant environment variable setup, mate! 🎬Love how you've made the test parameters configurable via environment variables. The pattern is consistent across all three numeric parameters with proper parsing and sensible fallbacks. It's like having cheat codes for your favorite game - gives you the control when you need it!
27-27: disable_shrinking environment integration looks sweetThe
AMARU_DISABLE_SHRINKINGenvironment variable integration follows the same boolean pattern aspersist_on_success. Checking for "1" is a solid approach that avoids ambiguity. Clean as a whistle!
31-31: persist_on_success env var update is consistentUpdated to use the same boolean environment variable pattern as the new
disable_shrinkingflag. Consistency is key, like having a matching outfit - everything just works better together!simulation/amaru-sim/README.md (1)
17-31: Top-notch overview section, champion! 📚This new Overview section is like a good movie trailer - gives you just enough info to understand what's what without spoiling the experience. Linking to the specific source files is particularly helpful for developers diving into the codebase.
simulation/amaru-sim/src/simulator/simulate.rs (4)
57-57: SimulateConfig struct updated perfectlyAdding the
disable_shrinkingboolean field to the config struct is clean and straightforward. Fits right in with the existing fields like it was always meant to be there!
293-320: Conditional shrinking logic is spot on, legend! 🎯This is brilliant work! The conditional logic is crystal clear:
- Extract the test function once (line 293)
- Run the test and handle failures appropriately
- If shrinking is disabled → immediately display failure with zero shrinks
- If shrinking is enabled → perform shrinking then display
It's like having a difficulty setting in a game - sometimes you want the full challenge (shrinking enabled), sometimes you just want to see what broke (shrinking disabled). The implementation preserves existing behavior while adding the new option seamlessly.
363-363: Nice formatting touch in display_failureAdding the newline after the saved schedule message improves readability. Small details like this make the difference between good and great user experience - like proper kerning in movie credits!
511-511: Test configurations updated consistentlyBoth test cases now include the
disable_shrinking: falseflag, maintaining backwards compatibility while supporting the new feature. Consistent as a good sequel - keeps what worked while adding new capabilities!Also applies to: 582-582
Codecov Report❌ Patch coverage is
... and 3 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Signed-off-by: Stevan A <stevana@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
simulation/amaru-sim/README.md (3)
27-29: Trim the duplicated sentence in the third bulletLines 27-29 repeat themselves like a scratched vinyl – “property-based test and property that uses the simulator” shows up twice. Let’s bin the echo to keep the doc tight.
-* The property-based test and property that uses the simulator, defined in - [`src/simulator/mod.rs`](src/simulator/mod.rs); - and property that uses the simulator is defined; +* The property-based test that drives the simulator, defined in + [`src/simulator/mod.rs`](src/simulator/mod.rs);
41-52: Add a language hint to the env-vars blockmarkdownlint is right – the code fence at Line 41 is missing a language tag.
Stick “bash” on it so editors and CI stop whingeing.-``` +```bash AMARU_NUMBER_OF_TESTS=50 # Set the number of test cases to generate. \ ... cargo test run_simulator--- `57-86`: **Label the debug-output fence as plain text** Same craic here: give the second fence a language to quiet MD040. “text” does the trick and won’t colour anything weird. ```diff -``` +```text Minimised input (0 shrinks): ... Seed: 42</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 436e531cab1fb1643e676ae7c2c914d773dba8b1 and 0bf67a657032b0b1053218b480a27982ae98ae13. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `simulation/amaru-sim/README.md` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧠 Learnings (2)</summary> <details> <summary>📓 Common learnings</summary>Learnt from: stevana
PR: #210
File: simulation/amaru-sim/src/simulator/simulate.rs:264-277
Timestamp: 2025-05-12T14:21:27.470Z
Learning: The team plans to replace the out-of-process test insimulation/amaru-sim/src/simulator/simulate.rswith an in-process NodeHandle implementation in the future, eliminating the need for hard-coded binary paths (../../target/debug/echo) and making tests more reliable.</details> <details> <summary>simulation/amaru-sim/README.md (5)</summary> Learnt from: stevana PR: pragma-org/amaru#210 File: simulation/amaru-sim/src/simulator/simulate.rs:264-277 Timestamp: 2025-05-12T14:21:27.470Z Learning: The team plans to replace the out-of-process test in `simulation/amaru-sim/src/simulator/simulate.rs` with an in-process NodeHandle implementation in the future, eliminating the need for hard-coded binary paths (`../../target/debug/echo`) and making tests more reliable. Learnt from: abailly PR: pragma-org/amaru#250 File: simulation/amaru-sim/tests/data/stake-distribution.json:2-18 Timestamp: 2025-06-13T09:06:16.025Z Learning: The file `simulation/amaru-sim/tests/data/stake-distribution.json` contains fully generated, non-sensitive private key values used solely for testing; they are considered safe to keep in the repository despite resembling real secrets. Learnt from: abailly PR: pragma-org/amaru#228 File: crates/amaru-stores/src/rocksdb/consensus.rs:89-128 Timestamp: 2025-05-21T18:58:48.631Z Learning: The InMemConsensusStore implementation in crates/amaru-stores/src/rocksdb/consensus.rs will be fleshed out incrementally on a by-need basis, driven by test requirements rather than implementing all functionality upfront. Learnt from: abailly PR: pragma-org/amaru#75 File: crates/amaru/src/consensus/mod.rs:164-165 Timestamp: 2025-02-03T11:15:22.640Z Learning: In the Amaru project, chain selection operations (roll_forward and rollback) should use separate result types to leverage the type system for preventing impossible states, rather than using runtime checks or panics. Learnt from: rkuhn PR: pragma-org/amaru#206 File: crates/pure-stage/src/simulation/running.rs:240-242 Timestamp: 2025-05-09T13:09:47.915Z Learning: Cloning messages in the pure-stage crate should be avoided for performance reasons. The current implementation in SimulationRunning deliberately avoids duplicating message data structures. </details> </details><details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> <details> <summary>simulation/amaru-sim/README.md</summary> 41-41: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 57-57: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)</summary> * GitHub Check: Snapshots (preprod, 1, 10.1.4) * GitHub Check: Build on ubuntu-latest with target riscv32im-risc0-zkvm-elf * GitHub Check: Build on windows-latest with target x86_64-pc-windows-msvc * GitHub Check: Coverage </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
rkuhn
left a comment
rkuhn
left a comment
There was a problem hiding this comment.
Too late for review: LGTM thanks!
3 participants
Summary by CodeRabbit
New Features
Documentation
Style