Fix #848 - formatting and assertion improvements#857
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAcross multiple integration test files, assertion patterns were standardized to return the assertion macro’s Result directly instead of using the ? operator followed by Ok(()). Several helpers now conclude with the assertion expression as the final return value. Assertions were migrated to project-specific macros (assert_eq_prop, assert_ne_prop, assert_gte_prop, assert_lte_prop, assert_prop) and error message formatting within assertion macros was unified. Minor doc comment and punctuation tweaks were made. One helper in test_account_selection (assert_supports_isrc6) now returns Result and callers updated to await.unwrap(). No public API surface outside tests was modified. Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/test_blocks_generation.rs (1)
75-77: Implement assert_tx_succeeded_pre_confirmed and propagate its Result at call sites.tests/integration/common/utils.rs currently contains a no‑op that prints "TODO" and returns (). Change it to return Result<(), anyhow::Error> and implement the checks: fetch the transaction receipt, assert ExecutionResult::Succeeded, assert finality_status() == TransactionFinalityStatus::PreConfirmed, and return Ok(()) or anyhow::bail on failure.
Update all test call sites to propagate the Result (use ? or make the helper the tail expression), e.g. in tests/integration/test_blocks_generation.rs at lines 75–77, 108–110, 165–172, 337–341, 461–463 (and any other callers).
🧹 Nitpick comments (1)
tests/integration/test_messaging.rs (1)
978-983: Avoid borrowing from a temporary JSON value.Borrowing a field from the temporary returned by send_custom_rpc is harder to read and easy to regress. Bind the response first, then index.
Apply:
- let generated_l2_txs_raw = - &devnet.send_custom_rpc("devnet_postmanFlush", json!({})).await.unwrap() - ["generated_l2_transactions"]; - let generated_l2_txs = generated_l2_txs_raw.as_array().unwrap(); + let flush_resp = devnet.send_custom_rpc("devnet_postmanFlush", json!({})).await.unwrap(); + let generated_l2_txs = flush_resp["generated_l2_transactions"].as_array().unwrap();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
tests/integration/common/assertions.rs(10 hunks)tests/integration/common/utils.rs(5 hunks)tests/integration/test_abort_blocks.rs(1 hunks)tests/integration/test_account_impersonation.rs(2 hunks)tests/integration/test_account_selection.rs(4 hunks)tests/integration/test_advancing_time.rs(1 hunks)tests/integration/test_blocks_generation.rs(7 hunks)tests/integration/test_dump_and_load.rs(1 hunks)tests/integration/test_messaging.rs(3 hunks)tests/integration/test_minting.rs(1 hunks)tests/integration/test_subscription_to_tx_status.rs(1 hunks)tests/integration/test_trace.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: Abeeujah
PR: 0xSpaceShard/starknet-devnet#848
File: tests/integration/test_blocks_generation.rs:65-78
Timestamp: 2025-09-18T13:11:58.957Z
Learning: The function assert_tx_succeeded_pre_confirmed in tests/integration/common/utils.rs was not converted to return Result<(), anyhow::Error> and still returns unit type (), unlike assert_tx_succeeded_accepted which was converted to return Result.
Learnt from: Abeeujah
PR: 0xSpaceShard/starknet-devnet#848
File: tests/integration/test_blocks_generation.rs:141-162
Timestamp: 2025-09-18T13:15:00.238Z
Learning: The function assert_tx_succeeded_pre_confirmed in tests/integration/common/utils.rs is currently unimplemented - it only contains a TODO message printed via eprintln! and has all the actual assertion logic commented out. The function still returns unit type () rather than Result<(), anyhow::Error> and essentially acts as a no-op stub.
Learnt from: Abeeujah
PR: 0xSpaceShard/starknet-devnet#848
File: tests/integration/test_blocks_generation.rs:141-162
Timestamp: 2025-09-18T13:15:00.238Z
Learning: In tests/integration/common/utils.rs, the function assert_tx_succeeded_pre_confirmed currently contains only a TODO comment and doesn't perform any actual assertions, making it effectively unimplemented despite having its signature updated to return Result<(), anyhow::Error>.
Learnt from: Abeeujah
PR: 0xSpaceShard/starknet-devnet#848
File: tests/integration/test_blocks_generation.rs:141-162
Timestamp: 2025-09-18T13:15:00.238Z
Learning: In tests/integration/common/utils.rs, the function assert_tx_succeeded_pre_confirmed currently contains only a TODO comment and doesn't perform any actual assertions, making it effectively unimplemented despite having its signature updated to return Result<(), anyhow::Error>.
📚 Learning: 2025-09-18T13:11:58.957Z
Learnt from: Abeeujah
PR: 0xSpaceShard/starknet-devnet#848
File: tests/integration/test_blocks_generation.rs:65-78
Timestamp: 2025-09-18T13:11:58.957Z
Learning: The function assert_tx_succeeded_pre_confirmed in tests/integration/common/utils.rs was not converted to return Result<(), anyhow::Error> and still returns unit type (), unlike assert_tx_succeeded_accepted which was converted to return Result.
Applied to files:
tests/integration/test_subscription_to_tx_status.rstests/integration/test_account_impersonation.rstests/integration/common/assertions.rstests/integration/test_dump_and_load.rstests/integration/test_advancing_time.rstests/integration/test_messaging.rstests/integration/common/utils.rstests/integration/test_minting.rstests/integration/test_blocks_generation.rstests/integration/test_account_selection.rstests/integration/test_trace.rstests/integration/test_abort_blocks.rs
📚 Learning: 2025-09-18T13:15:00.238Z
Learnt from: Abeeujah
PR: 0xSpaceShard/starknet-devnet#848
File: tests/integration/test_blocks_generation.rs:141-162
Timestamp: 2025-09-18T13:15:00.238Z
Learning: In tests/integration/common/utils.rs, the function assert_tx_succeeded_pre_confirmed currently contains only a TODO comment and doesn't perform any actual assertions, making it effectively unimplemented despite having its signature updated to return Result<(), anyhow::Error>.
Applied to files:
tests/integration/test_subscription_to_tx_status.rstests/integration/test_account_impersonation.rstests/integration/test_dump_and_load.rstests/integration/test_advancing_time.rstests/integration/test_messaging.rstests/integration/common/utils.rstests/integration/test_minting.rstests/integration/test_blocks_generation.rstests/integration/test_account_selection.rstests/integration/test_trace.rstests/integration/test_abort_blocks.rs
📚 Learning: 2025-09-18T13:15:00.238Z
Learnt from: Abeeujah
PR: 0xSpaceShard/starknet-devnet#848
File: tests/integration/test_blocks_generation.rs:141-162
Timestamp: 2025-09-18T13:15:00.238Z
Learning: The function assert_tx_succeeded_pre_confirmed in tests/integration/common/utils.rs is currently unimplemented - it only contains a TODO message printed via eprintln! and has all the actual assertion logic commented out. The function still returns unit type () rather than Result<(), anyhow::Error> and essentially acts as a no-op stub.
Applied to files:
tests/integration/test_subscription_to_tx_status.rstests/integration/test_account_impersonation.rstests/integration/test_dump_and_load.rstests/integration/test_advancing_time.rstests/integration/test_messaging.rstests/integration/common/utils.rstests/integration/test_minting.rstests/integration/test_blocks_generation.rstests/integration/test_account_selection.rstests/integration/test_trace.rstests/integration/test_abort_blocks.rs
📚 Learning: 2025-06-16T07:35:24.725Z
Learnt from: FabijanC
PR: 0xSpaceShard/starknet-devnet#799
File: crates/starknet-devnet-core/src/starknet/mod.rs:240-246
Timestamp: 2025-06-16T07:35:24.725Z
Learning: In the Starknet devnet codebase, `get_class_hash_at` has different behaviors at different layers: at the internal state level (blockifier), it returns `Ok(ClassHash(Felt::ZERO))` for undeployed addresses, while at the RPC layer, this gets converted to `Err(Error::ContractNotFound)`. When checking if a contract is deployed at the internal state level, use `is_ok_and(|h| h.0 == Felt::ZERO)` to detect undeployed addresses.
Applied to files:
tests/integration/test_blocks_generation.rstests/integration/test_account_selection.rs
🔇 Additional comments (26)
tests/integration/common/assertions.rs (3)
1-1: LGTM!The backtick formatting for the value
falsein the doc comment improves documentation clarity.
27-27: LGTM!The punctuation improvements in the doc comments enhance readability and consistency.
Also applies to: 99-99, 135-135
38-39: LGTM!The error message formatting standardization using explicit interpolation (
{left_val:?}and{right_val:?}) is more readable and consistent across all assertion macros.Also applies to: 53-54, 74-75, 89-90, 110-111, 125-126, 146-147, 161-162
tests/integration/test_trace.rs (1)
59-62: LGTM!The simplification from explicit error propagation with
?followed byOk(())to returning the assertion macro result directly is consistent with the broader test suite refactor pattern. This improves consistency and reduces boilerplate code.tests/integration/test_abort_blocks.rs (1)
37-37: LGTM!The refactoring aligns with the broader test suite pattern of returning assertion macro results directly instead of using explicit error propagation with
?andOk(()). This creates more consistent error handling across test helpers.tests/integration/test_dump_and_load.rs (1)
51-51: LGTM!The change from explicit error propagation to returning the assertion macro result directly is consistent with the broader test suite refactoring pattern. This simplifies the code and standardizes error handling across test functions.
tests/integration/test_subscription_to_tx_status.rs (1)
51-51: LGTM!The refactoring to return the assertion macro result directly instead of using explicit error propagation is consistent with the broader test suite changes. This creates more uniform error handling patterns across the codebase.
tests/integration/test_minting.rs (1)
50-50: LGTM!The change to return the assertion result directly aligns with the consistent refactoring pattern across the test suite. This eliminates unnecessary boilerplate and creates uniform error handling.
tests/integration/test_advancing_time.rs (1)
37-37: LGTM!Both functions now return the assertion macro result directly instead of using explicit error propagation with
?followed byOk(()). This creates consistent error handling patterns and reduces boilerplate code while maintaining the same functional behavior.Also applies to: 43-43
tests/integration/common/utils.rs (5)
38-38: LGTM!The import adjustments appropriately add the needed assertion macros for the refactored test utilities.
243-243: LGTM!The function now returns the assertion result directly instead of explicitly returning
Ok(()), which is consistent with the broader test suite refactoring pattern.
450-453: LGTM!The variable rename from
etoelimproves readability, and the error message format using explicit interpolation ({el:?}) is more consistent with the updated assertion patterns.
490-490: LGTM!The refactoring to return the assertion macro result directly is consistent with the broader changes across test utilities and eliminates unnecessary boilerplate.
601-601: LGTM!Converting from block comment to inline comment improves code consistency and readability.
tests/integration/test_account_impersonation.rs (2)
15-15: Good switch to project assertion macros.Importing assert_eq_prop and assert_gte_prop aligns this file with the propagating-Result pattern.
260-261: Propagating assertions look correct.
- Checking ExecutionResult via assert_eq_prop with ? is consistent.
- Final-expression assert_gte_prop cleanly returns Result from the helper.
Also applies to: 266-267
tests/integration/test_messaging.rs (2)
183-189: Tail-return assertion in trace validator.Using assert_eq_prop as the final expression keeps the helper concise and consistent.
208-221: Return the assertion result directly.assert_eq_prop!(resp_body, expected_resp) as the tail expression is spot on.
tests/integration/test_blocks_generation.rs (5)
34-38: Assertion message formatting tweak LGTM.The assert_prop! messages are clearer; leaving the “but found:” suffix to the macro’s formatter is consistent with the suite.
Also applies to: 44-48
182-184: Nice tail-expression return.assert_eq_prop!(balance, expected) as the final expression simplifies the helper.
195-198: Consistent use of propagating assertion.Latest and pre-confirmed nonce checks now return the macro result directly.
210-215: Consistent storage assertion.Good use of tail-returning assertion for latest storage.
227-232: Consistent class-hash assertion.Returning the assertion result directly keeps helpers uniform.
tests/integration/test_account_selection.rs (3)
65-66: Use of assert_eq_prop as tail return is consistent.Good replacement of trailing Ok(()) with the macro result.
237-253: Helper now returns Result and uses propagating assertion.assert_supports_isrc6 returning Result<(), anyhow::Error> and asserting via assert_eq_prop improves consistency and failure messages.
260-261: Call sites updated appropriately..await.unwrap() aligns with the test style elsewhere.
Also applies to: 269-270
Usage related changes
None
Development related changes
Checklist:
./scripts/format.sh./scripts/clippy_check.sh./scripts/check_unused_deps.sh./scripts/check_spelling.sh./website/README.mdSummary by CodeRabbit
Tests
Documentation
Refactor
Style
Chores