[9] Review request : JDK-8066798: [TEST] Make java/lang/invoke/LFCaching tests use lib/testlibrary/jdk/testlibrary/TimeLimitedRunner.java to define their number of iterations

[Konstantin Shefov]

Vladimir, Paul,

Please, look at this fix 
http://cr.openjdk.java.net/~kshefov/8066798/webrev.04

Thanks
-Konstantin

On 10.12.2014 18:05, Igor Ignatyev wrote:
> cool. reviewed.
>
> -- 
> Igor
> On 12/10/2014 06:01 PM, Konstantin Shefov wrote:
>> Igor, I changed to printf and indent:
>>
>> http://cr.openjdk.java.net/~kshefov/8066798/webrev.04
>>
>> -Konstantin
>>
>> On 10.12.2014 16:39, Igor Ignatyev wrote:
>>>>  147 testCounter++;
>>>>  148                 }
>>>
>>>>  155 throw new Error(String.format("%d of %d test cases
>>>> FAILED! %n"
>>>>  156                     + "Rerun the test with the same \"-Dseed=\"
>>>> option as in the log file!",
>>>>  157                     failCounter, testCounter));
>>> incorrect indent
>>>
>>>>  System.err.println(String.format("All %d test cases PASSED!",
>>>> testCounter));
>>> PrintStream::printf
>>>
>>> otherwise LGTM
>>>
>>> Thanks,
>>> Igor
>>>
>>> On 12/10/2014 03:25 PM, Konstantin Shefov wrote:
>>>>
>>>> On 09.12.2014 16:50, Igor Ignatyev wrote:
>>>>> on last thing to think about:
>>>>> does it make sense to move 'if (!run.passed) { ... } else { ... }'
>>>>> code into TestRun class?
>>>> Yes, here is a webrev:
>>>> http://cr.openjdk.java.net/~kshefov/8066798/webrev.03
>>>>>
>>>>>
>>>>> On 12/09/2014 04:03 PM, Konstantin Shefov wrote:
>>>>>> Igor,
>>>>>>
>>>>>> I changed the webrev:
>>>>>> http://cr.openjdk.java.net/~kshefov/8066798/webrev.02
>>>>>>
>>>>>> -Konstantin
>>>>>>
>>>>>> On 09.12.2014 15:03, Igor Ignatyev wrote:
>>>>>>> Konstantin,
>>>>>>>
>>>>>>> well, now I don't see any points to have a lambda. you can simple
>>>>>>> make
>>>>>>> it a method in TestRunInfo and rename TestRunInfo into TestRun.
>>>>>>>
>>>>>>>>  174 t.printStackTrace();
>>>>>>>>  175 System.err.println("FAILED");
>>>>>>> you don't print exception message.
>>>>>>>
>>>>>>> -- 
>>>>>>> Igor
>>>>>>>
>>>>>>> On 12/09/2014 02:19 PM, Konstantin Shefov wrote:
>>>>>>>> Hi Igor
>>>>>>>>
>>>>>>>> Thanks for reviewing.
>>>>>>>> I have made some changes, added a separate class to store the
>>>>>>>> variables.
>>>>>>>> "doneIterations" is not local for lambda, it is incremented after
>>>>>>>> each
>>>>>>>> lambda execution.
>>>>>>>> Also I changed to catch Exception.
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~kshefov/8066798/webrev.01
>>>>>>>>
>>>>>>>> -Konstantin
>>>>>>>>
>>>>>>>> On 09.12.2014 12:28, Igor Ignatyev wrote:
>>>>>>>>> Hi Konstantin,
>>>>>>>>>
>>>>>>>>> 0. I don't like static variables which you introduced. were I 
>>>>>>>>> you,
>>>>>>>>> I'd
>>>>>>>>> create a class which encapsulates them.
>>>>>>>>> + it looks like
>>>>>>>>>  - doneIterations is a local for the lambda
>>>>>>>>>  - totalIterations is almost affectively final
>>>>>>>>>  - testCounter/failCounter are just informative variable and 
>>>>>>>>> can be
>>>>>>>>> omitted
>>>>>>>>> so you just need a way to store/get passed value. that can be 
>>>>>>>>> done
>>>>>>>>> via
>>>>>>>>> mutable box, e.g. new boolean[1], AtomicBoolean.
>>>>>>>>>
>>>>>>>>> 1. why do you catch Throwable? please catch the most specific
>>>>>>>>> exception
>>>>>>>>> 2. at lines 174-176, you lose exception information, it's better
>>>>>>>>> not
>>>>>>>>> to print exception's stack trace, but pass the exception to 
>>>>>>>>> ctor of
>>>>>>>>> Error.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Igor
>>>>>>>>>
>>>>>>>>> On 12/09/2014 11:51 AM, Konstantin Shefov wrote:
>>>>>>>>>> Hello,
>>>>>>>>>>
>>>>>>>>>> Please review the test enhancement
>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8066798
>>>>>>>>>> Webrev is http://cr.openjdk.java.net/~kshefov/8066798/webrev.00
>>>>>>>>>>
>>>>>>>>>> Test has been modified to use
>>>>>>>>>> lib/testlibrary/jdk/testlibrary/TimeLimitedRunner.java class to
>>>>>>>>>> define
>>>>>>>>>> its number of iterations depending on the given timeout.
>>>>>>>>>> Also @ignore tag has been removed from
>>>>>>>>>> test/java/lang/invoke/LFCaching/LFGarbageCollectedTest.java
>>>>>>>>>> because
>>>>>>>>>> JDK-8057020 has been fixed in JDK 9.
>>>>>>>>>>
>>>>>>>>>> Thanks
>>>>>>>>>>
>>>>>>>>>> -Konstantin
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>
>>>>
>>