On Thu, 28 Apr 2022 13:17:59 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> Basic but fairly comprehensive set of tests for `IdentityHashMap`. The patch >> in the bug report that breaks `IdentityHashMap` now causes several cases in >> this new test to fail. There's more that could be done, but the new tests >> cover most of the core functions of `IdentityHashMap`. Unfortunately it >> seems difficult to merge this with the existing, comprehensive Collections >> tests (e.g., MOAT.java) because those tests implicity rely on >> `equals()`-based contract instead of the special-purpose `==`-based contract >> used by `IdentityHashMap`. > > test/jdk/java/util/IdentityHashMap/Basic.java line 257: > >> 255: checkEntries(map.entrySet(), entry(k1b, v1b), >> 256: entry(k2, v2)); >> 257: } > > Would an additional check `assertFalse(map.equals(map2));` be useful here > (and other similar tests where we do "remove"). I don't know if you noticed that previous versions checked the map's contents with `map.equals(map2)` and either asserted true or false depending on the situation. I pulled most of those out when I added `checkEntries`. The reason is that `checkEntries` and `checkElements` are supposed to check the exact contents of the map or the collection, and they fail if there is a missing, different, or extra entry or element. If that's true we don't need to test `map.equals`. I don't think it calling `map.equals` adds any value or does any checking that the `checkX` methods don't already do. Of course this relies on `checkEntries` and `checkElements` to do their jobs properly, so we should make sure they do! And also we need to test that the `equals` method is doing its job as well. There are a couple tests for it already, and they test at least the basic cases. But it's possible I might have missed something. In any case, if we believe the `checkX` methods are sufficient, and if we believe that the tests for `equals` are also sufficient, then I don't think we need to add assertions about `equals` in any tests other than for `equals` itself. ------------- PR: https://git.openjdk.java.net/jdk/pull/8354