RFR: 8266182: Automate manual steps listed in the test jdk/sun/security/pkcs12/ParamsTest.java [v6]

[Weijun Wang]

On Fri, 13 Aug 2021 09:47:53 GMT, Abdul Kolarkunnu <akolarkunnu at 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