RFR: 8366182: Some PKCS11Tests are being skipped when they shouldn't

[Mikhail Yankelevich]

On Thu, 4 Sep 2025 16:54:48 GMT, Matthew Donovan <mdonovan at openjdk.org> wrote:

>> test/jdk/sun/security/pkcs11/Cipher/TestKATForGCM.java line 330:
>> 
>>> 328:                 String osName = System.getProperty("os.name");
>>> 329:                 int idx = ver.indexOf(".");
>>> 330:                 double major = Double.parseDouble(ver.substring(0, idx));
>> 
>> IMO the split between major version and minor is a bit hard to read. Wouldn't it be easier to just get a major.minor version entirely with something like:
>> 
>> 
>> String[] splitParts = ver.split("//.");
>> Double.parseDouble(splitParts.length > 1
>>         ? splitParts[0] + "." + splitParts[1] 
>>         : splitParts[0]);
>> 
>> 
>> This way it will always take a full double value (1.13 will not be the same as 1.1, as it's now as far as I can see) and would be a bit easier to understand 
>> 
>> The checking for only major version could be either `doubleVersion<4 && doubleVersion>=3` or even cleaner, using floor function `Math.floor(doubleVersion)`
>> 
>> And the same for the other files.
>> 
>> What do you think?
>
> I may be misunderstanding your comment but it looks like you're using a single, `double` value to compare versions. The original code did that but it doesn't work in all cases. The problem occurs when checking a range such as 
> 
> `version >= 3.11 && version < 3.12`
> 
> If the version number is 3.111, then the comparison is `true` and tests are skipped, even though _version_ 3.111 is "greater" than 3.12. So this code creates two doubles: `major` and `minor` to do separate comparisons of the values. 
> 
> To further complicate things, NSS also has versions of the form `x.y.z`. The original code combined the 'y' and 'z' components to go from 3.11.9 to 3.119. My change would create major version 3.0 and minor version 11.9.

Ah, you're right, I didn't think of this case! This approach will be easier then, I agree. 

Still, `ver.substring(idx+1)` may cause errors in cases like this [NSS 3.15.3.1 release notes](https://nss-crypto.org/reference/security/nss/legacy/nss_releases/nss_3.15.3.1_release_notes/index.html#mozilla-projects-nss-nss-3-15-3-1-release-notes). It will try to convert 15.3.1 to a double. Do you think changing to something like this would be worth it?


String verSubstring = ver.substring(idx+1);
String[] splitParts = verSubstring.split("//.");
Double minor = Double.parseDouble(splitParts.length > 1
                              ? splitParts[0] + "." + splitParts[1] 
                              : splitParts[0]);

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/27095#discussion_r2324586473