On Wed, 15 Jul 2026 20:21:58 GMT, Sean Mullan <[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.
>
>> 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`.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/31812#discussion_r3591220986
