test(locked_outpoints): add unit tests for ChangeSet merge and is_empty#436
Open
KrishnaShuk wants to merge 1 commit intobitcoindevkit:masterfrom
Open
test(locked_outpoints): add unit tests for ChangeSet merge and is_empty#436KrishnaShuk wants to merge 1 commit intobitcoindevkit:masterfrom
KrishnaShuk wants to merge 1 commit intobitcoindevkit:masterfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lambaaryan011
left a comment
There was a problem hiding this comment.
Issues Found
-
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?
-
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
-
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)
Member
|
@lambaaryan011 you only waste everyone's time with this LLM review. |
|
@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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR adds unit test coverage for
locked_outpoints::ChangeSet, which previously had zero inline tests. It tests theMergetrait implementations, ensuring that merge() and is_empty() behave correctly under various conditions.Notes to the reviewers
extend()semantics where the incomingotherduplicate outpoints overwriteselfassert!(...)) verified cleanly bycargo clippy.Txid::from_byte_arrayinternally to maintain deterministic speed without relying onrand.Changelog notice
Checklists
All Submissions:
just pbefore pushingNew Features:
Bugfixes: