On Wed, 15 Jul 2026 22:38:41 GMT, Artur Barashev <[email protected]> wrote:
>>> Thanks for your review!
>>>
>>> I immediately spotted one opportunity to reduce the number of spawned VMs
>>> further, from 90 to 60. Currently, the test also disables each cipher suite
>>> by its full suite name:
>>>
>>> `tests.add(new String[] { suiteName, suiteName, "disabled" });`
>>>
>>> However, the purpose of this test is to verify the behavior of disabling
>>> bulk cipher algorithms, not disabling cipher suites by name. I therefore
>>> propose removing this case entirely.
>>>
>>> Other than that, I would prefer to keep the test in its current form, as it
>>> exercises all supported cipher suites. If you agree, I'll update the PR
>>> accordingly.
>>
>> Yes, that will be fine. Thanks.
>
> I would suggest to further reduce the number of spawned VMs to 30 by removing
> the `otherBulk` tests. Instead select the `otherSuite` in `testHandshake` and
> then run `TLSHandshakeTest` twice: with the given `suite` and with the
> `otherSuite`. Then `expectedDisabled` argument can also be removed as
> redundant: the handshake should fail with `suite` and succeed with
> `otherSuite`.
Nice suggestion! I usually favor straightforward tests that are easy to read
and understand, but in this case its fine.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/31812#discussion_r3593795227
