Skip to content

serviceability-cli: prune User test literals to only asserted fields - #4235

Open
ayushsingh82 wants to merge 1 commit into
malbeclabs:mainfrom
ayushsingh82:fix/prune-user-test-literals
Open

serviceability-cli: prune User test literals to only asserted fields#4235
ayushsingh82 wants to merge 1 commit into
malbeclabs:mainfrom
ayushsingh82:fix/prune-user-test-literals

Conversation

@ayushsingh82

Copy link
Copy Markdown
Contributor

Progress on #4084

Summary of Changes

  • test_cli_user_delete, test_cli_user_request_ban, and test_cli_user_update each build a full 22-field User literal only to hand it back through the mocked get_user. None of the three tests assert on any field of that value — execute() only cares that get_user returns a user so the command can proceed to delete_user/update_user/request_ban_user.
  • Replaced each with User::default() (the manual impl Default for User added in serviceability: change user feed_pk to vec of feed_pks #4080), with a comment noting why no field is load-bearing.

This is a first, narrow slice of #4084 — scoped to smartcontract/cli/src/user/*. The issue also names smartcontract/sdk/rs and crates/doublezero-daemon-cli, which have their own User literals (some with fields that likely are asserted on and need per-test judgment, per the issue's caveat about User::default()'s non-neutral values). Left those for follow-up PRs rather than one large mechanical sweep, per the issue's own guidance.

Diff Breakdown

Category Files Lines (+/-) Net
Tests 3 +14 / -80 -66
Total 3 +14 / -80 -66

Pure test cleanup — no production code touched.

Testing Verification

  • cargo test --lib user:: in smartcontract/cli: all 29 user-module tests pass, including the 3 changed.
  • cargo clippy --lib -- -Dclippy::all -D warnings: clean.
  • Manually verified each of the 3 tests never reads a field of the user value it constructs — only its presence (via get_user returning Ok) drives the command under test.

test_cli_user_{delete,request_ban,update} construct a User only to
hand it back through the mocked get_user; execute() never reads any
of its fields, so the 22-field literal was pure noise. Replaced with
User::default(), letting the manual Default impl (malbeclabs#4080) do what it
was added for.

First slice of malbeclabs#4084 (cli/src/user/*); the sdk/rs and
doublezero-daemon-cli literals it also names are left for follow-up
PRs, per the issue's own file-by-file guidance.
@ayushsingh82
ayushsingh82 requested a review from a team August 26, 2026 16:45
@ayushsingh82

Copy link
Copy Markdown
Contributor Author

@nikw9944 PR is up: prunes the User test literals in smartcontract/cli/src/user/* down to only the fields each test asserts on, per #4084. Ready for CI.

@ayushsingh82

Copy link
Copy Markdown
Contributor Author

The only red check here is changelog / check-changelog; all 30 others pass. This PR is a test-only cleanup (pruning User literals in smartcontract/cli/src/user/* tests down to the asserted fields), no behavior change, so it seems like a skip-changelog case in line with #4094 / #4089. @nikw9944 could you add the label? Happy to add a CHANGELOG line instead if you'd prefer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant