Skip to content

Conversation

@UdjinM6
Copy link

@UdjinM6 UdjinM6 commented Nov 29, 2025

Issue being fixed or feature implemented

qt tests fail sometimes: https://github.com/UdjinM6/dash/actions/runs/19789400004/job/56700920992#step:10:203

The reason is io is a valid set of base58 characters (ybEBNswQyMLQPjMXN6ytuRetioxS4CrAu2 is a valid address for example), so one of preexisting addresses can match the filter and row count check will fail.

What was done?

Use non-base58 character io -> i0

How Has This Been Tested?

Breaking Changes

n/a

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

@UdjinM6 UdjinM6 added this to the 23.1 milestone Nov 29, 2025
@github-actions
Copy link

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

@coderabbitai
Copy link

coderabbitai bot commented Nov 29, 2025

Walkthrough

This pull request updates test data in the address book test file by replacing literal string values and search patterns. Test labels are changed from containing "io" to "i0" (e.g., "io - new A" becomes "i0 - new A"), and search queries are similarly updated from "io" to "i0" while preserving their wildcard patterns. Test assertions and verification logic remain unmodified.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

This change involves straightforward, repetitive literal string replacements in test data within a single file. No logic modifications, control flow changes, or functional behavior updates are present.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: fixing an intermittent qt test failure by replacing invalid base58 characters in test filters.
Description check ✅ Passed The description is directly related to the changeset, clearly explaining the issue (io is valid base58), the solution (use i0 instead), and why this matters for the tests.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 199ac79 and 262ea3a.

📒 Files selected for processing (1)
  • src/qt/test/addressbooktests.cpp (2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{cpp,h,hpp,cc}

📄 CodeRabbit inference engine (CLAUDE.md)

Dash Core implementation must be written in C++20, requiring at least Clang 16 or GCC 11.1

Files:

  • src/qt/test/addressbooktests.cpp
src/qt/**/*.{cpp,h}

📄 CodeRabbit inference engine (CLAUDE.md)

GUI implementation in src/qt/ must use Qt 5

Files:

  • src/qt/test/addressbooktests.cpp
🧠 Learnings (1)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
⏰ 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). (7)
  • GitHub Check: linux64_tsan-build / Build source
  • GitHub Check: Lint / Run linters
  • GitHub Check: x86_64-w64-mingw32 / Build depends
  • GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
  • GitHub Check: x86_64-apple-darwin / Build depends
  • GitHub Check: x86_64-pc-linux-gnu / Build depends
  • GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (1)
src/qt/test/addressbooktests.cpp (1)

160-160: LGTM! Correct fix for intermittent test failure.

The change from "io" to "i0" is correct because '0' (zero) is not part of the Base58 alphabet, ensuring this label will never accidentally match valid Base58 addresses in the address book.


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 and usage tips.

Copy link
Collaborator

@kwvg kwvg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 262ea3a

Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 262ea3a

@PastaPastaPasta PastaPastaPasta merged commit 93d8697 into dashpay:develop Dec 2, 2025
57 of 60 checks passed
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