On Thu, 10 Mar 2022 16:10:31 GMT, XenoAmess <d...@openjdk.java.net> wrote:
>> XenoAmess has updated the pull request incrementally with five additional
>> commits since the last revision:
>>
>> - clean out tests
>> - Remove 'randomness' keyword.
>> - Cleanup and commenting.
>> - initial rewrite of WhiteBoxResizeTest
>> - Restore WhiteBoxResizeTest.java
>
> test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 290:
>
>> 288: cases.addAll(genPopulatedCapacityCases(12, 16));
>> 289: cases.addAll(genPopulatedCapacityCases(13, 32));
>> 290: cases.addAll(genPopulatedCapacityCases(64, 128));
>
> @stuart-marks should there better be a loop from 1 to 64?
The difficulty with having a loop instead of constants is that the expected
value now needs to be computed. We could probably use `tableSizeFor` to do
this. But this is starting to get uncomfortably close to a testing antipattern
which is to use the code under test as part of the test. If a bug is introduced
into `tableSizeFor`, it could introduce the error into both the actual value
and the expected value, covering up the bug. (This is related to one of the
flaws with the Enum/ConstantDirectoryOptimalCapacity test.) Now we _hope_ that
`tableSizeFor` is correct and tested, but that verges on having tests depend on
each other in a subtle and uncomfortable way.
Recall that one of the original cases was that the table size computation
(int) (numMappings / 0.75f) + 1
gives a value one too large: for 12 mappings it results in 17, giving a table
size of 32. The (12, 16) case is sufficient to verify that this is fixed. Then
the adjacent cases are there to make sure nothing weird is going on. (You could
argue that (11, 16) isn't necessary.)
I threw in the (64, 128) case because of the loop to 64, but it's not
necessarily testing anything useful. If you think we need more cases we can add
more at the boundaries, such as (24, 32) and (25, 64).
-------------
PR: https://git.openjdk.java.net/jdk/pull/7431
