test(locked_outpoints): add unit tests for ChangeSet merge and is_empty#436
test(locked_outpoints): add unit tests for ChangeSet merge and is_empty#436KrishnaShuk wants to merge 2 commits into
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.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
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
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)
|
@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. |
|
I would like to see more Merge tests like this for the |
|
@KrishnaShuk GitHub is complaining that your signing key is not verified. |
|
Sure, I'll add wallet::ChangeSet merge tests as a follow-up commit in this PR.
I've_ added my GPG key to GitHub. The commit is now showing as verified now. |
|
While we're testing ChangeSet invariants, I think we should look into #470 for this PR as well. |
ValuedMammal
left a comment
ValuedMammal
left a comment
There was a problem hiding this comment.
The debug_assert in ChangeSet::merge only fires in debug mode, meaning reassignment is actually permitted in release builds, which is surprising.
I think the correct merge semantics should be: "If self is_none and other is_some, then proceed with the assignment, else debug_assert other is none or self equals other (to catch bugs during development)."
cargo test -r --lib -- wallet::changeset::test
// Merging other changeset should succeed and ignore other descriptor
#[cfg(not(debug_assertions))]
#[test]
fn test_merge_must_not_change_descriptor() {
use crate::test_utils::*;
use bitcoin::secp256k1::Secp256k1;
let secp = Secp256k1::new();
let (desc_1, _) = Descriptor::parse_descriptor(&secp, get_test_tr_single_sig()).unwrap();
let (desc_2, _) = Descriptor::parse_descriptor(&secp, get_test_wpkh()).unwrap();
let mut changeset = ChangeSet {
descriptor: Some(desc_1.clone()),
..Default::default()
};
let other_descriptor_changeset = ChangeSet {
descriptor: Some(desc_2),
..Default::default()
};
Merge::merge(&mut changeset, other_descriptor_changeset);
assert_eq!(
changeset.descriptor,
Some(desc_1),
"descriptor must never change"
);
}| } | ||
|
|
||
| #[test] | ||
| fn test_is_empty_with_descriptor() { |
There was a problem hiding this comment.
It would be more clear to the reader if the test name reflected the expectation.
| fn test_is_empty_with_descriptor() { | |
| fn test_is_non_empty_with_descriptor() { |
| } | ||
|
|
||
| #[test] | ||
| fn test_is_empty_with_network() { |
There was a problem hiding this comment.
| fn test_is_empty_with_network() { | |
| fn test_is_non_empty_with_network() { |
| } | ||
|
|
||
| #[test] | ||
| fn test_is_empty_with_entries() { |
There was a problem hiding this comment.
| fn test_is_empty_with_entries() { | |
| fn test_is_non_empty_with_entries() { |
There was a problem hiding this comment.
When I saw the test name and read through what the test tests, I was confused. @ValuedMammal made a suggestion in src/wallet/changeset.rs to update the test names so they reflect the expected outcome rather than just the method being called. I think that applies to this too.
5 participants
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: