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

Reply via email to