-
-
Notifications
You must be signed in to change notification settings - Fork 699
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
assertThat(collection).doesNotHaveDuplicates() is not sorted, even if the collection is #3331
Comments
Fair enough, if I understand this correctly the fix should consist of using a linked hash set to store the duplicates. @Bananeweizen do you want to contribute this one? |
Yes, I can do that. I still have to understand if the currently used comparator has some useful meaning for another (unknown to me) aspect. If not, I'd also expect this to be a trivial replacement of the TreeSet by a LinkedHashSet. |
If memory serves, I think we used a treeset to have consistent ordering in our tests, this can be achieved better with a linked hash set though. |
@Bananeweizen can I take this for my first contribution, would help me in getting familiar with the PR process |
sure |
TreeSet with custom comparator is used to compare objects in Set collections. Hence simply changing TreeSet to LinkedHashSet does not work. Added fix as part of this PR #3333 |
@Bananeweizen quick question regarding the expected ordering. Given the following input which is found in the test
|
Quite honestly, I had not even thought about that until you asked. I think I would be fine with both. The question of which one would be more nice for setting a breakpoint afterwards also depends, on whether I as the developer consider the first duplicate wrong (e.g. the first Merry should not have been added to begin with), or the second one. More typical might be to consider the second duplicate wrong, so I can understand that you would eventually prefer option 2. |
@kulgan good point, I agree with @Bananeweizen that there isn't much difference between the two options, so I would go with the simplest implementation on this one |
I agree with @Bananeweizen when I started implementation i considered second Merry to be wrong and added in duplicates. I will make the necessary changes to switch to option 2, if that's okay ? |
Go for it @ranjitshinde91, thanks! |
updated the same PR with these changes |
The error message of
assertThat(collection).doesNotHaveDuplicates()
lists the duplicates in a different order than the elements of the collection actually have. That is not an error per-se, but can make it harder to find the root cause of the failing assertion. So this might also be labeled improvement instead of bug.Test case reproducing the bug
Note that the list gets sorted in the assertion. Still the output is not sorted, because the duplicates are collected in a set which is not insertion order stable.
The text was updated successfully, but these errors were encountered: