Hi, I have updated the patch after the review. webrev: http://cr.openjdk.java.net/~ykantser/6461635/webrev.04/
Changes: 1. Removed BasicTests.sh from ProblemList 2. Use sun.tools.jar.Main to create jars. 3. Application.java uses try-with-resources. 4. Changed name of nested test class from Impl to TestMain. Mattias ----- Original Message ----- From: mattias.tobias...@oracle.com To: alan.bate...@oracle.com Cc: serviceability-dev@openjdk.java.net Sent: Monday, December 2, 2013 2:59:16 PM GMT +01:00 Amsterdam / Berlin / Bern / Rome / Stockholm / Vienna Subject: Re: RFR (S): 6461635: [TESTBUG] BasicTests.sh test fails intermittently Thanks for the feedback! Q: Do you plan to remove the no-longer-exists BasicTests.sh from the exclude list (jdk/test/ProblemList.txt), I didn't see this in webrev.03. A: Yes, I forgot about ProblemList.txt. I will remove the test from the list. Q: I wasn't aware of JDKToolLauncher and just wonder if it is necessary for it to launch the jar command. In other areas then we invoke jar directly via sun.tools.jar.Main and this avoids needing to launch a new VM. A: Ok, I will use sun.tools.jar insted. Q: One thing about the shell tests is that launch the target VM with the VM options that are specified to jtreg via -vmoption. Are these used now? (if not maybe this is a separate task for the test infrastructure library). A: Both -vmoptions and -javaoptions are added by function ProcessTools.executeTestJvm() Q: Application.java - looks like this could use try-with-resources. A: Yes. I will fix that. Q: jdk/test/sun/tools/jstatd/JstatdTest.java - is this meant to be in the webrev? A: JstatdTest.java is not directly related to this fix, but it needed to be updated because of changes in the common testlibrary class ProcessThread.java. JstatdTest expected ProcessThread to verify that exit code was 0 and stdout was empty. Since ProcessThread is a common testlibrary function that more tests will use, I do not want that function to verify exit code and stdout. That check has been moved from ProcessThread to JstatdTest. JstatdTest was the only test that used ProcessThread, so no other tests needed to be updated because of this change in ProcessThread. Q: class Impl. If you are looking for an alternative name then something like TestMain might be more obvious. A: In version webrev.03, the Impl classes are no longer used. Instead there are only 1 java file that contains both setup and test code. The test code is in a nested class. There are very small changes between webrev.00 and webrev.03 other than this merge of the two java classes for each test. Mattias ----- Original Message ----- From: alan.bate...@oracle.com To: mattias.tobias...@oracle.com Cc: serviceability-dev@openjdk.java.net Sent: Monday, December 2, 2013 2:21:32 PM GMT +01:00 Amsterdam / Berlin / Bern / Rome / Stockholm / Vienna Subject: Re: RFR (S): 6461635: [TESTBUG] BasicTests.sh test fails intermittently On 02/12/2013 10:51, Mattias Tobiasson wrote: Hi, Could someone please review this? Leonid Mesnik has looked at it and he is ok with the changes. Unfortunately he is not a reviewer. I am waiting with another test fix that is dependent on this fix. webrev: http://cr.openjdk.java.net/~miauno/6461635/webrev.03/ I went through the changes in the webrev and it looks very good and nice to see more shell tests going away. Do you plan to remove the no-longer-exists BasicTests.sh from the exclude list (jdk/test/ProblemList.txt), I didn't see this in webrev.03. I wasn't aware of JDKToolLauncher and just wonder if it is necessary for it to launch the jar command. In other areas then we invoke jar directly via sun.tools.jar.Main and this avoids needing to launch a new VM. One thing about the shell tests is that launch the target VM with the VM options that are specified to jtreg via -vmoption. Are these used now? (if not maybe this is a separate task for the test infrastructure library). Some small comments as I read through it: Application.java - looks like this could use try-with-resources. jdk/test/sun/tools/jstatd/JstatdTest.java - is this meant to be in the webrev? class Impl. If you are looking for an alternative name then something like TestMain might be more obvious. Otherwise, as I said, this is good work and nice to see BasicTests running again (as he has been excluded for a long time, I think mostly due to the redefine/Xshare issue). -Alan
