Re: RFR: 8266182: Automate manual steps listed in the test jdk/sun/security/pkcs12/ParamsTest.java [v4]
On Mon, 9 Aug 2021 12:54:40 GMT, Weijun Wang 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 48: > >> 46: import java.nio.file.Files; >> 47: import java.nio.file.Path; >> 48: import java.nio.file.Paths; > > IntelliJ IDEA shows quite some imports are useless. Removed all unused imports > test/jdk/sun/security/pkcs12/KeytoolOpensslInteropTest.java line 100: > >> 98: OutputAnalyzer output = ProcessTools.executeCommand( >> 99: opensslPath, "version") >> 100: .shouldHaveExitValue(0); > > This looks like a good time to ensure the version is 1.1.* or at least 1.* (I > haven't tested with 1.0.* versions). Also, when trying out with 3.0.0, there > are only 2 failures (line 119 generating weak file failed without -legacy. > line 479 succeeded with a warning). ok, added version check ProcessTools.executeCommand(path, "version").shouldHaveExitValue(0).shouldMatch("1.1.*"); I tested with 1.0.2a version, test fails because some of the info messages are different. eg: 'MAC: sha256, Iteration 1' missing from stdout/stderr 1.0.2a produces output as "MAC Iteration 1" . ": sha256" is not there. - PR: https://git.openjdk.java.net/jdk/pull/4413
Re: RFR: 8266182: Automate manual steps listed in the test jdk/sun/security/pkcs12/ParamsTest.java [v4]
On Mon, 9 Aug 2021 13:04:03 GMT, Weijun Wang 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 88: > >> 86: testWithJavaCommands(); >> 87: testWithOpensslCommands(opensslPath); >> 88: } else { > > I still think it's better to succeed with a warning when the openssl binary > cannot be found. IMHO it's a little unfriendly to force people setting up a > system property to let the test pass. It's also dependent on runner machines > and the user might have to adjust their scripts or launcher all the time. I > would rather keep the old test/data if I want to ensure the test gets run > everywhere. > > Also, it might also help if the test can search for openssl, maybe simply > from `/usr/bin/openssl` or `/usr/local/bin/openssl`, but this means you might > need to check for the version. > > Third, is it OK to let the system property pointing to the binary itself > directly? When I was trying out this test I was using the openssl I built and > it's not in a `bin` sub-directory. Ok, changed openssl selection flow as below: 1. Check whether test.openssl.path is set and it's the the preferred version(1.1.*) of openssl 2. If above property doesn't set, then look for already installed openssl (version 1.1.*) in system path /usr/bin/openssl or /usr/local/bin/openssl 3. if above is also not available try to download openssl from artifactory If any of above 3 succeeds then perform all tests, otherwise skip all openssl command dependent tests. - PR: https://git.openjdk.java.net/jdk/pull/4413
Re: RFR: 8266182: Automate manual steps listed in the test jdk/sun/security/pkcs12/ParamsTest.java [v4]
On Mon, 9 Aug 2021 13:10:05 GMT, Weijun Wang 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 580: > >> 578: return SecurityTools.keytool(s); >> 579: } >> 580: > > I feel the lines below should go to a test library. Moved fetchOpenssl() API to test/lib/jdk/test/lib/artifacts/OpensslArtifactFetcher.java - PR: https://git.openjdk.java.net/jdk/pull/4413
Re: RFR: 8266182: Automate manual steps listed in the test jdk/sun/security/pkcs12/ParamsTest.java [v4]
On Mon, 9 Aug 2021 11:06:25 GMT, Abdul Kolarkunnu 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 fix. Code looks fine. I'm only concerned on the failure when openssl is not found. test/jdk/sun/security/pkcs12/KeytoolOpensslInteropTest.java line 48: > 46: import java.nio.file.Files; > 47: import java.nio.file.Path; > 48: import java.nio.file.Paths; IntelliJ IDEA shows quite some imports are useless. test/jdk/sun/security/pkcs12/KeytoolOpensslInteropTest.java line 88: > 86: testWithJavaCommands(); > 87: testWithOpensslCommands(opensslPath); > 88: } else { I still think it's better to succeed with a warning when the openssl binary cannot be found. IMHO it's a little unfriendly to force people setting up a system property to let the test pass. It's also dependent on runner machines and the user might have to adjust their scripts or launcher all the time. I would rather keep the old test/data if I want to ensure the test gets run everywhere. Also, it might also help if the test can search for openssl, maybe simply from `/usr/bin/openssl` or `/usr/local/bin/openssl`, but this means you might need to check for the version. Third, is it OK to let the system property pointing to the binary itself directly? When I was trying out this test I was using the openssl I built and it's not in a `bin` sub-directory. test/jdk/sun/security/pkcs12/KeytoolOpensslInteropTest.java line 100: > 98: OutputAnalyzer output = ProcessTools.executeCommand( > 99: opensslPath, "version") > 100: .shouldHaveExitValue(0); This looks like a good time to ensure the version is 1.1.* or at least 1.* (I haven't tested with 1.0.? versions). Also, when trying out with 3.0.0, there are only 2 failures (line 119 generating weak file without -legacy. line 479 succeeds with a warning). test/jdk/sun/security/pkcs12/KeytoolOpensslInteropTest.java line 580: > 578: return SecurityTools.keytool(s); > 579: } > 580: I feel the lines below should go to a test library. - PR: https://git.openjdk.java.net/jdk/pull/4413
Re: RFR: 8266182: Automate manual steps listed in the test jdk/sun/security/pkcs12/ParamsTest.java [v4]
> 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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/4413/files - new: https://git.openjdk.java.net/jdk/pull/4413/files/20d6bb5f..fe17653f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4413&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4413&range=02-03 Stats: 1463 lines in 10 files changed: 633 ins; 830 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/4413.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4413/head:pull/4413 PR: https://git.openjdk.java.net/jdk/pull/4413