<AWT Dev> [9] Review request for 8014755: [TEST_BUG] frames didn't closed after execution some awt/dnd/ tests

[Petr Pchelko]

Hello, Oleg.

Looks good.

With best regards. Petr.

On 14 мая 2014 г., at 19:02, Oleg Pekhovskiy <oleg.pekhovskiy at 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.Bylokhov at 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.
>>> 
>>