JDK 10 RFR of JDK-8183377: Refactor java/lang/ClassLoader/deadlock shell tests to java

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

JDK 10 RFR of JDK-8183377: Refactor java/lang/ClassLoader/deadlock shell tests to java

Amy Lu-2
java/lang/ClassLoader/deadlock/TestCrossDelegate.sh
java/lang/ClassLoader/deadlock/TestOneWayDelegate.sh

Please review this patch to refactor the shell tests to java.

bug: https://bugs.openjdk.java.net/browse/JDK-8183377
http://cr.openjdk.java.net/~amlu/8183377/webrev.00/

Thanks,
Amy
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: JDK 10 RFR of JDK-8183377: Refactor java/lang/ClassLoader/deadlock shell tests to java

Mandy Chung

> On Jul 13, 2017, at 5:14 PM, Amy Lu <[hidden email]> wrote:
>
> java/lang/ClassLoader/deadlock/TestCrossDelegate.sh
> java/lang/ClassLoader/deadlock/TestOneWayDelegate.sh
>
> Please review this patch to refactor the shell tests to java.
>
> bug: https://bugs.openjdk.java.net/browse/JDK-8183377
> http://cr.openjdk.java.net/~amlu/8183377/webrev.00/

I suggest to use the scratch directory instead of user.dir so that the files will be cleaned up.

An alternative to SetupLoader class would be use the testlibray CompileUtils to compile the classes in the specified destination.

Mandy
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: JDK 10 RFR of JDK-8183377: Refactor java/lang/ClassLoader/deadlock shell tests to java

Weijun Wang

> On Jul 14, 2017, at 4:23 PM, Mandy Chung <[hidden email]> wrote:
>
>
>> On Jul 13, 2017, at 5:14 PM, Amy Lu <[hidden email]> wrote:
>>
>> java/lang/ClassLoader/deadlock/TestCrossDelegate.sh
>> java/lang/ClassLoader/deadlock/TestOneWayDelegate.sh
>>
>> Please review this patch to refactor the shell tests to java.
>>
>> bug: https://bugs.openjdk.java.net/browse/JDK-8183377
>> http://cr.openjdk.java.net/~amlu/8183377/webrev.00/
>
> I suggest to use the scratch directory instead of user.dir so that the files will be cleaned up.

I haven’t reviewed the code but aren’t scratch and ${user.dir} the same thing?

--Max

>
> An alternative to SetupLoader class would be use the testlibray CompileUtils to compile the classes in the specified destination.
>
> Mandy

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: JDK 10 RFR of JDK-8183377: Refactor java/lang/ClassLoader/deadlock shell tests to java

Mandy Chung

> On Jul 14, 2017, at 4:26 PM, Weijun Wang <[hidden email]> wrote:
>
>
>> On Jul 14, 2017, at 4:23 PM, Mandy Chung <[hidden email]> wrote:
>>
>>
>>> On Jul 13, 2017, at 5:14 PM, Amy Lu <[hidden email]> wrote:
>>>
>>> java/lang/ClassLoader/deadlock/TestCrossDelegate.sh
>>> java/lang/ClassLoader/deadlock/TestOneWayDelegate.sh
>>>
>>> Please review this patch to refactor the shell tests to java.
>>>
>>> bug: https://bugs.openjdk.java.net/browse/JDK-8183377
>>> http://cr.openjdk.java.net/~amlu/8183377/webrev.00/
>>
>> I suggest to use the scratch directory instead of user.dir so that the files will be cleaned up.
>
> I haven’t reviewed the code but aren’t scratch and ${user.dir} the same thing?

Good catch.  I mis-read it as “user.home”.  Then that part is okay.

I think compiling the other classes to a destination explicitly to replace:
  29  * @build Alice Bob SupAlice SupBob
  30  * @run driver SetupLoader

will make the test clearer.  It’d be good to make that change.

Mandy
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: JDK 10 RFR of JDK-8183377: Refactor java/lang/ClassLoader/deadlock shell tests to java

Amy Lu-2
On 7/14/17 4:41 PM, Mandy Chung wrote:
> I think compiling the other classes to a destination explicitly to replace:
>    29  * @build Alice Bob SupAlice SupBob
>    30  * @run driver SetupLoader
>
> will make the test clearer.  It’d be good to make that change.

Webrev updated: http://cr.openjdk.java.net/~amlu/8183377/webrev.01/

Please review.

Thanks,
Amy
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: JDK 10 RFR of JDK-8183377: Refactor java/lang/ClassLoader/deadlock shell tests to java

Mandy Chung

> On Jul 17, 2017, at 12:49 AM, Amy Lu <[hidden email]> wrote:
>
> On 7/14/17 4:41 PM, Mandy Chung wrote:
>> I think compiling the other classes to a destination explicitly to replace:
>>   29  * @build Alice Bob SupAlice SupBob
>>   30  * @run driver SetupLoader
>>
>> will make the test clearer.  It’d be good to make that change.
>
> Webrev updated: http://cr.openjdk.java.net/~amlu/8183377/webrev.01/

Looks good.  One minor suggestion is to place the source files under a directory showing its package hierachy e.g. src/comSA/Alice.java, src/comSB/Bob.java

No need for a new webrev.

thanks
Mandy

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: JDK 10 RFR of JDK-8183377: Refactor java/lang/ClassLoader/deadlock shell tests to java

Amy Lu-2
On 7/25/17 1:15 AM, Mandy Chung wrote:
>> Webrev updated:http://cr.openjdk.java.net/~amlu/8183377/webrev.01/
> Looks good.  One minor suggestion is to place the source files under a directory showing its package hierachy e.g. src/comSA/Alice.java, src/comSB/Bob.java
>
> No need for a new webrev.
Thank you Mandy for reviewing.

Pushed with change as suggested.

Thanks,
Amy


Loading...