On Wed, 5 Mar 2025 20:05:53 GMT, Fernando Guallini <fguall...@openjdk.org>
wrote:
>> test/jdk/sun/security/pkcs12/KeytoolOpensslInteropTest.java line 78:
>>
>>> 76: testWithOpensslCommands(opensslPath);
>>> 77: } else {
>>> 78: // since the current version of openssl is not available,
>>> skip all
>>
>> If this test is to check interoperability with OpenSSL, shouldn't we throw a
>> skippedexception or an error instead of just running with java?
>
> Well, the test is also checking with java commands if openssl is available
> (line 75), then it makes sense to keep it when it is not available as it does
> not rely on Openssl.
My concern is that a Pass result is ambiguous: we may or may not have verified
interoperability with openssl. If the Java portion of the test is valid and
tests functionality not covered in other tests then it should be its own test.
This test should either run with openssl or throw a SkippedException because
openssl is not available.
>> test/lib/jdk/test/lib/security/OpensslArtifactFetcher.java line 79:
>>
>>> 77: }
>>> 78: }
>>> 79: return verifyOpensslVersion(path, OPENSSL_BUNDLE_VERSION) ?
>>> path : null;
>>
>> Do we want to keep this version check? On the one hand, it ensures that the
>> system binaries or binaries specified via the system property will be a
>> specific, known version, but on the other hand, the only way to run this
>> test with a different version of the library is to change the code. Instead,
>> can we just log the version that is used when the test is run?
>
> I considered removing this check as it seems redundant, but then I realised
> it was introduced to ensure the bundle version matches the version specified
> by the artifact name. It's possible that the artifact's filename could show a
> different version than the actual binaries with `-version`. On the other
> hand, most tests don't run this extra verification, that could be a good
> reason to remove it.
I don't have as strong an opinion about this check. If you want to keep it,
though, then test really needs to be skipped or failed if the correct version
of openssl isn't available. There are a lot of other versions of openssl that I
could run this test with and I'd see Pass results when I wasn't actually
running openssl at all.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23613#discussion_r1983305018
PR Review Comment: https://git.openjdk.org/jdk/pull/23613#discussion_r1983334180
