On Wed, 15 Jul 2026 14:43:21 GMT, Sean Mullan <[email protected]> wrote:
>> Andreas Chmielewski has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Remove unused imports
>
> test/jdk/javax/net/ssl/ciphersuites/BulkCipherDisabledAlgorithms.java line 1:
>
>> 1: /*
>
> While this reduces the number of VMs spawned, 90 VMs are still spawned. I
> think there is still some redundancy that can be eliminated.
>
> This test is very comprehensive, in that it tests the disabling of every
> cipher suite by the full name and its bulk cipher algorithm. However, the
> logic involved in disabling the full cipher suite name
> TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 is the same as disabling
> TLS_DHE_RSA_WITH_AES_256_GCM_SHA384 or any other suite name, it is just a
> match on the cipher suite name. I don't think it is necessary to test the
> disabling of every suite which uses the same logic. Probably one suite for
> TLS 1.2 and one for TLS 1.3 would be sufficient.
>
> For the bulk cipher algorithm, there is also some redundancy. Probably
> testing one suite per bulk cipher for each protocol (1.2 and 1.3) would be
> sufficient, but some redundancy here is probably fine as this was the main
> issue that we needed a test for.
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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/31812#discussion_r3589827621
