test: use abort instead of teardown in contracts/access failure tests - #472
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughExpected-failure tests across three Move test suites now explicitly abort after failure-triggering operations, replacing numeric abort sentinels, normal completion, and selected cleanup paths. ChangesExpected-failure test updates
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #472 +/- ##
==========================================
+ Coverage 96.51% 96.57% +0.05%
==========================================
Files 34 34
Lines 3414 3414
Branches 799 799
==========================================
+ Hits 3295 3297 +2
Misses 73 73
+ Partials 46 44 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![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 (3)
contracts/access/tests/two_step_tests.move (1)
506-532: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHelper functions retain dead cleanup unreachable after the expected-abort call.
expect_wrapper_mismatch(line 512,second.return_val(obj, borrow_token)) andexpect_object_mismatch(line 528,wrapper.return_val(bogus_cap, borrow_token)) are each the expected failure point (EWrongTwoStepTransferWrapper/EWrongTwoStepTransferObject), yet both helpers continue withunwrap/deletecalls afterward that can never execute. Same root cause as the equivalent helpers indelayed_tests.move.As per path instructions, STYLEGUIDE.md says expected-failure tests should "not perform any cleanup or normal termination logic" and should "end at the failure boundary."
♻️ Proposed fix
fun expect_wrapper_mismatch( mut first: two_step_transfer::TwoStepTransferWrapper<DummyCap>, mut second: two_step_transfer::TwoStepTransferWrapper<DummyCap>, ctx: &mut TxContext, ) { let (obj, borrow_token) = first.borrow_val(); second.return_val(obj, borrow_token); - - let DummyCap { id } = first.unwrap(ctx); - id.delete(); - let DummyCap { id } = second.unwrap(ctx); - id.delete(); }fun expect_object_mismatch( mut wrapper: two_step_transfer::TwoStepTransferWrapper<DummyCap>, ctx: &mut TxContext, ) { let (borrowed_cap, borrow_token) = wrapper.borrow_val(); let DummyCap { id } = borrowed_cap; id.delete(); let bogus_cap = new_cap(ctx); wrapper.return_val(bogus_cap, borrow_token); - - let DummyCap { id } = wrapper.unwrap(ctx); - id.delete(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/access/tests/two_step_tests.move` around lines 506 - 532, Remove the unreachable cleanup and normal-termination logic after the expected aborts in expect_wrapper_mismatch and expect_object_mismatch. End each helper immediately after second.return_val(obj, borrow_token) and wrapper.return_val(bogus_cap, borrow_token), respectively, leaving the helpers focused solely on reaching the expected failure boundary.Source: Path instructions
contracts/access/tests/access_control_tests.move (1)
1156-1170: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winResidual cleanup before
abortviolates the abort-boundary convention.
cancel_admin_transfer_rejects_non_root(line 1168) andcancel_delay_change_rejects_non_root(line 1956) both callclock::destroy_for_testing(clk)after the failing operation and beforeabort 999. This is exactly the "cleanup after failure" pattern the rest of the file (and this PR) removes elsewhere - the call is unreachable dead code since the preceding line always aborts.As per path instructions, STYLEGUIDE.md states tests should "not perform any cleanup or normal termination logic; the test should end at the failure boundary with an explicit
abort."♻️ Proposed fix (apply to both tests)
scenario.next_tx(`@0xC`); let mut ac = take_ac(&scenario); ac.cancel_default_admin_transfer(scenario.ctx()); - clock::destroy_for_testing(clk); abort 999 }scenario.next_tx(`@0xB`); let mut ac = take_ac(&scenario); ac.cancel_default_admin_delay_change(&clk, scenario.ctx()); - clock::destroy_for_testing(clk); abort 999 }Also applies to: 1944-1958
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/access/tests/access_control_tests.move` around lines 1156 - 1170, Remove the unreachable clock::destroy_for_testing(clk) cleanup calls from both cancel_admin_transfer_rejects_non_root and cancel_delay_change_rejects_non_root, leaving each test to end immediately after the expected-failure operation with abort 999.Source: Path instructions
contracts/access/tests/delayed_tests.move (1)
376-516: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHelper functions still contain dead cleanup code unreachable after the guaranteed-abort call.
attempt_double_schedule,attempt_execute_before_delay,attempt_early_unwrap,attempt_double_unwrap,expect_cancel_without_pending,expect_execute_without_pending,expect_unwrap_without_pending,expect_return_wrong_wrapper, andexpect_return_wrong_objectall place object-deletion /clock::destroy_for_testingstatements after the call that is expected to abort (e.g. the secondschedule_transfer/schedule_unwrapcall, or the mismatchedreturn_val/cancel_schedule/execute_transfer/unwrapcall). Since these helpers are each called from exactly oneexpected_failuretest, this trailing code is unreachable.unwrap_wrong_action_fails(line 346-347) in this same file already demonstrates the pattern of dropping such trailing cleanup in favor of a minimal binding +abort, so these helpers are inconsistent with the convention the rest of the PR applies.As per path instructions, STYLEGUIDE.md's testing convention says expected-failure tests should "not perform any cleanup or normal termination logic" and should "end at the failure boundary."
Similar trims apply to the other listed helpers.♻️ Example fix for `expect_cancel_without_pending`
fun expect_cancel_without_pending( mut wrapper: delayed_transfer::DelayedTransferWrapper<DummyCap>, ctx: &mut TxContext, ) { wrapper.cancel_schedule(); - - let mut clk = clock::create_for_testing(ctx); - clk.set_for_testing(0); - wrapper.schedule_unwrap(&clk, ctx); - clk.set_for_testing(1); - let obj = wrapper.unwrap(&clk, ctx); - let DummyCap { id } = obj; - id.delete(); - clock::destroy_for_testing(clk); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/access/tests/delayed_tests.move` around lines 376 - 516, Remove all unreachable cleanup and post-failure logic from the listed expected-failure helpers: attempt_double_schedule, attempt_execute_before_delay, attempt_early_unwrap, attempt_double_unwrap, expect_cancel_without_pending, expect_execute_without_pending, expect_unwrap_without_pending, expect_return_wrong_wrapper, and expect_return_wrong_object. Make each helper end at the operation expected to abort, retaining only the minimal setup and bindings needed to reach that failure, consistent with unwrap_wrong_action_fails and the testing convention.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contracts/access/tests/delayed_tests.move`:
- Line 167: Remove the trailing bare `abort` statements from the
expected-failure test cases in the delayed access tests, including the case
around the helper call at this location. Apply the same cleanup to every
matching trailing `abort` in the file, leaving the helper calls as the
terminating statements.
---
Nitpick comments:
In `@contracts/access/tests/access_control_tests.move`:
- Around line 1156-1170: Remove the unreachable clock::destroy_for_testing(clk)
cleanup calls from both cancel_admin_transfer_rejects_non_root and
cancel_delay_change_rejects_non_root, leaving each test to end immediately after
the expected-failure operation with abort 999.
In `@contracts/access/tests/delayed_tests.move`:
- Around line 376-516: Remove all unreachable cleanup and post-failure logic
from the listed expected-failure helpers: attempt_double_schedule,
attempt_execute_before_delay, attempt_early_unwrap, attempt_double_unwrap,
expect_cancel_without_pending, expect_execute_without_pending,
expect_unwrap_without_pending, expect_return_wrong_wrapper, and
expect_return_wrong_object. Make each helper end at the operation expected to
abort, retaining only the minimal setup and bindings needed to reach that
failure, consistent with unwrap_wrong_action_fails and the testing convention.
In `@contracts/access/tests/two_step_tests.move`:
- Around line 506-532: Remove the unreachable cleanup and normal-termination
logic after the expected aborts in expect_wrapper_mismatch and
expect_object_mismatch. End each helper immediately after second.return_val(obj,
borrow_token) and wrapper.return_val(bogus_cap, borrow_token), respectively,
leaving the helpers focused solely on reaching the expected failure boundary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 39099e2f-3f6a-4cb9-86ee-6bf6809ccdc9
📒 Files selected for processing (3)
contracts/access/tests/access_control_tests.movecontracts/access/tests/delayed_tests.movecontracts/access/tests/two_step_tests.move
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contracts/access/tests/access_control_tests.move`:
- Line 40: Remove the #[allow(lint(share_owned))] attribute from the test-only
declaration in the access control tests, then update the affected test code to
resolve the underlying share_owned lint without any lint-suppression attributes.
- Around line 1160-1162: Remove the unreachable clock::destroy_for_testing
cleanup calls from the expected-failure tests near
ac.cancel_default_admin_transfer and the corresponding second occurrence,
leaving abort immediately after the operation that is expected to fail.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d3de5cdb-1b8a-4a3c-890f-47adec3d5b51
📒 Files selected for processing (1)
contracts/access/tests/access_control_tests.move
2 participants
Aligning with STYLEGUIDE.md
PR Checklist
Summary by CodeRabbit
abortafter the failure-triggering action.