Hi, I've missed one file in the changeset reviewed before, therefore the push failed and I have to ask for a quick re-review.
The missing change is here: http://cr.openjdk.java.net/~akulyakh/8153992_02/test/testlibrary/jdk/test/lib/dcmd/FileJcmdExecutor.java.udiff.html I've verified that all the hotspot tests have passed with that change. The full changeset (reviewed before): CR: https://bugs.openjdk.java.net/browse/JDK-8153992 "Some SVC tests fail on compact2 due to an unnecessary test library dependency" Webrev: http://cr.openjdk.java.net/~akulyakh/8153992_02 Best regards, Alexander ----- Forwarded Message ----- From: [email protected] To: [email protected], [email protected] Cc: [email protected] Sent: Thursday, April 21, 2016 8:48:23 PM GMT +03:00 Iraq Subject: Re: RFR:8153992:Some hotspot tests fail on compact2 due to an unnecessary test library dependency The patch looks fine to me. Due to ProcessTools.getVmInputArguments() dependency, any test using ProcessTools has @modules java.management even it does not use this method. It’d be good to refactor ProcessTools.getVmInputArguments() and maybe other test library to eliminate unnecessary dependency. Shura has cleaned up jdk/test/lib/testlibrary in the jdk side: https://bugs.openjdk.java.net/browse/JDK-8139430 Can you file a separate issue to refactor the hotspot test library and similar fix to JDK-8139430 can be applied in the future? thanks Mandy > On Apr 21, 2016, at 5:23 AM, Alexander Kulyakhtin > <[email protected]> wrote: > > Mandy, > > Thank you very much for your review. > > I have updated the fix per your comments, making ProcessTools.getProcessId() > return long. > > The function ProcessTools.getVmInputArguments(), although does depend on the > API from the java.management, is not used by any of the tests addressed by > the CR. > > Best regards, > Alexander > > ----- Original Message ----- > From: [email protected] > To: [email protected] > Cc: [email protected] > Sent: Thursday, April 21, 2016 3:11:46 PM GMT +03:00 Iraq > Subject: Re: RFR:8153992:Some hotspot tests fail on compact2 due to an > unnecessary test library dependency > > Hi, > > Could you, please, review this fix (updated to address the findings of the > previous review) > > CR: https://bugs.openjdk.java.net/browse/JDK-8153992 "Some SVC tests fail on > compact2 due to an unnecessary test library dependency" > Webrev: http://cr.openjdk.java.net/~akulyakh/8153992_02/index.html > > Before the fix the ProcessTools.getProcessId() used the > ManagementFactory.getRuntimeMXBean() API. > The API is not available on compact2 and below. Therefore the tests failed. > We are changing the ProcessTools.getProcessId() method to use the JDK 9 > Process.getPid(). This eliminates the unnecessary dependency making the tests > pass on compact2. > > Since, with this change ProcessTools.getProcessId() now returns long we are > also trivially modifying all the affected tests. > > Best regards, > Alexander > > > > ----- Original Message ----- > From: [email protected] > To: [email protected] > Cc: [email protected] > Sent: Thursday, April 21, 2016 12:03:14 AM GMT +03:00 Iraq > Subject: Re: RFR:8153989:Some SVC tests fail on compact2 due to an > unnecessary test library dependency > > >> On Apr 20, 2016, at 9:06 AM, Alexander Kulyakhtin >> <[email protected]> wrote: >> >> Hi, >> >> Could you, please, review this small tests-only fix: >> >> CR: https://bugs.openjdk.java.net/browse/JDK-8153992 "Some SVC tests fail on >> compact2 due to an unnecessary test library dependency" >> Webrev: >> http://cr.openjdk.java.net/~akulyakh/8153992/test/testlibrary/jdk/test/lib/ProcessTools.java.udiff.html >> >> Before the fix the ProcessTools.getProcessId() used the >> ManagementFactory.getRuntimeMXBean() API. >> The API is not available on compact2 and below. Therefore the tests failed. >> >> We are changing the ProcessTools.getProcessId() method to use the JDK 9 >> Process.getPid(). This eliminates the unnecessary dependency making the >> tests pass on compact2. >> > > This looks okay. But I see that getVmInputArguments calls > ManagementFactory.getRuntimeMXBean. So ProcessTools still has a dependency > on java.management. > > The jdk test library ProcessTools::getProcessId has been long ago to call > Process::getPid and the method is changed to return long. I thought that > similar change would have been made in the hotspot test library at that time. > >> I am not sure how acceptable it is to cast from long to int this change. If >> it is not acceptable we can change the return type to long. >> This however, will cause massive changes throughout the hotspot tests which >> presently expect getProcessId() to return int. > > IMO it would be good to return long or have the callsite to call > ProcessHandle.current().getPid(). > > Mandy >
