8285405: add test and check for negative argument to HashMap::newHashMap et al #9036

jaikiran · 2022-06-06T06:57:23Z

Can I please get a review of this change which addresses https://bugs.openjdk.java.net/browse/JDK-8285405?

I've added the test for LinkedHashMap.newLinkedHashMap(int) in the existing test/jdk/java/util/LinkedHashMap/Basic.java since that test has tests for various APIs of this class.

For WeakHashMap.newWeakHashMap and HashMap.newHashMap, I have created new test classes under relevant locations, since these classes already have test classes (almost) per API/feature in those locations.

Progress

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

Issue

JDK-8285405: add test and check for negative argument to HashMap::newHashMap et al

Reviewers

Lance Andersen (@LanceAndersen - Reviewer) ⚠️ Review applies to 9ccebd79

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9036/head:pull/9036
$ git checkout pull/9036

Update a local copy of the PR:
$ git checkout pull/9036
$ git pull https://git.openjdk.org/jdk pull/9036/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9036

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9036.diff

bridgekeeper · 2022-06-06T06:58:57Z

👋 Welcome back jpai! 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-06-06T07:01:30Z

@jaikiran 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.

…Map et al

mlbridge · 2022-06-08T06:08:15Z

Webrevs

openjdk · 2022-06-08T17:03:56Z

@jaikiran 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:

8285405: add test and check for negative argument to HashMap::newHashMap et al

Reviewed-by: 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 2 new commits pushed to the master branch:

dae4c49: 8286197: C2: Optimize MemorySegment shape in int loop
94b473e: 8280454: G1: ClassLoaderData verification keeps CLDs live that causes problems with VerifyDuringGC during Remark

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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-06-09T23:12:45Z

Note that I've integrated JDK-8284780 (HashSet static factories) so maybe this PR could be updated with tests for those as well. Also, if you edit LinkedHashSet.java, please update the copyright date. Forgot to do that in other other PR.

jaikiran · 2022-06-10T03:25:38Z

Hello Stuart, I'll include tests for those in this PR shortly.

…shSet and LinkedHashSet

…ashSet APIs

openjdk-notifier · 2022-06-10T08:21:57Z

@jaikiran Please do not rebase or force-push to an active PR as it invalidates existing review comments. All changes will be squashed into a single commit automatically when integrating. See OpenJDK Developers’ Guide for more information.

jaikiran · 2022-06-10T08:29:52Z

I've now update the PR to include tests for HashSet.newHashSet and LinkedHashSet.newLinkedHashSet APIs. Additionally, their implementation too have has been modified to do the necessary negative param check. The copyright year on the LinkedHashSet.java file has also been updated accordingly.

P.S: Please ignore the bot message which talks about rebase/force-push. No rebase or force-push was done (only the recommended merge command was used to merge with latest master branch). I will check with the relevant team to see if there's some bug in the bot which generates that message. I have seen that happen recently on few other PRs as well.

stuart-marks

Changes to the files in src are all fine.

I've made a couple individual comments on the NewHashMap.java test. But modifying two files and adding three new files seems rather bulky for what is essentially five copies of the same test. Perhaps they should be consolidated. They could all be written in a single file using a single data provider for five different test methods. (They could even be written as a combo test with a data provider that provides the factory method to call, but that seems like excess complexity.)

I'd suggest adding the data provider and test cases to HashMap/WhiteBoxResizeTest. That test is already testing the factory methods for the five implementations to ensure the right capacity is allocated. Testing them for negative values is only a small step beyond that.

A combo test of {-1, -42, MIN_VALUE} x { newHM, newLHM, ... } doesn't seem to me to warrant the complexity that would entail. Testing different negative values doesn't add much. I'd suggest having a data provider that provides the different factory methods as an IntFunction and then just passes -1 and ensures that the exception is thrown.

stuart-marks · 2022-06-23T00:53:31Z

test/jdk/java/util/HashMap/NewHashMap.java

+        return new Object[][]{new Object[]{-1},
+                new Object[]{Integer.MIN_VALUE},
+                new Object[]{-42}};
+    }


Can be rewritten more concisely as follows:

return new Object[][] { { -1 }, { Integer.MIN_VALUE }, { -42 } };

stuart-marks · 2022-06-23T00:55:08Z

test/jdk/java/util/HashMap/NewHashMap.java

+        var h = HashMap.newHashMap(val);
+        assertNotNull(h);
+        assertEquals(h.size(), 0, "Unexpected size of HashMap created with numMappings: " + val);
+    }


I don't think we need tests for non-negative values. Aren't these covered in HashMap/WhiteBoxResizeTest? Testing of size is also a bit superfluous here.

bridgekeeper · 2022-07-21T07:55:30Z

@jaikiran This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

jaikiran · 2022-07-21T09:30:29Z

Work is in progress, I'll be reviving this shortly.

openjdk bot added the core-libs core-libs-dev@openjdk.org label Jun 6, 2022

8285405: add test and check for negative argument to HashMap::newHash…

9ccebd7

…Map et al

jaikiran force-pushed the 8285405 branch from 6def1d7 to 9ccebd7 Compare June 8, 2022 04:11

jaikiran marked this pull request as ready for review June 8, 2022 06:01

openjdk bot added the rfr Pull request is ready for review label Jun 8, 2022

LanceAndersen approved these changes Jun 8, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 8, 2022

jaikiran added 3 commits June 10, 2022 12:50

Merge latest from master branch

ef426d9

add a check for negative numElements very early in the new APIs of Ha…

0501617

…shSet and LinkedHashSet

Add tests for the new HashSet#newHashSet and LinkedHashSet#newLinkedH…

07bbaa9

…ashSet APIs

add newline at the end of the new file

ba159d1

stuart-marks reviewed Jun 23, 2022

View reviewed changes

jaikiran closed this Aug 9, 2022

jaikiran deleted the 8285405 branch August 9, 2022 09:32

jaikiran mentioned this pull request Aug 9, 2022

8285405: add test and check for negative argument to HashMap::newHashMap et al #9806

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8285405: add test and check for negative argument to HashMap::newHashMap et al #9036

8285405: add test and check for negative argument to HashMap::newHashMap et al #9036

jaikiran commented Jun 6, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jun 6, 2022

openjdk bot commented Jun 6, 2022

mlbridge bot commented Jun 8, 2022 •

edited

Loading

openjdk bot commented Jun 8, 2022 •

edited

Loading

stuart-marks commented Jun 9, 2022 •

edited

Loading

jaikiran commented Jun 10, 2022

openjdk-notifier bot commented Jun 10, 2022

jaikiran commented Jun 10, 2022

stuart-marks left a comment

stuart-marks Jun 23, 2022

stuart-marks Jun 23, 2022

bridgekeeper bot commented Jul 21, 2022

jaikiran commented Jul 21, 2022

8285405: add test and check for negative argument to HashMap::newHashMap et al #9036

8285405: add test and check for negative argument to HashMap::newHashMap et al #9036

Conversation

jaikiran commented Jun 6, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jun 6, 2022

openjdk bot commented Jun 6, 2022

mlbridge bot commented Jun 8, 2022 • edited Loading

Webrevs

openjdk bot commented Jun 8, 2022 • edited Loading

stuart-marks commented Jun 9, 2022 • edited Loading

jaikiran commented Jun 10, 2022

openjdk-notifier bot commented Jun 10, 2022

jaikiran commented Jun 10, 2022

stuart-marks left a comment

Choose a reason for hiding this comment

stuart-marks Jun 23, 2022

Choose a reason for hiding this comment

stuart-marks Jun 23, 2022

Choose a reason for hiding this comment

bridgekeeper bot commented Jul 21, 2022

jaikiran commented Jul 21, 2022

jaikiran commented Jun 6, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Jun 8, 2022 •

edited

Loading

openjdk bot commented Jun 8, 2022 •

edited

Loading

stuart-marks commented Jun 9, 2022 •

edited

Loading