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

[Abdul Kolarkunnu]

On Fri, 13 Aug 2021 15:30:26 GMT, Weijun Wang <weijun at openjdk.org> wrote:

>> 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
>
> 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.

Though I had kept it in intentional like that, to make sure to give proper openssl path if user is supplying it explicitly.
Anyhow I changed it now.
1. Check whether property test.openssl.path is set and it's the
    preferred version(1.1.*) of openssl, then return that path.
2. Else look for already installed openssl (version 1.1.*) in system
   path /usr/bin/openssl or /usr/local/bin/openssl, then return that
    path.
3. Else try to download openssl (version 1.1.*) from the artifactory
     and return that path, if download fails then return null.

> 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/`.

corrected

> 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.")`.

Changed it in to .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?

ya, it was by mistake. I tested from different local workspace, that' why I couldn't catch it in my testing.
Now moved to package jdk.test.lib.artifacts.

> 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.

Moved getSystemOpensslPath, verifyOpensslVersion and System.getProperty("test.openssl.path") to OpensslArtifactFetcher class. And renamed  fetchOpenssl to getOpenssl1dot1dotStar.

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

PR: https://git.openjdk.java.net/jdk/pull/4413