Re: RFR: 8285295: Need better testing for IdentityHashMap [v5]
> 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`. Stuart Marks has updated the pull request incrementally with one additional commit since the last revision: Add comments on tests that were missing them. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8354/files - new: https://git.openjdk.java.net/jdk/pull/8354/files/4bb25edf..fb877d93 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8354&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8354&range=03-04 Stats: 19 lines in 1 file changed: 19 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8354.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8354/head:pull/8354 PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v4]
On Fri, 6 May 2022 16:59:16 GMT, Lance Andersen wrote: >> Stuart Marks has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Add some assertions for entrySet.equals and keySet.equals > > 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 Thanks @LanceAndersen I've added some comments. - PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v4]
On Wed, 4 May 2022 19:16:14 GMT, Stuart Marks 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`. > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > Add some assertions for entrySet.equals and keySet.equals 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 - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v4]
On Fri, 6 May 2022 06:39:59 GMT, XenoAmess wrote: > > 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. - PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v4]
On Thu, 5 May 2022 23:46:24 GMT, Stuart Marks wrote: > 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? - PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v4]
On Wed, 4 May 2022 19:16:14 GMT, Stuart Marks 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`. > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > Add some assertions for entrySet.equals and keySet.equals > There should probably be something like > [test/jdk/java/util/Collections/Wrappers.java](https://github.com/openjdk/jdk/blob/master/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. - PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v4]
On Wed, 4 May 2022 19:16:14 GMT, Stuart Marks 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`. > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > Add some assertions for entrySet.equals and keySet.equals Thanks Jaikiran. Could I have a Reviewer take a look at this please? - PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v4]
On Wed, 4 May 2022 19:16:14 GMT, Stuart Marks 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`. > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > Add some assertions for entrySet.equals and keySet.equals 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]). [GH‑6532]: https://github.com/openjdk/jdk/pull/6532#issuecomment-1104709021 [GH‑8259]: https://github.com/openjdk/jdk/pull/8259 [test/jdk/java/util/Collections/Wrappers.java]: https://github.com/openjdk/jdk/blob/master/test/jdk/java/util/Collections/Wrappers.java - PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v2]
On Wed, 4 May 2022 15:02:43 GMT, liach wrote:
>> test/jdk/java/util/IdentityHashMap/Basic.java line 500:
>>
>>> 498: Box newKey = new Box(k1a);
>>> 499: Box newVal = new Box(v1a);
>>> 500: 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.
Thank you @liach and Stuart. I had overlooked the lambda aspect of this.
-
PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v4]
On Wed, 4 May 2022 19:16:14 GMT, Stuart Marks 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`. > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > Add some assertions for entrySet.equals and keySet.equals Hello Stuart, these changes look good to me. - Marked as reviewed by jpai (Committer). PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v3]
On Wed, 4 May 2022 18:46:20 GMT, Stuart Marks 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`. > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > Improve wording of comment. 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. - PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v4]
> 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`. Stuart Marks has updated the pull request incrementally with one additional commit since the last revision: Add some assertions for entrySet.equals and keySet.equals - Changes: - all: https://git.openjdk.java.net/jdk/pull/8354/files - new: https://git.openjdk.java.net/jdk/pull/8354/files/bf4af51f..4bb25edf Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8354&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8354&range=02-03 Stats: 32 lines in 1 file changed: 32 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8354.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8354/head:pull/8354 PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v2]
On Wed, 4 May 2022 14:55:25 GMT, Jaikiran Pai wrote: >> Stuart Marks has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Assertions over return values. Some refinement of equals() testing. >> - Add comment about Map.Entry identity not guaranteed. > > test/jdk/java/util/IdentityHashMap/Basic.java line 50: > >> 48: // that a map's entrySet contains exactly the expected mappings. In most >> cases, keys and >> 49: // values should be compared by identity, not equality. However, the >> identities of Map.Entry >> 50: // 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. - PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v2]
On Wed, 4 May 2022 15:02:43 GMT, liach wrote:
>> test/jdk/java/util/IdentityHashMap/Basic.java line 500:
>>
>>> 498: Box newKey = new Box(k1a);
>>> 499: Box newVal = new Box(v1a);
>>> 500: 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.
-
PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v3]
> 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`. Stuart Marks has updated the pull request incrementally with one additional commit since the last revision: Improve wording of comment. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8354/files - new: https://git.openjdk.java.net/jdk/pull/8354/files/d66ad0b8..bf4af51f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8354&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8354&range=01-02 Stats: 6 lines in 1 file changed: 0 ins; 1 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/8354.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8354/head:pull/8354 PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v2]
On Fri, 29 Apr 2022 03:00:40 GMT, Stuart Marks 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`.
>
> Stuart Marks has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Assertions over return values. Some refinement of equals() testing.
> - Add comment about Map.Entry identity not guaranteed.
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.
test/jdk/java/util/IdentityHashMap/Basic.java line 50:
> 48: // that a map's entrySet contains exactly the expected mappings. In most
> cases, keys and
> 49: // values should be compared by identity, not equality. However, the
> identities of Map.Entry
> 50: // 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?
test/jdk/java/util/IdentityHashMap/Basic.java line 500:
> 498: Box newKey = new Box(k1a);
> 499: Box newVal = new Box(v1a);
> 500: 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?
-
PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v2]
On Wed, 4 May 2022 14:57:19 GMT, Jaikiran Pai wrote:
>> Stuart Marks has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Assertions over return values. Some refinement of equals() testing.
>> - Add comment about Map.Entry identity not guaranteed.
>
> test/jdk/java/util/IdentityHashMap/Basic.java line 500:
>
>> 498: Box newKey = new Box(k1a);
>> 499: Box newVal = new Box(v1a);
>> 500: 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.
-
PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v2]
On Fri, 29 Apr 2022 03:00:40 GMT, Stuart Marks 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`. > > Stuart Marks has updated the pull request incrementally with two additional > commits since the last revision: > > - Assertions over return values. Some refinement of equals() testing. > - Add comment about Map.Entry identity not guaranteed. 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. - PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v2]
> 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`. Stuart Marks has updated the pull request incrementally with two additional commits since the last revision: - Assertions over return values. Some refinement of equals() testing. - Add comment about Map.Entry identity not guaranteed. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8354/files - new: https://git.openjdk.java.net/jdk/pull/8354/files/5857fe6f..d66ad0b8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8354&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8354&range=00-01 Stats: 117 lines in 1 file changed: 84 ins; 2 del; 31 mod Patch: https://git.openjdk.java.net/jdk/pull/8354.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8354/head:pull/8354 PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap
On Fri, 29 Apr 2022 00:06:32 GMT, Stuart Marks wrote: >> 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. 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. - PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap
On Thu, 28 Apr 2022 13:17:59 GMT, Jaikiran Pai 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
Re: RFR: 8285295: Need better testing for IdentityHashMap
On Thu, 28 Apr 2022 13:22:34 GMT, Jaikiran Pai 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 337:
>
>> 335: public void testPutOverwrite() {
>> 336: Box newVal = new Box(v1a);
>> 337: 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.
-
PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap
On Fri, 22 Apr 2022 03:37:27 GMT, Stuart Marks 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 337:
> 335: public void testPutOverwrite() {
> 336: Box newVal = new Box(v1a);
> 337: 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?
-
PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap
On Fri, 22 Apr 2022 03:37:27 GMT, Stuart Marks 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 244:
> 242: public void testKeySetNoRemove() {
> 243: Set keySet = map.keySet();
> 244: keySet.remove(new Box(k1a));
Should we assert here that this returns `false`?
test/jdk/java/util/IdentityHashMap/Basic.java line 253:
> 251: public void testKeySetRemove() {
> 252: Set keySet = map.keySet();
> 253: keySet.remove(k1a);
Similarly should we assert `true` here and few other places in this PR which
does the remove?
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").
test/jdk/java/util/IdentityHashMap/Basic.java line 325:
> 323: Box newKey = new Box(k1a);
> 324: Box newVal = new Box(v1a);
> 325: map.put(newKey, newVal);
Should we capture the return value and assert that it is `null`?
-
PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap
On Wed, 27 Apr 2022 03:28:09 GMT, Stuart Marks wrote: >> test/jdk/java/util/IdentityHashMap/Basic.java line 77: >> >>> 75: E[] contents = (E[]) c.toArray(); >>> 76: >>> 77: 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); [AssertJ]: https://assertj.github.io/doc - PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap
On Wed, 27 Apr 2022 03:11:58 GMT, liach 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 75:
>
>> 73: private void checkContents(Collection c, BiPredicate p,
>> E... given) {
>> 74: @SuppressWarnings("unchecked")
>> 75: 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?
> test/jdk/java/util/IdentityHashMap/Basic.java line 77:
>
>> 75: E[] contents = (E[]) c.toArray();
>> 76:
>> 77: 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.
-
PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap
On Fri, 22 Apr 2022 03:37:27 GMT, Stuart Marks 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`. 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. - PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap
On Fri, 22 Apr 2022 03:37:27 GMT, Stuart Marks 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`.
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.
test/jdk/java/util/IdentityHashMap/Basic.java line 75:
> 73: private void checkContents(Collection c, BiPredicate p,
> E... given) {
> 74: @SuppressWarnings("unchecked")
> 75: 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.
test/jdk/java/util/IdentityHashMap/Basic.java line 77:
> 75: E[] contents = (E[]) c.toArray();
> 76:
> 77: 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.
-
PR: https://git.openjdk.java.net/jdk/pull/8354
RFR: 8285295: Need better testing for IdentityHashMap
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`. - Commit messages: - Refactor contents checking to use checkElements() and checkEntries(). - Rename some tests. - Rename isIdenticalEntry to hasIdenticalKeyValue. - Finish writing all pending tests except remove(k,v) and replace(k,v1,v2). - Some cleanup and more tests. - Initial cut at IdentityHashMap tests. Changes: https://git.openjdk.java.net/jdk/pull/8354/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8354&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8285295 Stats: 546 lines in 1 file changed: 546 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8354.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8354/head:pull/8354 PR: https://git.openjdk.java.net/jdk/pull/8354
