On Fri, 10 Jun 2022 08:26:02 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> 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. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > add newline at the end of the new file 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. test/jdk/java/util/HashMap/NewHashMap.java line 45: > 43: new Object[]{Integer.MIN_VALUE}, > 44: new Object[]{-42}}; > 45: } Can be rewritten more concisely as follows: return new Object[][] { { -1 }, { Integer.MIN_VALUE }, { -42 } }; test/jdk/java/util/HashMap/NewHashMap.java line 71: > 69: assertNotNull(h); > 70: assertEquals(h.size(), 0, "Unexpected size of HashMap created > with numMappings: " + val); > 71: } 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. ------------- PR: https://git.openjdk.org/jdk/pull/9036
