[Feat] :: Writen Tests for PlayerTrait #152
Conversation
WalkthroughAdds a new test module for Player, exposes two u256 helper functions as public in Player model, and registers the test module in the library. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Tester as sozo test
participant TestMod as test::model_test_player
participant Player as models::player::PlayerTrait impl
participant Helpers as models::player::get_high/get_low
Tester->>TestMod: Run test cases
TestMod->>Player: init/reinit, gain_xp(), get_xp(), level(), equip(), take_damage(), armor()
alt Requires bit extraction
TestMod->>Helpers: get_high(u256), get_low(u256)
Helpers-->>TestMod: u128 parts
end
Player-->>TestMod: Updated state/assertable values
TestMod-->>Tester: Assertions pass/fail
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
src/models/player.cairo (1)
354-360: Publicizing helpers only for tests is fine; consider gating to avoid API creep.Tests need
get_high/get_low, but widening the API is avoidable:
- Option A (preferred): gate the helpers for test builds to keep production surface minimal.
- pub fn get_high(val: u256) -> u128 { + #[cfg(test)] + pub fn get_high(val: u256) -> u128 { val.high } - pub fn get_low(val: u256) -> u128 { + #[cfg(test)] + pub fn get_low(val: u256) -> u128 { val.low }If these are intentionally part of the public API, add a brief comment stating they are stable exports.
src/lib.cairo (1)
89-89: Gate the test module with cfg(test).Prevents shipping an empty test namespace in non-test builds and keeps intent clear.
pub mod test { @@ - pub mod model_test_player; + #[cfg(test)] + pub mod model_test_player; }src/test/model_test_player.cairo (9)
23-26: Remove unused helper.
mock_erc1155_addressis defined but never used.- fn mock_erc1155_address() -> ContractAddress { - contract_address_const::<0x5678>() - }
140-147: Use runtime slot capacity instead of a hardcoded constant.Keeps the test resilient to future changes.
- while i < DEFAULT_MAX_EQUIPPABLE_SLOT { + let slot_cap = player.max_equip_slot; + while i < slot_cap { player.equipped.append(i.into()); i += 1; };
183-187: Compute expected HP from the initialized state, not a duplicated constant.- assert(is_alive, 'PLAYER_SHOULD_BE_ALIVE'); - assert(player.hp == DEFAULT_HP - 100, 'HP_SHOULD_BE_REDUCED'); + let original_hp = player.hp + 0; // pre-damage baseline + assert(is_alive, 'PLAYER_SHOULD_BE_ALIVE'); + assert(player.hp == original_hp - 100, 'HP_SHOULD_BE_REDUCED');
229-231: Minor: Assert-by-sum is good; prefer deriving expected from flags for readability.Optional: compute
expected = (10 /*upper*/ + 5 /*lower*/ + 3 /*head*/)to self-document the reasoning.
282-286: Avoid a 256-bit literal; use structuredu256init for Cairo portability.Large hex literals may depend on compiler behavior. This form is explicit and clearer.
- let test_val: u256 = 0x123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0; + let test_val: u256 = u256 { + low: 0x89ABCDEF01234567_u128, + high: 0x123456789ABCDEF0_u128, + };
300-307: Use baseline HP instead of a duplicated constant.- let is_alive = player.receive_damage(100); + let base_hp = player.hp; + let is_alive = player.receive_damage(100); assert(is_alive, 'SHOULD_SURVIVE_DAMAGE'); - assert(player.hp < DEFAULT_HP, 'HP_SHOULD_BE_REDUCED'); + assert(player.hp < base_hp, 'HP_SHOULD_BE_REDUCED');
323-325: Derive expected HP from the pre-hit baseline.- let expected_hp = DEFAULT_HP - 37 - 187; + let base_hp = player.max_hp; + let expected_hp = base_hp - 37 - 187; assert(player.hp == expected_hp, 'HP_CAL_SHOULD_BE_CORRECT');
127-149: DRY up slot-filling loops with a small helper.Reduces repetition and clarifies intent.
// Place inside the tests module fn fill_equipped_to_capacity(ref player: Player) { let cap = player.max_equip_slot; let mut i: u32 = 0; while i < cap { player.equipped.append(i.into()); i += 1; }; }Then call
fill_equipped_to_capacity(player);in both tests.Also applies to: 169-176
239-260: Optional: add tests for uncovered trait methods.Consider including:
has_vehicle_equipped(setbody.backoroff_bodywith a Vehicle-typed id).is_equippable/is_equipped(assert true/false paths per type_id).
If helpful, I can draft minimal fixtures for vehicle and type ids.Also applies to: 310-325
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lib.cairo(1 hunks)src/models/player.cairo(1 hunks)src/test/model_test_player.cairo(1 hunks)
🔇 Additional comments (4)
src/test/model_test_player.cairo (4)
239-244: Good coverage on armor interaction.The assertions correctly validate fixed reductions and zero-damage behavior.
Also applies to: 255-260
262-279: Nice sanity checks for simple pass-throughs.
get_multiplierandis_availablebaselines are covered.
291-307: End-to-end progression test reads well.Covers level-up, equip, and damage flow succinctly.
339-349: Duplicate-equip panics as expected TheBody::equip_itemcall assertsself.can_equip(item_id)withErrors::CANNOT_EQUIP, so the test’s#[should_panic(expected: ('CANNOT EQUIP',))]will fire correctly.
| const DEFAULT_HP: u256 = 500; | ||
| pub const DEFAULT_MAX_EQUIPPABLE_SLOT: u32 = 10; | ||
| const WAIST_MAX_SLOTS: u32 = 8; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid duplicating production constants in tests (drift risk).
DEFAULT_HP and DEFAULT_MAX_EQUIPPABLE_SLOT mirror production values. If those change, tests silently become wrong. Capture runtime values from the initialized player instead.
Example adjustments:
- Use
let base_hp = player.hp;orplayer.max_hpinstead ofDEFAULT_HP. - Use
let slot_cap = player.max_equip_slot;instead ofDEFAULT_MAX_EQUIPPABLE_SLOT.
See concrete diffs below in relevant tests.
🤖 Prompt for AI Agents
In src/test/model_test_player.cairo around lines 15 to 18, the test file defines
DEFAULT_HP and DEFAULT_MAX_EQUIPPABLE_SLOT which duplicate production constants;
update tests to read these values from the initialized player instance at
runtime instead of hardcoding them: remove or stop using DEFAULT_HP and
DEFAULT_MAX_EQUIPPABLE_SLOT and replace usages with fetching player.hp (or
player.max_hp) and player.max_equip_slot (or the corresponding accessor) after
creating/initializing the player in each test so the tests always reflect the
actual production values.
2 participants
🚀 [Feat] Add Comprehensive Tests for PlayerTrait
🛠️ Issue Reference
equipandis_equippable#57📚 Description
This PR introduces unit and edge case tests for all functions in the
PlayerTrait, excluding the newly introduced initialization function for players as discussed.The focus is on ensuring correctness, stability, and preventing regressions across core player-related logic.
✅ Changes Applied
PlayerTraitequipandis_equippable#57sozo build sozo test -f player::tests🔍 Evidence
Summary by CodeRabbit