assertEquals(set,set) now requires same ordering which it didn't before

[Elisedlund-ericsson]

In TestNG 7.4.0(and all previous versions?) the following example worked fined and both sets where considered equal.

        HashSet<Integer> set1 = new LinkedHashSet<>();
        HashSet<Integer> set2 = new LinkedHashSet<>(); //same values different order.
        set1.add(1);
        set1.add(2);
        set2.add(2);
        set2.add(1);
        Assert.assertEquals(set1, set2);

the AbstractSet.equals() implementation in Java and used by most Set implementations define equality as the two sets containing the same elements but not necessarily in the same order.

on the latest master of TestNG however the above example no longer works and throws:
"java.lang.AssertionError: Iterators not same element order: expected: 1 and actual: 2"

the line causing this issue is:
https://github.com/cbeust/testng/blob/cd85bcb7afebbaecd2299cbc72632e27f89a441b/testng-asserts/src/main/java/org/testng/Assert.java#L1459
which now iterates both sets and require the same ordering.

as Objects.equals(actual, expected) is already know to be true it would have made more sense to just return true instead. (so it functions as before.
It also makes more sense for TestNGs assertEquals(set,set) to align with Java's definition of set.equals(set) as it was before.

also even the comment in the same methods implies that you don't want to check ordering:
https://github.com/cbeust/testng/blob/cd85bcb7afebbaecd2299cbc72632e27f89a441b/testng-asserts/src/main/java/org/testng/Assert.java#L1447

[Elisedlund-ericsson]

another reason why this behavior change is weird is because there are already another method that does a element order comparison:
Assert.assertEqualsDeep(set1, set2, message);

having to differently named methods doing the same thing is ofcourse very confusing.

[krmahadevan]

@Elisedlund-ericsson - One of the reasons behind this change is attributed to #2540

Please take a look at that. The behaviour was changed in the master because of the afore-mentioned defect.

@juherr - Suggestions ?

[Elisedlund-ericsson]

1. ) 7.3:

  assertEquals(Collection c1, Collection c2) // DO check order
  assertEquals(Set c1, Set c2) //NOT check order

2. ) in 7.4.0? : you changed to;

  assertEquals(Collection c1, Collection c2) //NOT check order (technically the specific equals() decide if order is important)
  assertEquals(Set c1, Set c2) //NOT check order

which cases issue #2540 thanks to behavior change as it alters test case results.

3. ) in master: you changed to;

  assertEquals(Collection c1, Collection c2) //DO check order
  assertEquals(Set c1, Set c2) //DO check order

causing a regression again but the other way around.
This is causing multiple test cases for us to fail as they the set are not in the same order, I guess this is likely to cause the same issue as #2540 for OpenJDK also.

Personally I think the change nr 2.) you did to use Objects.equals() for Collections is actually is actually to most logical
as the method is called assertEquals were I guess the objects own equals definition is the best or most correct one instead of inventing your own TestNG specific definition of object equality, consider that:
This would mean that assertEquals is more or less a shorthand for assertTrue(c1.equals(c2))

Assert.assertTrue(c1.equals(c2)) //works fine
Assert.assertEquals(c1, c2); //fails because of order difference, (Set are unordered!) 

javadoc of Set.iterator:

    /**
     * Returns an iterator over the elements in this set.  The elements are
     * returned in no particular order (unless this set is an instance of some
     * class that provides a guarantee).
     *
     * @return an iterator over the elements in this set
     */
    Iterator<E> iterator();

even meaning that the same set instance could technically give different order every time.
Assert.assertEquals(set1, set); //would pass however thanks to a == check.

Going for nr 2.) which I think is the most logical solution unfortunately it causes regression issues OpenJDK (and forsure others) meaning that the best solution is probably to stick with the original behavior 1.)
replacing one regression issue with another 2.) -> 3.) does not make sense to me at all.

#2460

[krmahadevan]

@Elisedlund-ericsson would you be willing to propose a fix for this via a pull request ?
We can help get it merged

[Elisedlund-ericsson]

sorry for the delay.

there should now exist an pullrequest fixing this issue,
#2648