RFR(S) : 8180386: remove jdk.testlibrary.TimeLimitedRunner

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR(S) : 8180386: remove jdk.testlibrary.TimeLimitedRunner

Igor Ignatyev
http://cr.openjdk.java.net/~iignatyev//8180386/webrev.00/index.html
>  lines changed: 2 ins; 88 del; 6 mod;

Hi all,

could you please review this small fix which removes TimeLimitedRunner class from jdk testlibrary? we have two TimeLimitedRunner classes one in jdk test library and another in the the toplevel test library, there is no difference b/w these two library classes (besides location and package name), so the tests were simply updated to use TimeLimitedRunner from the toplevel test library.

this fix is a part of ongoing effort on merging and cleaning up our test libraries[1].

webrev: http://cr.openjdk.java.net/~iignatyev//8180386/webrev.00/index.html
JBS: https://bugs.openjdk.java.net/browse/JDK-8180386
testing: affected tests (java/lang/invoke)

[1] https://bugs.openjdk.java.net/browse/JDK-8075327

Thanks,
-- Igor
Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) : 8180386: remove jdk.testlibrary.TimeLimitedRunner

Igor Ignatyev
Hi,

still looking for a Reviewer.

Cheers,
-- Igor

> On May 15, 2017, at 2:00 PM, Igor Ignatyev <[hidden email]> wrote:
>
> http://cr.openjdk.java.net/~iignatyev//8180386/webrev.00/index.html
>> lines changed: 2 ins; 88 del; 6 mod;
>
> Hi all,
>
> could you please review this small fix which removes TimeLimitedRunner class from jdk testlibrary? we have two TimeLimitedRunner classes one in jdk test library and another in the the toplevel test library, there is no difference b/w these two library classes (besides location and package name), so the tests were simply updated to use TimeLimitedRunner from the toplevel test library.
>
> this fix is a part of ongoing effort on merging and cleaning up our test libraries[1].
>
> webrev: http://cr.openjdk.java.net/~iignatyev//8180386/webrev.00/index.html
> JBS: https://bugs.openjdk.java.net/browse/JDK-8180386
> testing: affected tests (java/lang/invoke)
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8075327
>
> Thanks,
> -- Igor

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) : 8180386: remove jdk.testlibrary.TimeLimitedRunner

Mandy Chung
In reply to this post by Igor Ignatyev

> On May 15, 2017, at 2:00 PM, Igor Ignatyev <[hidden email]> wrote:
>
> http://cr.openjdk.java.net/~iignatyev//8180386/webrev.00/index.html
>> lines changed: 2 ins; 88 del; 6 mod;
>
> Hi all,
>
> could you please review this small fix which removes TimeLimitedRunner class from jdk testlibrary? we have two TimeLimitedRunner classes one in jdk test library and another in the the toplevel test library, there is no difference b/w these two library classes (besides location and package name), so the tests were simply updated to use TimeLimitedRunner from the toplevel test library.
>
> this fix is a part of ongoing effort on merging and cleaning up our test libraries[1].
>
> webrev: http://cr.openjdk.java.net/~iignatyev//8180386/webrev.00/index.html

This comment is not related to the change.  The package name `jdk.test.lib.wrappers` is odd.  TimeLimitedRunner and InfiniteLoop classes could simply be in the jdk.test.lib package or jdk.test.lib.util package.

Mandy

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) : 8180386: remove jdk.testlibrary.TimeLimitedRunner

Igor Ignatyev

> On May 18, 2017, at 2:34 PM, Mandy Chung <[hidden email]> wrote:
>> On May 15, 2017, at 2:00 PM, Igor Ignatyev <[hidden email]> wrote:
>>
>> http://cr.openjdk.java.net/~iignatyev//8180386/webrev.00/index.html
>>> lines changed: 2 ins; 88 del; 6 mod;
>>
>> Hi all,
>>
>> could you please review this small fix which removes TimeLimitedRunner class from jdk testlibrary? we have two TimeLimitedRunner classes one in jdk test library and another in the the toplevel test library, there is no difference b/w these two library classes (besides location and package name), so the tests were simply updated to use TimeLimitedRunner from the toplevel test library.
>>
>> this fix is a part of ongoing effort on merging and cleaning up our test libraries[1].
>>
>> webrev: http://cr.openjdk.java.net/~iignatyev//8180386/webrev.00/index.html
>
> This comment is not related to the change.  The package name `jdk.test.lib.wrappers` is odd.  TimeLimitedRunner and InfiniteLoop classes could simply be in the jdk.test.lib package or jdk.test.lib.util package.
>
> Mandy
>

agree, I'll file an RFE to find a better package for TimeLimitedRunner and InfiniteLoop classes.

-- Igor

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) : 8180386: remove jdk.testlibrary.TimeLimitedRunner

Mandy Chung

> On May 18, 2017, at 2:37 PM, Igor Ignatyev <[hidden email]> wrote:
>
>
>> On May 18, 2017, at 2:34 PM, Mandy Chung <[hidden email]> wrote:
>> This comment is not related to the change.  The package name `jdk.test.lib.wrappers` is odd.  TimeLimitedRunner and InfiniteLoop classes could simply be in the jdk.test.lib package or jdk.test.lib.util package.
>>
>> Mandy
>>
>
> agree, I'll file an RFE to find a better package for TimeLimitedRunner and InfiniteLoop classes.

Why not doing the rename with this patch?

Mandy

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) : 8180386: remove jdk.testlibrary.TimeLimitedRunner

Igor Ignatyev

> On May 18, 2017, at 2:41 PM, Mandy Chung <[hidden email]> wrote:
>
>
>> On May 18, 2017, at 2:37 PM, Igor Ignatyev <[hidden email]> wrote:
>>
>>
>>> On May 18, 2017, at 2:34 PM, Mandy Chung <[hidden email]> wrote:
>>> This comment is not related to the change.  The package name `jdk.test.lib.wrappers` is odd.  TimeLimitedRunner and InfiniteLoop classes could simply be in the jdk.test.lib package or jdk.test.lib.util package.
>>>
>>> Mandy
>>>
>>
>> agree, I'll file an RFE to find a better package for TimeLimitedRunner and InfiniteLoop classes.
>
> Why not doing the rename with this patch?

there are hotspot tests which use jdk.test.lib.wrappers.TimeLimitedRunner and InfiniteLoop, so it will require changes in hotspot repo. I'd prefer to have removing classes from jdk testlibrary and renaming classes in top level testlibrary as separate patches for clarity purposes.

-- Igor
Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) : 8180386: remove jdk.testlibrary.TimeLimitedRunner

Igor Ignatyev
Hi Mandy,

8180793[1], which moved j.t.l.wrappers.* to j.t.lib package, has been propagated to jdk10/jdk10. I have updated the patch accordingly -- http://cr.openjdk.java.net/~iignatyev//8180386/webrev.01/index.html 
could you please take a look?

[1] https://bugs.openjdk.java.net/browse/JDK-8180793

Thanks,
-- Igor

> On May 18, 2017, at 2:49 PM, Igor Ignatyev <[hidden email]> wrote:
>
>>
>> On May 18, 2017, at 2:41 PM, Mandy Chung <[hidden email]> wrote:
>>
>>
>>> On May 18, 2017, at 2:37 PM, Igor Ignatyev <[hidden email]> wrote:
>>>
>>>
>>>> On May 18, 2017, at 2:34 PM, Mandy Chung <[hidden email]> wrote:
>>>> This comment is not related to the change.  The package name `jdk.test.lib.wrappers` is odd. TimeLimitedRunner and InfiniteLoop classes could simply be in the jdk.test.lib package or jdk.test.lib.util package.
>>>>
>>>> Mandy
>>>>
>>>
>>> agree, I'll file an RFE to find a better package for TimeLimitedRunner and InfiniteLoop classes.
>>
>> Why not doing the rename with this patch?
>
> there are hotspot tests which use jdk.test.lib.wrappers.TimeLimitedRunner and InfiniteLoop, so it will require changes in hotspot repo. I'd prefer to have removing classes from jdk testlibrary and renaming classes in top level testlibrary as separate patches for clarity purposes.
>
> -- Igor

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) : 8180386: remove jdk.testlibrary.TimeLimitedRunner

Igor Ignatyev
still looking for a Reviewer.

-- Igor

> On May 31, 2017, at 2:57 PM, Igor Ignatyev <[hidden email]> wrote:
>
> Hi Mandy,
>
> 8180793[1], which moved j.t.l.wrappers.* to j.t.lib package, has been propagated to jdk10/jdk10. I have updated the patch accordingly -- http://cr.openjdk.java.net/~iignatyev//8180386/webrev.01/index.html 
> could you please take a look?
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8180793
>
> Thanks,
> -- Igor
>
>> On May 18, 2017, at 2:49 PM, Igor Ignatyev <[hidden email]> wrote:
>>
>>>
>>> On May 18, 2017, at 2:41 PM, Mandy Chung <[hidden email]> wrote:
>>>
>>>
>>>> On May 18, 2017, at 2:37 PM, Igor Ignatyev <[hidden email]> wrote:
>>>>
>>>>
>>>>> On May 18, 2017, at 2:34 PM, Mandy Chung <[hidden email]> wrote:
>>>>> This comment is not related to the change.  The package name `jdk.test.lib.wrappers` is odd. TimeLimitedRunner and InfiniteLoop classes could simply be in the jdk.test.lib package or jdk.test.lib.util package.
>>>>>
>>>>> Mandy
>>>>>
>>>>
>>>> agree, I'll file an RFE to find a better package for TimeLimitedRunner and InfiniteLoop classes.
>>>
>>> Why not doing the rename with this patch?
>>
>> there are hotspot tests which use jdk.test.lib.wrappers.TimeLimitedRunner and InfiniteLoop, so it will require changes in hotspot repo. I'd prefer to have removing classes from jdk testlibrary and renaming classes in top level testlibrary as separate patches for clarity purposes.
>>
>> -- Igor
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) : 8180386: remove jdk.testlibrary.TimeLimitedRunner

roger riggs
Hi Igor,

It is odd that test/TEST.ROOT shows up in the webrev with no changes.

Seeing @library with both /lib/testlibrary and /test/lib makes me wonder
when that will get resolved?

Otherwise, looks fine

Thanks, Roger


On 6/7/2017 2:44 AM, Igor Ignatyev wrote:

> still looking for a Reviewer.
>
> -- Igor
>> On May 31, 2017, at 2:57 PM, Igor Ignatyev <[hidden email]> wrote:
>>
>> Hi Mandy,
>>
>> 8180793[1], which moved j.t.l.wrappers.* to j.t.lib package, has been propagated to jdk10/jdk10. I have updated the patch accordingly -- http://cr.openjdk.java.net/~iignatyev//8180386/webrev.01/index.html
>> could you please take a look?
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8180793
>>
>> Thanks,
>> -- Igor
>>
>>> On May 18, 2017, at 2:49 PM, Igor Ignatyev <[hidden email]> wrote:
>>>
>>>> On May 18, 2017, at 2:41 PM, Mandy Chung <[hidden email]> wrote:
>>>>
>>>>
>>>>> On May 18, 2017, at 2:37 PM, Igor Ignatyev <[hidden email]> wrote:
>>>>>
>>>>>
>>>>>> On May 18, 2017, at 2:34 PM, Mandy Chung <[hidden email]> wrote:
>>>>>> This comment is not related to the change.  The package name `jdk.test.lib.wrappers` is odd. TimeLimitedRunner and InfiniteLoop classes could simply be in the jdk.test.lib package or jdk.test.lib.util package.
>>>>>>
>>>>>> Mandy
>>>>>>
>>>>> agree, I'll file an RFE to find a better package for TimeLimitedRunner and InfiniteLoop classes.
>>>> Why not doing the rename with this patch?
>>> there are hotspot tests which use jdk.test.lib.wrappers.TimeLimitedRunner and InfiniteLoop, so it will require changes in hotspot repo. I'd prefer to have removing classes from jdk testlibrary and renaming classes in top level testlibrary as separate patches for clarity purposes.
>>>
>>> -- Igor

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) : 8180386: remove jdk.testlibrary.TimeLimitedRunner

Igor Ignatyev
Hi Roger,

please see my comments inline.

-- Igor

> On Jun 7, 2017, at 7:48 AM, Roger Riggs <[hidden email]> wrote:
>
> Hi Igor,
>
> It is odd that test/TEST.ROOT shows up in the webrev with no changes.
it seems there were trailing spaces at line#13[1] which my vi removed on closing. if you want, I can revert that before pushing.

[1]
-# run. Tests that are not headful are "headless."
+# run. Tests that are not headful are "headless."


> Seeing @library with both /lib/testlibrary and /test/lib makes me wonder when that will get resolved?
this will be resolved that the rest of testlibrary from /lib/testlibrary will be merged w/ library in /test/lib by JDK-8075327[2]. I expect at least 4 other sub-tasks.

[2] https://bugs.openjdk.java.net/browse/JDK-8075327
>
> Otherwise, looks fine
thank you for your review.

> Thanks, Roger
>
> On 6/7/2017 2:44 AM, Igor Ignatyev wrote:
>> still looking for a Reviewer.
>>
>> -- Igor
>>> On May 31, 2017, at 2:57 PM, Igor Ignatyev <[hidden email]> wrote:
>>>
>>> Hi Mandy,
>>>
>>> 8180793[1], which moved j.t.l.wrappers.* to j.t.lib package, has been propagated to jdk10/jdk10. I have updated the patch accordingly -- http://cr.openjdk.java.net/~iignatyev//8180386/webrev.01/index.html
>>> could you please take a look?
>>>
>>> [1] https://bugs.openjdk.java.net/browse/JDK-8180793
>>>
>>> Thanks,
>>> -- Igor
>>>
>>>> On May 18, 2017, at 2:49 PM, Igor Ignatyev <[hidden email]> wrote:
>>>>
>>>>> On May 18, 2017, at 2:41 PM, Mandy Chung <[hidden email]> wrote:
>>>>>
>>>>>
>>>>>> On May 18, 2017, at 2:37 PM, Igor Ignatyev <[hidden email]> wrote:
>>>>>>
>>>>>>
>>>>>>> On May 18, 2017, at 2:34 PM, Mandy Chung <[hidden email]> wrote:
>>>>>>> This comment is not related to the change.  The package name `jdk.test.lib.wrappers` is odd. TimeLimitedRunner and InfiniteLoop classes could simply be in the jdk.test.lib package or jdk.test.lib.util package.
>>>>>>>
>>>>>>> Mandy
>>>>>>>
>>>>>> agree, I'll file an RFE to find a better package for TimeLimitedRunner and InfiniteLoop classes.
>>>>> Why not doing the rename with this patch?
>>>> there are hotspot tests which use jdk.test.lib.wrappers.TimeLimitedRunner and InfiniteLoop, so it will require changes in hotspot repo. I'd prefer to have removing classes from jdk testlibrary and renaming classes in top level testlibrary as separate patches for clarity purposes.
>>>>
>>>> -- Igor
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) : 8180386: remove jdk.testlibrary.TimeLimitedRunner

roger riggs
Hi Igor,

Removing trailing spaces is fine.

Thanks, Roger


On 6/7/2017 11:29 AM, Igor Ignatyev wrote:

> Hi Roger,
>
> please see my comments inline.
>
> -- Igor
>
>> On Jun 7, 2017, at 7:48 AM, Roger Riggs <[hidden email]> wrote:
>>
>> Hi Igor,
>>
>> It is odd that test/TEST.ROOT shows up in the webrev with no changes.
> it seems there were trailing spaces at line#13[1] which my vi removed on closing. if you want, I can revert that before pushing.
>
> [1]
> -# run. Tests that are not headful are "headless."
> +# run. Tests that are not headful are "headless."
>
>
>> Seeing @library with both /lib/testlibrary and /test/lib makes me wonder when that will get resolved?
> this will be resolved that the rest of testlibrary from /lib/testlibrary will be merged w/ library in /test/lib by JDK-8075327[2]. I expect at least 4 other sub-tasks.
>
> [2] https://bugs.openjdk.java.net/browse/JDK-8075327
>> Otherwise, looks fine
> thank you for your review.
>
>> Thanks, Roger
>>
>> On 6/7/2017 2:44 AM, Igor Ignatyev wrote:
>>> still looking for a Reviewer.
>>>
>>> -- Igor
>>>> On May 31, 2017, at 2:57 PM, Igor Ignatyev <[hidden email]> wrote:
>>>>
>>>> Hi Mandy,
>>>>
>>>> 8180793[1], which moved j.t.l.wrappers.* to j.t.lib package, has been propagated to jdk10/jdk10. I have updated the patch accordingly -- http://cr.openjdk.java.net/~iignatyev//8180386/webrev.01/index.html
>>>> could you please take a look?
>>>>
>>>> [1] https://bugs.openjdk.java.net/browse/JDK-8180793
>>>>
>>>> Thanks,
>>>> -- Igor
>>>>
>>>>> On May 18, 2017, at 2:49 PM, Igor Ignatyev <[hidden email]> wrote:
>>>>>
>>>>>> On May 18, 2017, at 2:41 PM, Mandy Chung <[hidden email]> wrote:
>>>>>>
>>>>>>
>>>>>>> On May 18, 2017, at 2:37 PM, Igor Ignatyev <[hidden email]> wrote:
>>>>>>>
>>>>>>>
>>>>>>>> On May 18, 2017, at 2:34 PM, Mandy Chung <[hidden email]> wrote:
>>>>>>>> This comment is not related to the change.  The package name `jdk.test.lib.wrappers` is odd. TimeLimitedRunner and InfiniteLoop classes could simply be in the jdk.test.lib package or jdk.test.lib.util package.
>>>>>>>>
>>>>>>>> Mandy
>>>>>>>>
>>>>>>> agree, I'll file an RFE to find a better package for TimeLimitedRunner and InfiniteLoop classes.
>>>>>> Why not doing the rename with this patch?
>>>>> there are hotspot tests which use jdk.test.lib.wrappers.TimeLimitedRunner and InfiniteLoop, so it will require changes in hotspot repo. I'd prefer to have removing classes from jdk testlibrary and renaming classes in top level testlibrary as separate patches for clarity purposes.
>>>>>
>>>>> -- Igor

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) : 8180386: remove jdk.testlibrary.TimeLimitedRunner

Mandy Chung
In reply to this post by Igor Ignatyev

> On May 31, 2017, at 2:57 PM, Igor Ignatyev <[hidden email]> wrote:
>
> Hi Mandy,
>
> 8180793[1], which moved j.t.l.wrappers.* to j.t.lib package, has been propagated to jdk10/jdk10. I have updated the patch accordingly -- http://cr.openjdk.java.net/~iignatyev//8180386/webrev.01/index.html <http://cr.openjdk.java.net/~iignatyev//8180386/webrev.01/index.html>
> could you please take a look?

Looks okay.

Mandy