Skip to content

test(locked_outpoints): add unit tests for ChangeSet merge and is_empty#436

Open
KrishnaShuk wants to merge 2 commits intobitcoindevkit:masterfrom
KrishnaShuk:test/locked-outpoints-changeset
Open

test(locked_outpoints): add unit tests for ChangeSet merge and is_empty#436
KrishnaShuk wants to merge 2 commits intobitcoindevkit:masterfrom
KrishnaShuk:test/locked-outpoints-changeset

Conversation

@KrishnaShuk
Copy link
Copy Markdown

@KrishnaShuk KrishnaShuk commented Apr 8, 2026

Description

This PR adds unit test coverage for locked_outpoints::ChangeSet, which previously had zero inline tests. It tests the Merge trait implementations, ensuring that merge and is_empty behave correctly under various conditions.

Notes to the reviewers

  • Added 6 deterministic tests verifying:
    • Default initialization is empty
    • Merging with empty datasets are clean no-ops
    • Disjoint changeset merges successfully combine outpoints
    • Overwriting logic accurately fulfills the extend() semantics where the incoming other duplicate outpoints overwrite self
  • Uses idiomatic boolean assertions (assert!(...)) verified cleanly by cargo clippy.
  • Tests use Txid::from_byte_array internally to maintain deterministic speed without relying on rand.

Changelog notice

Checklists

All Submissions:

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@ValuedMammal ValuedMammal added the tests New or improved tests label Apr 8, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 8, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.68%. Comparing base (826b19f) to head (54e43c1).
⚠️ Report is 37 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #436      +/-   ##
==========================================
+ Coverage   80.04%   80.68%   +0.64%     
==========================================
  Files          24       24              
  Lines        5336     5504     +168     
  Branches      242      244       +2     
==========================================
+ Hits         4271     4441     +170     
+ Misses        987      984       -3     
- Partials       78       79       +1     
Flag Coverage Δ
rust 80.68% <100.00%> (+0.64%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown

@lambaaryan011 lambaaryan011 left a comment

Choose a reason for hiding this comment

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

Issues Found

  1. Missing Test for Multiple Merges

    • The code only tests 2 merges (true → false)
    • Should test 3+ merges (true → false → true → false)
    • Example: What if merge happens 10 times?
  2. Missing Implementation Code

    • Tests check merge() but we can't see merge() code
    • Need to see the actual implementation
    • Can't verify tests are correct without it
  3. Unclear Checklist

    • "breaks the existing API" is confusing
    • Does it break or not? Need clarity

Good Things

  • Tests are written correctly
  • Uses assert!() properly
  • Deterministic (no random data)

@luisschwab
Copy link
Copy Markdown
Member

@lambaaryan011 you only waste everyone's time with this LLM review.

@lambaaryan011
Copy link
Copy Markdown

@luisschwab That’s fair — my earlier comment wasn’t very useful.

I’ll take a closer look at the implementation and follow up with more concrete feedback.

@ValuedMammal
Copy link
Copy Markdown
Collaborator

I would like to see more Merge tests like this for the wallet::ChangeSet type.

@ValuedMammal
Copy link
Copy Markdown
Collaborator

@KrishnaShuk GitHub is complaining that your signing key is not verified.

@KrishnaShuk
Copy link
Copy Markdown
Author

Sure, I'll add wallet::ChangeSet merge tests as a follow-up commit in this PR.

@KrishnaShuk GitHub is complaining that your signing key is not verified.

I've_ added my GPG key to GitHub. The commit is now showing as verified now.

@ValuedMammal ValuedMammal moved this to In Progress in BDK Wallet May 6, 2026
@ValuedMammal ValuedMammal added this to the Wallet 3.1.0 milestone May 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tests New or improved tests

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

4 participants