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

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

[mandy chung]

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

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

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

[David Holmes]

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

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

[Ioi Lam]

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

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

[Igor Ignatyev]

> On Aug 10, 2017, at 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  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//runtime/Metaspace/classes', but 'test.classes' points to 
'JTwork/classes//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

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

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

[Igor Ignatyev]

> On Aug 10, 2017, at 9:22 PM, David Holmes  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.
> 
> 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.
> 
> 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.
> 
> Thanks,
> David
> -
> 
>> testing: :hotspot_all, {jdk,langtools,nashorn,jaxp}/test/:tier[1-3]
>> Thanks,
>> -- Igor

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