Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: introduce testing configs #1119

Merged
merged 4 commits into from
Jul 16, 2024
Merged

feat: introduce testing configs #1119

merged 4 commits into from
Jul 16, 2024

Conversation

edisontim
Copy link
Collaborator

@edisontim edisontim commented Jul 15, 2024

PR Type

Enhancement, Tests


Description

  • Renamed set_explore_config to set_exploration_config in both configuration setup and provider.
  • Updated import paths for spawn_eternum and deploy_system across multiple test files.
  • Refactored setup functions in various test files to use new utility functions.
  • Split testing utilities into multiple modules for better organization.
  • Added new modules for configuration-related, constants, general, system-related, and world-related testing utilities.

Changes walkthrough 📝

Relevant files
Enhancement
8 files
index.ts
Rename set_explore_config to set_exploration_config in configuration
setup

sdk/packages/eternum/src/config/index.ts

  • Renamed set_explore_config to set_exploration_config.
+1/-1     
index.ts
Rename `set_explore_config` to `set_exploration_config` in provider

sdk/packages/eternum/src/provider/index.ts

  • Renamed set_explore_config to set_exploration_config.
+1/-1     
testing.cairo
Split testing utilities into multiple modules                       

contracts/src/utils/testing.cairo

  • Split testing utilities into multiple modules.
+5/-213 
config.cairo
Add configuration-related testing utilities module             

contracts/src/utils/testing/config.cairo

  • Added new module for configuration-related testing utilities.
+119/-0 
constants.cairo
Add constants module for testing                                                 

contracts/src/utils/testing/constants.cairo

  • Added new module for constants used in testing.
+43/-0   
general.cairo
Add general testing utilities module                                         

contracts/src/utils/testing/general.cairo

  • Added new module for general testing utilities.
+103/-0 
systems.cairo
Add system-related testing utilities module                           

contracts/src/utils/testing/systems.cairo

  • Added new module for system-related testing utilities.
+65/-0   
world.cairo
Add world-related testing utilities module                             

contracts/src/utils/testing/world.cairo

  • Added new module for world-related testing utilities.
+60/-0   
Tests
21 files
combat.cairo
Update import path for `spawn_eternum` in combat tests     

contracts/src/models/combat.cairo

  • Updated import path for spawn_eternum.
+1/-1     
owner.cairo
Update import path for `spawn_eternum` in owner tests       

contracts/src/models/owner.cairo

  • Updated import path for spawn_eternum.
+1/-1     
resources.cairo
Update import paths for spawn_eternum and deploy_system in resources
tests

contracts/src/models/resources.cairo

  • Updated import paths for spawn_eternum and deploy_system.
+2/-2     
road.cairo
Update import path for `spawn_eternum` in road tests         

contracts/src/models/road.cairo

  • Updated import path for spawn_eternum.
+1/-1     
bank.cairo
Update import paths for spawn_eternum and deploy_system in bank tests

contracts/src/systems/bank/tests/bank.cairo

  • Updated import paths for spawn_eternum and deploy_system.
+1/-1     
liquidity.cairo
Update import paths for spawn_eternum and deploy_system in liquidity
tests

contracts/src/systems/bank/tests/liquidity.cairo

  • Updated import paths for spawn_eternum and deploy_system.
+1/-1     
swap.cairo
Update import paths for spawn_eternum and deploy_system in swap tests

contracts/src/systems/bank/tests/swap.cairo

  • Updated import paths for spawn_eternum and deploy_system.
+1/-1     
hyperstructure_config_tests.cairo
Update import paths for spawn_eternum and deploy_system in
hyperstructure config tests

contracts/src/systems/config/tests/hyperstructure_config_tests.cairo

  • Updated import paths for spawn_eternum and deploy_system.
+1/-1     
tests.cairo
Update import paths for spawn_eternum and deploy_system in guild tests

contracts/src/systems/guild/tests.cairo

  • Updated import paths for spawn_eternum and deploy_system.
+1/-1     
tests.cairo
Update import paths for various utility functions in hyperstructure
tests

contracts/src/systems/hyperstructure/tests.cairo

  • Updated import paths for spawn_eternum, deploy_system, and other
    utility functions.
  • +5/-2     
    internal_leveling_tests.cairo
    Update import paths for spawn_eternum and deploy_system in internal
    leveling tests

    contracts/src/systems/leveling/tests/internal_leveling_tests.cairo

    • Updated import paths for spawn_eternum and deploy_system.
    +1/-1     
    realm_leveling_tests.cairo
    Update import paths for various utility functions in realm leveling
    tests

    contracts/src/systems/leveling/tests/realm_leveling_tests.cairo

  • Updated import paths for spawn_eternum, deploy_system, and other
    utility functions.
  • +2/-1     
    tests.cairo
    Refactor setup function and update import paths in map tests

    contracts/src/systems/map/tests.cairo

  • Refactored setup function to use new utility functions.
  • Updated import paths for various utility functions.
  • +66/-134
    tests.cairo
    Update import paths for various utility functions in realm tests

    contracts/src/systems/realm/tests.cairo

  • Updated import paths for spawn_eternum, deploy_system, and other
    utility functions.
  • +7/-3     
    resource_approval_system_tests.cairo
    Update import paths for spawn_eternum and deploy_system in resource
    approval system tests

    contracts/src/systems/resources/tests/resource_approval_system_tests.cairo

    • Updated import paths for spawn_eternum and deploy_system.
    +1/-1     
    resource_transfer_system_tests.cairo
    Update import paths for spawn_eternum and deploy_system in resource
    transfer system tests

    contracts/src/systems/resources/tests/resource_transfer_system_tests.cairo

    • Updated import paths for spawn_eternum and deploy_system.
    +1/-1     
    accept_order.cairo
    Update import paths for various utility functions in trade system
    accept order tests

    contracts/src/systems/trade/tests/trade_systems_tests/accept_order.cairo

  • Updated import paths for spawn_eternum, deploy_system, and other
    utility functions.
  • +3/-1     
    cancel_order.cairo
    Update import paths for various utility functions in trade system
    cancel order tests

    contracts/src/systems/trade/tests/trade_systems_tests/cancel_order.cairo

  • Updated import paths for spawn_eternum, deploy_system, and other
    utility functions.
  • +3/-1     
    create_order.cairo
    Refactor setup function and update import paths in trade system create
    order tests

    contracts/src/systems/trade/tests/trade_systems_tests/create_order.cairo

  • Refactored setup function to use new utility functions.
  • Updated import paths for various utility functions.
  • +42/-43 
    road_systems_tests.cairo
    Update import paths for spawn_eternum and deploy_system in road
    systems tests

    contracts/src/systems/transport/tests/road_systems_tests.cairo

    • Updated import paths for spawn_eternum and deploy_system.
    +1/-1     
    travel_systems_tests.cairo
    Update import paths for spawn_eternum and deploy_system in travel
    systems tests

    contracts/src/systems/transport/tests/travel_systems_tests.cairo

    • Updated import paths for spawn_eternum and deploy_system.
    +1/-1     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    vercel bot commented Jul 15, 2024

    The latest updates on your projects. Learn more about Vercel for Git ↗︎

    Name Status Preview Comments Updated (UTC)
    eternum ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 16, 2024 10:44am

    Copy link
    Contributor

    @mentatbot mentatbot bot left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    This pull request introduces a significant refactoring of the testing utilities in the Eternum project. It separates the testing utilities into different modules (config, constants, general, systems, and world), which improves organization and maintainability. The changes also include updates to the map tests to use these new utilities, and some minor adjustments to the SDK to maintain consistency with the contract function names. Overall, this refactoring should make it easier to write and maintain tests in the future. The changes look good, but it might be helpful to add more comments explaining the rationale behind some of the constant value changes and the new utility functions.

    Thanks for using MentatBot. Give comments a 👍 or 👎 to help me improve!

    contracts/src/systems/map/tests.cairo Show resolved Hide resolved
    contracts/src/systems/map/tests.cairo Show resolved Hide resolved
    contracts/src/systems/map/tests.cairo Show resolved Hide resolved
    contracts/src/systems/map/tests.cairo Show resolved Hide resolved
    contracts/src/systems/map/tests.cairo Show resolved Hide resolved
    contracts/src/utils/testing/config.cairo Outdated Show resolved Hide resolved
    contracts/src/utils/testing/constants.cairo Show resolved Hide resolved
    sdk/packages/eternum/src/config/index.ts Show resolved Hide resolved
    sdk/packages/eternum/src/provider/index.ts Show resolved Hide resolved
    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Function Renaming
    The function set_explore_config was renamed to set_exploration_config. Ensure all references to this function are updated across all files to maintain consistency and avoid runtime errors.

    Function Renaming
    The function set_explore_config was renamed to set_exploration_config in the provider. Ensure that this change is reflected in all calls to this function to prevent any issues with undefined function calls.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Add error handling to asynchronous function

    Since set_exploration_config is an asynchronous function, consider adding error
    handling with try-catch to manage potential rejections or errors during the
    transaction execution.

    sdk/packages/eternum/src/provider/index.ts [680-683]

     public async set_exploration_config(props: SystemProps.SetExplorationConfigProps) {
         const { wheat_burn_amount, fish_burn_amount, reward_amount, shards_mines_fail_probability, signer } = props;
    -    return await this.executeAndCheckTransaction(signer, {
    +    try {
    +        return await this.executeAndCheckTransaction(signer, {
    +    } catch (error) {
    +        console.error('Failed to set exploration config:', error);
    +        throw error;
    +    }
     
    Suggestion importance[1-10]: 9

    Why: Adding error handling to the asynchronous function is a best practice that enhances the robustness and reliability of the code by managing potential errors during execution.

    9
    Use named constants instead of magic numbers to enhance code readability

    Replace magic numbers with named constants to improve code readability and
    maintainability.

    contracts/src/systems/trade/tests/trade_systems_tests/create_order.cairo [56]

    -let taker_id = 12_u128;
    +const DEFAULT_TAKER_ID: u128 = 12;
    +let taker_id = DEFAULT_TAKER_ID;
     
    Suggestion importance[1-10]: 5

    Why: Using named constants instead of magic numbers is a best practice that enhances readability, but the impact on the overall code quality is relatively minor.

    5
    Maintainability
    Refactor the setup function to improve code organization and readability

    Refactor the setup function to separate concerns, improving readability and
    maintainability. Consider breaking down the function into smaller, more focused
    functions like deployContracts, initializeResources, and createArmy.

    contracts/src/systems/map/tests.cairo [149-195]

    -fn setup() -> (IWorldDispatcher, u128, u128, IMapSystemsDispatcher, ICombatContractDispatcher) {
    -    let world = spawn_eternum();
    -    starknet::testing::set_block_timestamp(TIMESTAMP);
    +fn deployContracts(world: IWorldDispatcher) -> (IRealmSystemsDispatcher, ICombatContractDispatcher, IMapSystemsDispatcher) {
         let config_systems_address = deploy_system(world, config_systems::TEST_CLASS_HASH);
         set_combat_config(config_systems_address);
         set_capacity_config(config_systems_address);
         set_stamina_config(config_systems_address);
         set_speed_config(config_systems_address);
         set_mercenaries_config(config_systems_address);
         set_tick_config(config_systems_address);
         set_exploration_config(config_systems_address);
         set_weight_config(config_systems_address);
    -    starknet::testing::set_contract_address(contract_address_const::<'realm_owner'>());
         let realm_systems_dispatcher = deploy_realm_systems(world);
         let combat_systems_dispatcher = deploy_combat_systems(world);
         let map_systems_dispatcher = deploy_map_systems(world);
    -    let realm_position = get_default_realm_pos();
    -    let realm_entity_id = spawn_realm(world, realm_systems_dispatcher, realm_position);
    +    (realm_systems_dispatcher, combat_systems_dispatcher, map_systems_dispatcher)
    +}
    +fn initializeResources(world: IWorldDispatcher, realm_entity_id: u128) {
         deploy_dev_resource_systems(world)
             .mint(
                 realm_entity_id,
                 array![
                     (ResourceTypes::WHEAT, INITIAL_WHEAT_BALANCE),
                     (ResourceTypes::FISH, INITIAL_FISH_BALANCE),
                     (ResourceTypes::KNIGHT, INITIAL_KNIGHT_BALANCE)
                 ]
                     .span()
             );
    +}
    +fn createArmy(world: IWorldDispatcher, combat_systems_dispatcher: ICombatContractDispatcher, realm_entity_id: u128) -> u128 {
         let troops = Troops {
             knight_count: INITIAL_KNIGHT_BALANCE.try_into().unwrap(),
             paladin_count: 0,
             crossbowman_count: 0
         };
    -    let realm_army_unit_id: u128 = create_army_with_troops(
    -        world, combat_systems_dispatcher, realm_entity_id, troops, false
    -    );
    +    create_army_with_troops(world, combat_systems_dispatcher, realm_entity_id, troops, false)
    +}
    +fn setup() -> (IWorldDispatcher, u128, u128, IMapSystemsDispatcher, ICombatContractDispatcher) {
    +    let world = spawn_eternum();
    +    starknet::testing::set_block_timestamp(TIMESTAMP);
    +    starknet::testing::set_contract_address(contract_address_const::<'realm_owner'>());
    +    let (realm_systems_dispatcher, combat_systems_dispatcher, map_systems_dispatcher) = deployContracts(world);
    +    let realm_position = get_default_realm_pos();
    +    let realm_entity_id = spawn_realm(world, realm_systems_dispatcher, realm_position);
    +    initializeResources(world, realm_entity_id);
    +    let realm_army_unit_id = createArmy(world, combat_systems_dispatcher, realm_entity_id);
         (world, realm_entity_id, realm_army_unit_id, map_systems_dispatcher, combat_systems_dispatcher)
    -}
     
    Suggestion importance[1-10]: 8

    Why: The refactoring suggestion improves code maintainability and readability by separating concerns into smaller, more focused functions, making the code easier to understand and manage.

    8
    Replace repeated code blocks with a loop to improve code maintainability

    Replace the repeated calls to dev_resource_systems.mint with a loop to reduce
    redundancy and improve maintainability.

    contracts/src/systems/trade/tests/trade_systems_tests/create_order.cairo [58-77]

    -dev_resource_systems
    -    .mint(
    -        maker_id,
    -        array![
    -            (ResourceTypes::STONE, 100),
    -            (ResourceTypes::GOLD, 100),
    -            (ResourceTypes::DONKEY, 20_000)
    -        ]
    -            .span()
    -    );
    -dev_resource_systems
    -    .mint(
    -        taker_id,
    -        array![
    -            (ResourceTypes::STONE, 500),
    -            (ResourceTypes::GOLD, 500),
    -            (ResourceTypes::DONKEY, 20_000)
    -        ]
    -            .span()
    -    );
    +for (entity_id, stone, gold, donkey) in [
    +    (maker_id, 100, 100, 20_000),
    +    (taker_id, 500, 500, 20_000)
    +] {
    +    dev_resource_systems
    +        .mint(
    +            entity_id,
    +            array![
    +                (ResourceTypes::STONE, stone),
    +                (ResourceTypes::GOLD, gold),
    +                (ResourceTypes::DONKEY, donkey)
    +            ]
    +                .span()
    +        );
    +}
     
    Suggestion importance[1-10]: 8

    Why: The suggestion effectively reduces redundancy and improves maintainability by using a loop, making the code cleaner and easier to manage.

    8
    Improve variable naming for clarity

    Consider using a more descriptive variable name for txExplore to reflect its
    purpose, such as explorationTransaction.

    sdk/packages/eternum/src/config/index.ts [196]

    -const txExplore = await provider.set_exploration_config({
    +const explorationTransaction = await provider.set_exploration_config({
     
    Suggestion importance[1-10]: 7

    Why: The suggestion improves code readability by using a more descriptive variable name, which helps in understanding the purpose of the variable.

    7
    Refactor repeated balance checks into a helper function for clarity and reusability

    Use a helper function to reduce redundancy and improve code clarity when getting
    resource balances and asserting them.

    contracts/src/systems/trade/tests/trade_systems_tests/create_order.cairo [108-113]

    -let maker_stone_balance = ResourceImpl::get(world, (maker_id, ResourceTypes::STONE)).balance;
    -assert(maker_stone_balance == 0, 'm stone balance should be 0');
    -let maker_gold_balance = ResourceImpl::get(world, (maker_id, ResourceTypes::GOLD)).balance;
    -assert(maker_gold_balance == 0, 'm gold balance should be 0');
    +def check_balance(entity_id, resource_type, expected_balance, message):
    +    balance = ResourceImpl::get(world, (entity_id, resource_type)).balance;
    +    assert balance == expected_balance, message;
     
    +check_balance(maker_id, ResourceTypes::STONE, 0, 'm stone balance should be 0');
    +check_balance(maker_id, ResourceTypes::GOLD, 0, 'm gold balance should be 0');
    +
    Suggestion importance[1-10]: 7

    Why: The helper function improves code clarity and reusability, making the balance checks more concise and easier to read.

    7
    Use named constants instead of magic numbers

    Replace the magic numbers in the create method call with named constants to improve
    code readability and maintainability.

    contracts/src/utils/testing/general.cairo [19-30]

    +DEFAULT_REALM_ID = 1
    +RESOURCE_TYPES_PACKED = 0x20309
    +RESOURCE_TYPES_COUNT = 3
    +DEFAULT_CITY_COUNT = 5
    +DEFAULT_HARBOR_COUNT = 5
    +DEFAULT_RIVER_COUNT = 5
    +DEFAULT_REGION_COUNT = 5
    +DEFAULT_WONDER_COUNT = 1
    +DEFAULT_ORDER = 1
    +
     realm_entity_id = realm_systems_dispatcher.create(
    -    1, 0x20309, 3, 5, 5, 5, 5, 1, 1, position
    +    DEFAULT_REALM_ID, RESOURCE_TYPES_PACKED, RESOURCE_TYPES_COUNT,
    +    DEFAULT_CITY_COUNT, DEFAULT_HARBOR_COUNT, DEFAULT_RIVER_COUNT,
    +    DEFAULT_REGION_COUNT, DEFAULT_WONDER_COUNT, DEFAULT_ORDER, position
     )
     
    Suggestion importance[1-10]: 7

    Why: Replacing magic numbers with named constants improves code readability and maintainability, making the code easier to understand and modify in the future.

    7
    Possible issue
    Add error handling for realm creation failures

    Consider adding error handling for the create method in the spawn_realm function to
    handle potential failures in realm creation.

    contracts/src/utils/testing/general.cairo [19-30]

    -realm_entity_id = realm_systems_dispatcher.create(
    -    1, 0x20309, 3, 5, 5, 5, 5, 1, 1, position
    -)
    +try:
    +    realm_entity_id = realm_systems_dispatcher.create(
    +        1, 0x20309, 3, 5, 5, 5, 5, 1, 1, position
    +    )
    +except Exception as e:
    +    raise Exception(f"Failed to create realm: {e}")
     
    Suggestion importance[1-10]: 8

    Why: Adding error handling for the create method in the spawn_realm function is crucial for robustness, as it ensures that potential failures in realm creation are properly managed and reported.

    8
    Add checks to prevent integer overflow when setting large numeric values

    Consider checking for potential integer overflow when setting large constants such
    as 1_000_000 for set_capacity_config.

    contracts/src/systems/trade/tests/trade_systems_tests/create_order.cairo [51]

    +let max_capacity = 1_000_000;
    +assert max_capacity > 0, "Capacity must be positive";
     ICapacityConfigDispatcher { contract_address: config_systems_address }
    -    .set_capacity_config(DONKEY_ENTITY_TYPE, 1_000_000);
    +    .set_capacity_config(DONKEY_ENTITY_TYPE, max_capacity);
     
    Suggestion importance[1-10]: 6

    Why: Adding a check for integer overflow is a good practice, but the risk of overflow with the given value is minimal. The suggestion is still valuable for robustness.

    6
    Enhancement
    Use a for loop for clarity and performance in position generation

    Refactor the generate_realm_positions function to use a for loop instead of a while
    loop for better clarity and performance.

    contracts/src/utils/testing/general.cairo [70-81]

    -count = 0
    -while (count < MAX_REALMS_PER_ADDRESS + 1) {
    +for count in range(MAX_REALMS_PER_ADDRESS + 1):
         positions.append(Position {
    -        x: start_pos.x + count.into(),
    -        y: start_pos.y + count.into(),
    -        entity_id: start_pos.entity_id + count.into()
    +        x: start_pos.x + count,
    +        y: start_pos.y + count,
    +        entity_id: start_pos.entity_id + count
         })
    -    count += 1
    -}
     
    Suggestion importance[1-10]: 6

    Why: Refactoring the generate_realm_positions function to use a for loop enhances code clarity and can potentially improve performance, making the logic more straightforward.

    6
    Use a dictionary for resource management in realm creation

    Use a dictionary to map resource types to their counts in the create method to
    enhance code readability and scalability.

    contracts/src/utils/testing/general.cairo [19-30]

    +resources = {
    +    'stone': 5,
    +    'coal': 5,
    +    'gold': 5
    +}
     realm_entity_id = realm_systems_dispatcher.create(
    -    1, 0x20309, 3, 5, 5, 5, 5, 1, 1, position
    +    1, resources, position
     )
     
    Suggestion importance[1-10]: 4

    Why: While using a dictionary for resource management can enhance readability and scalability, the suggested code does not align well with the existing method signature and may require additional changes to be functional.

    4

    @bob0005 bob0005 merged commit a8c51c6 into main Jul 16, 2024
    19 checks passed
    @bob0005 bob0005 deleted the feat/testing-utils branch July 16, 2024 10:55
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants