RFR(XS) : 8186095 : upgrade to jtreg 4.2 b08

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

RFR(XS) : 8186095 : upgrade to jtreg 4.2 b08

Igor Ignatyev
http://cr.openjdk.java.net/~iignatyev//8186095/webrev.00/index.html
> 14 lines changed: 1 ins; 0 del; 13 mod;

Hi all,

could you please review this small patch which bumps up jtreg version?
besides updating jib profiles and all TEST.ROOT files, the fix updates hotspot/test/runtime/Metaspace/FragmentMetaspaceSimple.java test not to rely on having "library" test.Empty class in 'test.classes' and put test.Empty class in the workdir instead.

testing: :hotspot_all, {jdk,langtools,nashorn,jaxp}/test/:tier[1-3]

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

Re: RFR(XS) : 8186095 : upgrade to jtreg 4.2 b08

David Holmes
Hi Igor,

On 11/08/2017 2:02 PM, Igor Ignatyev wrote:
> http://cr.openjdk.java.net/~iignatyev//8186095/webrev.00/index.html
>> 14 lines changed: 1 ins; 0 del; 13 mod;
>
> Hi all,
>
> could you please review this small patch which bumps up jtreg version?
> besides updating jib profiles and all TEST.ROOT files,

That all looks fine (though deploy should not be in there).

> the fix updates
> hotspot/test/runtime/Metaspace/FragmentMetaspaceSimple.java test not to
> rely on having "library" test.Empty class in 'test.classes' and put
> test.Empty class in the workdir instead.

Sorry I'm not following this part. You made two changes:

1. Added @library /test/lib

What is this doing? (For that matter what is the existing "classes"
entry supposed to mean ??? how is "classes" a library?)

2. Instead of the test reading from test.classes you are using the
ClassfileInstaller to copy the class to the working directory.

How does this make a difference to anything? If the test wouldn't find
the class in test.classes, doesn't that mean ClassfileInstaller will
also fail to find it?

Thanks,
David
-----

> testing: :hotspot_all, {jdk,langtools,nashorn,jaxp}/test/:tier[1-3]
>
> Thanks,
> -- Igor
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(XS) : 8186095 : upgrade to jtreg 4.2 b08

David Holmes
On 11/08/2017 2:31 PM, Igor Ignatyev wrote:

>
>> On Aug 10, 2017, at 9:22 PM, David Holmes <[hidden email]> wrote:
>>
>> Hi Igor,
>>
>> On 11/08/2017 2:02 PM, Igor Ignatyev wrote:
>>> http://cr.openjdk.java.net/~iignatyev//8186095/webrev.00/index.html
>>>> 14 lines changed: 1 ins; 0 del; 13 mod;
>>> Hi all,
>>> could you please review this small patch which bumps up jtreg version?
>>> besides updating jib profiles and all TEST.ROOT files,
>>
>> That all looks fine (though deploy should not be in there).
>>
>>> the fix updates
>>> hotspot/test/runtime/Metaspace/FragmentMetaspaceSimple.java test not to
>>> rely on having "library" test.Empty class in 'test.classes' and put
>>> test.Empty class in the workdir instead.
>>
>> Sorry I'm not following this part. You made two changes:
>>
>> 1. Added @library /test/lib
> /test/lib is needed for ClassFileInstaller.

Okay.

>>
>> What is this doing? (For that matter what is the existing "classes" entry supposed to mean ??? how is "classes" a library?)
> existing 'classes' is the directory in hotspot/test/runtime/Metaspace/ which contains source of test.Empty.

Okay.

>>
>> 2. Instead of the test reading from test.classes you are using the ClassfileInstaller to copy the class to the working directory.
>>
>> How does this make a difference to anything? If the test wouldn't find the class in test.classes, doesn't that mean ClassfileInstaller will also fail to find it?
> test.classes points to the directory w/ classes from a test, but not from test libraries. directories w/ all needed classes (either from a test or from libraries) are added to classpath and 'test.class.path'. ClassFileInstaller uses class loader to get resources, test.Empty will be in CP, so ClassFileInstaller will have access to it.

Sorry still don't understand the change. Where does:

@build test.Empty

place Empty.class? If not in test.classes then how has this test ever
passed? I'm assuming the change is needed because it no longer passes
with the updated jtreg.

Thanks,
David

>>
>> Thanks,
>> David
>> -----
>>
>>> testing: :hotspot_all, {jdk,langtools,nashorn,jaxp}/test/:tier[1-3]
>>> Thanks,
>>> -- Igor
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(XS) : 8186095 : upgrade to jtreg 4.2 b08

Igor Ignatyev

> On Aug 10, 2017, at 9:46 PM, David Holmes <[hidden email]> wrote:
>
> On 11/08/2017 2:31 PM, Igor Ignatyev wrote:
>>> On Aug 10, 2017, at 9:22 PM, David Holmes <[hidden email]> wrote:
>>>
>>> Hi Igor,
>>>
>>> On 11/08/2017 2:02 PM, Igor Ignatyev wrote:
>>>> http://cr.openjdk.java.net/~iignatyev//8186095/webrev.00/index.html
>>>>> 14 lines changed: 1 ins; 0 del; 13 mod;
>>>> Hi all,
>>>> could you please review this small patch which bumps up jtreg version?
>>>> besides updating jib profiles and all TEST.ROOT files,
>>>
>>> That all looks fine (though deploy should not be in there).
>>>
>>>> the fix updates
>>>> hotspot/test/runtime/Metaspace/FragmentMetaspaceSimple.java test not to
>>>> rely on having "library" test.Empty class in 'test.classes' and put
>>>> test.Empty class in the workdir instead.
>>>
>>> Sorry I'm not following this part. You made two changes:
>>>
>>> 1. Added @library /test/lib
>> /test/lib is needed for ClassFileInstaller.
>
> Okay.
>
>>>
>>> What is this doing? (For that matter what is the existing "classes" entry supposed to mean ??? how is "classes" a library?)
>> existing 'classes' is the directory in hotspot/test/runtime/Metaspace/ which contains source of test.Empty.
>
> Okay.
>
>>>
>>> 2. Instead of the test reading from test.classes you are using the ClassfileInstaller to copy the class to the working directory.
>>>
>>> How does this make a difference to anything? If the test wouldn't find the class in test.classes, doesn't that mean ClassfileInstaller will also fail to find it?
>> test.classes points to the directory w/ classes from a test, but not from test libraries. directories w/ all needed classes (either from a test or from libraries) are added to classpath and 'test.class.path'. ClassFileInstaller uses class loader to get resources, test.Empty will be in CP, so ClassFileInstaller will have access to it.
>
> Sorry still don't understand the change. Where does:
>
> @build test.Empty
>
> place Empty.class? If not in test.classes then how has this test ever passed? I'm assuming the change is needed because it no longer passes with the updated jtreg.
build places it in a library dedicated directory, in this case it will be 'JTwork/classes/<N>/runtime/Metaspace/classes', but 'test.classes' points to 'JTwork/classes/<N>/runtime/Metaspace/FragmentMetaspaceSimple.d'.  'test.class.path' will have both (separated by path separator), classpath has these two paths and couple others.

>
> Thanks,
> David
>
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>> testing: :hotspot_all, {jdk,langtools,nashorn,jaxp}/test/:tier[1-3]
>>>> Thanks,
>>>> -- Igor

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

Re: RFR(XS) : 8186095 : upgrade to jtreg 4.2 b08

Ioi Lam
In reply to this post by David Holmes


On 8/10/17 9:46 PM, David Holmes wrote:

> On 11/08/2017 2:31 PM, Igor Ignatyev wrote:
>>
>>> On Aug 10, 2017, at 9:22 PM, David Holmes <[hidden email]>
>>> wrote:
>>>
>>> Hi Igor,
>>>
>>> On 11/08/2017 2:02 PM, Igor Ignatyev wrote:
>>>> http://cr.openjdk.java.net/~iignatyev//8186095/webrev.00/index.html
>>>>> 14 lines changed: 1 ins; 0 del; 13 mod;
>>>> Hi all,
>>>> could you please review this small patch which bumps up jtreg version?
>>>> besides updating jib profiles and all TEST.ROOT files,
>>>
>>> That all looks fine (though deploy should not be in there).
>>>
>>>> the fix updates
>>>> hotspot/test/runtime/Metaspace/FragmentMetaspaceSimple.java test
>>>> not to
>>>> rely on having "library" test.Empty class in 'test.classes' and put
>>>> test.Empty class in the workdir instead.
>>>
>>> Sorry I'm not following this part. You made two changes:
>>>
>>> 1. Added @library /test/lib
>> /test/lib is needed for ClassFileInstaller.
>
> Okay.
>
>>>
>>> What is this doing? (For that matter what is the existing "classes"
>>> entry supposed to mean ??? how is "classes" a library?)
>> existing 'classes' is the directory in
>> hotspot/test/runtime/Metaspace/ which contains source of test.Empty.
>
> Okay.
>
>>>
>>> 2. Instead of the test reading from test.classes you are using the
>>> ClassfileInstaller to copy the class to the working directory.
>>>
>>> How does this make a difference to anything? If the test wouldn't
>>> find the class in test.classes, doesn't that mean ClassfileInstaller
>>> will also fail to find it?
>> test.classes points to the directory w/ classes from a test, but not
>> from test libraries. directories w/ all needed classes (either from a
>> test or from libraries) are added to classpath and 'test.class.path'.
>> ClassFileInstaller uses class loader to get resources, test.Empty
>> will be in CP, so ClassFileInstaller will have access to it.
>
> Sorry still don't understand the change. Where does:
>
> @build test.Empty
>
> place Empty.class? If not in test.classes then how has this test ever
> passed? I'm assuming the change is needed because it no longer passes
> with the updated jtreg.
>
Hi David,

   27  * @build test.Empty

puts test.Empty class somewhere in the classpath

   28  * @run driver ClassFileInstaller test.Empty

ClassFileInstaller uses Class.getResourceAsStream("test/Empty.class") to
read the contents of this class (from the CLASSPATH), and writes it to
./test/Empty.class

   55         String fileName = "test" + sep + "Empty.class";
   56         File file = new File(fileName);
   57         byte buff[] = read(file);

here the test reads "./test/Empty.class" into a buffer. (When the test
runs, the current directory is the "scratch" directory).

The old version of this test assumed that JTREG builds the Empty.class
into a certain location, but that has changed with the jtreg 4.2 b08
feature.

ClassFileInstaller is the proper way of reading the bytes of class
files, and has been used by most other test cases.
FragmentMetaspaceSimple seems to be the only exception.

Thanks
- Ioi




> Thanks,
> David
>
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>> testing: :hotspot_all, {jdk,langtools,nashorn,jaxp}/test/:tier[1-3]
>>>> Thanks,
>>>> -- Igor
>>

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

Re: RFR(XS) : 8186095 : upgrade to jtreg 4.2 b08

roger riggs
Hi  Igor,

In (some of) the TEST.ROOT files, maintenance of the files can be
simplified by removing the version number from
the comment.  The version that is in jaxp is reasonable:  "# Minimum
jtreg version" and should be applied
to the hotspot, jdk, langtools, and nashorn TEST.ROOT files.

Thanks, Roger

On 8/11/2017 1:29 AM, David Holmes wrote:

> Thanks Ioi, and Igor, for clarifying.
>
> David
>
>
> On 11/08/2017 3:23 PM, Ioi Lam wrote:
>>
>>
>> On 8/10/17 9:46 PM, David Holmes wrote:
>>> On 11/08/2017 2:31 PM, Igor Ignatyev wrote:
>>>>
>>>>> On Aug 10, 2017, at 9:22 PM, David Holmes
>>>>> <[hidden email]> wrote:
>>>>>
>>>>> Hi Igor,
>>>>>
>>>>> On 11/08/2017 2:02 PM, Igor Ignatyev wrote:
>>>>>> http://cr.openjdk.java.net/~iignatyev//8186095/webrev.00/index.html
>>>>>>> 14 lines changed: 1 ins; 0 del; 13 mod;
>>>>>> Hi all,
>>>>>> could you please review this small patch which bumps up jtreg
>>>>>> version?
>>>>>> besides updating jib profiles and all TEST.ROOT files,
>>>>>
>>>>> That all looks fine (though deploy should not be in there).
>>>>>
>>>>>> the fix updates
>>>>>> hotspot/test/runtime/Metaspace/FragmentMetaspaceSimple.java test
>>>>>> not to
>>>>>> rely on having "library" test.Empty class in 'test.classes' and put
>>>>>> test.Empty class in the workdir instead.
>>>>>
>>>>> Sorry I'm not following this part. You made two changes:
>>>>>
>>>>> 1. Added @library /test/lib
>>>> /test/lib is needed for ClassFileInstaller.
>>>
>>> Okay.
>>>
>>>>>
>>>>> What is this doing? (For that matter what is the existing
>>>>> "classes" entry supposed to mean ??? how is "classes" a library?)
>>>> existing 'classes' is the directory in
>>>> hotspot/test/runtime/Metaspace/ which contains source of test.Empty.
>>>
>>> Okay.
>>>
>>>>>
>>>>> 2. Instead of the test reading from test.classes you are using the
>>>>> ClassfileInstaller to copy the class to the working directory.
>>>>>
>>>>> How does this make a difference to anything? If the test wouldn't
>>>>> find the class in test.classes, doesn't that mean
>>>>> ClassfileInstaller will also fail to find it?
>>>> test.classes points to the directory w/ classes from a test, but
>>>> not from test libraries. directories w/ all needed classes (either
>>>> from a test or from libraries) are added to classpath and
>>>> 'test.class.path'. ClassFileInstaller uses class loader to get
>>>> resources, test.Empty will be in CP, so ClassFileInstaller will
>>>> have access to it.
>>>
>>> Sorry still don't understand the change. Where does:
>>>
>>> @build test.Empty
>>>
>>> place Empty.class? If not in test.classes then how has this test
>>> ever passed? I'm assuming the change is needed because it no longer
>>> passes with the updated jtreg.
>>>
>> Hi David,
>>
>>    27  * @build test.Empty
>>
>> puts test.Empty class somewhere in the classpath
>>
>>    28  * @run driver ClassFileInstaller test.Empty
>>
>> ClassFileInstaller uses Class.getResourceAsStream("test/Empty.class")
>> to read the contents of this class (from the CLASSPATH), and writes
>> it to ./test/Empty.class
>>
>>    55         String fileName = "test" + sep + "Empty.class";
>>    56         File file = new File(fileName);
>>    57         byte buff[] = read(file);
>>
>> here the test reads "./test/Empty.class" into a buffer. (When the
>> test runs, the current directory is the "scratch" directory).
>>
>> The old version of this test assumed that JTREG builds the
>> Empty.class into a certain location, but that has changed with the
>> jtreg 4.2 b08 feature.
>>
>> ClassFileInstaller is the proper way of reading the bytes of class
>> files, and has been used by most other test cases.
>> FragmentMetaspaceSimple seems to be the only exception.
>>
>> Thanks
>> - Ioi
>>
>>
>>
>>
>>> Thanks,
>>> David
>>>
>>>>>
>>>>> Thanks,
>>>>> David
>>>>> -----
>>>>>
>>>>>> testing: :hotspot_all, {jdk,langtools,nashorn,jaxp}/test/:tier[1-3]
>>>>>> Thanks,
>>>>>> -- Igor
>>>>
>>

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

Re: RFR(XS) : 8186095 : upgrade to jtreg 4.2 b08

Mandy Chung
In reply to this post by Igor Ignatyev

On 8/10/17 9:02 PM, Igor Ignatyev wrote:
> http://cr.openjdk.java.net/~iignatyev//8186095/webrev.00/index.html
hotspot/test/runtime/Metaspace/FragmentMetaspaceSimple.java

26 * @library /test/lib classes
   27  * @build test.Empty
28 * @run driver ClassFileInstaller test.Empty
jdk.test.lib.compiler.CompilerUtils is easy to use. I would suggest to
convert these implicit compile+copy steps into an explicit setup step to
compile ${test.src}/test/Empty.java into the destination directory. It'd
be easy to read and understand. Bumping up jtreg version is fine. Mandy

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

Re: RFR(XS) : 8186095 : upgrade to jtreg 4.2 b08

Igor Ignatyev
Hi Mandy,

thank you for your review.

although jdk.test.lib.compiler.CompilerUtils is easy to use, it introduces dependency on jdk.compiler module and currently FragmentMetaspaceSimple.java depends only on java.base module and I would prefer it remain so.

Thanks,
-- Igor


> On Aug 11, 2017, at 12:47 PM, mandy chung <[hidden email]> wrote:
> On 8/10/17 9:02 PM, Igor Ignatyev wrote:
>> http://cr.openjdk.java.net/~iignatyev//8186095/webrev.00/index.html <http://cr.openjdk.java.net/~iignatyev//8186095/webrev.00/index.html>
> hotspot/test/runtime/Metaspace/FragmentMetaspaceSimple.java
>   26  * @library /test/lib classes
>   27  * @build test.Empty
>   28  * @run driver ClassFileInstaller test.Empty
>
> jdk.test.lib.compiler.CompilerUtils is easy to use. I would suggest
> to convert these implicit compile+copy steps into an explicit
> setup step to compile ${test.src}/test/Empty.java into
> the destination directory.  It'd be easy to read and understand.
>
> Bumping up jtreg version is fine.
>
> Mandy

Loading...