Skip to content

test(wallets): make the address-generation assertion genuinely isolate the regression - #922

Merged
lklimek merged 1 commit into
v1.0-devfrom
test/add-receiving-address-fixture-isolation
Jul 22, 2026
Merged

test(wallets): make the address-generation assertion genuinely isolate the regression#922
lklimek merged 1 commit into
v1.0-devfrom
test/add-receiving-address-fixture-isolation

Conversation

@Claudius-Maginificent

@Claudius-Maginificent Claudius-Maginificent commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Why this PR exists

  • Problem: PR test(wallets): assert address-generation status, not just dialog-open state #920's regression test for the "Add Receiving Address" button claimed to catch a specific regression, but didn't.
  • What breaks without it: A code review (round 2 of the grumpy-review on the original fix) proved the claim false: wallet_screen_harness() never registers its fixture wallet with WalletBackend, so load_core_addresses_for_receive's self-heal branch fires the exact same "Generating a new address…" status regardless of whether the button handler's own explicit queue_core_address_request call exists. Removing that explicit call and re-running the test still passed — the assertion added in test(wallets): assert address-generation status, not just dialog-open state #920 to guard against exactly that regression didn't actually guard against it.

What was done

Added registered_wallet_screen_harness(), used only by add_receiving_address_button_opens_receive_dialog: registers the fixture wallet with the backend and waits (bounded 5s timeout) for a genuine non-empty monitored-address snapshot before the click, so load_core_addresses_for_receive takes its non-self-heal branch and the status can only originate from the button handler's own explicit call. The other three dialog tests are untouched, still using the original in-memory fixture — no risk to their coverage.

Also trims an 8-line tombstone comment in the button handler (narrating removed-code history that already lives in the original PR's commit message) down to present-state rationale only.

Testing

  • Acceptance criterion, verified directly: removed the explicit queue_core_address_request call from the button handler — test now fails (FAILED, panics on the missing status assertion). Restored the call — test passes again.
  • cargo test --test kittest --all-features wallets_screen — full module, 7/7 pass by name, no regressions in the other three dialog tests.
  • cargo clippy --all-features --bin dash-evo-tool -- -D warnings and --test kittest -- -D warnings — clean.
  • cargo fmt --all -- --check — clean.

Breaking changes

None.

Checklist

  • Tests added/updated
  • cargo fmt --all
  • Scoped cargo clippy clean
  • User-facing docs (not needed — test-only fix)

Attribution

🤖 Co-authored by Claudius the Magnificent AI Agent

Summary by CodeRabbit

  • Tests
    • Improved automated coverage for the Wallets screen’s “Add Receiving Address” interaction.
    • Added test setup for registered wallets with monitored receiving addresses, ensuring the receive dialog flow is validated in a realistic state.
  • Documentation
    • Clarified the inline explanation of how adding a receiving address mirrors the existing Receive and New Address flow.

No user-facing behavior changes were introduced.

…e the regression

The prior "strengthened" assertion (59d4b38) claimed to catch a dropped
queue_core_address_request call, but the fixture wallet was never
registered with WalletBackend, so load_core_addresses_for_receive's
self-heal branch fired the identical status regardless — the assertion
passed even with the explicit call removed.

Add registered_wallet_screen_harness(), used only by
add_receiving_address_button_opens_receive_dialog: registers the fixture
wallet with the backend and waits (bounded 5s timeout) for a genuine
non-empty monitored-address snapshot before the click, so
load_core_addresses_for_receive takes its non-self-heal branch and the
status can only originate from the button handler's own explicit call.
The other three dialog tests are untouched, still using the original
in-memory fixture.

Verified: removing the explicit queue_core_address_request call now
fails this test; restoring it passes again. Full wallets_screen module
(7 tests), both clippy targets, and fmt all clean.

Also trims an 8-line tombstone comment in the button handler down to
present-state rationale only.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f4decad2-f501-4162-aa8b-1a2b5cae9aab

📥 Commits

Reviewing files that changed from the base of the PR and between 81fddd0 and 11a232c.

📒 Files selected for processing (2)
  • src/ui/wallets/wallets_screen/mod.rs
  • tests/kittest/wallets_screen.rs

📝 Walkthrough

Walkthrough

The change adds a reusable wallet screen harness, introduces a registered-wallet fixture that waits for monitored receive addresses, updates the add-address dialog test to use it, and clarifies the handler’s inline flow comment.

Changes

Wallet screen flow

Layer / File(s) Summary
Registered wallet harness setup
tests/kittest/wallets_screen.rs
Extracts shared harness construction and adds a registered-wallet fixture that waits for monitored receive addresses before rendering the screen.
Add-address interaction validation
src/ui/wallets/wallets_screen/mod.rs, tests/kittest/wallets_screen.rs
Documents the Receive and New Address flow and runs the add-address dialog test with the registered-wallet fixture.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: lklimek

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the test-focused change to better isolate the address-generation regression.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch test/add-receiving-address-fixture-isolation

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@lklimek
lklimek marked this pull request as ready for review July 22, 2026 07:19
@thepastaclaw

thepastaclaw commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator

🕓 Ready for review — 44 ahead in queue (commit 11a232c)
Queue position: 45/72 · 2 reviews active
ETA: start ~06:16 UTC · complete ~06:44 UTC (median 28m across 30 recent reviews; 2 slots)
Queued 12h 18m ago · Last checked: 2026-07-22 19:40 UTC

@lklimek
lklimek merged commit bc477e7 into v1.0-dev Jul 22, 2026
6 checks passed
@lklimek
lklimek deleted the test/add-receiving-address-fixture-isolation branch July 22, 2026 08:14
orchardpaytl pushed a commit to orchardpaytl/orchardpay that referenced this pull request Jul 22, 2026
…e the regression (dashpay#922)

The prior "strengthened" assertion (59d4b38) claimed to catch a dropped
queue_core_address_request call, but the fixture wallet was never
registered with WalletBackend, so load_core_addresses_for_receive's
self-heal branch fired the identical status regardless — the assertion
passed even with the explicit call removed.

Add registered_wallet_screen_harness(), used only by
add_receiving_address_button_opens_receive_dialog: registers the fixture
wallet with the backend and waits (bounded 5s timeout) for a genuine
non-empty monitored-address snapshot before the click, so
load_core_addresses_for_receive takes its non-self-heal branch and the
status can only originate from the button handler's own explicit call.
The other three dialog tests are untouched, still using the original
in-memory fixture.

Verified: removing the explicit queue_core_address_request call now
fails this test; restoring it passes again. Full wallets_screen module
(7 tests), both clippy targets, and fmt all clean.

Also trims an 8-line tombstone comment in the button handler down to
present-state rationale only.

Co-authored-by: Lukasz Klimek <842586+lklimek@users.noreply.github.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
orchardpaytl pushed a commit to orchardpaytl/orchardpay that referenced this pull request Jul 22, 2026
The crate-rename conflict resolution for upstream dashpay#922 (test(wallets):
make the address-generation assertion genuinely isolate the regression)
dropped the WalletOrigin import along with the dash_evo_tool:: duplicate
imports it was merged next to.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.

3 participants