On Fri, 15 Apr 2022 05:58:32 GMT, liach <d...@openjdk.java.net> wrote:
> Explicitly implement `remove` and `replace` in `IdentityHashMap` to compare > values by identity. Updated API documentation of these two methods > ([Preview](https://cr.openjdk.java.net/~liach/8178355/IdentityHashMap.html#remove(java.lang.Object,java.lang.Object))) > to mention such behavior. test/jdk/java/util/IdentityHashMap/DefaultRemoveReplace.java line 68: > 66: } > 67: } > 68: } Overall comments on this test. It does test the right set of four cases (positive and negative calls to `replace` and `remove`). However, it's **one** test that tests the four cases, instead of four tests. Using the same state makes it hard to add or maintain test cases in the future. It also trusts the return value of the method calls and doesn't make any assertions over the actual contents of the map. Without assertions over the expected contents, a method could behavior incorrectly and cause unrelated and confusing errors downstream, or even cause errors to be missed. I'd thus recommend the following: * Convert this to a Test-NG test and use a `@BeforeTest` method to set up a test fixture consisting of a map containing the desired entries. * Make each test case into its own test method that performs the method call and then makes assertions over the return value and expected result state of the map. Individual test methods is probably fine; no need to use a data provider for this. * Probably add a "null hypothesis" test to ensure that the test fixture itself has the right properties, similar to the assertions at lines 38-44. * Instead of fiddling around with strings, which have additional complexity caused by interning of string literals, I'd suggest using the technique I used in [JDK-8285295](https://bugs.openjdk.java.net/browse/JDK-8285295) and create a `record Box(int i) { }` class. With this it's easy to get multiple instances that are != but are equals. * After further thought, maybe additional cases are necessary. For example, tests of calls to `remove` and `replace` with a key that is equals() but != to a key in the map. ------------- PR: https://git.openjdk.java.net/jdk/pull/8259
