add tests for engine and processors #2918
Conversation
WalkthroughOhayo, sensei! The pull request introduces several asynchronous test functions in the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Domo arigato for your attention to detail, sensei! The new tests are a fantastic addition to our testing framework! 🍵🥷 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
crates/dojo/core-cairo-test/src/world.cairo (3)
109-112: Consider enhancing error handling for deploy_syscall, sensei! 🎯While the code works, unwrapping the result directly might not provide the best error context in case of deployment failures.
Consider wrapping the error with more context:
- let (contract, _) = starknet::syscalls::deploy_syscall( - class_hash.try_into().unwrap(), 0, calldata, false, - ) - .unwrap(); + let (contract, _) = starknet::syscalls::deploy_syscall( + class_hash.try_into().unwrap(), 0, calldata, false, + ) + .expect('Failed to deploy contract');
164-176: The resource registration looks solid, sensei! But let's make it even better! 🌟The match expression handles all resource types cleanly, but the
try_into().unwrap()calls could benefit from better error handling.Consider adding specific error messages for each conversion:
- world.register_event(namespace.clone(), (*ch).try_into().unwrap()); + world.register_event(namespace.clone(), (*ch).try_into().expect('Invalid event class hash'));
Line range hint
190-218: The permission syncing looks great, sensei! Just one suggestion! 🔐The contract lookup error handling could be more descriptive.
Consider enhancing the error message:
- _ => panic!("Contract not found"), + _ => panic!("Contract not found in namespace: {}", *namespace),crates/torii/indexer/src/test.rs (3)
609-610: Strengthen the assertion to verify the exact number of entities createdOhayo sensei! In the test, we are asserting that
entity_count > 0, which verifies that at least one entity is created. To ensure that all entities are processed correctly, consider asserting the exact number of entities expected.You can apply this diff to strengthen the assertion:
- assert!(entity_count > 0, "Entity should be created after StoreSetRecord event"); + assert_eq!(entity_count, EXPECTED_ENTITY_COUNT, "Unexpected number of entities created");Replace
EXPECTED_ENTITY_COUNTwith the actual expected number of entities.
702-705: Handle potential errors when fetching the head cursorSensei, currently, the code uses
unwrap_or(0)when fetching the head cursor, which may mask errors during database access. Consider usingunwrap()or handling the error explicitly to ensure any issues are surfaced during the test.You can apply this diff to handle potential errors:
- let head: i64 = sqlx::query_scalar("SELECT MAX(value) FROM cursors WHERE key = 'head'") - .fetch_one(&pool) - .await - .unwrap_or(0); + let head: i64 = sqlx::query_scalar("SELECT MAX(value) FROM cursors WHERE key = 'head'") + .fetch_one(&pool) + .await + .unwrap();
709-710: Consider a graceful shutdown of the engine instead of abortingSensei, using
engine_handle.abort()forcefully stops the engine task, which might not allow for proper cleanup. Implementing a graceful shutdown mechanism can help ensure resources are properly released.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
crates/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 (55)
crates/dojo/core-cairo-test/src/lib.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/benchmarks.cairo(12 hunks)crates/dojo/core-cairo-test/src/tests/contract.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/event/event.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/helpers/event.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/helpers/helpers.cairo(6 hunks)crates/dojo/core-cairo-test/src/tests/helpers/model.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo(6 hunks)crates/dojo/core-cairo-test/src/tests/model/model.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/storage/packing.cairo(6 hunks)crates/dojo/core-cairo-test/src/tests/utils/hash.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/utils/key.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/utils/layout.cairo(4 hunks)crates/dojo/core-cairo-test/src/tests/utils/misc.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/world/acl.cairo(9 hunks)crates/dojo/core-cairo-test/src/tests/world/contract.cairo(7 hunks)crates/dojo/core-cairo-test/src/tests/world/event.cairo(16 hunks)crates/dojo/core-cairo-test/src/tests/world/metadata.cairo(6 hunks)crates/dojo/core-cairo-test/src/tests/world/model.cairo(17 hunks)crates/dojo/core-cairo-test/src/tests/world/namespace.cairo(3 hunks)crates/dojo/core-cairo-test/src/tests/world/storage.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/world/world.cairo(10 hunks)crates/dojo/core-cairo-test/src/utils.cairo(1 hunks)crates/dojo/core-cairo-test/src/world.cairo(7 hunks)crates/dojo/core/src/contract/components/upgradeable.cairo(2 hunks)crates/dojo/core/src/contract/components/world_provider.cairo(2 hunks)crates/dojo/core/src/event/component.cairo(2 hunks)crates/dojo/core/src/event/event.cairo(2 hunks)crates/dojo/core/src/lib.cairo(3 hunks)crates/dojo/core/src/meta/introspect.cairo(2 hunks)crates/dojo/core/src/meta/layout.cairo(2 hunks)crates/dojo/core/src/model/component.cairo(2 hunks)crates/dojo/core/src/model/definition.cairo(1 hunks)crates/dojo/core/src/model/metadata.cairo(2 hunks)crates/dojo/core/src/model/model.cairo(2 hunks)crates/dojo/core/src/model/model_value.cairo(1 hunks)crates/dojo/core/src/model/storage.cairo(4 hunks)crates/dojo/core/src/storage/database.cairo(1 hunks)crates/dojo/core/src/storage/entity_model.cairo(6 hunks)crates/dojo/core/src/storage/layout.cairo(19 hunks)crates/dojo/core/src/storage/packing.cairo(7 hunks)crates/dojo/core/src/storage/storage.cairo(11 hunks)crates/dojo/core/src/utils/layout.cairo(2 hunks)crates/dojo/core/src/utils/misc.cairo(1 hunks)crates/dojo/core/src/utils/serde.cairo(1 hunks)crates/dojo/core/src/utils/snf_test.cairo(2 hunks)crates/dojo/core/src/world/errors.cairo(1 hunks)crates/dojo/core/src/world/iworld.cairo(8 hunks)crates/dojo/core/src/world/resource.cairo(1 hunks)crates/dojo/core/src/world/storage.cairo(24 hunks)crates/dojo/core/src/world/world_contract.cairo(43 hunks)crates/torii/indexer/src/test.rs(3 hunks)examples/simple/src/lib.cairo(4 hunks)examples/spawn-and-move/src/actions.cairo(7 hunks)examples/spawn-and-move/src/models.cairo(3 hunks)
✅ Files skipped from review due to trivial changes (49)
- crates/dojo/core/src/utils/serde.cairo
- crates/dojo/core-cairo-test/src/lib.cairo
- crates/dojo/core-cairo-test/src/tests/utils/hash.cairo
- crates/dojo/core-cairo-test/src/tests/event/event.cairo
- crates/dojo/core/src/model/model_value.cairo
- crates/dojo/core-cairo-test/src/tests/utils/key.cairo
- crates/dojo/core/src/model/model.cairo
- crates/dojo/core-cairo-test/src/utils.cairo
- crates/dojo/core/src/model/definition.cairo
- crates/dojo/core/src/lib.cairo
- crates/dojo/core/src/world/resource.cairo
- crates/dojo/core-cairo-test/src/tests/world/event.cairo
- crates/dojo/core-cairo-test/src/tests/world/world.cairo
- crates/dojo/core-cairo-test/src/tests/helpers/event.cairo
- crates/dojo/core/src/utils/misc.cairo
- crates/dojo/core-cairo-test/src/tests/helpers/model.cairo
- crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo
- crates/dojo/core/src/meta/introspect.cairo
- crates/dojo/core-cairo-test/src/tests/storage/packing.cairo
- crates/dojo/core-cairo-test/src/tests/model/model.cairo
- crates/dojo/core-cairo-test/src/tests/world/contract.cairo
- crates/dojo/core/src/storage/database.cairo
- crates/dojo/core/src/world/errors.cairo
- examples/simple/src/lib.cairo
- crates/dojo/core-cairo-test/src/tests/utils/misc.cairo
- crates/dojo/core-cairo-test/src/tests/world/storage.cairo
- examples/spawn-and-move/src/actions.cairo
- crates/dojo/core/src/model/component.cairo
- crates/dojo/core-cairo-test/src/tests/world/metadata.cairo
- crates/dojo/core-cairo-test/src/tests/benchmarks.cairo
- crates/dojo/core/src/event/event.cairo
- crates/dojo/core/src/meta/layout.cairo
- crates/dojo/core-cairo-test/src/tests/world/acl.cairo
- crates/dojo/core/src/utils/snf_test.cairo
- crates/dojo/core/src/utils/layout.cairo
- crates/dojo/core-cairo-test/src/tests/world/model.cairo
- crates/dojo/core/src/contract/components/world_provider.cairo
- crates/dojo/core/src/model/metadata.cairo
- examples/spawn-and-move/src/models.cairo
- crates/dojo/core-cairo-test/src/tests/helpers/helpers.cairo
- crates/dojo/core/src/contract/components/upgradeable.cairo
- crates/dojo/core/src/world/storage.cairo
- crates/dojo/core/src/storage/storage.cairo
- crates/dojo/core/src/storage/entity_model.cairo
- crates/dojo/core/src/world/iworld.cairo
- crates/dojo/core/src/storage/layout.cairo
- crates/dojo/core/src/world/world_contract.cairo
- crates/dojo/core/src/model/storage.cairo
- crates/dojo/core/src/storage/packing.cairo
🔇 Additional comments (9)
crates/dojo/core/src/event/component.cairo (3)
6-6: Ohayo! Nice type parameter addition, sensei! ✨The explicit Event type parameter improves type safety and makes the relationship between the event type and its wrapper clearer.
15-15: Consistent type parameter usage, sensei! 🎯The Event type parameter addition here maintains consistency with the IDeployedEventImpl implementation above.
Line range hint
26-26: Excellent completion of the type system improvements, sensei! 🌟This final Event type parameter addition rounds out the changes nicely, creating a more robust and type-safe event system across all three trait implementations.
crates/dojo/core-cairo-test/src/tests/world/namespace.cairo (1)
34-34: Ohayo! Nice cleanup of panic message formatting, sensei! 🎯The removal of trailing commas in panic message tuples makes the code more consistent and cleaner. The changes maintain the same functionality while improving code style.
Also applies to: 48-48, 73-73
crates/dojo/core-cairo-test/src/tests/contract.cairo (1)
180-180: Consistent formatting maintained here too, sensei! ✨The panic message formatting aligns perfectly with the changes in namespace.cairo, maintaining a unified style across the codebase.
crates/dojo/core-cairo-test/src/tests/utils/layout.cairo (1)
10-11: Ohayo! Beautiful formatting improvements, sensei! 🌟The consistent formatting of array definitions and
span()method calls greatly improves code readability:
- Moving
.span()to new lines makes the code structure clearer- Proper indentation of closing brackets enhances visual hierarchy
This style makes the code much easier to scan and maintain!
Also applies to: 25-26, 39-40, 56-57
crates/dojo/core-cairo-test/src/world.cairo (2)
58-64: Ohayo! The ContractDef initialization looks clean, sensei! ✨The struct initialization follows good practices with consistent formatting.
72-72: Nice error handling, sensei! 🛡️The panic message clearly indicates why init_calldata can't be set for address descriptors.
Also applies to: 79-79
crates/torii/indexer/src/test.rs (1)
834-838: Verify the expected number of entities created matches the number of transactionsSensei, the test sends 3 spawn transactions concurrently but asserts that only 2 entities are created. Please verify whether this is the expected behavior. If only 2 entities are created consistently, kindly add a comment explaining why this occurs.
|
Hey @g4titanx, thanks for the contribution here! Would you mind addressing first the rebase + the ensuring correct linting of the code? Too much diffs that are not in the scope of the PR which makes it heavier to review. :) Thank you! |
hey @glihm how do i rebase to the specific one you work with and also what's the lint command used for this project? |
Rebasing on main will be ok. 👍 |
|
And the lint command is: |
962bf01 to
cd015ad
Compare
|
done @glihm |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
crates/torii/indexer/src/test.rs (6)
545-549: Handle potential errors instead of usingunwrap()Ohayo, sensei! In this section:
let res = world .grant_writer(&compute_bytearray_hash("ns"), &ContractAddress(actions_address)) .send_with_cfg(&TxnConfig::init_wait()) .await .unwrap();Using
unwrap()will cause the test to panic if an error occurs. Consider handling the error gracefully usingexpect()with a meaningful message or using proper error handling to aid in debugging.
565-566: Handle executor errors gracefullyOhayo, sensei! In the spawned executor task:
tokio::spawn(async move { executor.run().await.unwrap(); });Using
unwrap()here may cause the background task to panic ifexecutor.run()returns an error. Consider handling the error appropriately, perhaps logging it usingtracing::error!, to prevent unexpected panics.
702-705: Avoid masking errors when fetching the head cursorOhayo, sensei! When fetching the head cursor:
let head: i64 = sqlx::query_scalar("SELECT MAX(value) FROM cursors WHERE key = 'head'") .fetch_one(&pool) .await .unwrap_or(0);Using
unwrap_or(0)might hide issues where the cursor is not properly initialized. It would be better to handle theOptionexplicitly or useexpect()to surface potential errors during testing.
707-707: Enhance assertion messages for clarityOhayo, sensei! The assertion:
assert!(head >= 0, "Engine should initialize with valid head cursor");Could be improved by including the actual
headvalue in the message:assert!( head >= 0, "Engine should initialize with valid head cursor, but got head = {}", head );This provides more context if the test fails.
814-815: Ensure entities are created as expectedOhayo, sensei! You assert that the processor creates at least one entity:
assert!(!entities.is_empty(), "Processor should create at least one entity");Consider specifying the exact number of entities expected to strengthen the test's effectiveness.
632-710: Improve error handling intest_engine_backoff_and_recoveryOhayo, sensei! In the test:
tokio::spawn(async move { if let Err(e) = engine.start().await { eprintln!("Engine error: {:?}", e); } });As mentioned earlier, consider using
tracing::error!instead ofeprintln!for consistent logging.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
spawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (1)
crates/torii/indexer/src/test.rs(3 hunks)
🔇 Additional comments (2)
crates/torii/indexer/src/test.rs (2)
519-630: Great job on comprehensive test documentationOhayo, sensei! The
test_processor_events_handlingfunction is well-documented and clearly outlines the test's purpose and steps. This enhances readability and maintainability.
779-792: Clarify the expected number of entities createdOhayo, sensei! In this loop, you send three spawn transactions:
for i in 0..3 { let tx = account .execute_v1(vec![Call { to: actions_address, selector: get_selector_from_name("spawn").unwrap(), calldata: vec![], }]) .send() .await .unwrap(); txs.push(tx.transaction_hash); }However, later you assert that exactly two entities are created:
assert_eq!( entities.len(), 2, "Processor consistently creates 2 entities under concurrent load" );Could you clarify why only two entities are expected when three transactions are sent? This might indicate an inconsistency that needs to be addressed.
✅ Verification successful
Expected behavior: Test verifies concurrent processing consistency
Ohayo, sensei! I can confirm this is actually expected behavior. The test
test_concurrent_event_processingis specifically designed to verify that under concurrent load, the system consistently creates exactly 2 entities, even when processing 3 concurrent spawn transactions. This is documented in the test's comments:/// 3. Expected entity creation behavior (creates 2 entities consistently)The test is intentionally verifying that the concurrent processing maintains consistent behavior and data integrity.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find the complete test function containing this spawn loop ast-grep --pattern 'async fn $test_name($_) { $$$ for i in 0..3 { let tx = account .execute_v1(vec![Call { to: actions_address, selector: get_selector_from_name("spawn").unwrap(), calldata: vec![], }]) .send() .await .unwrap(); $$$ } $$$ }' # Also search for any comments about concurrent processing rg -B 5 "concurrent" crates/torii/indexer/src/test.rsLength of output: 1476
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2918 +/- ##
==========================================
+ Coverage 55.75% 56.23% +0.47%
==========================================
Files 445 445
Lines 57627 57910 +283
==========================================
+ Hits 32129 32564 +435
+ Misses 25498 25346 -152 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/indexer/src/test.rs(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
crates/torii/indexer/src/test.rs
[warning] 25-25: Incorrect ordering of imports: 'use tracing::{info, error}' should be 'use tracing::{error, info}'
[warning] 673-676: Incorrect code formatting: EngineConfig initialization should be formatted in a single line with proper spacing
🔇 Additional comments (4)
crates/torii/indexer/src/test.rs (4)
696-698: Use structured logging instead of eprintln!Replace error logging with tracing for consistent logging practices.
- error!("Engine error: {:?}", e); + tracing::error!("Engine error: {:?}", e);
519-630: LGTM! Well-structured test for event processing.Ohayo, sensei! The test implementation is thorough and well-documented, covering essential aspects of event processing with clear assertions and verifications.
714-834: LGTM! Excellent concurrent testing implementation.The test effectively validates concurrent event processing with proper logging, clear assertions, and thorough verification of data consistency.
676-679:⚠️ Potential issueFix EngineConfig formatting.
The EngineConfig initialization should be formatted in a single line.
- let config = EngineConfig { - polling_interval: Duration::from_millis(100), - ..Default::default() - }; + let config = EngineConfig { polling_interval: Duration::from_millis(100), ..Default::default() };Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/torii/indexer/src/test.rs (2)
519-630: Add assertions for event messages.Ohayo, sensei! The test comprehensively verifies entity creation and model registration. Consider adding assertions to verify the event_messages table to ensure events are properly recorded:
assert_eq!(keys, format!("{:#x}/", account.address())); + +// Verify event messages +let event_count = count_table("event_messages", &pool).await; +assert_eq!(event_count, 2, "Should have two events: spawn and set_player_config");
779-792: Document magic numbers and expected behavior.Ohayo, sensei! The test uses magic numbers without explaining their significance:
- Why are exactly 3 concurrent transactions sent?
- Why do we expect exactly 2 entities to be created?
Consider adding constants with descriptive names and comments explaining these expectations:
+ // We send multiple transactions to test concurrent processing + const CONCURRENT_TX_COUNT: usize = 3; + // The system creates 2 entities: one for the player and one for the game state + const EXPECTED_ENTITY_COUNT: usize = 2; + let mut txs = vec![]; - for i in 0..3 { + for i in 0..CONCURRENT_TX_COUNT { let tx = account .execute_v1(vec![Call { // ... assert_eq!( entities.len(), - 2, + EXPECTED_ENTITY_COUNT, "Processor consistently creates 2 entities under concurrent load" );Also applies to: 827-831
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/indexer/src/test.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (2)
crates/torii/indexer/src/test.rs (2)
4-4: LGTM!Ohayo, sensei! The addition of
Durationimport is well-placed and necessary for the new test functions.
694-696: Use structured logging instead ofeprintln!Ohayo, sensei! In the engine task:
- error!("Engine error: {:?}", e); + error!(error = ?e, "Engine error");This change follows structured logging best practices.
|
@glihm please review |
this pr fixes #1679
Summary by CodeRabbit