RE: [PATCH] 8244199: [TestBug]: Update sun/management/jmxremote/bootstrap tests
Adding jmx-dev and security-dev. -Original Message- From: Thejasvi Voniadka Sent: Monday, May 4, 2020 12:56 PM To: core-libs-...@openjdk.java.net Subject: [PATCH] 8244199: [TestBug]: Update sun/management/jmxremote/bootstrap tests Hi, May I please find a sponsor for this patch? Bug: https://bugs.openjdk.java.net/browse/JDK-8244199 Description: The test "test/jdk/sun/management/jmxremote/bootstrap/RmiSslBootstrapTest.sh" fails intermittently on a lower release. While investigating the cause for the failure, I felt the diagnostic messaging built into the test was not adequate. Some of the issues observed: 1. If the test throws a RuntimeException, the full stack trace is not displayed. 2. The test has several logging statements. However, the logging environment is not properly initialized to allow levels such as FINE when needed. 3. The logging sequence could be improved: some messages end up at stdout, and others at stderr, which makes it harder to follow the output. The patch is to improve the test in these lines. Webrev: http://cr.openjdk.java.net/~tvoniadka/reviews/8244199/webrev.full.00 I have also used this opportunity to clean-up the test code as a whole, in lines of removal of redundant logic, formatting, coding guidelines, etc If the above patch looks too confusing and is hard to follow, here is a simpler version that shows only the core logic changes (I intend to submit only the full patch to be committed though): http://cr.openjdk.java.net/~tvoniadka/reviews/8244199/webrev.simple.00 The patch has been tested on mach5, and all jmxremote tests passed.
[PATCH] 8244199: [TestBug]: Update sun/management/jmxremote/bootstrap tests
Hello, A quick follow-up on this request.. -Original Message- From: Thejasvi Voniadka Sent: Monday, May 4, 2020 12:56 PM To: core-libs-...@openjdk.java.net Subject: [PATCH] 8244199: [TestBug]: Update sun/management/jmxremote/bootstrap tests Hi, May I please find a sponsor for this patch? Bug: https://bugs.openjdk.java.net/browse/JDK-8244199 Description: The test "test/jdk/sun/management/jmxremote/bootstrap/RmiSslBootstrapTest.sh" fails intermittently on a lower release. While investigating the cause for the failure, I felt the diagnostic messaging built into the test was not adequate. Some of the issues observed: 1. If the test throws a RuntimeException, the full stack trace is not displayed. 2. The test has several logging statements. However, the logging environment is not properly initialized to allow levels such as FINE when needed. 3. The logging sequence could be improved: some messages end up at stdout, and others at stderr, which makes it harder to follow the output. The patch is to improve the test in these lines. Webrev: http://cr.openjdk.java.net/~tvoniadka/reviews/8244199/webrev.full.00 I have also used this opportunity to clean-up the test code as a whole, in lines of removal of redundant logic, formatting, coding guidelines, etc If the above patch looks too confusing and is hard to follow, here is a simpler version that shows only the core logic changes (I intend to submit only the full patch to be committed though): http://cr.openjdk.java.net/~tvoniadka/reviews/8244199/webrev.simple.00 The patch has been tested on mach5, and all jmxremote tests passed.
RFR: 8244199: [TestBug]: Update sun/management/jmxremote/bootstrap tests
Hi, May someone please sponsor this patch? Bug: https://bugs.openjdk.java.net/browse/JDK-8244199 Description: The test "test/jdk/sun/management/jmxremote/bootstrap/RmiSslBootstrapTest.sh" fails intermittently on a lower release. While investigating the cause for the failure, I felt the diagnostic messaging built into the test was not adequate. Some of the issues observed: 1. If the test throws a RuntimeException, the full stack trace is not displayed. 2. The test has several logging statements. However, the logging environment is not properly initialized to allow levels such as FINE and FINER. 3. The logging sequence could be improved: some messages end up at stdout, and others at stderr, which makes it harder to follow the output. The patch is to improve the test in these lines. Webrev: http://cr.openjdk.java.net/~tvoniadka/reviews/8244199/webrev.full.00 I have also used this opportunity to clean-up the test code as a whole, in lines of removal of redundant logic, formatting, coding guidelines, etc If the above patch looks too confusing and is hard to follow, here is a simpler version that shows only the core logic changes (I intend to submit the full patch to be pushed though): http://cr.openjdk.java.net/~tvoniadka/reviews/8244199/webrev.simple.00 The patch has been tested on mach5, and all jmxremote tests passed.
RE: RFR: 8244199: [TestBug]: Update sun/management/jmxremote/bootstrap tests
Ping.. -Original Message- From: Thejasvi Voniadka Sent: Friday, May 8, 2020 6:17 AM To: core-libs-...@openjdk.java.net; jmx-...@openjdk.java.net; security-dev@openjdk.java.net Subject: RFR: 8244199: [TestBug]: Update sun/management/jmxremote/bootstrap tests Hi, May someone please sponsor this patch? Bug: https://bugs.openjdk.java.net/browse/JDK-8244199 Description: The test "test/jdk/sun/management/jmxremote/bootstrap/RmiSslBootstrapTest.sh" fails intermittently on a lower release. While investigating the cause for the failure, I felt the diagnostic messaging built into the test was not adequate. Some of the issues observed: 1. If the test throws a RuntimeException, the full stack trace is not displayed. 2. The test has several logging statements. However, the logging environment is not properly initialized to allow levels such as FINE and FINER. 3. The logging sequence could be improved: some messages end up at stdout, and others at stderr, which makes it harder to follow the output. The patch is to improve the test in these lines. Webrev: http://cr.openjdk.java.net/~tvoniadka/reviews/8244199/webrev.full.00 I have also used this opportunity to clean-up the test code as a whole, in lines of removal of redundant logic, formatting, coding guidelines, etc If the above patch looks too confusing and is hard to follow, here is a simpler version that shows only the core logic changes (I intend to submit the full patch to be pushed though): http://cr.openjdk.java.net/~tvoniadka/reviews/8244199/webrev.simple.00 The patch has been tested on mach5, and all jmxremote tests passed.
RFR: 8272708: [Test]: Cleanup: test/jdk/security/infra/java/security/cert/CertPathValidator/certification/BuypassCA.java no longer needs ocspEnabled
Hi, Please review this simple clean-up fix to remove an unused variable. The test passed on all platforms after this clean-up. - Commit messages: - 8272708: [Test]: Cleanup: test/jdk/security/infra/java/security/cert/CertPathValidator/certification/BuypassCA.java no longer needs ocspEnabled Changes: https://git.openjdk.java.net/jdk/pull/5182/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5182&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8272708 Stats: 6 lines in 1 file changed: 0 ins; 4 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5182.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5182/head:pull/5182 PR: https://git.openjdk.java.net/jdk/pull/5182
Integrated: 8272708: [Test]: Cleanup: test/jdk/security/infra/java/security/cert/CertPathValidator/certification/BuypassCA.java no longer needs ocspEnabled
On Thu, 19 Aug 2021 12:11:21 GMT, Thejasvi Voniadka wrote: > Hi, > > Please review this simple clean-up fix to remove an unused variable. The test > passed on all platforms after this clean-up. This pull request has now been integrated. Changeset: 4bd37c31 Author: Thejasvi Voniadka Committer: Abdul Kolarkunnu URL: https://git.openjdk.java.net/jdk/commit/4bd37c31525b69db8d55c0c3aaf74c95024f Stats: 6 lines in 1 file changed: 0 ins; 4 del; 2 mod 8272708: [Test]: Cleanup: test/jdk/security/infra/java/security/cert/CertPathValidator/certification/BuypassCA.java no longer needs ocspEnabled Reviewed-by: rhalade - PR: https://git.openjdk.java.net/jdk/pull/5182
RFR: 8273646: Add openssl from path variable also in to Default System Openssl Path in OpensslArtifactFetcher
The test "sun/security/pkcs12/KeytoolOpensslInteropTest.java" performs interoperability checks between JDK and openssl with respect to certain keystore operations. The test requires a suitable version of openssl to be available on the machine it runs. The test at present looks only at the following absolute paths for openssl (in that order): - Commit messages: - 8273646: Add openssl from path variable also in to Default System Openssl Path in OpensslArtifactFetcher Changes: https://git.openjdk.java.net/jdk/pull/5523/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5523&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8273646 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5523.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5523/head:pull/5523 PR: https://git.openjdk.java.net/jdk/pull/5523
Re: RFR: 8273646: Add openssl from path variable also in to Default System Openssl Path in OpensslArtifactFetcher
On Wed, 15 Sep 2021 21:49:01 GMT, Weijun Wang wrote: > My understanding is that the $PATH variable usually already contains > `/usr/bin` and `/usr/local/bin`. If you switch to $PATH, maybe it's no more > necessary to try out those 2 anymore? > > Just curious, what is the full path of openssl on your Cygwin? On the machine I checked, it reads as follows: PATH='C:\\Cygwin\\usr\\local\\bin;C:\\Cygwin\\bin;C:\\WINDOWS\\system32;C:\\WINDOWS;C:\\WINDOWS\\System32\\Wbem;C:\\WINDOWS\\System32\\WindowsPowerShell\\v1.0;C:\\WINDOWS\\System32\\OpenSSH;C:\\cygwin\\bin;C:\\Users\\jpgansible\\AppData\\Local\\Microsoft\\WindowsApps;%JAVA_HOME%\\bin;C:\\Cygwin\\usr\\local\\bin' \\ - PR: https://git.openjdk.java.net/jdk/pull/5523
Re: RFR: 8273646: Add openssl from path variable also in to Default System Openssl Path in OpensslArtifactFetcher
On Wed, 15 Sep 2021 21:49:01 GMT, Weijun Wang wrote: >> The test "sun/security/pkcs12/KeytoolOpensslInteropTest.java" performs >> interoperability checks between JDK and openssl with respect to certain >> keystore operations. The test requires a suitable version of openssl to be >> available on the machine it runs. Some mechanisms are used to discover >> openssl, among which searching through a predefined set of paths is also >> one. The test at present looks only at the following absolute paths (in that >> order): > > My understanding is that the $PATH variable usually already contains > `/usr/bin` and `/usr/local/bin`. If you switch to $PATH, maybe it's no more > necessary to try out those 2 anymore? > > Just curious, what is the full path of openssl on your Cygwin? @wangweij, just a quick follow-up to make sure this change makes sense given the value of PATH variable on some of those machines (I mentioned it in the comment above). It appears it is true that openssl is indeed found inside /usr/local/bin directory, but on Cygwin-based systems, this actually translates to something like "C:\Cygwin\usr\local\bin" system path. Since we are explicitly passing "/usr/local/bin/openssl" to the process launcher, it fails to launch on such machines. We could have probably passed "C:\Cygwin...", but that would indicate some sort of prior knowledge towards where Cygwin is installed exactly. Hence, just an additional attempt with no absolute paths could succeed. - PR: https://git.openjdk.java.net/jdk/pull/5523
Re: RFR: 8273646: Add openssl from path variable also in to Default System Openssl Path in OpensslArtifactFetcher
On Mon, 20 Sep 2021 12:04:50 GMT, Weijun Wang wrote: >> The test "sun/security/pkcs12/KeytoolOpensslInteropTest.java" performs >> interoperability checks between JDK and openssl with respect to certain >> keystore operations. The test requires a suitable version of openssl to be >> available on the machine it runs. Some mechanisms are used to discover >> openssl, among which searching through a predefined set of paths is also >> one. The test at present looks only at the following absolute paths (in that >> order): > > OK, I understand why the previous 2 paths are not enough. The current change > looks fine to me. > > One small suggestion: since you're are using openssl from $PATH, I assume > it's actually unnecessary to search in `/usr/bin` anymore. Maybe > `/usr/local/bin` is still useful. Yes, I think your suggestion makes sense @wangweij. I will modify the patch today to remove /usr/bin/openssl, do a quick round of testing, and will re-upload. - PR: https://git.openjdk.java.net/jdk/pull/5523
Re: RFR: 8273646: Add openssl from path variable also in to Default System Openssl Path in OpensslArtifactFetcher [v2]
> The test "sun/security/pkcs12/KeytoolOpensslInteropTest.java" performs > interoperability checks between JDK and openssl with respect to certain > keystore operations. The test requires a suitable version of openssl to be > available on the machine it runs. Some mechanisms are used to discover > openssl, among which searching through a predefined set of paths is also one. > The test at present looks only at the following absolute paths (in that > order): Thejasvi Voniadka has updated the pull request incrementally with one additional commit since the last revision: 8273646: Add openssl from path variable also in to Default System Openssl Path in OpensslArtifactFetcher - Changes: - all: https://git.openjdk.java.net/jdk/pull/5523/files - new: https://git.openjdk.java.net/jdk/pull/5523/files/b1767767..3556e3ec Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5523&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5523&range=00-01 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/5523.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5523/head:pull/5523 PR: https://git.openjdk.java.net/jdk/pull/5523
Integrated: 8273646: Add openssl from path variable also in to Default System Openssl Path in OpensslArtifactFetcher
On Wed, 15 Sep 2021 08:01:30 GMT, Thejasvi Voniadka wrote: > The test "sun/security/pkcs12/KeytoolOpensslInteropTest.java" performs > interoperability checks between JDK and openssl with respect to certain > keystore operations. The test requires a suitable version of openssl to be > available on the machine it runs. Some mechanisms are used to discover > openssl, among which searching through a predefined set of paths is also one. > The test at present looks only at the following absolute paths (in that > order): This pull request has now been integrated. Changeset: a5108a60 Author:Thejasvi Voniadka Committer: Abdul Kolarkunnu URL: https://git.openjdk.java.net/jdk/commit/a5108a605e6a1e427d90dbeeb8630a3d01d6b405 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod 8273646: Add openssl from path variable also in to Default System Openssl Path in OpensslArtifactFetcher Reviewed-by: weijun - PR: https://git.openjdk.java.net/jdk/pull/5523
