[9] Review request : JDK-8059070: [TESTBUG] java/lang/invoke/LFCaching/LFMultiThreadCachingTest.java failed - timeout

[Igor Ignatyev]

Konstantin,

I haven't looked at code religiously, so I wouldn't say that I have 
reviewed it. however I have some comments, please see them inline.

On 10/16/2014 02:03 PM, Konstantin Shefov wrote:
> Paul,
>
> Thanks for reviewing
>
> In the jtreg scripts of the three existing LFCaching tests timeout is
> set explicitly to 300 seconds. The file currently being changed is not a
> test itself, it is parent class of tests.
> In fact we can unset this explicit timeout and use the current fix
> together with default JTREG timeout and jtreg timeout factor option.
> These test cases are randomly generated, so the more iterations, the
> better, but in fact we need at least one iteration.
>
> As for "if (avgIterTime > 2 * remainTime)", I think 2 is enough.
>
> -Konstantin
>
> On 16.10.2014 13:30, Paul Sandoz wrote:
>> On Oct 16, 2014, at 10:43 AM, Konstantin Shefov
>> <konstantin.shefov at oracle.com> wrote:
>>
>>> Gently reminder
>>>
>>> On 14.10.2014 16:58, Konstantin Shefov wrote:
>>>> Hello,
>>>>
>>>> Please review the test bug fix
>>>> https://bugs.openjdk.java.net/browse/JDK-8059070
>>>> Webrev is http://cr.openjdk.java.net/~kshefov/8059070/webrev.00/
>>>>
>>    45     private static final long TIMEOUT = 300000;
>>
>> Does jtreg define a system property for this value? and is 300s the
>> same as what jtreg defines as the default?
unfortunately, jtreg doesn't define a property for timeout, but I think 
it should do. we need to file an RFE against jtreg to add such a 
property and an RFE against these tests/testlibrary to use this 
property. could you please file them?
>>
>> The online documentation (jtreg -onlineHelp) says the default is 2
>> minutes, but that might be out of date.
>>
>> Might be more readable to use:
>>
>>    TimeUnit.SECONDS.toMillis(300);
>>
>>
>>   143             long passedTime = new Date().getTime() - startTime;
>>
>> Why don't you use System.currentTimeMillis() instead of "new
>> Date().getTime()" ?
>>
>>
>>   145             double timeoutFactor = new
>> Double(System.getProperty("test.timeout.factor", "1.0"));
>>
>> You can that pull out into a static and perhaps merge with TIMEOUT.
+ you should use jdk.testlibrary.Utils::adjustTimeout instead of writing 
this code again.
>>
>>
>>   147             if (avgIterTime > 2 * remainTime) {
>>
>> That seems sufficient but it will be interesting to see if
>> intermittent failures still occur due to high variance.
>>
>> Paul.
>


Igor