On Fri, 4 Mar 2022 02:27:53 GMT, Stuart Marks <sma...@openjdk.org> wrote:
>> XenoAmess has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> cast several float to double before calculation
>
> OK, I took a look at HashMapsPutAllOverAllocateTableTest.java. It's certainly
> a good start at testing stuff in this area. However, I notice that
>
> test/jdk/java/util/HashMap/WhiteBoxResizeTest.java
>
> already exists and tests kind-of similar things. I think it would be
> preferable to enhance this test with additional assertions since it already
> has a bunch of machinery to inspect the various internals. It tests both
> HashMap and LinkedHashMap, so I think it would be ok to add a WeakHashMap
> test here as well. I note however that it relies on LinkedHashMap being a
> subclass of HashMap, which WeakHashMap is not, and so there will be a certain
> amount of messiness adding in the handling for WeakHashMap.
>
> Once this is in place though I think adding the additional test cases would
> fit in well here. In particular, it should be possible to test the proper
> table lengths for the copy-constructor and putAll cases (from your original
> bug report) as well as the premature table expansion in WeakHashMap.
>
> This test could use some cleanup as well. For example, the
> capacityTestInitialCapacity() method has a list of suppliers that it loops
> over. This is probably better expressed as a data provider driving a test
> method that tests one case. There's a bit of an art to setting this up.
> TestNG wants each test case to be `Object[][]` which is kind of a pain if you
> want to use a lambda. A trick is to have a method that returns `Object[]`
> representing one test case, and have the parameters of this method provide
> the target types for the lambdas. For example:
>
> Object[] testcase(String label, Supplier<Map<Integer,Integer>> s,
> Consumer<Map<Integer,Integer>> c, int expectedCapacity) {
> return new Object[] { label, s, c, expectedCapacity };
> }
>
> and then have the actual data provider just be a series of calls to this
> method in an array literal:
>
> @DataProvider
> public Object[][] allCases() {
> return new Object[][] {
> testcase("HMputAll", () -> new HashMap<>(), m -> m.putAll(MAP12),
> 16),
> testcase("WHMputAll", () -> new WeakHashMap<>(), m ->
> m.putAll(MAP12), 16),
> testcase("HMcopyctor", () -> new HashMap<>(MAP12), m -> { }, 16),
> testcase("WHMcopyctor", () -> new WeakHashMap<>(MAP12), m -> { },
> 16),
> };
> }
>
> This lets you write the lambdas without a cast that provides the target type.
>
> The test method calls the supplier, passes the map to the consumer, gets the
> table length, and asserts that it's equal to the expected length. Note that
> I've set this up with separate supplier and consumer in order to accommodate
> both the copy constructor cases and the putAll cases. Finally, the label
> string isn't used by the test method, but it's useful if one of the cases
> fails. TestNG will print out the arguments of a failing case and the label
> helps a lot here, as the string form of the lambda is basically
> unintelligible.
@stuart-marks
done but cannot pass that test, because WeakHashMap have no such mechanism that
do not create table[] when empty, only create it when first element exist, but
HashMap do.
-------------
PR: https://git.openjdk.java.net/jdk/pull/7431
