feat: generic snapshots #233
Conversation
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
WalkthroughAlrighty, here's the lowdown: The snapshot test framework got a solid tune-up, now supporting multiple networks and epochs instead of just Preprod. There's a new script to auto-generate Rust test cases based on available snapshots, and error handling is beefed up for missing files. Some trait implementations and dependencies got sorted too. Changes
Sequence Diagram(s)sequenceDiagram
participant Script as Bash Script
participant FS as File System
participant RustIncl as generated_compare_snapshot_test_cases.incl
participant TestRunner as Rust Test Runner
Script->>FS: Scan snapshots directory for network folders
Script->>FS: Find all summary__rewards_summary_<epoch>.snap files
Script->>RustIncl: Write test cases for each (network, epoch)
TestRunner->>RustIncl: Run generated test cases
RustIncl->>TestRunner: Call compare_snapshot(network, epoch)
Poem
✨ Finishing Touches
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 (
|
Codecov ReportAttention: Patch coverage is
... and 17 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
a0186df to
48a820d
Compare
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (34)
crates/amaru/tests/snapshots/preprod/summary__rewards_summary_163.snapis excluded by!**/*.snapcrates/amaru/tests/snapshots/preprod/summary__rewards_summary_164.snapis excluded by!**/*.snapcrates/amaru/tests/snapshots/preprod/summary__rewards_summary_165.snapis excluded by!**/*.snapcrates/amaru/tests/snapshots/preprod/summary__rewards_summary_166.snapis excluded by!**/*.snapcrates/amaru/tests/snapshots/preprod/summary__rewards_summary_167.snapis excluded by!**/*.snapcrates/amaru/tests/snapshots/preprod/summary__rewards_summary_168.snapis excluded by!**/*.snapcrates/amaru/tests/snapshots/preprod/summary__rewards_summary_169.snapis excluded by!**/*.snapcrates/amaru/tests/snapshots/preprod/summary__rewards_summary_170.snapis excluded by!**/*.snapcrates/amaru/tests/snapshots/preprod/summary__rewards_summary_171.snapis excluded by!**/*.snapcrates/amaru/tests/snapshots/preprod/summary__rewards_summary_172.snapis excluded by!**/*.snapcrates/amaru/tests/snapshots/preprod/summary__rewards_summary_173.snapis excluded by!**/*.snapcrates/amaru/tests/snapshots/preprod/summary__rewards_summary_174.snapis excluded by!**/*.snapcrates/amaru/tests/snapshots/preprod/summary__rewards_summary_175.snapis excluded by!**/*.snapcrates/amaru/tests/snapshots/preprod/summary__rewards_summary_176.snapis excluded by!**/*.snapcrates/amaru/tests/snapshots/preprod/summary__rewards_summary_177.snapis excluded by!**/*.snapcrates/amaru/tests/snapshots/preprod/summary__rewards_summary_178.snapis excluded by!**/*.snapcrates/amaru/tests/snapshots/preprod/summary__rewards_summary_179.snapis excluded by!**/*.snapcrates/amaru/tests/snapshots/preprod/summary__stake_distribution_163.snapis excluded by!**/*.snapcrates/amaru/tests/snapshots/preprod/summary__stake_distribution_164.snapis excluded by!**/*.snapcrates/amaru/tests/snapshots/preprod/summary__stake_distribution_165.snapis excluded by!**/*.snapcrates/amaru/tests/snapshots/preprod/summary__stake_distribution_166.snapis excluded by!**/*.snapcrates/amaru/tests/snapshots/preprod/summary__stake_distribution_167.snapis excluded by!**/*.snapcrates/amaru/tests/snapshots/preprod/summary__stake_distribution_168.snapis excluded by!**/*.snapcrates/amaru/tests/snapshots/preprod/summary__stake_distribution_169.snapis excluded by!**/*.snapcrates/amaru/tests/snapshots/preprod/summary__stake_distribution_170.snapis excluded by!**/*.snapcrates/amaru/tests/snapshots/preprod/summary__stake_distribution_171.snapis excluded by!**/*.snapcrates/amaru/tests/snapshots/preprod/summary__stake_distribution_172.snapis excluded by!**/*.snapcrates/amaru/tests/snapshots/preprod/summary__stake_distribution_173.snapis excluded by!**/*.snapcrates/amaru/tests/snapshots/preprod/summary__stake_distribution_174.snapis excluded by!**/*.snapcrates/amaru/tests/snapshots/preprod/summary__stake_distribution_175.snapis excluded by!**/*.snapcrates/amaru/tests/snapshots/preprod/summary__stake_distribution_176.snapis excluded by!**/*.snapcrates/amaru/tests/snapshots/preprod/summary__stake_distribution_177.snapis excluded by!**/*.snapcrates/amaru/tests/snapshots/preprod/summary__stake_distribution_178.snapis excluded by!**/*.snapcrates/amaru/tests/snapshots/preprod/summary__stake_distribution_179.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
conformance-tests/scripts/generate.mjs(1 hunks)crates/amaru-kernel/src/network.rs(2 hunks)crates/amaru/Cargo.toml(1 hunks)crates/amaru/build.rs(1 hunks)crates/amaru/tests/summary.rs(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: Build on ubuntu-24.04 with target aarch64-unknown-linux-musl
- GitHub Check: Build on ubuntu-latest with target x86_64-unknown-linux-gnu
- 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: Build on macos-latest with target aarch64-apple-darwin
- GitHub Check: Coverage
- GitHub Check: Snapshots (preprod, 10.1.4)
- GitHub Check: Simulation
- 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: Build on ubuntu-24.04 with target aarch64-unknown-linux-musl
- GitHub Check: Build on macos-latest with target aarch64-apple-darwin
- GitHub Check: Build on ubuntu-latest with target x86_64-unknown-linux-gnu
🔇 Additional comments (13)
crates/amaru/Cargo.toml (1)
59-59: Fair dinkum, good addition mate!Adding
amaru-kernelas a build dependency makes perfect sense for the build script to accessNetworkNametypes when generating those test cases. It's like giving your build script the right tools for the job - no point trying to hammer a nail with a screwdriver, eh?crates/amaru-kernel/src/network.rs (2)
17-17: Spot on with the import!Adding the
pallas_addresses::Networkimport to support the conversion trait - clean as a whistle!
237-245: Crikey, that's a neat conversion implementation!The
From<NetworkName> for Networktrait is bang on - mapping Mainnet to Mainnet and everything else to Testnet makes perfect sense sincepallas_addressesonly has those two variants. It's like choosing between Neo's red pill or blue pill - there's only two options in the Matrix, mate!conformance-tests/scripts/generate.mjs (2)
109-109: Path update looks bonzer!The source path update from
"crates/amaru/src/ledger/rewards.rs"to"crates/amaru-ledger/src/summary/rewards.rs"aligns with the code reorganization. It's like updating your GPS when they change the street names - gotta keep up with the times!
110-114: Bloody brilliant defensive programming!Adding the file existence check with proper error handling is top-notch! Rather than letting the script fail mysteriously later, you're catching the problem early and giving a clear error message. It's like checking if your keys are in your pocket before you lock the door - saves you a world of trouble later, mate!
crates/amaru/tests/summary.rs (4)
68-71: Ace automation with the generated test cases!Including the generated test cases from the build script is like having a robot butler that automatically sets up all your tests - pure genius! No more manually adding test cases when new snapshots appear.
74-74: Function signature makeover looks mint!Generalizing from
compare_preprod_snapshottocompare_snapshotwith aNetworkNameparameter is the right move for network-agnostic testing. It's like upgrading from a one-trick pony to a Swiss Army knife!
95-102: Dynamic snapshot paths - you beauty!Using
insta::with_settings!to set the snapshot path dynamically based on the network name is brilliant organization. Each network gets its own snapshot folder - tidy as a cricket pitch!
116-123: Consistent snapshot handling - top shelf!Same dynamic snapshot path approach for the rewards summary - consistency is key, like having matching socks!
crates/amaru/build.rs (4)
15-22: G'day! Nice clean imports, mate!The import additions look spot on - you've got all the essential bits for file operations and the NetworkName enum. Clean as a whistle!
88-88: Don't forget to handle the Result, mate!Since we're suggesting the function return a Result, you'll need to handle it in main() too.
- generated_compare_snapshot_test_cases(); + if let Err(e) = generated_compare_snapshot_test_cases() { + panic!("Failed to generate snapshot test cases: {}", e); + }
55-62: The string parsing logic is solid as a rock, legend!Love how you're using
strip_prefixandstrip_suffix- that's some proper Rust idiomatic code right there! The pattern matching for extracting epoch numbers is clean and handles edge cases nicely. This bit's tighter than a drum!
24-31:✅ Verification successful
Solid enum conversion, but mind the gap!
This helper function is clean as a fresh Foster's, but I'm wondering if we're covering all the NetworkName variants here. Like making sure we didn't miss any characters in our ensemble cast, you know?
Let me check if all NetworkName variants are handled:
🏁 Script executed:
#!/bin/bash # Description: Check if all NetworkName enum variants are covered in the conversion function # Expected: All enum variants should be handled in the match statement ast-grep --pattern $'enum NetworkName { $$$ }'Length of output: 49
Ah, let’s give our enum a proper once-over—time to crack open the code and see every NetworkName variant in action.
🏁 Script executed:
#!/bin/bash # Locate the NetworkName enum and list its variants rg -n -A 5 "enum NetworkName" .Length of output: 361
All NetworkName variants covered—looks shipshape!
I checked
crates/amaru-kernel/src/network.rsand saw exactly these variants:
- Mainnet
- Preprod
- Preview
- Testnet(u32)
Your
network_name_to_stringmatch handles each one. No gaps here—good on ya!
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
scripts/generate-snapshot-test-cases (2)
18-43: Network name mapping is top-notch, but consider some edge cases!The
network_name_to_stringfunction handles the main suspects beautifully - mainnet, preprod, preview, and even testnet variants with magic numbers. Though like any good RPG character build, you might want to consider what happens with unexpected inputs.Consider adding some logging for unknown network names to help with debugging:
*) # Fallback: capitalize first letter + echo "Warning: Unknown network '$network', using fallback capitalization" >&2 echo "${network^}" ;;
66-80: File operations are safe but consider atomic writes!The approach of truncating and writing is fine for a generated file, but if this script might run concurrently (like in a CI pipeline), you might want to consider atomic writes to avoid race conditions.
Consider using a temporary file for atomic writes:
-# Create/truncate the output file -> "$DEST_PATH" +# Create temporary file for atomic write +TEMP_FILE=$(mktemp) +trap 'rm -f "$TEMP_FILE"' EXIT # Write test cases to file for TEST_CASE in "${TEST_CASES[@]}"; do NETWORK="${TEST_CASE%:*}" EPOCH="${TEST_CASE#*:}" NETWORK_ENUM=$(network_name_to_string "$NETWORK") - echo "#[test_case(NetworkName::$NETWORK_ENUM, $EPOCH)]" >> "$DEST_PATH" + echo "#[test_case(NetworkName::$NETWORK_ENUM, $EPOCH)]" >> "$TEMP_FILE" done # Append the test function content -cat >> "$DEST_PATH" << 'EOF' +cat >> "$TEMP_FILE" << 'EOF' #[ignore] pub fn compare_snapshot_test_case(network_name: NetworkName, epoch: u64) { let epoch = Epoch::from(epoch); compare_snapshot(network_name, epoch) } EOF + +# Atomic move +mv "$TEMP_FILE" "$DEST_PATH"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/amaru/tests/generated_compare_snapshot_test_cases.incl(1 hunks)crates/amaru/tests/summary.rs(3 hunks)scripts/generate-snapshot-test-cases(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/amaru/tests/summary.rs
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Simulation
- GitHub Check: Coverage
- GitHub Check: Build on macos-latest with target aarch64-apple-darwin
- GitHub Check: Build on ubuntu-latest with target x86_64-unknown-linux-gnu
- GitHub Check: Build on ubuntu-latest with target aarch64-unknown-linux-musl
- GitHub Check: Build on windows-latest with target x86_64-pc-windows-msvc
- GitHub Check: Snapshots (preprod, 10.1.4)
- GitHub Check: Sanity
🔇 Additional comments (5)
crates/amaru/tests/generated_compare_snapshot_test_cases.incl (2)
1-18: G'day mate! All tests are on the bench - is this the plan?Fair dinkum, every single test case is marked with
#[ignore], which means they'll be sitting on the sidelines like a substitute player in FIFA. Just want to double-check this is intentional for your e2e test strategy, yeah?
19-22: Clean as a whistle, this function!The test function implementation is spot on - simple conversion from
u64toEpochand delegating tocompare_snapshot. It's like a perfect assist in a footy match - does exactly what it needs to do without any fancy footwork.scripts/generate-snapshot-test-cases (3)
1-16: Solid defensive play with the error checking!Love the upfront validation - checking both the destination file and snapshots directory exist before diving in. It's like checking your equipment before a big game, proper preparation prevents poor performance!
56-60: Regex pattern matching is chef's kiss!The pattern
^summary__rewards_summary_([0-9]+)\.snap$is bang on for extracting epoch numbers. Clean capture group usage that would make even the most pedantic code reviewer happy as Larry.
82-91: Script finale is clean as a new pair of boots!The heredoc approach for appending the test function is textbook perfect - readable, maintainable, and does exactly what it says on the tin. The confirmation message at the end is a nice touch too!
Make sure
snapshotstests (run viamake test-e2e) are network agnostic. Also generate thetest_caseannotation based onsnapfiles. This paves the way to having tests run pick up all generated files.Summary by CodeRabbit
New Features
Bug Fixes
Tests