Added Missing Comprehensive Testing Suite and Test Coverage #161
Conversation
WalkthroughAdds multiple comprehensive test modules and a test runner under src/test, introducing end-to-end, integration, performance, and error-focused test suites that deploy dispatchers, use cheat utilities and event spies, and exercise lifecycle, validation, concurrency, boundary, and rate-limit scenarios. No production API changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Test Runner
participant T as Test Case
participant Sess as SessionActions
participant Play as PlayerActions
participant Gear as GearActions
Note over Dev,T: High-level integration flow
Dev->>T: invoke integration test
T->>Sess: create_session(duration, max_tx)
Sess-->>T: session_id
T->>Play: create_player(addr)
Play-->>T: player_id
T->>Gear: admin_spawn_item(player_id, spec)
Gear-->>T: item_id
T->>Sess: validate_session(session_id, player_addr)
Sess-->>T: ok
T->>Gear: equip(player_id, item_id)
Gear-->>T: equipped
T->>Play: deal_damage(player_id, targets[], weapon?)
Play-->>T: results / events
alt Batch operations
T->>Play: batch_deal_damage(batches[][])
Play-->>T: batch results
end
opt Session expiry & renewal
T->>Sess: advance_time(delta)
T->>Sess: is_expired(session_id)
Sess-->>T: true
T->>Sess: renew_session(session_id, duration)
Sess-->>T: renewed
end
T-->>Dev: assertions
sequenceDiagram
autonumber
actor T as Session Test
participant Sess as SessionActions
Note over T,Sess: Session lifecycle and validation
T->>Sess: create_session(min_duration, max_tx)
Sess-->>T: session_id
T->>Sess: remaining(session_id)
Sess-->>T: tx_left, time_left
T->>Sess: validate_for_action(session_id)
Sess-->>T: ok
T->>Sess: advance_time(+delta)
T->>Sess: is_expired(session_id)
Sess-->>T: [true|false]
alt needs renewal
T->>Sess: needs_renewal(session_id)
Sess-->>T: true
T->>Sess: renew_session(session_id, duration)
Sess-->>T: renewed
else invalid params
T->>Sess: create_session(invalid_duration, tx)
Sess--x T: panic INVALID_DURATION
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 14
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/test/comprehensive_error_test.cairo(1 hunks)src/test/comprehensive_gear_test.cairo(1 hunks)src/test/comprehensive_integration_test.cairo(1 hunks)src/test/comprehensive_performance_test.cairo(1 hunks)src/test/comprehensive_player_test.cairo(1 hunks)src/test/comprehensive_session_test.cairo(1 hunks)src/test/test_runner.cairo(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test
| let core_dispatcher = create_core_dispatcher(); | ||
| let player = sample_player(); | ||
|
|
||
| start_cheat_caller_address(core_dispatcher.contract_address, player); | ||
| start_cheat_block_timestamp(core_dispatcher.contract_address, 1000); | ||
|
|
||
| // Try operation with invalid contract state (admin is zero) | ||
| let items = array![1_u256]; | ||
| core_dispatcher.pick_items(items); | ||
|
|
There was a problem hiding this comment.
Invalid-contract-state branch is unreachable with current setup.
Here we call pick_items as a non-admin player and expect an INVALID_CONTRACT_STATE panic. The call will bail out at the earlier permission gate (INSUFFICIENT_PERMISSIONS) long before any contract-state check executes, so the expected panic string will never surface. Please set up the dispatcher so the caller can pass the permission check (or pre-configure the contract into the invalid state you want to exercise) before asserting on that error path.
🤖 Prompt for AI Agents
In src/test/comprehensive_error_test.cairo around lines 370 to 379, the test
calls core_dispatcher.pick_items as a non-admin and expects
INVALID_CONTRACT_STATE, but the call fails earlier with
INSUFFICIENT_PERMISSIONS; adjust the setup so the caller passes the permission
check before triggering the invalid-contract-state branch. Either make the
caller the admin (or grant the player admin permission) prior to calling
pick_items, or mutate the dispatcher contract state to set admin=0 while the
caller already has permission, then invoke pick_items and assert
INVALID_CONTRACT_STATE.
| #[test] | ||
| #[should_panic(expected: ('INVALID_COUNT_TOO_HIGH',))] | ||
| fn test_overflow_protection() { | ||
| let core_dispatcher = create_core_dispatcher(); | ||
| let admin = contract_address_const::<0x999>(); | ||
|
|
||
| start_cheat_caller_address(core_dispatcher.contract_address, admin); | ||
| start_cheat_block_timestamp(core_dispatcher.contract_address, 1000); | ||
|
|
||
| // Test with extremely large values that could cause overflow | ||
| // This would be tested with actual gear details containing large counts | ||
| let gear_details = array![]; // Would contain details with count > 1000000 | ||
| core_dispatcher.spawn_items(gear_details); | ||
|
|
There was a problem hiding this comment.
Overflow test lacks the overflow stimulus.
We expect INVALID_COUNT_TOO_HIGH, yet we pass an empty gear_details array. Without pushing a count beyond the cap (or otherwise constructing data that hits the overflow branch), the method has nothing to overflow and will either succeed or trigger a different validation. Populate gear_details with entries whose counts exceed the documented limit so the target panic can actually fire.
🤖 Prompt for AI Agents
In src/test/comprehensive_error_test.cairo around lines 418 to 431, the test
annotated with should_panic expected INVALID_COUNT_TOO_HIGH but passes an empty
gear_details array so the overflow branch is never exercised; update the test to
construct and pass a gear_details array containing at least one gear entry
struct with its count field set above the documented cap (e.g., > 1_000_000) or
otherwise craft the input that triggers the overflow validation, then call
core_dispatcher.spawn_items(gear_details) as before so the function hits the
overflow branch and the expected panic occurs.
| // Test material consumption logic | ||
| let materials_before = 100_u256; // Mock material count | ||
| gear_dispatcher.upgrade_gear(1_u256, session_id); | ||
|
|
||
| // In a real test, we would verify materials were consumed | ||
| let expected_materials_after = materials_before - 10; // Assuming 10 materials per upgrade | ||
| assert(expected_materials_after == 90, 'Materials consumed'); | ||
|
|
There was a problem hiding this comment.
Replace tautological material assertion with a real state check.
Line 95 hard-codes expected_materials_after to 90, so the assertion is always true regardless of what upgrade_gear does. We never observe contract storage or an emitted event, meaning the test still passes if no materials are deducted (or if the call reverts before the comparison). Please interrogate the dispatcher for the player’s material balance before/after (or assert on a specific event payload) so the test actually guards material consumption.
| // Test equipping conflicting items (e.g., two weapons) | ||
| let weapon1 = 1_u256; | ||
| let weapon2 = 2_u256; | ||
|
|
||
| // In a real implementation, this would check for equipment slot conflicts | ||
| let items: Array<u256> = array![weapon1, weapon2]; | ||
| gear_dispatcher.equip(items, session_id); | ||
|
|
There was a problem hiding this comment.
Test never asserts the conflict behaviour.
The comment says we are “testing equipping conflicting items”, yet the body just calls equip and ignores the result. If the slot-conflict guard regresses tomorrow, this test keeps passing. Either mark it with #[should_panic(expected = (...))] and let the call revert, or inspect dispatcher state/events afterward to confirm the conflict was detected.
🤖 Prompt for AI Agents
In src/test/comprehensive_gear_test.cairo around lines 167 to 174, the test
calls gear_dispatcher.equip with two weapons but never asserts the expected
conflict outcome; update the test to actually verify conflict handling by either
(A) marking the test to expect a revert/panic (e.g., use the test harness
attribute or #[should_panic(expected = "...")] or wrap the call in an
assert_revert helper so the equip call failing is treated as success), or (B)
change the test to capture and assert the returned Result/Err value or inspect
dispatcher state/events after the call to confirm no items were equipped and an
appropriate error/event was emitted; pick one approach and implement the
corresponding assertion so the test fails if the slot-conflict guard regresses.
| let forge_materials: Array<u256> = array![1_u256, 2_u256, 3_u256]; | ||
| let forged_item = gear_dispatcher.forge(forge_materials, session_id); | ||
|
|
||
| // Test forging result | ||
| assert(forged_item == 0 || forged_item > 0, 'Forging completed'); | ||
|
|
There was a problem hiding this comment.
Forging assertion is vacuous.
forged_item == 0 || forged_item > 0 is true for every u256, so after we reach this point the assertion can never fail. Capture the actual forging outcome (e.g., expected item id, emitted event, or inventory change) and compare against that instead so the test meaningfully protects the forge logic.
🤖 Prompt for AI Agents
In src/test/comprehensive_gear_test.cairo around lines 210-215, the assertion
`forged_item == 0 || forged_item > 0` is vacuous and must be replaced with a
meaningful check: determine the expected forging outcome (either a deterministic
item id, the emitted forge event payload, or an inventory change) and assert
against that. Modify the test to capture the forge return value and/or
transaction events, then assert the returned item id equals the expected id (or
is present in the captured events) or query the player's inventory after forging
and assert it contains the new item id; if forging is non-deterministic, assert
a specific emitted event with the new id was produced rather than the current
vacuous expression.
| fn test_session_remaining_calculations() { | ||
| let session_dispatcher = create_session_dispatcher(); | ||
|
|
||
| // Test remaining transactions calculation | ||
| let remaining_tx = session_dispatcher.calculate_remaining_transactions(25, 100); | ||
| assert(remaining_tx == 75, 'Should have 75 transactions left'); | ||
|
|
||
| let no_remaining_tx = session_dispatcher.calculate_remaining_transactions(100, 100); | ||
| assert(no_remaining_tx == 0, 'Should have 0 transactions left'); | ||
|
|
||
| // Test remaining time calculation | ||
| let remaining_time = session_dispatcher.calculate_session_time_remaining(1000, 3600); | ||
| // This would need to be tested with proper block timestamp mocking | ||
| assert(remaining_time >= 0, 'Should have remaining time'); | ||
| } |
There was a problem hiding this comment.
Remaining-calculation test asserts a tautology.
calculate_session_time_remaining(1000, 3600) returns a deterministic number based on the current block timestamp, yet the test only checks >= 0, which is always true for unsigned durations. We never pin the timestamp or compare to an expected value, so the test cannot catch regressions. Please freeze the block timestamp with the cheat API and assert on the exact remaining time (and its transition as the timestamp advances) to make this case meaningful.
🤖 Prompt for AI Agents
In src/test/comprehensive_session_test.cairo around lines 282 to 296, the
remaining-time test only asserts a tautology (>= 0); instead freeze the block
timestamp using the cheat API to a known value, call
calculate_session_time_remaining with a known session end timestamp and assert
the exact expected remaining_time (end_ts - frozen_ts), then advance the frozen
timestamp (cheat warp/increase) and assert the remaining_time decreased
accordingly and finally set timestamp past the end_ts and assert it returns 0;
modify the test to pin timestamps and include assertions for at least these
three states (before, after advancing, and past end).
| fn test_suite_completeness() { | ||
| // Verify all major game systems are covered | ||
| assert(true, 'Session tests implemented'); | ||
| assert(true, 'Gear tests implemented'); | ||
| assert(true, 'Player tests implemented'); | ||
| assert(true, 'Integration tests implemented'); | ||
| assert(true, 'Error handling tests implemented'); | ||
| assert(true, 'Performance tests implemented'); | ||
| assert(true, 'Security tests implemented'); | ||
| } |
There was a problem hiding this comment.
Replace tautological asserts with real completeness checks.
Line 91 just asserts true, so the test will succeed even if every referenced module is empty or missing real cases. That gives a false sense of coverage and contradicts issue #145’s goal of guaranteeing actual test implementations. Please make this test consume real signals—e.g., call manifest/helpers from each suite or query a shared registry—instead of asserting constants.
🤖 Prompt for AI Agents
In src/test/test_runner.cairo around lines 89 to 98, the test_suite_completeness
function uses tautological assert(true, ...) calls which always pass; replace
them with actual checks that consume real signals from each test suite. For each
listed suite (Session, Gear, Player, Integration, Error handling, Performance,
Security) call the corresponding manifest/helper function or query the shared
test registry to obtain a count or boolean indicating tests are registered
(e.g., get_session_test_count(), session_tests_registered(), or
registry.lookup("session")), and then assert that the returned value is
non-zero/true (e.g., assert(count > 0, 'Session tests implemented')). Ensure
imports/externs are added for those helpers and provide a clear failure message
per suite.
| fn test_coverage_metrics() { | ||
| // Test coverage areas | ||
| let session_coverage = 95; // % | ||
| let gear_coverage = 90; // % | ||
| let player_coverage = 92; // % | ||
| let integration_coverage = 85; // % | ||
| let error_coverage = 88; // % | ||
| let performance_coverage = 80; // % | ||
| let security_coverage = 93; // % | ||
|
|
||
| // Verify minimum coverage thresholds | ||
| assert(session_coverage >= 90, 'Session coverage sufficient'); | ||
| assert(gear_coverage >= 85, 'Gear coverage sufficient'); | ||
| assert(player_coverage >= 85, 'Player coverage sufficient'); | ||
| assert(integration_coverage >= 80, 'Integration coverage sufficient'); | ||
| assert(error_coverage >= 85, 'Error coverage sufficient'); | ||
| assert(performance_coverage >= 75, 'Performance coverage sufficient'); | ||
| assert(security_coverage >= 90, 'Security coverage sufficient'); | ||
| } |
There was a problem hiding this comment.
Hook coverage assertions to real data.
The coverage values on Lines 103‑109 are hard-coded literals, so these assertions pass regardless of the real coverage report. That’s misleading and will mask regressions the moment coverage drops. Wire this up to actual metrics (e.g., exported constants produced during the run or computed in build scripts) or drop the test until such integration exists.
🤖 Prompt for AI Agents
In src/test/test_runner.cairo around lines 101 to 119 the coverage values are
hard-coded literals which makes the assertions meaningless; replace the literal
assignments with real metrics by importing or reading the coverage values
produced by your build/test pipeline (e.g., import exported constants from the
coverage reporter module or load the generated coverage JSON/artifact into the
test), then use those runtime variables in the asserts; if no such exported
metrics exist yet, remove or mark this test as skipped/disabled until you have a
stable mechanism to supply real coverage data so the assertions reflect actual
coverage.
| #[test] | ||
| fn test_quality_metrics() { | ||
| // Test quality indicators | ||
| let edge_cases_covered = true; | ||
| let boundary_conditions_tested = true; | ||
| let error_scenarios_handled = true; | ||
| let performance_benchmarked = true; | ||
| let security_validated = true; | ||
| let integration_verified = true; | ||
|
|
||
| assert(edge_cases_covered, 'Edge cases covered'); | ||
| assert(boundary_conditions_tested, 'Boundary conditions tested'); | ||
| assert(error_scenarios_handled, 'Error scenarios handled'); | ||
| assert(performance_benchmarked, 'Performance benchmarked'); | ||
| assert(security_validated, 'Security validated'); | ||
| assert(integration_verified, 'Integration verified'); | ||
| } |
There was a problem hiding this comment.
Validate quality metrics against real checks.
Lines 124‑129 assign true to every flag before asserting it, which means the test can never fail—no matter the quality gaps. Please replace these placeholders with genuine status checks (for example, aggregate results emitted by the underlying suites) or remove the test to avoid a false green.
🤖 Prompt for AI Agents
In src/test/test_runner.cairo around lines 121 to 137, the test_quality_metrics
unit test currently sets all quality flags to true then asserts them, producing
a false-positive; replace those hardcoded booleans with real checks (e.g.,
query/aggregate the actual test suite results, lints, security scan outputs,
performance benchmark pass/fail indicators, or integration-test summaries) or
remove the test entirely. Specifically, fetch or accept as inputs the real
status values produced by the underlying suites/plugins and assert those values
(or assert derived aggregated booleans), and ensure any I/O or async checks are
handled appropriately so the test fails when any real metric indicates a
problem.
| #[test] | ||
| fn test_execution_validation() { | ||
| // Validate test execution environment | ||
| assert(true, 'Test environment ready'); | ||
| assert(true, 'All dependencies available'); | ||
| assert(true, 'Test data properly mocked'); | ||
| } |
There was a problem hiding this comment.
Avoid unconditional “environment ready” assertions.
assert(true, ...) on Lines 188‑190 does not verify that dependencies or fixtures are actually available, so setup regressions will slip through. Either drop this function or make it exercise tangible readiness checks (e.g., probing required cheatcodes/resources and failing when they are absent).
🤖 Prompt for AI Agents
In src/test/test_runner.cairo around lines 185 to 191, the
test_execution_validation test uses unconditional assert(true, ...) calls which
do not validate environment readiness; replace or remove this test and instead
perform real checks: probe required cheatcodes/resources (e.g., assert the
presence of required files, mock objects, RPC endpoints, or framework-provided
readiness functions) and assert on those results so the test fails when a
dependency is missing, or delete the whole function if no tangible readiness
checks are possible.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
src/test/comprehensive_session_test.cairo (6)
3-12: Tighten imports; avoid duplicated protocol constants.
get_block_timestampandSessionKeyaren’t used in this file; consider removing to keep tests lean.- Duplicating
MIN/MAX_DURATIONandMAX_TRANSACTIONSin tests risks drift. If the contract exposes constants or a shared module, import from there to keep limits single‑sourced.
67-77: Add boundary assertion at exact expiry.Right now you test “before” and “after”; add the equality edge (ts == start + duration) to lock semantics.
- // Move time forward past expiration - start_cheat_block_timestamp(session_dispatcher.contract_address, 5000); + // Move time forward to exact expiration boundary + start_cheat_block_timestamp(session_dispatcher.contract_address, 4600); + + // At the boundary, treat as expired (expected: false from `check_session_expiry`) + let expired_at_boundary = session_dispatcher.check_session_expiry(1000, VALID_DURATION); + assert(!expired_at_boundary, 'Session should be expired at its exact end time'); + + // Move time forward past expiration + start_cheat_block_timestamp(session_dispatcher.contract_address, 5000);
90-100: Cover used > max case for transaction limit.Add a clamp test to ensure no negative/underflow paths leak through when used exceeds max.
let has_transactions = session_dispatcher.check_transaction_limit(50, VALID_TRANSACTIONS); assert(has_transactions, 'Should have transactions left'); let no_transactions = session_dispatcher .check_transaction_limit(VALID_TRANSACTIONS, VALID_TRANSACTIONS); assert(!no_transactions, 'Should have no transactions left'); + + // Used beyond max should still report no remaining (clamped) + let overused = session_dispatcher.check_transaction_limit(VALID_TRANSACTIONS + 1, VALID_TRANSACTIONS); + assert(!overused, 'Used greater than max should report no remaining');
113-131: Assert post‑renewal state.After
renew_session, immediately assert that renewal is no longer needed at the same threshold.let renewed = session_dispatcher .renew_session(session_id, VALID_DURATION, VALID_TRANSACTIONS); assert(renewed, 'Session should be renewed'); + + // After renewal at ts=4500 with 3600s duration, time_left=3600 > 300 → no renewal needed + let needs_renewal_after = session_dispatcher.check_session_needs_renewal(session_id, 300); + assert(!needs_renewal_after, 'Should not need renewal immediately after renewing');
224-252: Name vs behavior: not actually concurrent.This creates sessions sequentially for two players. Consider renaming to
test_session_multi_player_creationor simulate near‑simultaneous creation at the same timestamp if the intent is to stress ID uniqueness.
254-286: Add negative test: validation fails for wrong player.Complement the happy‑path by ensuring sessions aren’t valid for another address.
fn test_session_validation_for_action() { let session_dispatcher = create_session_dispatcher(); let player = sample_player(); @@ session_dispatcher.validate_session_for_action(session_id); @@ } + + #[test] + fn test_session_validation_wrong_player() { + let session_dispatcher = create_session_dispatcher(); + let player1 = contract_address_const::<0xAAA>(); + let player2 = contract_address_const::<0xBBB>(); + + start_cheat_caller_address(session_dispatcher.contract_address, player1); + start_cheat_block_timestamp(session_dispatcher.contract_address, 1000); + let session_id = session_dispatcher.create_session_key(VALID_DURATION, VALID_TRANSACTIONS); + stop_cheat_caller_address(session_dispatcher.contract_address); + + // Validate should fail for a different player + let valid_for_other = session_dispatcher.validate_session(session_id, player2); + assert(!valid_for_other, 'Session should not validate for a different player'); + + stop_cheat_block_timestamp(session_dispatcher.contract_address); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/test/comprehensive_error_test.cairo(1 hunks)src/test/comprehensive_gear_test.cairo(1 hunks)src/test/comprehensive_integration_test.cairo(1 hunks)src/test/comprehensive_performance_test.cairo(1 hunks)src/test/comprehensive_player_test.cairo(1 hunks)src/test/comprehensive_session_test.cairo(1 hunks)src/test/test_runner.cairo(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/comprehensive_player_test.cairo
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test
🔇 Additional comments (17)
src/test/comprehensive_performance_test.cairo (3)
3-8: BringArrayTraitinto scope so the performance suite compilesThese tests call
.append(),.len(), and.at()onArrayvalues throughout the file, butarray::ArrayTraitis not imported. Cairo requires that trait in scope for those methods; without it the module fails to compile. Please add the import alongside the existinguseblock.use snforge_std::{ declare, ContractClassTrait, DeclareResultTrait, start_cheat_caller_address, stop_cheat_caller_address, start_cheat_block_timestamp, stop_cheat_block_timestamp, get_class_hash, spy_events, EventSpyAssertionsTrait, }; + use array::ArrayTrait;
259-280: Fix the mismatched timestamp cheat cleanup intest_session_validation_performanceThis test starts
start_cheat_block_timestamp(session_dispatcher.contract_address, …)but later callsstop_cheat_block_timestamp(gear_dispatcher.contract_address). Because no timestamp cheat was ever started for the gear dispatcher, that stop call panics immediately, leaving the session cheat running and breaking the suite. Stop the cheat on the same dispatcher you started it on.- stop_cheat_block_timestamp(gear_dispatcher.contract_address); + stop_cheat_block_timestamp(session_dispatcher.contract_address);
288-304: Stop the session timestamp cheat intest_session_auto_renewal_performanceWe start
start_cheat_block_timestamp(session_dispatcher.contract_address, 1000)to create the session, but never stop it. That cheat remains active while later tests run, skewing timestamps and producing cascading failures. Add the missing stop before switching to the gear dispatcher.let session_id = session_dispatcher.create_session_key(3600, 100); stop_cheat_caller_address(session_dispatcher.contract_address); + stop_cheat_block_timestamp(session_dispatcher.contract_address); // Test auto-renewal performance start_cheat_caller_address(gear_dispatcher.contract_address, player); start_cheat_block_timestamp(gear_dispatcher.contract_address, 4300); // Close to expirationsrc/test/comprehensive_gear_test.cairo (3)
90-96: Replace the placeholder material consumption check with a real assertion
materials_beforeis a hard-coded constant andexpected_materials_after == 90is always true, so this test says nothing about whetherupgrade_gearactually burns materials. Please fetch the real material balance (via a getter or emitted event) before/after the upgrade and assert on that, or expect a specific event payload confirming the deduction.
167-174: Assert the slot-conflict behaviour instead of ignoring the resultThis “conflict” test just calls
gear_dispatcher.equipand exits. If the conflict guard regresses, the test still passes. Either wrap the call in anassert_panic/#[should_panic(expected = ...)]or query dispatcher state afterward to prove the second weapon was rejected.
209-215: Make the forging assertion meaningful
forged_item == 0 || forged_item > 0holds for everyu256, so the test provides no protection. Capture the actual forging outcome—e.g., expected item id, emitted event, or inventory diff—and assert on that concrete value.src/test/comprehensive_integration_test.cairo (2)
82-127: Stop timestamp cheats on the dispatchers they were started on
start_cheat_block_timestampis invoked forsession_dispatcher(Line 84) but later we callstop_cheat_block_timestamp(gear_dispatcher.contract_address)(Line 126). Because no timestamp cheat was started for the gear dispatcher, that call panics and leaves the session cheat running. Update this test—and the others below with the same pattern—to stop the cheat onsession_dispatcher.contract_address(and add separate start/stop pairs for other dispatchers only when they actually use them).
135-206: Sweep the module for timestamp stop mismatches
test_session_with_gear_operations,test_session_expiration_during_operations,test_session_renewal_during_operations, and several later scenarios all start the block-timestamp cheat onsession_dispatcher.contract_addressbut stop it ongear_dispatcher.contract_address. Each mismatch triggers a panic and leaves the original cheat active. Please align every start/stop pair with the same dispatcher (and add missing stops where necessary) before this suite can land.src/test/test_runner.cairo (4)
88-98: Replace tautological completeness asserts with real checksEvery assertion here is
assert(true, …), so the test can never fail even if whole suites are missing. Wire this test to concrete signals (counts, registration flags, etc.) from each module or drop it until such data exists.
101-119: Hook the coverage assertions to live metricsThe coverage values are hard-coded literals, so these checks stay green regardless of actual coverage. Import the real metrics produced by your tooling (or skip the test until you can) and assert against those instead of fixed numbers.
121-137: Stop asserting booleans you just set totrue
edge_cases_coveredand the other flags are set totrueimmediately before being asserted, which makes the test meaningless. Swap these for real status signals from the underlying suites or remove the test.
185-191: Validate the execution environment with real probesAsserting
truethree times never verifies readiness. Replace these placeholders with tangible checks (e.g., confirming required cheatcodes/resources are available) or drop the test.src/test/comprehensive_error_test.cairo (2)
368-379: Reach the intendedINVALID_CONTRACT_STATEbranchCalling
pick_itemsas a non-admin player still trips the earlierINSUFFICIENT_PERMISSIONSguard, so the expectedINVALID_CONTRACT_STATEpanic never fires. Grant the caller the required permission (or impersonate the admin) before mutating state into the invalid configuration you want to exercise; only then assert on theINVALID_CONTRACT_STATEpanic.
424-431: Stimulate the overflow before expectingINVALID_COUNT_TOO_HIGHThis test passes an empty
gear_detailsarray, sospawn_itemsnever sees an oversized count and the expected overflow panic cannot occur. Populategear_detailswith an entry whose count exceeds the documented cap (e.g., >1_000_000) so the overflow branch is actually hit.src/test/comprehensive_session_test.cairo (3)
36-54: LGTM on base happy‑path.Good use of cheat caller/timestamp and cleanup with matching stop calls.
299-303: Fix tautological remaining‑time assertion with pinned timestamps.Current check
>= 0can’t fail for unsigned durations; freeze and advance time, assert exact values.- // Test remaining time calculation - let remaining_time = session_dispatcher.calculate_session_time_remaining(1000, 3600); - // This would need to be tested with proper block timestamp mocking - assert(remaining_time >= 0, 'Should have remaining time'); + // Remaining time calculation with pinned timestamps + start_cheat_block_timestamp(session_dispatcher.contract_address, 1000); + let remaining_time_start = session_dispatcher.calculate_session_time_remaining(1000, 3600); + assert(remaining_time_start == 3600, 'Expected 3600s remaining at start'); + + // Advance 200s + start_cheat_block_timestamp(session_dispatcher.contract_address, 1200); + let remaining_time_after = session_dispatcher.calculate_session_time_remaining(1000, 3600); + assert(remaining_time_after == 2400, 'Expected 2400s after advancing 200s'); + + // Past expiry clamps to 0 + start_cheat_block_timestamp(session_dispatcher.contract_address, 5000); + let remaining_time_zero = session_dispatcher.calculate_session_time_remaining(1000, 3600); + assert(remaining_time_zero == 0, 'Expected 0 after expiration'); + + stop_cheat_block_timestamp(session_dispatcher.contract_address);
318-332: No action needed:session_key_address == playermatches current contract logic. The system explicitly uses the player address as the session key (// Using player as session key for nowinsrc/systems/session.cairo), and there’s no getter to derive a different key.
| use starknet::{ContractAddress, contract_address_const}; | ||
| use snforge_std::{ | ||
| declare, ContractClassTrait, DeclareResultTrait, start_cheat_caller_address, | ||
| stop_cheat_caller_address, start_cheat_block_timestamp, stop_cheat_block_timestamp, | ||
| }; | ||
| use coa::systems::player::{IPlayerDispatcher, IPlayerDispatcherTrait}; | ||
| use coa::systems::gear::{IGearDispatcher, IGearDispatcherTrait}; | ||
| use coa::systems::session::{ISessionActionsDispatcher, ISessionActionsDispatcherTrait}; | ||
| use coa::systems::core::{ICoreDispatcher, ICoreDispatcherTrait}; | ||
|
|
There was a problem hiding this comment.
Import array utilities so the error tests compile
This module declares Array<u256> variables and calls .append(), but never imports array::Array or array::ArrayTrait. Please bring both into scope; otherwise the code will not compile.
use snforge_std::{
declare, ContractClassTrait, DeclareResultTrait, start_cheat_caller_address,
stop_cheat_caller_address, start_cheat_block_timestamp, stop_cheat_block_timestamp,
};
+ use array::{Array, ArrayTrait};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| use starknet::{ContractAddress, contract_address_const}; | |
| use snforge_std::{ | |
| declare, ContractClassTrait, DeclareResultTrait, start_cheat_caller_address, | |
| stop_cheat_caller_address, start_cheat_block_timestamp, stop_cheat_block_timestamp, | |
| }; | |
| use coa::systems::player::{IPlayerDispatcher, IPlayerDispatcherTrait}; | |
| use coa::systems::gear::{IGearDispatcher, IGearDispatcherTrait}; | |
| use coa::systems::session::{ISessionActionsDispatcher, ISessionActionsDispatcherTrait}; | |
| use coa::systems::core::{ICoreDispatcher, ICoreDispatcherTrait}; | |
| use starknet::{ContractAddress, contract_address_const}; | |
| use snforge_std::{ | |
| declare, ContractClassTrait, DeclareResultTrait, start_cheat_caller_address, | |
| stop_cheat_caller_address, start_cheat_block_timestamp, stop_cheat_block_timestamp, | |
| }; | |
| use array::{Array, ArrayTrait}; | |
| use coa::systems::player::{IPlayerDispatcher, IPlayerDispatcherTrait}; | |
| use coa::systems::gear::{IGearDispatcher, IGearDispatcherTrait}; | |
| use coa::systems::session::{ISessionActionsDispatcher, ISessionActionsDispatcherTrait}; | |
| use coa::systems::core::{ICoreDispatcher, ICoreDispatcherTrait}; |
🤖 Prompt for AI Agents
In src/test/comprehensive_error_test.cairo around lines 3 to 12, the test uses
Array<u256> and calls .append() but doesn't import the array utilities; add the
necessary imports to the top use list — bring array::Array and array::ArrayTrait
into scope (e.g., add use array::Array; and use array::ArrayTrait;) so the Array
type and its methods are available and the file compiles.
| use starknet::{ContractAddress, contract_address_const}; | ||
| use snforge_std::{ | ||
| declare, ContractClassTrait, DeclareResultTrait, start_cheat_caller_address, | ||
| stop_cheat_caller_address, start_cheat_block_timestamp, stop_cheat_block_timestamp, | ||
| spy_events, EventSpyAssertionsTrait, | ||
| }; |
There was a problem hiding this comment.
Import Array/ArrayTrait for the gear tests to compile
This module annotates many locals as Array<u256> and calls .append()/.len(), but it never imports the array types/traits. Cairo requires array::Array for the type and array::ArrayTrait for the methods; without them the file does not compile.
use snforge_std::{
declare, ContractClassTrait, DeclareResultTrait, start_cheat_caller_address,
stop_cheat_caller_address, start_cheat_block_timestamp, stop_cheat_block_timestamp,
spy_events, EventSpyAssertionsTrait,
};
+ use array::{Array, ArrayTrait};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| use starknet::{ContractAddress, contract_address_const}; | |
| use snforge_std::{ | |
| declare, ContractClassTrait, DeclareResultTrait, start_cheat_caller_address, | |
| stop_cheat_caller_address, start_cheat_block_timestamp, stop_cheat_block_timestamp, | |
| spy_events, EventSpyAssertionsTrait, | |
| }; | |
| use starknet::{ContractAddress, contract_address_const}; | |
| use snforge_std::{ | |
| declare, ContractClassTrait, DeclareResultTrait, start_cheat_caller_address, | |
| stop_cheat_caller_address, start_cheat_block_timestamp, stop_cheat_block_timestamp, | |
| spy_events, EventSpyAssertionsTrait, | |
| }; | |
| use array::{Array, ArrayTrait}; |
🤖 Prompt for AI Agents
In src/test/comprehensive_gear_test.cairo around lines 3 to 8, the test file
uses Array<u256> and calls .append()/.len() but never imports the array
types/traits; add the necessary imports from the Cairo array module (import
array::Array and array::ArrayTrait) at the top of the file so the Array type and
its methods are available to the compiler.
| // Test forging with valid materials | ||
| let valid_materials: Array<u256> = array![1_u256, 2_u256, 3_u256]; | ||
| let valid_result = gear_dispatcher.forge(valid_materials, session_id); | ||
| assert(valid_result >= 0, 'Valid forging attempt'); | ||
|
|
There was a problem hiding this comment.
Strengthen the forging requirements test
valid_result >= 0 is always true for u256, so the “valid” branch can never fail. Please check the real success signal (event, inventory entry, specific id range) instead of asserting a tautology.
🤖 Prompt for AI Agents
In src/test/comprehensive_gear_test.cairo around lines 236 to 240, the assertion
`valid_result >= 0` is a tautology for u256 and doesn't verify a successful
forge; replace it with a concrete success check such as asserting the expected
event was emitted, verifying the player's inventory now contains the new gear
id, or checking that `valid_result` equals/falls within the expected success
id/code range. Update the test to fetch and assert the relevant event payload or
query the inventory/state to confirm the forged item exists (or that the
returned id is in the valid id range) and use that assertion instead of the `>=
0` check.
| use starknet::{ContractAddress, contract_address_const}; | ||
| use snforge_std::{ | ||
| declare, ContractClassTrait, DeclareResultTrait, start_cheat_caller_address, | ||
| stop_cheat_caller_address, start_cheat_block_timestamp, stop_cheat_block_timestamp, | ||
| spy_events, EventSpyAssertionsTrait, | ||
| }; | ||
| use coa::models::session::SessionKey; | ||
| use coa::models::gear::{Gear, GearType}; | ||
| use coa::systems::player::{IPlayerDispatcher, IPlayerDispatcherTrait}; | ||
| use coa::systems::gear::{IGearDispatcher, IGearDispatcherTrait}; | ||
| use coa::systems::session::{ISessionActionsDispatcher, ISessionActionsDispatcherTrait}; | ||
| use coa::systems::core::{ICoreDispatcher, ICoreDispatcherTrait}; |
There was a problem hiding this comment.
Import array utilities so integration tests build
This module uses Array types plus .len() and .at() on arrays, yet array::{Array, ArrayTrait} is never imported. Without them the code does not compile. Please add the import near the other use statements.
use snforge_std::{
declare, ContractClassTrait, DeclareResultTrait, start_cheat_caller_address,
stop_cheat_caller_address, start_cheat_block_timestamp, stop_cheat_block_timestamp,
spy_events, EventSpyAssertionsTrait,
};
+ use array::{Array, ArrayTrait};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| use starknet::{ContractAddress, contract_address_const}; | |
| use snforge_std::{ | |
| declare, ContractClassTrait, DeclareResultTrait, start_cheat_caller_address, | |
| stop_cheat_caller_address, start_cheat_block_timestamp, stop_cheat_block_timestamp, | |
| spy_events, EventSpyAssertionsTrait, | |
| }; | |
| use coa::models::session::SessionKey; | |
| use coa::models::gear::{Gear, GearType}; | |
| use coa::systems::player::{IPlayerDispatcher, IPlayerDispatcherTrait}; | |
| use coa::systems::gear::{IGearDispatcher, IGearDispatcherTrait}; | |
| use coa::systems::session::{ISessionActionsDispatcher, ISessionActionsDispatcherTrait}; | |
| use coa::systems::core::{ICoreDispatcher, ICoreDispatcherTrait}; | |
| use starknet::{ContractAddress, contract_address_const}; | |
| use snforge_std::{ | |
| declare, ContractClassTrait, DeclareResultTrait, start_cheat_caller_address, | |
| stop_cheat_caller_address, start_cheat_block_timestamp, stop_cheat_block_timestamp, | |
| spy_events, EventSpyAssertionsTrait, | |
| }; | |
| use array::{Array, ArrayTrait}; | |
| use coa::models::session::SessionKey; | |
| use coa::models::gear::{Gear, GearType}; | |
| use coa::systems::player::{IPlayerDispatcher, IPlayerDispatcherTrait}; | |
| use coa::systems::gear::{IGearDispatcher, IGearDispatcherTrait}; | |
| use coa::systems::session::{ISessionActionsDispatcher, ISessionActionsDispatcherTrait}; | |
| use coa::systems::core::{ICoreDispatcher, ICoreDispatcherTrait}; |
🤖 Prompt for AI Agents
In src/test/comprehensive_integration_test.cairo around lines 3 to 14, the test
uses Array types and calls .len()/.at() but never imports the array utilities;
add an import for array::{Array, ArrayTrait} alongside the other use statements
so the Array type and its methods are available to the module.
| #[test] | ||
| fn test_gas_usage_single_operations() { | ||
| let player_dispatcher = create_player_dispatcher(); | ||
| let gear_dispatcher = create_gear_dispatcher(); | ||
| let player = sample_player(); | ||
| let session_id = 12345; | ||
|
|
||
| start_cheat_caller_address(player_dispatcher.contract_address, player); | ||
| start_cheat_block_timestamp(player_dispatcher.contract_address, 1000); | ||
|
|
||
| // Benchmark single operations | ||
| player_dispatcher.new('CHAOS_MERCENARIES', session_id); | ||
|
|
||
| stop_cheat_caller_address(player_dispatcher.contract_address); | ||
| start_cheat_caller_address(gear_dispatcher.contract_address, player); | ||
|
|
||
| let items = array![1_u256]; | ||
| gear_dispatcher.equip(items, session_id); | ||
| gear_dispatcher.upgrade_gear(1_u256, session_id); | ||
|
|
||
| stop_cheat_caller_address(gear_dispatcher.contract_address); | ||
| stop_cheat_block_timestamp(gear_dispatcher.contract_address); | ||
| } |
There was a problem hiding this comment.
Align timestamp cheats in test_gas_usage_single_operations
Here we start a timestamp cheat on player_dispatcher.contract_address, but never stop it. Instead, we call stop_cheat_block_timestamp(gear_dispatcher.contract_address) even though no cheat was started for the gear dispatcher, so the test panics. Stop the player cheat after its block, and only stop the gear cheat if you actually start one.
player_dispatcher.new('CHAOS_MERCENARIES', session_id);
stop_cheat_caller_address(player_dispatcher.contract_address);
+ stop_cheat_block_timestamp(player_dispatcher.contract_address);
start_cheat_caller_address(gear_dispatcher.contract_address, player);
+ start_cheat_block_timestamp(gear_dispatcher.contract_address, 1000);
let items = array![1_u256];
gear_dispatcher.equip(items, session_id);
gear_dispatcher.upgrade_gear(1_u256, session_id);
stop_cheat_caller_address(gear_dispatcher.contract_address);
stop_cheat_block_timestamp(gear_dispatcher.contract_address);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[test] | |
| fn test_gas_usage_single_operations() { | |
| let player_dispatcher = create_player_dispatcher(); | |
| let gear_dispatcher = create_gear_dispatcher(); | |
| let player = sample_player(); | |
| let session_id = 12345; | |
| start_cheat_caller_address(player_dispatcher.contract_address, player); | |
| start_cheat_block_timestamp(player_dispatcher.contract_address, 1000); | |
| // Benchmark single operations | |
| player_dispatcher.new('CHAOS_MERCENARIES', session_id); | |
| stop_cheat_caller_address(player_dispatcher.contract_address); | |
| start_cheat_caller_address(gear_dispatcher.contract_address, player); | |
| let items = array![1_u256]; | |
| gear_dispatcher.equip(items, session_id); | |
| gear_dispatcher.upgrade_gear(1_u256, session_id); | |
| stop_cheat_caller_address(gear_dispatcher.contract_address); | |
| stop_cheat_block_timestamp(gear_dispatcher.contract_address); | |
| } | |
| #[test] | |
| fn test_gas_usage_single_operations() { | |
| let player_dispatcher = create_player_dispatcher(); | |
| let gear_dispatcher = create_gear_dispatcher(); | |
| let player = sample_player(); | |
| let session_id = 12345; | |
| start_cheat_caller_address(player_dispatcher.contract_address, player); | |
| start_cheat_block_timestamp(player_dispatcher.contract_address, 1000); | |
| // Benchmark single operations | |
| player_dispatcher.new('CHAOS_MERCENARIES', session_id); | |
| stop_cheat_caller_address(player_dispatcher.contract_address); | |
| stop_cheat_block_timestamp(player_dispatcher.contract_address); | |
| start_cheat_caller_address(gear_dispatcher.contract_address, player); | |
| start_cheat_block_timestamp(gear_dispatcher.contract_address, 1000); | |
| let items = array![1_u256]; | |
| gear_dispatcher.equip(items, session_id); | |
| gear_dispatcher.upgrade_gear(1_u256, session_id); | |
| stop_cheat_caller_address(gear_dispatcher.contract_address); | |
| stop_cheat_block_timestamp(gear_dispatcher.contract_address); | |
| } |
2 participants
Closes #145
Summary by CodeRabbit