8285295: Need better testing for IdentityHashMap #8354

stuart-marks · 2022-04-22T03:37:27Z

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.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (1 review required, with at least 1 reviewer)

Issue

JDK-8285295: Need better testing for IdentityHashMap

Reviewers

Jaikiran Pai (@jaikiran - Committer) ⚠️ Review applies to 4bb25edf
Lance Andersen (@LanceAndersen - Reviewer) ⚠️ Review applies to 4bb25edf

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/8354/head:pull/8354
$ git checkout pull/8354

Update a local copy of the PR:
$ git checkout pull/8354
$ git pull https://git.openjdk.java.net/jdk pull/8354/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8354

View PR using the GUI difftool:
$ git pr show -t 8354

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/8354.diff

bridgekeeper · 2022-04-22T03:38:37Z

👋 Welcome back smarks! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2022-04-22T03:40:25Z

@stuart-marks The following label will be automatically applied to this pull request:

core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

liach · 2022-04-23T14:27:46Z

Just curious, will entrySet/keySet/values tests be here or in a separate file? They are identity-based collections as well, as shown by their contains/removeAll/retainAll.

stuart-marks · 2022-04-26T01:23:40Z

Probably the same file. It would be nice to be able to re-use some general Set and Collection tests, but the identity semantics make this difficult to do without extensive refactoring.

**

Oh wait, removeAll and retainAll use the membership contract of the argument, not those of this collection. The current keySet/values/entrySet tests test toArray and contains, which seems almost sufficient. Well, maybe tests for remove on these view collections would be helpful, but I think that's about it.

…v2).

liach · 2022-04-27T03:11:58Z

test/jdk/java/util/IdentityHashMap/Basic.java

+    @SafeVarargs
+    private <E> void checkContents(Collection<E> c, BiPredicate<E,E> p, E... given) {
+        @SuppressWarnings("unchecked")
+        E[] contents = (E[]) c.toArray();


Maybe worthy to note that entry set toArray may return entries different from what the iterator returns, and some predicates may thus fail.

Yeah I could add a note to caution against checking identity of Entry instances. That doesn't occur anywhere in these tests, does it?

liach · 2022-04-27T03:13:21Z

test/jdk/java/util/IdentityHashMap/Basic.java

+        @SuppressWarnings("unchecked")
+        E[] contents = (E[]) c.toArray();
+
+        assertEquals(c.size(), given.length);


I believe testng has the expected values in front in the assertEquals methods, as embodied in the exception messages, so this should be assertEquals(given.length, c.size());. Applies to other places.

No, TestNG is assertEquals(actual, expected) which is irritatingly the opposite of JUnit.

https://github.com/cbeust/testng/blob/master/testng-asserts/src/main/java/org/testng/asserts/Assertion.java#L151

This will make things quite tedious when/if we convert to JUnit.

There’s a reason why I prefer using AssertJ, where the calls are:

Assertions.assertThat(actual) .isEqualTo(expected);

mlbridge · 2022-04-27T03:24:15Z

Webrevs

jaikiran · 2022-04-28T13:14:41Z

test/jdk/java/util/IdentityHashMap/Basic.java

+    @Test
+    public void testKeySetNoRemove() {
+        Set<Box> keySet = map.keySet();
+        keySet.remove(new Box(k1a));


Should we assert here that this returns false?

jaikiran · 2022-04-28T13:15:29Z

test/jdk/java/util/IdentityHashMap/Basic.java

+    @Test
+    public void testKeySetRemove() {
+        Set<Box> keySet = map.keySet();
+        keySet.remove(k1a);


Similarly should we assert true here and few other places in this PR which does the remove?

jaikiran · 2022-04-28T13:17:59Z

test/jdk/java/util/IdentityHashMap/Basic.java

+        checkElements(keySet, k1b, k2);
+        checkEntries(map.entrySet(), entry(k1b, v1b),
+                                     entry(k2, v2));
+    }


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.

Hello Stuart,

Thank you for the explanation.

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.

That makes sense.

jaikiran · 2022-04-28T13:19:41Z

test/jdk/java/util/IdentityHashMap/Basic.java

+    public void testPutNew() {
+        Box newKey = new Box(k1a);
+        Box newVal = new Box(v1a);
+        map.put(newKey, newVal);


Should we capture the return value and assert that it is null?

jaikiran · 2022-04-28T13:22:34Z

test/jdk/java/util/IdentityHashMap/Basic.java

+    @Test
+    public void testPutOverwrite() {
+        Box newVal = new Box(v1a);
+        map.put(k1a, newVal);


Should we capture the return value and assert that it's not null and is identity equal to v1a?

Perhaps, similarly at a few other places where we do put and putIfAbsent?

Yes, good point, I'll add checks of the return values in the appropriate places. There are several, including remove, computeX, put, etc. as you suggest here and in other comments.

stuart-marks · 2022-04-29T03:01:53Z

I've added checking of return values for I think everything that has a significant return value. I've elected to store the return value in a local variable and add a separate assert line. For example, instead of

assertNull(map.put(newKey, newVal));

I've done

Box r = map.put(newKey, newVal);
assertNull(r);

The reason is that I think it separates the test setup/action from the test assertions. I tried it the first way, but it felt like the lack of this separation made things messy.

I've also added a couple more tests over equals and added more asserts over equals to the keySet and entrySet view sets. (Note, the values collection is just a Collection and thus doesn't have a defined notion of equals.) The testing of the view collections could probably be made more comprehensive, but I think what's here is a good start.

Please take a look.

jaikiran · 2022-05-04T14:52:08Z

Hello Stuart, I had a look at the updates. This looks good to me, thank you for the changes. Just a couple of comments/questions that I've included at relevant lines.

jaikiran · 2022-05-04T14:55:25Z

test/jdk/java/util/IdentityHashMap/Basic.java

+// as its logic for testing collections equality is suspect. Use checkContents() to assert
+// that a map's entrySet contains exactly the expected mappings. In most cases, keys and
+// values should be compared by identity, not equality. However, the identities of Map.Entry
+// instances from an IdentityHashSet are not guaranteed; the keys and values they contain


I think I understand what you are saying here, but it took me a few reads to understand it. More so because of IdentityHashSet here, which I think is a typo.
So what's being stated here is that you cannot do something like:

IdentityHashMap m = new IdentityHashMap(); ... var e1 = m.entrySet(); var e2 = m.entrySet(); assertSame(e1, e2); // this isn't guaranteed to pass

Did I understand this right?

Yeah that comment was poorly worded, and indeed I meant the IdentityHashMap's entrySet and not IdentityHashSet. I've reworded it. I was trying to make a statement about exactly what needs to be compared if you want to make assertions about two maps being "equal" in the sense of IdentityHashMap behaving correctly. Specifically, given two maps m1 and m2, clearly we don't want either of

assertSame(m1, m2); assertSame(m1.entrySet(), m2.entrySet());

Instead, we want the Map.Entry instances to "match". However, given two entries entry1 and entry2 that are supposed to match, we also do not want

assertSame(entry1, entry2);

Instead, we want

assertSame(entry1.getKey(), entry2.getKey()); assertSame(entry1.getValue(), entry2,getValue());

The checkEntries method goes to some length to get this right.

jaikiran · 2022-05-04T14:57:19Z

test/jdk/java/util/IdentityHashMap/Basic.java

+        boolean[] called = new boolean[1];
+        Box newKey = new Box(k1a);
+        Box newVal = new Box(v1a);
+        Box r = map.computeIfAbsent(newKey, k -> { called[0] = true; return newVal; });


More of a curiosity than a review comment - I see that various places in this PR use a boolean array with one element instead of just a boolean type. Is that a personal coding preference or is there something more to it?

This just serves as a modifiable boolean like an AtomicBoolean. Remember lambdas can only use final local var references (due to how they work), and it cannot access or modify the local variable in the caller method.

Yes, that's it; you can't mutate a local variable captured by a lambda.

Thank you @liach and Stuart. I had overlooked the lambda aspect of this.

stuart-marks · 2022-05-04T19:15:14Z

I've added a few more assertions to cover equals of entrySet and keySet which I think were missing some cases. (Note that values returns a Collection which has no notion of equality.) I think this is ready now.

jaikiran

Hello Stuart, these changes look good to me.

ExE-Boss · 2022-05-05T13:55:53Z

As I said in GH‑6532:

There should probably be something like test/jdk/java/util/Collections/Wrappers.java to check that IdentityHashMap overrides all default methods from java.util.Map (with remove(K, V) and replace(K, V, V) depending on GH‑8259).

stuart-marks · 2022-05-05T23:07:52Z

Thanks Jaikiran. Could I have a Reviewer take a look at this please?

stuart-marks · 2022-05-05T23:46:24Z

There should probably be something like test/jdk/java/util/Collections/Wrappers.java

Maybe. The intent of the test is fine, which is to ensure that a default method doesn't get added that breaks the invariants of the wrapper. One problem is that it tests only the default methods of Collection and not of the other collections interfaces. Another is that "override all default methods" isn't exactly the right criterion; instead, the criterion should be "override all default methods that would otherwise break this collection's invariants." It would be nice if such a test could be written, but as it stands I think that Wrappers.java test is too simplistic.

XenoAmess · 2022-05-06T06:39:59Z

It would be nice if such a test could be written, but as it stands I think that Wrappers.java test is too simplistic.

Would adding Wrappers.java a method-name white-list mechanism suitable in this situation?

ExE-Boss · 2022-05-06T16:00:37Z

It would be nice if such a test could be written, but as it stands I think that Wrappers.java test is too simplistic.

Would adding Wrappers.java a method-name white-list mechanism suitable in this situation?

It should really be a method name and type whitelist.

LanceAndersen

I think you have done a nice job on the coverage.

It would be nice for future maintainers if you consider adding comments for all of the tests vs. a subset prior to pushing

openjdk · 2022-05-06T17:00:32Z

@stuart-marks This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8285295: Need better testing for IdentityHashMap

Reviewed-by: jpai, lancea

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 269 new commits pushed to the master branch:

080f3c5: 8286190: Add test to verify constant folding for Enum fields
1277f5d: 8286154: Fix 3rd party notices in test files
2dd4dfd: 8286291: G1: Remove unused segment allocator printouts
b9f4370: 8286189: G1: Change "wasted" memory to "unused" memory in reporting
c6eab98: 8285378: Remove unnecessary nop for C1 exception and deopt handler
3cdedf1: 8248863: Add search landing page to API documentation
fa1ca98: 8281429: PhiNode::Value() is too conservative for tripcount of CountedLoop
dd06cc6: 8283807: Handle CompileThreshold the same as other thresholds when scaled with -XX:CompileThresholdScaling
015cfda: 8262004: Classpath separator: Man page says semicolon; should be colon on Linux
9425ab2: 8286153: Remove redundant casts and other cleanup
... and 259 more: https://git.openjdk.java.net/jdk/compare/87faa85c59e94d66c3c61d997eacdd2dbe5a1772...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

stuart-marks · 2022-05-06T18:40:25Z

Thanks @LanceAndersen I've added some comments.

stuart-marks · 2022-05-06T18:40:54Z

/integrate

openjdk · 2022-05-06T18:41:48Z

Going to push as commit 5a1d8f7.
Since your change was applied there have been 269 commits pushed to the master branch:

080f3c5: 8286190: Add test to verify constant folding for Enum fields
1277f5d: 8286154: Fix 3rd party notices in test files
2dd4dfd: 8286291: G1: Remove unused segment allocator printouts
b9f4370: 8286189: G1: Change "wasted" memory to "unused" memory in reporting
c6eab98: 8285378: Remove unnecessary nop for C1 exception and deopt handler
3cdedf1: 8248863: Add search landing page to API documentation
fa1ca98: 8281429: PhiNode::Value() is too conservative for tripcount of CountedLoop
dd06cc6: 8283807: Handle CompileThreshold the same as other thresholds when scaled with -XX:CompileThresholdScaling
015cfda: 8262004: Classpath separator: Man page says semicolon; should be colon on Linux
9425ab2: 8286153: Remove redundant casts and other cleanup
... and 259 more: https://git.openjdk.java.net/jdk/compare/87faa85c59e94d66c3c61d997eacdd2dbe5a1772...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-05-06T18:42:08Z

@stuart-marks Pushed as commit 5a1d8f7.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Initial cut at IdentityHashMap tests.

9db8f71

openjdk bot added the core-libs core-libs-dev@openjdk.org label Apr 22, 2022

Some cleanup and more tests.

8900dec

stuart-marks added 4 commits April 26, 2022 17:06

Finish writing all pending tests except remove(k,v) and replace(k,v1,…

c2ff7d8

…v2).

Rename isIdenticalEntry to hasIdenticalKeyValue.

efc09bc

Rename some tests.

ce060d7

Refactor contents checking to use checkElements() and checkEntries().

5857fe6

stuart-marks changed the title ~~Initial cut at IdentityHashMap tests.~~ 8285295: Need better testing for IdentityHashMap Apr 27, 2022

liach reviewed Apr 27, 2022

View reviewed changes

stuart-marks marked this pull request as ready for review April 27, 2022 03:18

openjdk bot added the rfr Pull request is ready for review label Apr 27, 2022

Add comment about Map.Entry identity not guaranteed.

3f802a5

jaikiran reviewed Apr 28, 2022

View reviewed changes

Assertions over return values. Some refinement of equals() testing.

d66ad0b

jaikiran reviewed May 4, 2022

View reviewed changes

stuart-marks added 2 commits May 4, 2022 11:38

Improve wording of comment.

bf4af51

Add some assertions for entrySet.equals and keySet.equals

4bb25ed

jaikiran approved these changes May 5, 2022

View reviewed changes

LanceAndersen approved these changes May 6, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 6, 2022

Add comments on tests that were missing them.

fb877d9

openjdk bot added the integrated Pull request has been integrated label May 6, 2022

openjdk bot closed this May 6, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 6, 2022

8285295: Need better testing for IdentityHashMap #8354

8285295: Need better testing for IdentityHashMap #8354

Conversation

stuart-marks commented Apr 22, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Apr 22, 2022

openjdk bot commented Apr 22, 2022

liach commented Apr 23, 2022

stuart-marks commented Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlbridge bot commented Apr 27, 2022 • edited Loading

Webrevs

Choose a reason for hiding this comment

jaikiran Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaikiran Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

stuart-marks Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

stuart-marks commented Apr 29, 2022

jaikiran commented May 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaikiran May 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuart-marks commented May 4, 2022

jaikiran left a comment

Choose a reason for hiding this comment

ExE-Boss commented May 5, 2022

stuart-marks commented May 5, 2022

stuart-marks commented May 5, 2022

XenoAmess commented May 6, 2022

ExE-Boss commented May 6, 2022 • edited Loading

LanceAndersen left a comment

Choose a reason for hiding this comment

openjdk bot commented May 6, 2022 • edited Loading

stuart-marks commented May 6, 2022

stuart-marks commented May 6, 2022

openjdk bot commented May 6, 2022

openjdk bot commented May 6, 2022

stuart-marks commented Apr 22, 2022 •

edited by openjdk bot

Loading

stuart-marks commented Apr 26, 2022 •

edited

Loading

mlbridge bot commented Apr 27, 2022 •

edited

Loading

jaikiran Apr 28, 2022 •

edited

Loading

jaikiran Apr 28, 2022 •

edited

Loading

stuart-marks Apr 28, 2022 •

edited

Loading

jaikiran commented May 4, 2022 •

edited

Loading

jaikiran May 4, 2022 •

edited

Loading

ExE-Boss commented May 6, 2022 •

edited

Loading

openjdk bot commented May 6, 2022 •

edited

Loading