assertThat(collection).doesNotHaveDuplicates() is not sorted, even if the collection is

[Bananeweizen]

assertThat(collection).doesNotHaveDuplicates() is not sorted, even if the collection is

[Bananeweizen]

sure

[Bananeweizen]

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.
As I said, I would be fine with either variant.

[ranjitshinde91]

@Bananeweizen can I take this for my first contribution, would help me in getting familiar with the PR process

[joel-costigliola]

@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

[joel-costigliola]

Go for it @ranjitshinde91, thanks!

[joel-costigliola]

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?

[Bananeweizen]

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.

[joel-costigliola]

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.

[ranjitshinde91]

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

[kulgan]

@Bananeweizen quick question regarding the expected ordering. Given the following input which is found in the test ["Merry", "Frodo", null, null, "Merry", "Sam", "Frodo"], there are two possible ways to return the duplicates in order:

1. ["Merry", "Frodo", null]
2. [null, "Merry", "Frodo"]
   Option 1 starts with the first elements that is a duplicate, while the second starts with the first duplicated element. I think they are both valid in their own way, like for debugging purpose, option 2 tells me null is the first duplicated element, which I think is helpful, while option one seems like the natural way.
   I know @ranjitshinde91 implementation is based on 1. above, checking if option 2 can be considered.

[ranjitshinde91]

I agree with @Bananeweizen when I started implementation i considered second Merry to be wrong and added in duplicates.
Also 2nd Implementation is simpler and does not require second traversal of Interable done in private method.

I will make the necessary changes to switch to option 2, if that's okay ?

[ranjitshinde91]

updated the same PR with these changes