chore: use the new Katana runner proc macro #2465
Conversation
WalkthroughOhayo, sensei! This pull request introduces significant modifications to the testing framework by transitioning from the Changes
Possibly related PRs
Suggested labels
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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
Tip Early access features: enabledWe are currently testing new code review model(s) that may lead to higher noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (14)
crates/dojo-world/src/contracts/world_test.rs (1)
12-12: Ohayo once more, sensei! Function signature update looks great!The updated function signature with the
&RunnerCtxparameter is consistent with the new Katana runner integration. It's used effectively to initialize theaccountlater in the function.A small suggestion: Consider renaming the
sequencerparameter torunner_ctxorkatana_ctxto more accurately reflect its type and purpose. This might make the code slightly more self-documenting. What do you think, sensei?bin/sozo/tests/register_test.rs (1)
Line range hint
16-55: Sugoi changes, sensei! The test setup is now more elegant.The modifications to the test setup and execution look great. Here's what I noticed:
- The explicit
KatanaRunnerinstantiation has been removed, simplifying the code.- The
sequencerparameter is now used directly, which should provide the same functionality as before.- The core test logic remains consistent, indicating that the test's purpose hasn't changed.
These changes make the test more concise and easier to maintain. Excellent work!
For improved clarity, consider adding a brief comment explaining the purpose of the
RunnerCtxat the beginning of the function. For example:// `sequencer` provides the Katana runner context for the test environment async fn reregister_models(sequencer: &RunnerCtx) { // ... rest of the function }This would help future readers quickly understand the role of the
sequencerparameter.bin/sozo/tests/test_migrate.rs (1)
54-54: Ohayo, macro magic sensei!The new annotation looks fantastic! It's a great simplification of the test setup. However, I have a small suggestion:
Consider extracting the
copy_spawn_and_move_db().as_str()into a constant at the top of the file. This would improve readability and make it easier to update if needed. For example:const TEST_DB_DIR: &str = copy_spawn_and_move_db().as_str(); // Then use it in the annotation: #[katana_runner::test(db_dir = TEST_DB_DIR)]What do you think, sensei?
crates/sozo/ops/src/tests/utils.rs (2)
22-23: Ohayo again, sensei! Another test level up!Your
get_contract_address_from_stringfunction is looking sharp with its newkatana_runner::testoutfit! The updated signature with&RunnerCtxis totally in sync with the new style.One small suggestion to make it even more kawaii:
Consider adding the
fee = trueparameter to thekatana_runner::testattribute for consistency with the previous test. Like this:#[katana_runner::test(fee = true)]This way, all your tests will be perfectly balanced, as all things should be!
31-32: Ohayo once more, sensei! Your code is evolving faster than a Pokémon!Both
get_contract_address_from_world_with_world_readerandget_contract_address_from_string_with_world_readerhave leveled up nicely with their newkatana_runner::testattributes and&RunnerCtxparameters. It's like watching a perfect combo move in a fighting game!One small suggestion to make your code dojo even more harmonious:
Consider adding the
fee = trueparameter to thekatana_runner::testattribute for theget_contract_address_from_string_with_world_readerfunction, like this:#[katana_runner::test(fee = true)]This will maintain consistency across all your test functions, making your code as balanced as a zen garden!
Overall, fantastic work on modernizing these tests, sensei! Your code is truly becoming a masterpiece!
Also applies to: 47-48
crates/dojo-world/src/contracts/model_test.rs (1)
15-15: Ohayo, sensei! The new macro looks sugoi!The
#[katana_runner::test]macro simplifies the test setup nicely. It's a clean way to configure the runner with the database directory.Consider extracting the
copy_spawn_and_move_db().as_str()into a constant at the top of the file for better readability:const TEST_DB_DIR: &str = copy_spawn_and_move_db().as_str(); // Then use it in the macro #[katana_runner::test(db_dir = TEST_DB_DIR)]crates/sozo/ops/src/tests/auth.rs (3)
52-54: Ohayo, sensei! These changes are sugoi!The new Katana runner proc macro is working its magic here. The function signature update and the removal of the
KatanaRunnerinstantiation streamline our test setup beautifully.One small suggestion to make our code even more kawaii:
- let world = setup::setup_with_world(sequencer).await.unwrap(); + let world = setup::setup_with_world(sequencer).await?;This change would propagate the error and make our error handling more consistent. What do you think, sensei?
76-78: Ohayo again, sensei! More sugoi changes!The refactoring here mirrors the
auth_grant_writer_okfunction perfectly. It's like watching a synchronized swimming routine - everything's in harmony!Let's keep our error handling consistent here too:
- let world = setup::setup_with_world(sequencer).await.unwrap(); + let world = setup::setup_with_world(sequencer).await?;This change would make our code as smooth as a well-brewed matcha, sensei. What do you think?
111-118: Ohayo once more, sensei! These changes are as refreshing as a cool breeze on Mount Fuji!The refactoring in both
auth_grant_owner_okandauth_revoke_owner_okis consistent with the earlier changes. It's like watching a beautiful kata - each move perfectly executed!Let's keep our error handling consistent across all these functions:
- let world = setup::setup_with_world(sequencer).await.unwrap(); + let world = setup::setup_with_world(sequencer).await?;Also, sensei, I noticed this new line in
auth_grant_owner_ok:println!("sequencer logs: {:?}", sequencer.log_file_path());Is this logging necessary for the final version, or was it added for debugging purposes? If it's just for debugging, we might want to remove it or wrap it in a
#[cfg(debug_assertions)]attribute.What are your thoughts on these suggestions, sensei?
Also applies to: 147-152
crates/torii/core/src/sql_test.rs (1)
67-68: Ohayo, sensei! Great refactoring of the test setup!The switch to
#[katana_runner::test]with specific parameters is a nice touch. It simplifies the test setup and makes the configuration more explicit. The addition of thesequencerparameter is a smart move, allowing for a more streamlined test environment.One small suggestion:
Consider adding a brief comment explaining the purpose of the
accounts = 10parameter. This would help future developers understand why this specific number was chosen.crates/sozo/ops/src/tests/migration.rs (2)
60-61: Ohayo again, sensei! LGTM with a small suggestionThe changes to use
katana_runner::testandRunnerCtxare consistent with the new testing approach. Great job!A small suggestion: Consider renaming
declarersto something more specific, likesequencer_declarers, to make its origin and purpose clearer.Also applies to: 70-70
327-328: Ohayo for the last time, sensei! LGTM with a small suggestionThe changes in
migrate_with_auto_authorizeto usekatana_runner::testandRunnerCtxare good and consistent with the new testing approach.For
migration_with_mismatching_world_address_and_seed, the change to a synchronous function looks fine. However, it might be helpful to add a brief comment explaining why this test doesn't need to be asynchronous anymore. This would help future developers understand the reasoning behind this change.Also applies to: 350-350, 404-405
crates/dojo-world/src/manifest/manifest_test.rs (1)
288-289: Ohayo again, sensei! The function update is sugoi!The change to use
#[katana_runner::test]macro withRunnerCtxis a great improvement. It simplifies our test setup and reduces boilerplate code. The function body looks consistent with the previous implementation, which is excellent.However, I noticed a TODO comment about adding tests. Shall we address this, sensei?
Would you like me to help generate some additional test cases or open a GitHub issue to track this task?
crates/katana/runner/macro/src/config.rs (1)
160-176: Optimize by reducing unnecessary clones of expressionsOhayo sensei, in the
build_configfunction, cloningexprmight be unnecessary if the setter methods can accept references. This can improve performance by avoiding redundant copies.Consider updating the setter methods to accept
&syn::Exprand adjust the calls accordingly:-fn set_block_time(&mut self, block_time: syn::Expr, span: proc_macro2::Span) -> Result<(), syn::Error> { +fn set_block_time(&mut self, block_time: &syn::Expr, span: proc_macro2::Span) -> Result<(), syn::Error> { ... - config.set_block_time(expr.clone(), Spanned::span(&namevalue))? + config.set_block_time(expr, Spanned::span(&namevalue))?Apply similar changes to other setter methods to pass expressions by reference.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (15)
- bin/sozo/tests/register_test.rs (1 hunks)
- bin/sozo/tests/test_migrate.rs (2 hunks)
- crates/dojo-world/src/contracts/model_test.rs (2 hunks)
- crates/dojo-world/src/contracts/world_test.rs (1 hunks)
- crates/dojo-world/src/manifest/manifest_test.rs (2 hunks)
- crates/katana/runner/macro/src/config.rs (3 hunks)
- crates/katana/runner/macro/src/entry.rs (1 hunks)
- crates/katana/runner/tests/runner.rs (1 hunks)
- crates/sozo/ops/src/tests/auth.rs (5 hunks)
- crates/sozo/ops/src/tests/call.rs (7 hunks)
- crates/sozo/ops/src/tests/migration.rs (11 hunks)
- crates/sozo/ops/src/tests/model.rs (3 hunks)
- crates/sozo/ops/src/tests/utils.rs (3 hunks)
- crates/torii/core/src/sql_test.rs (4 hunks)
- crates/torii/grpc/src/server/tests/entities_test.rs (2 hunks)
Additional comments not posted (42)
crates/dojo-world/src/contracts/world_test.rs (3)
3-3: Ohayo, sensei! New import looks good!The import of
RunnerCtxfromkatana_runneris well-placed and necessary for the updated test function. Nice work!
11-11: Ohayo again, sensei! Excellent use of the new Katana runner!The new test annotation perfectly aligns with the PR objective of using the new Katana runner proc macro. It's a great way to integrate the test with the Katana runner framework and set up the database directory. Well done!
3-12: Ohayo, sensei! Overall, these changes are sugoi!The modifications to integrate the test with the new Katana runner proc macro are well-implemented. Here's a summary of the changes:
- Added import for
RunnerCtxfromkatana_runner.- Updated test annotation to use
katana_runner::test.- Modified function signature to include the
&RunnerCtxparameter.These changes successfully achieve the PR objective of using the new Katana runner proc macro. The test function body remains largely unchanged, which is good for maintaining the original test logic while improving the test infrastructure.
Great work, sensei! The code looks clean and well-integrated with the new runner.
bin/sozo/tests/register_test.rs (1)
6-6: Ohayo, sensei! Nice update to the test setup!The changes to the imports and function signature look good. Here's a breakdown:
- The new import
use katana_runner::RunnerCtx;introduces the Katana runner context.- The
#[katana_runner::test]attribute macro simplifies the test setup by specifying the database directory.- The updated function signature now takes a
&RunnerCtxparameter, which should provide the necessary test environment.These changes should make the test more streamlined and easier to manage.
Also applies to: 14-15
bin/sozo/tests/test_migrate.rs (3)
5-5: Ohayo, import statement sensei!The updated import looks great! It's perfectly aligned with the new test runner macro usage.
Line range hint
1-91: Ohayo, code review sensei! Let's wrap this up!Overall, the changes in this file are well-implemented and align perfectly with the PR objective of using the new Katana runner proc macro. Here's a summary of the improvements:
- The import statement has been updated to include the necessary
RunnerCtx.- The test function now uses a simplified setup with the
#[katana_runner::test]annotation.- The function signature has been updated to receive the
sequencercontext directly.These changes enhance the code's readability and maintainability. Great job, sensei! 🎉
55-55: Ohayo, function signature sensei!The updated function signature looks great! It simplifies access to the test runner's context and aligns perfectly with the new annotation.
Let's make sure the
sequencerparameter is used correctly throughout the function:This will help us confirm that the
sequencerparameter is being utilized effectively in the function body.Verification successful
Ohayo, sensei!
It appears the
sequencerparameter wasn't detected in the function body. To ensure it's being utilized correctly, let's perform a more thorough search:This script first attempts to locate any interactions with
sequencerusingast-grep. If it doesn't find any matches, it falls back to usingripgrepto perform a textual search within thetest_migrate_then_upgradefunction. This dual approach ensures that we comprehensively verify the usage of thesequencerparameter.
Ohayo, sensei!
Great news! 🎉 The
sequencerparameter is actively used within thetest_migrate_then_upgradefunction (let mut account = sequencer.account(0);). This confirms that the signature change is correctly implemented and enhances the function's functionality.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the 'sequencer' parameter in the test_migrate_then_upgrade function. # Test: Search for uses of 'sequencer' within the function. Expect: At least one usage. ast-grep --lang rust --pattern 'fn test_migrate_then_upgrade($_, $_) { $$$ sequencer.$_ $$$ }'Length of output: 102
Script:
#!/bin/bash # Description: Verify any usage of the 'sequencer' parameter within the test_migrate_then_upgrade function. ast-grep --lang rust --pattern 'fn test_migrate_then_upgrade($_, $_) { $$$ sequencer.$$ $$$ }' # If ast-grep doesn't yield results, use ripgrep as an alternative. rg 'sequencer' -A 5 bin/sozo/tests/test_migrate.rs | grep -A5 'fn test_migrate_then_upgrade'Length of output: 529
crates/sozo/ops/src/tests/utils.rs (2)
4-4: Ohayo, sensei! Nice import there!The addition of
use katana_runner::RunnerCtx;is spot on for the new test structure. It's like finding the perfect ingredient for our code ramen!
14-16: Ohayo, sensei! This test transformation is sugoi!The new
katana_runner::testattribute and updated function signature are like a well-choreographed anime fight scene - everything flows smoothly! Here's what makes it awesome:
- The attribute simplifies test setup, handling fee, accounts, and database configuration.
- Using
&RunnerCtxreduces boilerplate and improves test clarity.- The function body now leverages the
sequencercontext effectively.Great job on streamlining this test, sensei!
crates/dojo-world/src/contracts/model_test.rs (2)
6-6: Ohayo, sensei! LGTM on the new import!The addition of
use katana_runner::RunnerCtx;aligns perfectly with the changes in the test function. It's a necessary import for the new runner context.
16-16: Ohayo, sensei! The function signature update is on point!The new signature
async fn test_model(sequencer: &RunnerCtx)perfectly complements the macro change. It allows the test to receive the runner context directly, streamlining the setup process.Let's make sure the
sequencerparameter is used correctly throughout the function:Verification successful
Ohayo, sensei! The
sequencerparameter is properly utilized within thetest_modelfunction.The function effectively uses
sequencerto create the account and provider, ensuring a streamlined and consistent test setup.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the 'sequencer' parameter in the test_model function. # Test: Check if 'sequencer' is used to create the account and provider. rg --type rust -A 10 'fn test_model\(sequencer: &RunnerCtx\)' crates/dojo-world/src/contracts/model_test.rsLength of output: 582
crates/katana/runner/macro/src/entry.rs (1)
69-69: Ohayo, sensei! Consider improving error handling for db_dir parsing.The change to use
FromStr::from_stris interesting, but it might lead to unexpected panics. Here are some suggestions:
- Instead of
expect, consider usingmap_errto provide a more informative error message.- You might want to handle the error case more gracefully instead of panicking.
Here's a potential improvement:
cfg = quote_spanned! (last_stmt_start_span=> #cfg db_dir: Some(core::str::FromStr::from_str(#value) .map_err(|e| format!("Invalid db_dir path: {}", e)) .expect("Failed to parse db_dir")), );This way, if the path is invalid, you'll get a more helpful error message. However, it still panics, which might not be ideal. Consider returning a
Resultinstead of panicking, allowing the caller to handle the error.What do you think, sensei? Would you like me to suggest a non-panicking alternative?
Verification successful
Ohayo, sensei! The review comment has been verified successfully.
PathBufis no longer imported or used elsewhere incrates/katana/runner/macro/src/entry.rs, ensuring that the change is isolated to the specified line.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if PathBuf is still imported and used elsewhere in the file rg --type rust 'use std::path::PathBuf' crates/katana/runner/macro/src/entry.rs rg --type rust 'PathBuf::' crates/katana/runner/macro/src/entry.rsLength of output: 148
crates/sozo/ops/src/tests/call.rs (8)
2-2: Ohayo, sensei! LGTM on this import!The import of
RunnerCtxis necessary for the updated test function signatures. It's placed appropriately with other imports.
17-19: Ohayo, sensei! These changes look sugoi!The new attribute macro
#[katana_runner::test(db_dir = "/tmp/spawn-and-move-db")]and the updated function signature withsequencer: &RunnerCtxparameter simplify the test setup process. This change is consistent across all test functions and aligns with the PR objectives.
40-42: Ohayo again, sensei! These changes are consistent and kawaii!The modifications to
call_with_bad_namemirror those in the previous function. The new attribute macro and updated function signature maintain consistency across the test suite.
63-65: Ohayo once more, sensei! The consistency is strong with this one!The changes to
call_with_bad_entrypointmaintain the pattern established in the previous functions. The new attribute macro and updated function signature continue to enhance the test suite's consistency.
86-88: Ohayo yet again, sensei! The consistency continues to impress!The modifications to
call_with_bad_calldatafollow the same pattern as the previous functions. The new attribute macro and updated function signature maintain the cohesive structure of the test suite.
109-111: Ohayo once more, sensei! The consistency is truly sugoi!The changes to
call_with_contract_namecontinue the established pattern. The new attribute macro and updated function signature maintain the uniformity across the test suite, which is most excellent.
131-135: Ohayo for the last time, sensei! These changes are the cherry blossom on our code review tree!The modifications to
call_with_contract_addressmaintain the consistency we've seen throughout the file. The new attribute macro and updated function signature are present as expected. Additionally, the function body has been appropriately updated to use the newsequencerparameter, which is a nice touch!
Line range hint
1-153: Ohayo, sensei! Let's wrap up this sugoi review!Overall, the changes in this file are consistent and well-implemented. The new Katana runner proc macro has been successfully applied to all test functions, simplifying the test setup process and improving consistency across the test suite. These modifications align perfectly with the PR objectives.
Key improvements:
- Consistent use of the
#[katana_runner::test(db_dir = "/tmp/spawn-and-move-db")]attribute macro.- Updated function signatures to include the
sequencer: &RunnerCtxparameter.- Appropriate updates to function bodies where necessary to utilize the new
sequencerparameter.These changes should make the tests more maintainable and easier to set up. Great work, sensei!
crates/torii/grpc/src/server/tests/entities_test.rs (4)
11-11: Ohayo, sensei! New import looks good!The addition of
use katana_runner::RunnerCtx;aligns well with the changes in the test function. It's a necessary import for the new test setup using the Katana runner.
33-34: Ohayo again, sensei! These changes are sugoi!The new
katana_runner::testattribute macro and the updated function signature bring several benefits:
- Simplified test setup by specifying
accounts = 10anddb_dirdirectly in the macro.- Direct access to the
RunnerCtxthrough thesequencerparameter, eliminating the need for manualKatanaRunnerinstantiation.These changes align perfectly with the PR objective of using the new Katana runner proc macro. Great job on improving the test structure!
Line range hint
1-146: Ohayo one last time, sensei! Let's wrap this up!Overall, the changes to
entities_test.rsare very positive:
- The new Katana runner proc macro has been successfully implemented.
- Test setup has been simplified without altering the core test logic.
- Code readability and maintainability have improved.
These modifications align perfectly with the PR objective of using the new Katana runner proc macro. Great job on enhancing the test structure!
Just make sure to verify the
RunnerCtxcapabilities as suggested earlier. Otherwise, this looks ready to go!
34-34: Sugoi simplification, sensei!The removal of the
KatanaRunnerConfigandKatanaRunnerinstantiation code significantly simplifies the test setup. This change:
- Reduces boilerplate code
- Minimizes potential points of failure in the test setup
- Improves readability and maintainability of the test
However, it's crucial to ensure that all necessary functionality from the previous setup is still accessible through the new
RunnerCtx.Could you please verify that the
RunnerCtxprovides all the required capabilities for this test? Here's a script to help with the verification:Verification successful
Ohayo, sensei!
The
RunnerCtxis effectively utilized in the test, and no instances ofKatanaRunnerremain in the test files. The simplification of the test setup has been successfully verified.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the capabilities of RunnerCtx # Test: Check for RunnerCtx methods used in the test rg --type rust -A 5 'sequencer\.' crates/torii/grpc/src/server/tests/entities_test.rs # Test: Look for any KatanaRunner usages that might have been missed rg --type rust 'KatanaRunner' crates/torii/grpc/src/server/tests/Length of output: 615
crates/sozo/ops/src/tests/auth.rs (1)
7-7: Ohayo, sensei! LGTM on this import!The addition of
use katana_runner::RunnerCtx;is spot-on for the new Katana runner proc macro. It's like adding the perfect seasoning to our code ramen!crates/sozo/ops/src/tests/model.rs (3)
7-7: Ohayo, sensei! Nice import there!The addition of
RunnerCtximport looks good. It aligns well with the new test structure we're implementing.
18-20: Ohayo, test-writing sensei! This change is sugoi!The new
#[katana_runner::test]attribute and updated function signature are excellent improvements:
- It simplifies the test setup by specifying accounts and database directory directly in the attribute.
- The
RunnerCtxparameter allows for a more streamlined test execution.These changes should make the test more maintainable and easier to understand. Great job!
191-192: Ohayo, async-to-sync sensei! Nice refactoring!The change from an async test to a sync test looks good. It simplifies the test structure and suggests that asynchronous operations are no longer needed for this particular test.
However, to ensure everything is working as expected:
Could you please run the following command to check if there are any remaining async operations in this function?
This will help us confirm that the transition to a synchronous test is complete and correct.
Verification successful
Ohayo, sensei! No async operations detected.
The transition from an asynchronous to a synchronous test in
test_check_tag_or_read_defaulthas been successfully verified. There are no remaining async operations, ensuring the test operates correctly as intended.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining async operations in test_check_tag_or_read_default rg --type rust -A 10 'fn test_check_tag_or_read_default' crates/sozo/ops/src/tests/model.rsLength of output: 446
crates/torii/core/src/sql_test.rs (3)
11-11: Ohayo, sensei! New import looks good!The addition of
use katana_runner::RunnerCtx;is spot on for the new test setup. It's properly placed with the other imports, keeping our code organized and tidy.
199-200: Ohayo again, sensei! Consistency is key!I'm loving the consistency here. The changes to
test_load_from_remote_delmirror those in the previous function, maintaining a uniform approach to our tests. This consistency will make our codebase easier to understand and maintain.Keep up the great work in maintaining this level of consistency across our tests!
Line range hint
1-399: Ohayo, sensei! Let's wrap this up!Overall, the changes to this file are well-executed and consistent. The introduction of the
katana_runnertesting framework across all test functions simplifies the test setup and promotes uniformity. Here's a summary of the improvements:
- Consistent use of
#[katana_runner::test]annotation with specific parameters.- Removal of manual runner setup code, reducing boilerplate.
- Addition of
sequencer: &RunnerCtxparameter to all test functions, allowing for a more streamlined test environment.These changes should make our tests more maintainable and easier to understand. Great job on this refactoring, sensei!
crates/sozo/ops/src/tests/migration.rs (4)
23-23: Ohayo, sensei! LGTM: Katana runner integrationThe addition of
RunnerCtximport and the use ofkatana_runner::testattribute is a great improvement. This change simplifies test setup and provides a more consistent testing environment across different test cases.Also applies to: 37-38
76-77: Ohayo, sensei! LGTM, but I have a questionThe changes look good, especially the addition of the
block_timeparameter in thekatana_runner::testattribute. This should allow for more precise control over the testing environment.Could you clarify the purpose of setting
block_time = 1000? Is this milliseconds, seconds, or some other unit? It might be helpful to add a comment explaining the reasoning behind this specific value.Also applies to: 86-86
91-92: Ohayo, sensei! LGTM, but let's double-check somethingThe change from an async test to a synchronous one looks good. It simplifies the test and suggests that all operations are now synchronous.
However, could you please verify that all previously asynchronous operations in this test have been properly handled? It's important to ensure we haven't accidentally removed any necessary async behavior.
251-252: Ohayo, sensei! LGTM, but let's address the TODOThe changes to use
katana_runner::testandRunnerCtxare good and consistent with the new testing approach.I noticed there are commented-out sections related to checking artifact fields and metadata. Is there an existing issue tracking this TODO? If not, it might be helpful to create one to ensure it's not forgotten.
Also, could you provide some context on why these sections are commented out? Are they temporarily disabled due to the mentioned issue #2137?
Also applies to: 261-261
crates/dojo-world/src/manifest/manifest_test.rs (1)
8-8: Ohayo, sensei! The import change looks good!The update to import
RunnerCtxfromkatana_runneraligns well with the new macro-based test approach. It simplifies our imports and sets the stage for the updated test function.crates/katana/runner/macro/src/config.rs (6)
15-20: Upgrade field types tosyn::Exprfor enhanced flexibilityOhayo sensei, changing the configuration fields to
Option<syn::Expr>allows for more versatile and complex expressions in the macro attributes, enhancing flexibility.
52-52: Verify expression handling inset_db_dirmethodOhayo sensei, the
set_db_dirmethod now accepts asyn::Expr. Please ensure that this method correctly processes the expression to extract the desired database directory path, handling any potential parsing errors.
61-61: Verify expression handling inset_feemethodOhayo sensei, with
set_feenow acceptingsyn::Expr, ensure that the method evaluates the expression to obtain a boolean value for the fee configuration, and includes appropriate error handling for invalid expressions.
72-72: Verify expression handling inset_validationmethodOhayo sensei, since
validationis now asyn::Expr, please verify thatset_validationaccurately evaluates the expression to determine the validation setting, and gracefully handles any parsing issues.
85-85: Verify expression handling inset_block_timemethodOhayo sensei, the
set_block_timemethod now receives asyn::Expr. Ensure that it correctly evaluates the expression to set the block time, with appropriate error handling for invalid or unexpected expressions.
98-98: Verify expression handling inset_accountsmethodOhayo sensei, as
accountsis now asyn::Expr, please confirm thatset_accountsproperly processes the expression to configure accounts, and handles any potential errors during parsing.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2465 +/- ##
==========================================
+ Coverage 68.42% 68.44% +0.01%
==========================================
Files 368 368
Lines 48197 48176 -21
==========================================
- Hits 32981 32975 -6
+ Misses 15216 15201 -15 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
New Features
katana_runner::testattributes, enhancing test efficiency and readability across various test files.Bug Fixes
Refactor
KatanaRunnertoRunnerCtxin multiple test functions, simplifying the instantiation process and enhancing code maintainability.Tests
&RunnerCtxparameter, aligning with the new structure for better context management.