Skip to content

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

Open
KrishnaShuk wants to merge 1 commit 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 1 commit intobitcoindevkit:masterfrom
KrishnaShuk:test/locked-outpoints-changeset

Conversation

@KrishnaShuk
Copy link
Copy Markdown

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.25%. Comparing base (826b19f) to head (6c15a6c).
⚠️ Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #436      +/-   ##
==========================================
+ Coverage   80.04%   80.25%   +0.21%     
==========================================
  Files          24       24              
  Lines        5336     5393      +57     
  Branches      242      242              
==========================================
+ Hits         4271     4328      +57     
  Misses        987      987              
  Partials       78       78              
Flag Coverage Δ
rust 80.25% <100.00%> (+0.21%) ⬆️

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.

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: No status

Development

Successfully merging this pull request may close these issues.

4 participants