[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

[Vladimir Ivanov]

Looks good.

Best regards,
Vladimir Ivanov

On 12/11/14, 2:07 PM, Konstantin Shefov wrote:
> 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
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>
>