feat(core): add tests for event upgrades #2976
Conversation
WalkthroughOhayo, sensei! This PR introduces enhancements to event handling and model upgrade testing in the Dojo core Cairo test suite. The changes focus on adding new enumerations ( Changes
Possibly related PRs
Suggested labels
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/dojo/core-cairo-test/src/tests/world/model.cairo (1)
Line range hint
235-269: Consider enhancing test coverage for model upgrades.While the test is well-structured, consider adding assertions for:
- Pre-upgrade state validation
- Transition of other fields during upgrade
- Edge cases with different enum variants
Example enhancement:
#[test] fn test_upgrade_model_with_member_changed() { let caller = starknet::contract_address_const::<0xb0b>(); let world = deploy_world_for_model_upgrades(); let mut world_storage = dojo::world::WorldStorageTrait::new(world, @"dojo"); + + // Verify pre-upgrade state + let initial: FooModelMemberChanged = world_storage.read_model(caller); + assert!(initial.b == 456, 'unexpected initial state'); drop_all_events(world.contract_address); // ... existing test code ... // values previously set in deploy_world_for_model_upgrades let read: FooModelMemberChanged = world_storage.read_model(caller); assert!(read.a == (MyEnum::X(42), 189, 0)); assert!(read.b == 456); + + // Test with different enum variant + let different_variant = FooModelMemberChanged { + caller, + a: (MyEnum::Y(100), 189, 0), + b: 456, + }; + world_storage.write_model(caller, different_variant); + let read_different = world_storage.read_model(caller); + assert!(read_different.a == (MyEnum::Y(100), 189, 0)); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
crates/dojo/core-cairo-test/Scarb.lockis excluded by!**/*.lockcrates/torii/types-test/Scarb.lockis excluded by!**/*.lockexamples/simple/Scarb.lockis excluded by!**/*.lockexamples/spawn-and-move/Scarb.lockis excluded by!**/*.lockspawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (3)
crates/dojo/core-cairo-test/src/tests/helpers/event.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/world/event.cairo(3 hunks)crates/dojo/core-cairo-test/src/tests/world/model.cairo(1 hunks)
🔇 Additional comments (4)
crates/dojo/core-cairo-test/src/tests/helpers/event.cairo (2)
105-106: LGTM! Event registration is properly configured.The new event types are correctly added to the namespace definition.
66-78:⚠️ Potential issueOhayo sensei! Ensure enum variants match between helper and test files.
The
MyEnumin this file only has variantX: u8, while the test file's version includesY: u16. This mismatch could cause issues during event upgrades.Apply this diff to align with the test file:
enum MyEnum { X: u8, + Y: u16, }Likely invalid or redundant comment.
crates/dojo/core-cairo-test/src/tests/world/event.cairo (2)
201-228: LGTM! Well-structured test for event member changes.The test thoroughly validates the event upgrade process by checking:
- Event emission
- Selector correctness
- Class hash verification
- Address validation
281-291: LGTM! Proper validation of illegal changes.The test correctly verifies that attempting to upgrade with incompatible member types triggers the expected error.
| #[derive(Introspect, Copy, Drop, Serde)] | ||
| enum AnotherEnum { | ||
| X: u8, | ||
| } | ||
|
|
||
| #[derive(Introspect, Copy, Drop, Serde)] | ||
| #[dojo::event] | ||
| struct FooEventMemberIllegalChange { | ||
| #[key] | ||
| pub caller: ContractAddress, | ||
| pub a: AnotherEnum, | ||
| pub b: u128, | ||
| } |
There was a problem hiding this comment.
Type mismatch in AnotherEnum between files.
The AnotherEnum::X variant is u8 here but bool in the test file. This inconsistency will cause upgrade failures.
Apply this diff to match the test file:
enum AnotherEnum {
- X: u8,
+ X: bool,
}📝 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.
| #[derive(Introspect, Copy, Drop, Serde)] | |
| enum AnotherEnum { | |
| X: u8, | |
| } | |
| #[derive(Introspect, Copy, Drop, Serde)] | |
| #[dojo::event] | |
| struct FooEventMemberIllegalChange { | |
| #[key] | |
| pub caller: ContractAddress, | |
| pub a: AnotherEnum, | |
| pub b: u128, | |
| } | |
| #[derive(Introspect, Copy, Drop, Serde)] | |
| enum AnotherEnum { | |
| X: bool, | |
| } | |
| #[derive(Introspect, Copy, Drop, Serde)] | |
| #[dojo::event] | |
| struct FooEventMemberIllegalChange { | |
| #[key] | |
| pub caller: ContractAddress, | |
| pub a: AnotherEnum, | |
| pub b: u128, | |
| } |
| #[derive(Introspect, Copy, Drop, Serde, PartialEq)] | ||
| enum MyEnum { | ||
| X: u8, | ||
| Y: u16, | ||
| } | ||
|
|
||
| #[derive(Introspect, Copy, Drop, Serde)] | ||
| #[dojo::event] | ||
| struct FooEventMemberChanged { | ||
| #[key] | ||
| pub caller: ContractAddress, | ||
| pub a: (MyEnum, u8, u32), | ||
| pub b: u128, | ||
| } |
There was a problem hiding this comment.
Tuple type mismatch between event implementations.
The FooEventMemberChanged.a tuple includes an additional u32 compared to the helper version:
- Helper:
(MyEnum, u8) - Test:
(MyEnum, u8, u32)
Update the helper version to match:
struct FooEventMemberChanged {
#[key]
pub caller: ContractAddress,
- pub a: (MyEnum, u8),
+ pub a: (MyEnum, u8, u32),
pub b: u128,
}Committable suggestion skipped: line range outside the PR's diff.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2976 +/- ##
==========================================
- Coverage 57.13% 57.12% -0.01%
==========================================
Files 424 424
Lines 56198 56198
==========================================
- Hits 32107 32102 -5
- Misses 24091 24096 +5 ☔ View full report in Codecov by Sentry. |
Add new tests for event upgrade as it has been done for models in #2925.
Summary by CodeRabbit
New Features
Tests
Enhancements