RFR JDK-8216386: vmTestbase/nsk/jvmti/PopFrame/popframe005/TestDescription.java fails

[serguei.spitsyn at oracle.com]

Hi Alex,

It looks good to me.

Thanks,
Serguei


On 1/17/19 12:21 PM, Alex Menkov wrote:
> Hi Gary,
>
> On 01/17/2019 10:53, Gary Adams wrote:
>> I like the fact that test.timeout.factor is applied as a multiplier.
>>
>> It's not clear why an upper limit had to be added.
>
> As you noted there 3 cases where Thread.join() is called, but the 
> expected behavior is different. objWaiterThr2 and popFrameClsThr are 
> expected to exit, but objWaiterThr1 is expected to be alive after join 
> (i.e. the call is expected to take the time specified). I don't want 
> to increase the test run time (for timeout.factor == 10 it would take 
> 18 extra seconds), so I restricted the timeout for the case.
>
>> Is that 50 or 5 seconds?
>
> Thank you for the catch. It should be 5 seconds (5000 ms)
>
> Updated webrev:
> http://cr.openjdk.java.net/~amenkov/popframe005_wait_time/webrev.02/
>
> --alex
>
>>
>>   148             objWaiterThr1.join(Math.min(WAIT_TIME, 50000));
>>
>> Why are the other wait times not limited?
>>
>>   136             objWaiterThr2.join(WAIT_TIME);
>>
>> ...
>>
>>   169                 popFrameClsThr.join(WAIT_TIME);
>>
>>
>> If you need to apply the upper limit, then it would be better to 
>> apply it
>> at the beginning.
>>
>>    38     static final long WAIT_TIME = 
>> Math.min(Utils.adjustTimeout(2000),5000);
>>
>>
>>
>>
>>
>> On 1/16/19, 6:28 PM, Alex Menkov wrote:
>>> Hi all,
>>>
>>> please review a fix for
>>> https://bugs.openjdk.java.net/browse/JDK-8216386
>>> webrev:
>>> http://cr.openjdk.java.net/~amenkov/popframe005_wait_time/webrev/
>>>
>>> The fix updates WAIT_TIME to depend on test.timeout.factor system 
>>> property.
>>> WAIT_TIME value is used as argument of Thread.join().
>>> For the case when the thread is expected to be alive (i.e. 
>>> Thread.join() exits by timeout) the timeout value is restricted by 5 
>>> seconds to avoid long run time with big timeout.factor values.
>>>
>>> --alex
>>