[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

[Igor Ignatyev]

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
>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>
>