Add reputation test coverage to profile contract#49
Open
Michaelkingsdev wants to merge 1 commit into
Open
Conversation
Fills in the missing tests for the reputation module: bump, slash and admin_slash. Each one gets a happy path, every error variant it can return, the saturating add/sub and zero-delta edges, an auth-rejection case, and an idempotent-replay check. No production code changed, just tests plus their snapshots. Closes boundlessfi#29
📝 WalkthroughWalkthroughAdds a ChangesReputation Test Suite
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
🤖 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/profile/src/tests/reputation.rs`:
- Around line 181-191: In the function
bump_rejects_caller_without_events_contract_auth, replace the generic error
check assertion res.is_err() with a specific assertion that verifies the error
is the expected authorization failure variant (likely Error::Unauthorized or
Error::NotAdmin depending on what guard should trigger the failure). Apply the
same fix to the other two authorization rejection tests that use similar
patterns, ensuring each test verifies the specific error variant returned rather
than just confirming that some error occurred, which prevents false positives
from unrelated failures masking the test intent.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f14cc28c-2b4a-437e-86c5-174181211289
📒 Files selected for processing (26)
contracts/profile/src/tests/mod.rscontracts/profile/src/tests/reputation.rscontracts/profile/test_snapshots/tests/reputation/admin_slash_happy_path_decrements_reputation.1.jsoncontracts/profile/test_snapshots/tests/reputation/admin_slash_is_idempotent_on_replay.1.jsoncontracts/profile/test_snapshots/tests/reputation/admin_slash_rejects_non_admin_caller.1.jsoncontracts/profile/test_snapshots/tests/reputation/admin_slash_reverts_on_empty_reason.1.jsoncontracts/profile/test_snapshots/tests/reputation/admin_slash_reverts_when_paused.1.jsoncontracts/profile/test_snapshots/tests/reputation/admin_slash_reverts_when_profile_not_found.1.jsoncontracts/profile/test_snapshots/tests/reputation/admin_slash_saturates_at_zero.1.jsoncontracts/profile/test_snapshots/tests/reputation/bump_accepts_u32_max_delta_without_overflow.1.jsoncontracts/profile/test_snapshots/tests/reputation/bump_accumulates_across_calls.1.jsoncontracts/profile/test_snapshots/tests/reputation/bump_happy_path_increments_reputation.1.jsoncontracts/profile/test_snapshots/tests/reputation/bump_is_idempotent_on_replay.1.jsoncontracts/profile/test_snapshots/tests/reputation/bump_rejects_caller_without_events_contract_auth.1.jsoncontracts/profile/test_snapshots/tests/reputation/bump_reverts_when_events_contract_not_configured.1.jsoncontracts/profile/test_snapshots/tests/reputation/bump_reverts_when_paused.1.jsoncontracts/profile/test_snapshots/tests/reputation/bump_reverts_when_profile_not_found.1.jsoncontracts/profile/test_snapshots/tests/reputation/bump_zero_delta_is_noop_but_marks_seen.1.jsoncontracts/profile/test_snapshots/tests/reputation/slash_happy_path_decrements_reputation.1.jsoncontracts/profile/test_snapshots/tests/reputation/slash_is_idempotent_on_replay.1.jsoncontracts/profile/test_snapshots/tests/reputation/slash_rejects_caller_without_events_contract_auth.1.jsoncontracts/profile/test_snapshots/tests/reputation/slash_reverts_when_events_contract_not_configured.1.jsoncontracts/profile/test_snapshots/tests/reputation/slash_reverts_when_paused.1.jsoncontracts/profile/test_snapshots/tests/reputation/slash_reverts_when_profile_not_found.1.jsoncontracts/profile/test_snapshots/tests/reputation/slash_saturates_at_zero.1.jsoncontracts/profile/test_snapshots/tests/reputation/slash_zero_delta_is_noop.1.json
Comment on lines
+181
to
+191
| fn bump_rejects_caller_without_events_contract_auth() { | ||
| // Genuine auth rejection: the events contract is configured, but no | ||
| // authorization is provided for the bump call, so events.require_auth() | ||
| // fails and the host aborts the invocation. | ||
| let (ctx, user) = setup_with_user(); | ||
|
|
||
| ctx.env.mock_auths(&[]); | ||
| let res = ctx | ||
| .client | ||
| .try_bump_reputation(&user, &1, &reason(&ctx), &op_id(&ctx)); | ||
| assert!(res.is_err(), "unauthorized bump must be rejected"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
Assert specific auth-failure error variants, not just is_err().
The three auth-rejection tests (lines 181-191, 301-308, 409-419) only verify that a call fails; they do not verify it failed for the intended auth guard. This risks false positives: if any earlier guard (like require_not_paused() or a missing profile) returns an error, the test still passes.
Per the PR objectives and the pattern used throughout the rest of this test file, assert the specific Error variant returned when authorization fails (likely Error::Unauthorized or Error::NotAdmin depending on the guard).
Also applies to: 301-308, 409-419
🤖 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/profile/src/tests/reputation.rs` around lines 181 - 191, In the
function bump_rejects_caller_without_events_contract_auth, replace the generic
error check assertion res.is_err() with a specific assertion that verifies the
error is the expected authorization failure variant (likely Error::Unauthorized
or Error::NotAdmin depending on what guard should trigger the failure). Apply
the same fix to the other two authorization rejection tests that use similar
patterns, ensuring each test verifies the specific error variant returned rather
than just confirming that some error occurred, which prevents false positives
from unrelated failures masking the test intent.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of the test-coverage push (#25). This adds the reputation tests for the profile contract that #29 asks for.
What's here
tests/reputation.rscovering all three functions in the reputation module, built on the existingcommon.rsharness and following theadmin.rstemplate:No production code changed — tests plus their snapshots only.
Tests
cargo test -p boundless-profile→ 24 new reputation tests pass (13 existing admin tests filtered out below):cargo test --all→ green across the workspace (76 events + 37 profile, 0 failed).Proof
Note on snapshots
This PR intentionally only includes the new
test_snapshots/tests/reputation/files. Re-running the suite locally also rewrites a pile of unrelated events/profile snapshots (random wasm-hash/address bytes shift between machines on the same pinned SDK), but that drift isn't part of this change so I left it out.Closes #29
Summary by CodeRabbit