Skip to content

Conversation

pluveto
Copy link
Contributor

@pluveto pluveto commented Nov 21, 2024

Description

This update adds unit tests for the Merge trait implementations on BTreeMap, BTreeSet, Vec, and tuples. It also includes tests for the is_empty and take methods to ensure their correct functionality.

Notes to the Reviewers

  • The tests cover basic scenarios for merging collections and checking their emptiness.
  • Consider the potential method name conflict with BTreeSet's existing take method.

Changelog Notice

  • Added unit tests for merging functionality and utility methods (is_empty, take) on various collection types.

Checklists

All Submissions:

  • I've signed all my commits.
  • I followed the contribution guidelines.
  • I ran cargo fmt and cargo clippy before committing.

Copy link
Contributor

@LagginTimes LagginTimes left a comment

Choose a reason for hiding this comment

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

Would it be valuable to include tests for conflict scenarios as well? For example:

let mut map1: BTreeMap<i32, &str> = BTreeMap::new();
map1.insert(1, "a");
let mut map2: BTreeMap<i32, &str> = BTreeMap::new();
map2.insert(1, "b");

map1.merge(map2);

assert_eq!(map1.len(), 1);
assert_eq!(map1.get(&1), Some(&"b"));

@pluveto
Copy link
Contributor Author

pluveto commented Nov 25, 2024

I've added refactored empty check assertions and merge conflict tests. Squashed.

Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

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

ACK 871df7bb24b6c15a096d6b98bdfc7d6877cbd29c

@LagginTimes
Copy link
Contributor

ACK 665273d

Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

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

Great test additions. Here are some suggestions to make the tests more complete/easier to read.

@pluveto
Copy link
Contributor Author

pluveto commented Dec 1, 2024

Indeed, that's more readable. Updated.

Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

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

Thanks for pushing forward on this. Let me know if you have any questions.

@pluveto
Copy link
Contributor Author

pluveto commented Dec 3, 2024

Fixed and Rebased

Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

cACK a53f07b

@ValuedMammal ValuedMammal added this to the 1.1.0 milestone Jan 3, 2025
Copy link
Collaborator

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

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

ACK a53f07b may need a rebase to kick CI.

In a53f07b:

test: add tests for trait of operations on various data structures, including conflict scenarios and checking for empty structures

It would be better if the commit message stated that we're testing Merge.

@notmandatory
Copy link
Member

Please rebase to pick up CI changes from #1803.

…ncluding conflict scenarios and checking for empty structures
@notmandatory
Copy link
Member

I did the rebase. :-)

@notmandatory notmandatory merged commit 0a11c80 into bitcoindevkit:master Feb 4, 2025
23 checks passed
@ValuedMammal ValuedMammal mentioned this pull request Feb 4, 2025
41 tasks
@pluveto pluveto deleted the test/merge branch February 5, 2025 16:18
@ValuedMammal ValuedMammal mentioned this pull request Apr 30, 2025
39 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants