fix(dojo-core): add test to showcase serialization issue on options #3036
Conversation
glihm
changed the title
|
Ohayo, sensei! WalkthroughThis pull request makes three main changes. It simplifies the export of public items in the core library by replacing an explicit list with a wildcard export. Additionally, it introduces two new public types—a new enum ( Changes
Sequence Diagram(s)sequenceDiagram
participant Test as write_read_option_enums
participant World as World/Storage
participant Model as Model Data
Test->>World: Initialize world and request model (key=1)
World->>Model: Fetch model data with enum & option fields
Model-->>World: Return model (EnumOne::One, Option::Some(0))
World-->>Test: Provide model for assertion
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: 0
🧹 Nitpick comments (2)
crates/dojo/core-cairo-test/src/tests/helpers/helpers.cairo (1)
45-52: Consider documenting the serialization behavior, sensei!Since this struct is designed to showcase a serialization issue with options, it would be helpful to add documentation explaining the expected behavior and known limitations.
Add documentation like this:
#[derive(Drop, Serde, Debug)] #[dojo::model] +/// Struct used to demonstrate Option serialization behavior. +/// Note: Currently, Option::None is serialized as Option::Some(0), +/// which may lead to unexpected behavior. pub struct WithOptionAndEnums { #[key] pub id: u32, pub a: EnumOne, pub b: Option<u32>, }crates/dojo/core-cairo-test/src/tests/world/storage.cairo (1)
126-137: Excellent test case for documenting the serialization issue, sensei!The test effectively demonstrates the Option serialization behavior. However, we could enhance it by:
- Testing both Some and None cases
- Adding more detailed documentation about the workaround
Consider expanding the test like this:
#[test] fn write_read_option_enums() { let (mut world, _) = deploy_world_and_foo(); - let key: u32 = 1; + // Test Option::None (currently serializes as Some(0)) + let none_key: u32 = 1; + let wo: WithOptionAndEnums = world.read_model(none_key); + assert_eq!(wo.a, EnumOne::One); + assert_eq!(wo.b, Option::Some(0), "Option::None serializes as Some(0)"); - let wo: WithOptionAndEnums = world.read_model(key); - assert_eq!(wo.a, EnumOne::One); - // Should have been `Option::None`. Need to find a way to mitigate this issue - // using a `DojoOption` converter or customizing the serialization. - assert_eq!(wo.b, Option::Some(0)); + // Test Option::Some + let some_key: u32 = 2; + let model = WithOptionAndEnums { + id: some_key, + a: EnumOne::Two(42), + b: Option::Some(42) + }; + world.write_model(@model); + + let wo: WithOptionAndEnums = world.read_model(some_key); + assert_eq!(wo.a, EnumOne::Two(42)); + assert_eq!(wo.b, Option::Some(42), "Option::Some serializes correctly"); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/dojo/core-cairo-test/src/lib.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/helpers/helpers.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/world/storage.cairo(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: ensure-wasm
- GitHub Check: docs
- GitHub Check: build
🔇 Additional comments (2)
crates/dojo/core-cairo-test/src/lib.cairo (1)
46-46: Ohayo! LGTM on the wildcard export, sensei!The change to use
pub use helpers::*simplifies the exports, which is acceptable in a test context.crates/dojo/core-cairo-test/src/tests/helpers/helpers.cairo (1)
38-43: Ohayo! Nice enum definition, sensei!The
EnumOneenum is well-structured with appropriate derive macros and varied variant types, making it perfect for testing serialization.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3036 +/- ##
==========================================
- Coverage 56.31% 56.30% -0.01%
==========================================
Files 437 437
Lines 59013 59013
==========================================
- Hits 33232 33229 -3
- Misses 25781 25784 +3 ☔ View full report in Codecov by Sentry. |
@remybar this is the issue mentioned by message.
Let's see what could be the best solution for that, of it's something we need to be aware of, and accept the behavior.
But this is very error prone for users. We will document it if we don't have an evident fix without breaking storage backward compatibility.
Summary by CodeRabbit