On Fri, 13 Aug 2021 09:47:53 GMT, Abdul Kolarkunnu <akolarku...@openjdk.org>
wrote:
>> ParamsTest is an interop test between keytool <-> openssl. There are some
>> manual steps listed in jdk/sun/security/pkcs12/params/README to perform
>> after the execution of jtreg execution. So this test is to perform that
>> manual steps.
>
> Abdul Kolarkunnu has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8266182: Automate manual steps listed in the test
> jdk/sun/security/pkcs12/ParamsTest.java
Thanks for the change. The fallback to `testWithJavaCommands()`-only is fine.
Some comments.
test/jdk/sun/security/pkcs12/KeytoolOpensslInteropTest.java line 91:
> 89: opensslPath = OpensslArtifactFetcher.fetchOpenssl();
> 90: }
> 91: if (opensslPath != null && verifyOpensslVerion(opensslPath)) {
The code here is a little different from your description above. If user
provides a non-1.1.* openssl through `test.openssl.path` then you won't go look
into elsewhere.
Also, `getSystemOpensslPath()` and `OpensslArtifactFetcher.fetchOpenssl()`
already ensure version is 1.1.* so the check above is duplicated.
test/jdk/sun/security/pkcs12/KeytoolOpensslInteropTest.java line 614:
> 612: }
> 613:
> 614: private static boolean verifyOpensslVerion(String path) {
Oops, a typo in the method name. `s/Verion/Version/`.
test/jdk/sun/security/pkcs12/KeytoolOpensslInteropTest.java line 618:
> 616: ProcessTools.executeCommand(path, "version")
> 617: .shouldHaveExitValue(0)
> 618: .shouldMatch("1.1.*");
Precisely the regex should be `1\.1\..*`, or maybe you can simply call
`.shouldContain("1.1.")`.
test/lib/jdk/test/lib/artifacts/OpensslArtifactFetcher.java line 24:
> 22: */
> 23:
> 24: package jdk.test.lib.artifacts.openssl;
The file is not in this package. Somehow I don't think if it's not worth
creating a new package for it. Do you expect we will have more classes here?
test/lib/jdk/test/lib/artifacts/OpensslArtifactFetcher.java line 35:
> 33: public class OpensslArtifactFetcher {
> 34:
> 35: public static String fetchOpenssl() {
Can we rename this to `fetchOpenssl1.1.1`? Someday we will need to download
other versions. If so, you can probably move `getSystemOpensslPath`,
`verifyOpensslVersion`, and `System.getProperty("test.openssl.path")` here as
well. It's quite likely that they will be used together.
-------------
PR: https://git.openjdk.java.net/jdk/pull/4413
