Add some missing tests #392
Conversation
|
@cuviper Hello! I was wondering if there's any feedback on this PR. I've ensured all checks are passing and I'm happy to address any concerns or make changes as needed. Thank you! |
|
@cuviper Thanks for your correction and thanks for your time to review my PR!😊 All feedback has been addressed, and I've marked the relevant comments as resolved. Please let me know if there are any further questions! |
|
I will review again when I get a chance, but please stop pinging so much. You've repeated nearly the same comment now 4 times, which I guess you deleted and reposted. I see no urgency just for test coverage. |
| assert_eq!(result, Some((3, 4))); | ||
| assert_eq!(map.len(), 4); | ||
|
|
||
| map.swap_remove(&5); |
There was a problem hiding this comment.
This swap is a bad choice, because it makes the other shifts irrelevant. That is, I can change all of those other shifts to swap (e.g. as if the shift was implemented incorrectly), and the expected values here still pass!
Just forget this line, and the rest could be more reasonable.
shift_remove_index(1) -> (3, 4)shift_remove_index(1) -> (5, 6)-- because it shifted to fill the gap)shift_remove_index(2) -> (9, 10)-- because the last was never swapped, only shifted down twice, and it also tests that shift-removing the last entry is fineshift_remove_index(2) -> None
| assert_eq!(map.len(), 4); | ||
|
|
||
| map.swap_remove(&5); | ||
| assert_eq!(map.as_slice(), &[(1, 2), (9, 10), (7, 8)]); |
There was a problem hiding this comment.
Similarly here, the swap_remove makes it so we can't tell whether the first one really shifted or swapped. Remove that and you can compare the full shifted order instead.
| assert_eq!(result, Some((1, 3, 4))); | ||
| assert_eq!(map.len(), 4); | ||
|
|
||
| map.swap_remove(&5); |
There was a problem hiding this comment.
Again, don't use swap when testing shifts.
| assert_eq!(slice, &mut [(3, 30), (4, 40)]); | ||
|
|
||
| for i in 0..slice.len() { | ||
| slice.entries[i].value += 1; |
There was a problem hiding this comment.
| slice.entries[i].value += 1; | |
| slice[i] += 1; |
| index_set.insert(10); | ||
| index_set.insert(20); | ||
| index_set.insert(30); | ||
| assert_eq!(index_set.capacity(), 3); |
There was a problem hiding this comment.
I think this is too dependent on the growth strategy and load factor, which are implementation details of both hashbrown and this crate combined, but it would be ok if the test calls shrink_to_fit() first. We still can't be sure indices table won't have higher capacity, but the entries vec will shrink to the exact capacity, and we report the lesser.
The rest of the test is ok for the same reason -- the vec will grow exactly even if the indices table doesn't.
| set.insert(3); | ||
|
|
||
| let drain = set.drain(0..2); | ||
| assert_eq!(drain.as_slice(), &[1, 2]); |
There was a problem hiding this comment.
It would be worth testing the output of the drain iterator, and the state of the set afterward.
| let mut set: IndexSet<i32> = IndexSet::new(); | ||
| set.extend(vec![1, 2, 3, 4, 5, 6, 7, 8, 9, 10]); |
There was a problem hiding this comment.
This doesn't need a vec - just convert a direct array.
| let mut set: IndexSet<i32> = IndexSet::new(); | |
| set.extend(vec![1, 2, 3, 4, 5, 6, 7, 8, 9, 10]); | |
| let mut set: IndexSet<i32> = IndexSet::from([1, 2, 3, 4, 5, 6, 7, 8, 9, 10]); |
This or similar can be applied in a few of the new tests.
| let mut set: IndexSet<i32> = IndexSet::new(); | ||
| set.extend(vec![1, 2, 3, 4, 5, 6, 7, 8, 9, 10]); | ||
| set.truncate(5); |
There was a problem hiding this comment.
| let mut set: IndexSet<i32> = IndexSet::new(); | |
| set.extend(vec![1, 2, 3, 4, 5, 6, 7, 8, 9, 10]); | |
| set.truncate(5); | |
| let mut set: IndexSet<i32> = IndexSet::from([1, 2, 3, 4, 5]); |
| set.truncate(5); | ||
| let split_set: IndexSet<i32> = set.split_off(3); | ||
| assert_eq!(split_set.len(), 2); | ||
| assert_eq!(set.len(), 3); |
There was a problem hiding this comment.
How about checking the contents of each as well.
| set.insert(6); | ||
|
|
||
| set.pop(); |
There was a problem hiding this comment.
Seems irrelevant to insert and immediately pop here.
Hi,
Thanks for your time to review this PR.
By examining the existing code, we found that some tests can be added to improve the repo's overall test coverage. We've added some tests to cover missing unit tests in slice.rs, map.rs and set.rs. The tests we submitted have been carefully curated by us to ensure their behavior and effectiveness.
Thanks again for reviewing.