Hello, Oleg. Looks good.
With best regards. Petr. On 14 мая 2014 г., at 19:02, Oleg Pekhovskiy <oleg.pekhovs...@oracle.com> wrote: > Hi Petr, > > thank you for the review, here are my thoughts: > > 1. I added new method that way after analyzing the other methods and their > usage in jtreg tests. If you look at getExecutionCommand() you could see the > similar solution. Indeed my changes don't break the previous usages of > ProcessCommunicator. It's also possible to perform bunch run, but of course > without the termination ability. Another issue is that we must kill a child > VM explicitly if it doesn't exit automatically, but we must do this only > after test sequence is passed. > jtreg doesn't kill child VMs on timeout, this is the known problem (that's > why this JIRA issue exists). > > 2. I decided to leave it as is, as main classes in these tests are used in > two cases: applet itself and the main class for child VM. > > 3. I found this mistake and added it to the second version of fix. > > Regards, > Oleg > > On 14.05.2014 14:01, Petr Pchelko wrote: >> Hello, Oleg. >> >> 1. I'm not sure your about your new API in ProcessCommunicator. >> First of all, now the communicator can't be used twice in a single test to >> open and then close a bunch of processes. >> Also, you need to modify the tests themselves. >> I think it would be better to do the termination implicitly: you can get >> jtreg property with execution timeout, start a timer >> thread when you create a process and kill it after the timeout. In case >> doWaitFor finishes you kill the timer thread. >> >> 2. We normally remove .html if it's possible to convert test to a standalone >> app. >> >> 3. NoFormatsCrashTest lacks .java file. >> >> With best regards. Petr. >> >> On 13 мая 2014 г., at 14:55, Sergey Bylokhov <sergey.bylok...@oracle.com> >> wrote: >> >>> Hi. Oleg. >>> A few notes. >>> - Some tests contains empty/non-rethrow catch blocks. I guess we should >>> rethrow an exception and the test should fail in this case. >>> - Some tests use System.exit() which should be replaced by the exception. >>> >>> On 5/9/14 7:32 PM, Oleg Pekhovskiy wrote: >>>> Hi all, >>>> >>>> please review the fix >>>> http://cr.openjdk.java.net/~bagiras/9/8014755.1/ >>>> for >>>> https://bugs.openjdk.java.net/browse/JDK-8014755 >>>> >>>> These tests were moved from the closed workspace. >>>> >>>> The main idea of fix is to force termination of child JVM if it's not >>>> exited automatically after the test sequence. >>>> >>>> Tests accuracy was improved to cover all error cases. >>>> >>>> destroyProcess() method was added to >>>> test.java.awt.regtesthelpers.process.ProcessCommunicator >>>> for termination of child JVM when needed. >>>> >>>> Thanks, >>>> Oleg >>>> >>> >>> >>> -- >>> Best regards, Sergey. >>> >>
