RFR(XL/M) : 8178788: wrap JCStress test suite as jtreg tests

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

RFR(XL/M) : 8178788: wrap JCStress test suite as jtreg tests

Igor Ignatyev
http://cr.openjdk.java.net/~iignatyev//8178788/webrev.00/index.html
> 69903 lines changed: 69903 ins; 0 del; 0 mod;
(69524 lines are generated)

Hi all,

could you please review this patch which adds a jtreg test wrapper for jcstress test suite and jtreg tests which run jsctress tests thru this wrapper?

webrev: http://cr.openjdk.java.net/~iignatyev//8178788/webrev.00/index.html
JBS: https://bugs.openjdk.java.net/browse/JDK-8178788
testing: applications/jcstress

Thanks,
-- Igor

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

Re: RFR(XL/M) : 8178788: wrap JCStress test suite as jtreg tests

Vladimir Kozlov
Looks fine to me.

Vladimir

On 4/18/17 3:12 PM, Igor Ignatyev wrote:

> http://cr.openjdk.java.net/~iignatyev//8178788/webrev.00/index.html
>> 69903 lines changed: 69903 ins; 0 del; 0 mod;
> (69524 lines are generated)
>
> Hi all,
>
> could you please review this patch which adds a jtreg test wrapper for jcstress test suite and jtreg tests which run jsctress tests thru this wrapper?
>
> webrev: http://cr.openjdk.java.net/~iignatyev//8178788/webrev.00/index.html
> JBS: https://bugs.openjdk.java.net/browse/JDK-8178788
> testing: applications/jcstress
>
> Thanks,
> -- Igor
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(XL/M) : 8178788: wrap JCStress test suite as jtreg tests

David Holmes
In reply to this post by Igor Ignatyev
Hi Igor,

On 19/04/2017 8:12 AM, Igor Ignatyev wrote:

> http://cr.openjdk.java.net/~iignatyev//8178788/webrev.00/index.html
>> 69903 lines changed: 69903 ins; 0 del; 0 mod;
> (69524 lines are generated)
>
> Hi all,
>
> could you please review this patch which adds a jtreg test wrapper for jcstress test suite and jtreg tests which run jsctress tests thru this wrapper?
>
> webrev: http://cr.openjdk.java.net/~iignatyev//8178788/webrev.00/index.html
> JBS: https://bugs.openjdk.java.net/browse/JDK-8178788
> testing: applications/jcstress

This looks really good! Only concern is that according to the docs jtreg
doesn't seem to support multiple test declarations in a single file ??
Maybe the docs are out of date?

http://openjdk.java.net/jtreg/tag-spec.html

A few grammatical nits in the README:

23 The tests located under this directory run tests from Java
Concurrency Stress

from Java -> from the Java

24 [1] aka jcstress test suite. This suite aims to verify the correctness of

Suggest:

test suite [1] (a.k.a. jcstress). This suite ...

25 concurrency support in JDK.

in JDK -> in the JDK

27 All the tests are run thought test driver class -- JcstressRunner, which

s/thought/through the/

29 spawns a new JVM to run one jsctress test and checks that it is finished

is finished -> finishes

33 changed, one should make corresponding changes in the artifact
description in

in the -> to the

36 */Test.java files should never be modified directly, because they are

Insert "The" at start.

---

General Java style nits re long ling wrapping. You seem to be using a
consistent "indent by 8" style when wrapping long lines. The "usual"
style guidelines suggest a more structured indentation style, for example:

- when wrapping the arguments to a call align after the opening ( of the
call ie:

        throw new Error("TESTBUG: Can not resolve artifacts for "
                        + JcstressRunner.class.getName(), e);

- when you have chained method calls, try to align at the dots ie:

     return artifacts.get("org.openjdk.jcstress.jcstress-tests-all-0.3")
                     .toAbsolutePath();

and

ProcessBuilder pb = ProcessTools.createJavaProcessBuilder(getCmd(args))
                                 .redirectErrorStream(true)
                                 .redirectOutput(out.toFile());

---

test/applications/jcstress/TestGenerator.java

44  * This is a test generator, should be run only when jsctress is changed

Typo: jsctress -> jcstress

  51  * Use jcstress test suite to generates

generates -> generate

Strictly speaking the copyright years should only be updated when there
is a substantive change to the text of the file, so updating whenever
the files are regenerated is not really correct. This may need to be
manually adjusted when commiting updated files ... though if there is no
change other than to copyright year then the changes can simply be
ignored (hg revert) and no update pushed.

Thanks,
David
------

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

Re: RFR(XL/M) : 8178788: wrap JCStress test suite as jtreg tests

Igor Ignatyev
Hi David,

thank you for your comments, I've tried to address them all, please look at the incremental webrev: http://cr.openjdk.java.net/~iignatyev/8178788/webrev.00-01/index.html

regarding multiple test declarations, jtreg supports that, but in the current version it's not fully/officially supported and hence it's not documented. Jon has fixed a few jtreg bugs[1-3] related to this and AFAIK is working towards full support, so I hope it will be supported and documented in the next promoted build of jtreg.

[1] https://bugs.openjdk.java.net/browse/CODETOOLS-7900144 : Implement multiple tests in one file in jtreg
[2] https://bugs.openjdk.java.net/browse/CODETOOLS-7901914 : Failure to find multiple @test in a single file
[3] https://bugs.openjdk.java.net/browse/CODETOOLS-7901940 : support multiple @test in one test file

Thanks,
-- Igor

> On Apr 18, 2017, at 6:28 PM, David Holmes <[hidden email]> wrote:
>
> Hi Igor,
>
> On 19/04/2017 8:12 AM, Igor Ignatyev wrote:
>> http://cr.openjdk.java.net/~iignatyev//8178788/webrev.00/index.html
>>> 69903 lines changed: 69903 ins; 0 del; 0 mod;
>> (69524 lines are generated)
>>
>> Hi all,
>>
>> could you please review this patch which adds a jtreg test wrapper for jcstress test suite and jtreg tests which run jsctress tests thru this wrapper?
>>
>> webrev: http://cr.openjdk.java.net/~iignatyev//8178788/webrev.00/index.html
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8178788
>> testing: applications/jcstress
>
> This looks really good! Only concern is that according to the docs jtreg doesn't seem to support multiple test declarations in a single file ?? Maybe the docs are out of date?

>
> http://openjdk.java.net/jtreg/tag-spec.html
>
> A few grammatical nits in the README:
>
> 23 The tests located under this directory run tests from Java Concurrency Stress
>
> from Java -> from the Java
>
> 24 [1] aka jcstress test suite. This suite aims to verify the correctness of
>
> Suggest:
>
> test suite [1] (a.k.a. jcstress). This suite ...
>
> 25 concurrency support in JDK.
>
> in JDK -> in the JDK
>
> 27 All the tests are run thought test driver class -- JcstressRunner, which
>
> s/thought/through the/
>
> 29 spawns a new JVM to run one jsctress test and checks that it is finished
>
> is finished -> finishes
>
> 33 changed, one should make corresponding changes in the artifact description in
>
> in the -> to the
>
> 36 */Test.java files should never be modified directly, because they are
>
> Insert "The" at start.
>
> ---
>
> General Java style nits re long ling wrapping. You seem to be using a consistent "indent by 8" style when wrapping long lines. The "usual" style guidelines suggest a more structured indentation style, for example:
>
> - when wrapping the arguments to a call align after the opening ( of the call ie:
>
>       throw new Error("TESTBUG: Can not resolve artifacts for "
>                       + JcstressRunner.class.getName(), e);
>
> - when you have chained method calls, try to align at the dots ie:
>
>    return artifacts.get("org.openjdk.jcstress.jcstress-tests-all-0.3")
>                    .toAbsolutePath();
>
> and
>
> ProcessBuilder pb = ProcessTools.createJavaProcessBuilder(getCmd(args))
>                                .redirectErrorStream(true)
>                                .redirectOutput(out.toFile());
>
> ---
>
> test/applications/jcstress/TestGenerator.java
>
> 44  * This is a test generator, should be run only when jsctress is changed
>
> Typo: jsctress -> jcstress
>
> 51  * Use jcstress test suite to generates
>
> generates -> generate
>
> Strictly speaking the copyright years should only be updated when there is a substantive change to the text of the file, so updating whenever the files are regenerated is not really correct. This may need to be manually adjusted when commiting updated files ... though if there is no change other than to copyright year then the changes can simply be ignored (hg revert) and no update pushed.
>
> Thanks,
> David
> ------
>
>> Thanks,
>> -- Igor
>>

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

RE: RFR(XL/M) : 8178788: wrap JCStress test suite as jtreg tests

Frank Yuan
In reply to this post by Igor Ignatyev
Hi Igor

 

This is really a valuable work!

 

Would you like to pull your artifacts up to the common repo hg.openjdk.java.net/jdk10/hs/test? That more people(e.g. corelibs :))
can benefit from this.

 

Thanks

Frank

 

 


Subject:

RFR(XL/M) : 8178788: wrap JCStress test suite as jtreg tests


Date:

Tue, 18 Apr 2017 15:12:48 -0700


From:

Igor Ignatyev  <mailto:[hidden email]> <[hidden email]>


To:

[hidden email] <mailto:[hidden email]>  compiler
<mailto:[hidden email]> <[hidden email]>, HotSpot Open Source Developers
<mailto:[hidden email]> <[hidden email]>

 

http://cr.openjdk.java.net/~iignatyev//8178788/webrev.00/index.html
<http://cr.openjdk.java.net/~iignatyev/8178788/webrev.00/index.html>
> 69903 lines changed: 69903 ins; 0 del; 0 mod;
(69524 lines are generated)
 
Hi all,
 
could you please review this patch which adds a jtreg test wrapper for jcstress test suite and jtreg tests which run jsctress tests
thru this wrapper?
 
webrev: http://cr.openjdk.java.net/~iignatyev//8178788/webrev.00/index.html
<http://cr.openjdk.java.net/~iignatyev/8178788/webrev.00/index.html>
JBS: https://bugs.openjdk.java.net/browse/JDK-8178788
testing: applications/jcstress
 
Thanks,
-- Igor
 
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(XL/M) : 8178788: wrap JCStress test suite as jtreg tests

Igor Ignatyev
Hi Felix,

currently, <root>/test is not a jtreg testbase and it is not supported by make and the rest of infra, therefore it's not possible to have tests there. I don't want to mix up this fix w/ infra/make related work, and would prefer to think about this movement after (or as a part of) JEP 296 Consolidate the JDK Forest into a Single Repository.

actually, corelibs people are more than welcome to run hotspot/test, we (hotspot team) run jdk/test quite often and would really love to see other people running our tests before pushing changes which can affect hotspot and/or its tests.

-- Igor


> On Apr 18, 2017, at 8:32 PM, Frank Yuan <[hidden email]> wrote:
>
> Hi Igor
>  
> This is really a valuable work!
>  
> Would you like to pull your artifacts up to the common repo hg.openjdk.java.net/jdk10/hs/test? <http://hg.openjdk.java.net/jdk10/hs/test?> That more people(e.g. corelibs J) can benefit from this.
>  
> Thanks
> Frank
>  
>  
> Subject:
> RFR(XL/M) : 8178788: wrap JCStress test suite as jtreg tests
> Date:
> Tue, 18 Apr 2017 15:12:48 -0700
> From:
> Igor Ignatyev <[hidden email]> <mailto:[hidden email]>
> To:
> [hidden email] <mailto:[hidden email]> compiler <[hidden email]> <mailto:[hidden email]>, HotSpot Open Source Developers <[hidden email]> <mailto:[hidden email]>
>  
>
> http://cr.openjdk.java.net/~iignatyev//8178788/webrev.00/index.html <http://cr.openjdk.java.net/~iignatyev/8178788/webrev.00/index.html>
> > 69903 lines changed: 69903 ins; 0 del; 0 mod;
> (69524 lines are generated)
>  
> Hi all,
>  
> could you please review this patch which adds a jtreg test wrapper for jcstress test suite and jtreg tests which run jsctress tests thru this wrapper?
>  
> webrev: http://cr.openjdk.java.net/~iignatyev//8178788/webrev.00/index.html <http://cr.openjdk.java.net/~iignatyev/8178788/webrev.00/index.html>
> JBS: https://bugs.openjdk.java.net/browse/JDK-8178788 <https://bugs.openjdk.java.net/browse/JDK-8178788>
> testing: applications/jcstress
>  
> Thanks,
> -- Igor
>  

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

RE: RFR(XL/M) : 8178788: wrap JCStress test suite as jtreg tests

Frank Yuan
Hi Igor

 

The artifact which I talked about is the wrapper(JcstressRunner.java), I supposed there would be more people to create their tests
with jcstress facility if the wrapper is put to a common place, there have been e.g. failure_handler, lib in
hg.openjdk.java.net/jdk10/hs/test.

 

If we put the wrapper into hotspot repo, even after JEP 296, it would be under test/hotspot/.

 

Certainly, it's up to you:)

 

 

Thanks

Frank

 

From: Igor Ignatyev [mailto:[hidden email]]
Sent: Wednesday, April 19, 2017 1:47 PM
To: Frank Yuan
Cc: HotSpot Open Source Developers
Subject: Re: RFR(XL/M) : 8178788: wrap JCStress test suite as jtreg tests

 

Hi Felix,

 

currently, <root>/test is not a jtreg testbase and it is not supported by make and the rest of infra, therefore it's not possible to
have tests there. I don't want to mix up this fix w/ infra/make related work, and would prefer to think about this movement after
(or as a part of) JEP 296 Consolidate the JDK Forest into a Single Repository.

 

actually, corelibs people are more than welcome to run hotspot/test, we (hotspot team) run jdk/test quite often and would really
love to see other people running our tests before pushing changes which can affect hotspot and/or its tests.

 

-- Igor

 

 

On Apr 18, 2017, at 8:32 PM, Frank Yuan <[hidden email] <mailto:[hidden email]> > wrote:

 

Hi Igor

 

This is really a valuable work!

 

Would you like to pull your artifacts up to the common repo  <http://hg.openjdk.java.net/jdk10/hs/test?>
hg.openjdk.java.net/jdk10/hs/test? That more people(e.g. corelibs :)) can benefit from this.

 

Thanks

Frank

 

 


Subject:

RFR(XL/M) : 8178788: wrap JCStress test suite as jtreg tests


Date:

Tue, 18 Apr 2017 15:12:48 -0700


From:

Igor Ignatyev  <mailto:[hidden email]> <[hidden email]>


To:

 <mailto:[hidden email]> [hidden email] compiler
<mailto:[hidden email]> <[hidden email]>, HotSpot Open Source Developers
<mailto:[hidden email]> <[hidden email]>

 

 <http://cr.openjdk.java.net/~iignatyev/8178788/webrev.00/index.html>
http://cr.openjdk.java.net/~iignatyev//8178788/webrev.00/index.html
> 69903 lines changed: 69903 ins; 0 del; 0 mod;
(69524 lines are generated)
 
Hi all,
 
could you please review this patch which adds a jtreg test wrapper for jcstress test suite and jtreg tests which run jsctress tests
thru this wrapper?
 
webrev:  <http://cr.openjdk.java.net/~iignatyev/8178788/webrev.00/index.html>
http://cr.openjdk.java.net/~iignatyev//8178788/webrev.00/index.html
JBS:  <https://bugs.openjdk.java.net/browse/JDK-8178788> https://bugs.openjdk.java.net/browse/JDK-8178788
testing: applications/jcstress
 
Thanks,
-- Igor
 

 

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

Re: RFR(XL/M) : 8178788: wrap JCStress test suite as jtreg tests

harold seigel
In reply to this post by Igor Ignatyev
Hi Igor,

How long will these tests typically take to run?

Thanks, Harold


On 4/19/2017 1:47 AM, Igor Ignatyev wrote:

> Hi Felix,
>
> currently, <root>/test is not a jtreg testbase and it is not supported by make and the rest of infra, therefore it's not possible to have tests there. I don't want to mix up this fix w/ infra/make related work, and would prefer to think about this movement after (or as a part of) JEP 296 Consolidate the JDK Forest into a Single Repository.
>
> actually, corelibs people are more than welcome to run hotspot/test, we (hotspot team) run jdk/test quite often and would really love to see other people running our tests before pushing changes which can affect hotspot and/or its tests.
>
> -- Igor
>
>
>> On Apr 18, 2017, at 8:32 PM, Frank Yuan <[hidden email]> wrote:
>>
>> Hi Igor
>>  
>> This is really a valuable work!
>>  
>> Would you like to pull your artifacts up to the common repo hg.openjdk.java.net/jdk10/hs/test? <http://hg.openjdk.java.net/jdk10/hs/test?> That more people(e.g. corelibs J) can benefit from this.
>>  
>> Thanks
>> Frank
>>  
>>  
>> Subject:
>> RFR(XL/M) : 8178788: wrap JCStress test suite as jtreg tests
>> Date:
>> Tue, 18 Apr 2017 15:12:48 -0700
>> From:
>> Igor Ignatyev <[hidden email]> <mailto:[hidden email]>
>> To:
>> [hidden email] <mailto:[hidden email]> compiler <[hidden email]> <mailto:[hidden email]>, HotSpot Open Source Developers <[hidden email]> <mailto:[hidden email]>
>>  
>>
>> http://cr.openjdk.java.net/~iignatyev//8178788/webrev.00/index.html <http://cr.openjdk.java.net/~iignatyev/8178788/webrev.00/index.html>
>>> 69903 lines changed: 69903 ins; 0 del; 0 mod;
>> (69524 lines are generated)
>>  
>> Hi all,
>>  
>> could you please review this patch which adds a jtreg test wrapper for jcstress test suite and jtreg tests which run jsctress tests thru this wrapper?
>>  
>> webrev: http://cr.openjdk.java.net/~iignatyev//8178788/webrev.00/index.html <http://cr.openjdk.java.net/~iignatyev/8178788/webrev.00/index.html>
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8178788 <https://bugs.openjdk.java.net/browse/JDK-8178788>
>> testing: applications/jcstress
>>  
>> Thanks,
>> -- Igor
>>  

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

Re: RFR(XL/M) : 8178788: wrap JCStress test suite as jtreg tests

Igor Ignatyev
Hi Harold,

on my laptop, one test takes about 50 seconds to finish, so all 11554 tests will take around 577 700 seconds, which is about 1 week of machine time. I should have mentioned that at the very begging, we are not proposing to run them very frequently.

-- Igor

> On Apr 19, 2017, at 9:57 AM, harold seigel <[hidden email]> wrote:
>
> Hi Igor,
>
> How long will these tests typically take to run?
>
> Thanks, Harold
>
>
> On 4/19/2017 1:47 AM, Igor Ignatyev wrote:
>> Hi Felix,
>>
>> currently, <root>/test is not a jtreg testbase and it is not supported by make and the rest of infra, therefore it's not possible to have tests there. I don't want to mix up this fix w/ infra/make related work, and would prefer to think about this movement after (or as a part of) JEP 296 Consolidate the JDK Forest into a Single Repository.
>>
>> actually, corelibs people are more than welcome to run hotspot/test, we (hotspot team) run jdk/test quite often and would really love to see other people running our tests before pushing changes which can affect hotspot and/or its tests.
>>
>> -- Igor
>>
>>
>>> On Apr 18, 2017, at 8:32 PM, Frank Yuan <[hidden email]> wrote:
>>>
>>> Hi Igor
>>>  This is really a valuable work!
>>>  Would you like to pull your artifacts up to the common repo hg.openjdk.java.net/jdk10/hs/test? <http://hg.openjdk.java.net/jdk10/hs/test?><http://hg.openjdk.java.net/jdk10/hs/test? <http://hg.openjdk.java.net/jdk10/hs/test?>> That more people(e.g. corelibs J) can benefit from this.
>>>  Thanks
>>> Frank
>>>    Subject:
>>> RFR(XL/M) : 8178788: wrap JCStress test suite as jtreg tests
>>> Date:
>>> Tue, 18 Apr 2017 15:12:48 -0700
>>> From:
>>> Igor Ignatyev <[hidden email] <mailto:[hidden email]>> <mailto:[hidden email] <mailto:[hidden email]>>
>>> To:
>>> [hidden email] <mailto:[hidden email]> <mailto:[hidden email] <mailto:[hidden email]>> compiler <[hidden email] <mailto:[hidden email]>> <mailto:[hidden email] <mailto:[hidden email]>>, HotSpot Open Source Developers <[hidden email] <mailto:[hidden email]>> <mailto:[hidden email] <mailto:[hidden email]>>
>>>  
>>> http://cr.openjdk.java.net/~iignatyev//8178788/webrev.00/index.html <http://cr.openjdk.java.net/~iignatyev//8178788/webrev.00/index.html><http://cr.openjdk.java.net/~iignatyev/8178788/webrev.00/index.html <http://cr.openjdk.java.net/~iignatyev/8178788/webrev.00/index.html>>
>>>> 69903 lines changed: 69903 ins; 0 del; 0 mod;
>>> (69524 lines are generated)
>>>  Hi all,
>>>  could you please review this patch which adds a jtreg test wrapper for jcstress test suite and jtreg tests which run jsctress tests thru this wrapper?
>>>  webrev: http://cr.openjdk.java.net/~iignatyev//8178788/webrev.00/index.html <http://cr.openjdk.java.net/~iignatyev//8178788/webrev.00/index.html><http://cr.openjdk.java.net/~iignatyev/8178788/webrev.00/index.html <http://cr.openjdk.java.net/~iignatyev/8178788/webrev.00/index.html>>
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8178788 <https://bugs.openjdk.java.net/browse/JDK-8178788> <https://bugs.openjdk.java.net/browse/JDK-8178788 <https://bugs.openjdk.java.net/browse/JDK-8178788>>
>>> testing: applications/jcstress
>>>  Thanks,
>>> -- Igor

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

Re: RFR(XL/M) : 8178788: wrap JCStress test suite as jtreg tests

Igor Ignatyev
In reply to this post by Frank Yuan
Hi Felix,

I understand your point and use-case and believe it is important and useful, however I don't think we should do that now. 1st of all, current jcstress wrapper is able to run precompiled tests from jsctress-tests-all, so if people write their own jcstress based tests, they will still need to use jcstress to build and run them. 2nd, hotspot/test/applications/jcstress tests depend on the version of jcstress, and currently the version is defined in wrapper, so I'd prefer to have the wrapper as much closer to these tests as possible. that is to say, if we have high need for running custom jcstress tests, then we should invest more in jcstress driver/wrapper (and maybe infra) so it will be possible to have tests in jcstress format as part of our test bases, but this is out of scope of this RFE.

Thanks,
-- Igor

> On Apr 18, 2017, at 11:39 PM, Frank Yuan <[hidden email]> wrote:
>
> Hi Igor
>  
> The artifact which I talked about is the wrapper(JcstressRunner.java), I supposed there would be more people to create their tests with jcstress facility if the wrapper is put to a common place, there have been e.g. failure_handler, lib in hg.openjdk.java.net/jdk10/hs/test <http://hg.openjdk.java.net/jdk10/hs/test>.
>  
> If we put the wrapper into hotspot repo, even after JEP 296, it would be under test/hotspot/…
>  
> Certainly, it’s up to youJ
>  
>  
> Thanks
> Frank
>  
> From: Igor Ignatyev [mailto:[hidden email]]
> Sent: Wednesday, April 19, 2017 1:47 PM
> To: Frank Yuan
> Cc: HotSpot Open Source Developers
> Subject: Re: RFR(XL/M) : 8178788: wrap JCStress test suite as jtreg tests
>  
> Hi Felix,
>  
> currently, <root>/test is not a jtreg testbase and it is not supported by make and the rest of infra, therefore it's not possible to have tests there. I don't want to mix up this fix w/ infra/make related work, and would prefer to think about this movement after (or as a part of) JEP 296 Consolidate the JDK Forest into a Single Repository.
>  
> actually, corelibs people are more than welcome to run hotspot/test, we (hotspot team) run jdk/test quite often and would really love to see other people running our tests before pushing changes which can affect hotspot and/or its tests.
>  
> -- Igor
>  
>  
>> On Apr 18, 2017, at 8:32 PM, Frank Yuan <[hidden email] <mailto:[hidden email]>> wrote:
>>  
>> Hi Igor
>>  
>> This is really a valuable work!
>>  
>> Would you like to pull your artifacts up to the common repo hg.openjdk.java.net/jdk10/hs/test? <http://hg.openjdk.java.net/jdk10/hs/test?> That more people(e.g. corelibs J) can benefit from this.
>>  
>> Thanks
>> Frank
>>  
>>  
>> Subject:
>> RFR(XL/M) : 8178788: wrap JCStress test suite as jtreg tests
>> Date:
>> Tue, 18 Apr 2017 15:12:48 -0700
>> From:
>> Igor Ignatyev <[hidden email]> <mailto:[hidden email]>
>> To:
>> [hidden email] <mailto:[hidden email]> compiler <[hidden email]> <mailto:[hidden email]>, HotSpot Open Source Developers <[hidden email]> <mailto:[hidden email]>
>>  
>>
>> http://cr.openjdk.java.net/~iignatyev//8178788/webrev.00/index.html <http://cr.openjdk.java.net/~iignatyev/8178788/webrev.00/index.html>
>> > 69903 lines changed: 69903 ins; 0 del; 0 mod;
>> (69524 lines are generated)
>>  
>> Hi all,
>>  
>> could you please review this patch which adds a jtreg test wrapper for jcstress test suite and jtreg tests which run jsctress tests thru this wrapper?
>>  
>> webrev: http://cr.openjdk.java.net/~iignatyev//8178788/webrev.00/index.html <http://cr.openjdk.java.net/~iignatyev/8178788/webrev.00/index.html>
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8178788 <https://bugs.openjdk.java.net/browse/JDK-8178788>
>> testing: applications/jcstress
>>  
>> Thanks,
>> -- Igor

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

Re: RFR(XL/M) : 8178788: wrap JCStress test suite as jtreg tests

Igor Ignatyev
In reply to this post by Igor Ignatyev
Hi David,

thank for catching those, the incremental webrev : http://cr.openjdk.java.net/~iignatyev/8178788/webrev.01-02/

Cheers,
-- Igor

> On Apr 18, 2017, at 11:17 PM, David Holmes <[hidden email]> wrote:
>
> Hi Igor,
>
> Thanks for the jtreg info!
>
> You missed a couple of things in the README:
>
> 27 All the tests are run thought test driver class -- JcstressRunner, which
>
> s/thought/through the/
>
> And I just noticed an additional typo:
>
> 29 spawns a new JVM to run one jsctress test
>
> jsctress -> jcstress
>
> Thanks,
> David
> -----
>
> On 19/04/2017 12:51 PM, Igor Ignatyev wrote:
>> Hi David,
>>
>> thank you for your comments, I've tried to address them all, please look at the incremental webrev: http://cr.openjdk.java.net/~iignatyev/8178788/webrev.00-01/index.html
>>
>> regarding multiple test declarations, jtreg supports that, but in the current version it's not fully/officially supported and hence it's not documented. Jon has fixed a few jtreg bugs[1-3] related to this and AFAIK is working towards full support, so I hope it will be supported and documented in the next promoted build of jtreg.
>>
>> [1] https://bugs.openjdk.java.net/browse/CODETOOLS-7900144 : Implement multiple tests in one file in jtreg
>> [2] https://bugs.openjdk.java.net/browse/CODETOOLS-7901914 : Failure to find multiple @test in a single file
>> [3] https://bugs.openjdk.java.net/browse/CODETOOLS-7901940 : support multiple @test in one test file
>>
>> Thanks,
>> -- Igor
>>
>>> On Apr 18, 2017, at 6:28 PM, David Holmes <[hidden email]> wrote:
>>>
>>> Hi Igor,
>>>
>>> On 19/04/2017 8:12 AM, Igor Ignatyev wrote:
>>>> http://cr.openjdk.java.net/~iignatyev//8178788/webrev.00/index.html
>>>>> 69903 lines changed: 69903 ins; 0 del; 0 mod;
>>>> (69524 lines are generated)
>>>>
>>>> Hi all,
>>>>
>>>> could you please review this patch which adds a jtreg test wrapper for jcstress test suite and jtreg tests which run jsctress tests thru this wrapper?
>>>>
>>>> webrev: http://cr.openjdk.java.net/~iignatyev//8178788/webrev.00/index.html
>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8178788
>>>> testing: applications/jcstress
>>>
>>> This looks really good! Only concern is that according to the docs jtreg doesn't seem to support multiple test declarations in a single file ?? Maybe the docs are out of date?
>>
>>>
>>> http://openjdk.java.net/jtreg/tag-spec.html
>>>
>>> A few grammatical nits in the README:
>>>
>>> 23 The tests located under this directory run tests from Java Concurrency Stress
>>>
>>> from Java -> from the Java
>>>
>>> 24 [1] aka jcstress test suite. This suite aims to verify the correctness of
>>>
>>> Suggest:
>>>
>>> test suite [1] (a.k.a. jcstress). This suite ...
>>>
>>> 25 concurrency support in JDK.
>>>
>>> in JDK -> in the JDK
>>>
>>> 27 All the tests are run thought test driver class -- JcstressRunner, which
>>>
>>> s/thought/through the/
>>>
>>> 29 spawns a new JVM to run one jsctress test and checks that it is finished
>>>
>>> is finished -> finishes
>>>
>>> 33 changed, one should make corresponding changes in the artifact description in
>>>
>>> in the -> to the
>>>
>>> 36 */Test.java files should never be modified directly, because they are
>>>
>>> Insert "The" at start.
>>>
>>> ---
>>>
>>> General Java style nits re long ling wrapping. You seem to be using a consistent "indent by 8" style when wrapping long lines. The "usual" style guidelines suggest a more structured indentation style, for example:
>>>
>>> - when wrapping the arguments to a call align after the opening ( of the call ie:
>>>
>>>      throw new Error("TESTBUG: Can not resolve artifacts for "
>>>                      + JcstressRunner.class.getName(), e);
>>>
>>> - when you have chained method calls, try to align at the dots ie:
>>>
>>>   return artifacts.get("org.openjdk.jcstress.jcstress-tests-all-0.3")
>>>                   .toAbsolutePath();
>>>
>>> and
>>>
>>> ProcessBuilder pb = ProcessTools.createJavaProcessBuilder(getCmd(args))
>>>                               .redirectErrorStream(true)
>>>                               .redirectOutput(out.toFile());
>>>
>>> ---
>>>
>>> test/applications/jcstress/TestGenerator.java
>>>
>>> 44  * This is a test generator, should be run only when jsctress is changed
>>>
>>> Typo: jsctress -> jcstress
>>>
>>> 51  * Use jcstress test suite to generates
>>>
>>> generates -> generate
>>>
>>> Strictly speaking the copyright years should only be updated when there is a substantive change to the text of the file, so updating whenever the files are regenerated is not really correct. This may need to be manually adjusted when commiting updated files ... though if there is no change other than to copyright year then the changes can simply be ignored (hg revert) and no update pushed.
>>>
>>> Thanks,
>>> David
>>> ------
>>>
>>>> Thanks,
>>>> -- Igor
>>>>
>>

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

Re: RFR(XL/M) : 8178788: wrap JCStress test suite as jtreg tests

David Holmes
Looks good! Sorry I should have said no new webrev needed :)

Thanks,
David

On 20/04/2017 7:45 AM, Igor Ignatyev wrote:

> Hi David,
>
> thank for catching those, the incremental webrev : http://cr.openjdk.java.net/~iignatyev/8178788/webrev.01-02/
>
> Cheers,
> -- Igor
>
>> On Apr 18, 2017, at 11:17 PM, David Holmes <[hidden email]> wrote:
>>
>> Hi Igor,
>>
>> Thanks for the jtreg info!
>>
>> You missed a couple of things in the README:
>>
>> 27 All the tests are run thought test driver class -- JcstressRunner, which
>>
>> s/thought/through the/
>>
>> And I just noticed an additional typo:
>>
>> 29 spawns a new JVM to run one jsctress test
>>
>> jsctress -> jcstress
>>
>> Thanks,
>> David
>> -----
>>
>> On 19/04/2017 12:51 PM, Igor Ignatyev wrote:
>>> Hi David,
>>>
>>> thank you for your comments, I've tried to address them all, please look at the incremental webrev: http://cr.openjdk.java.net/~iignatyev/8178788/webrev.00-01/index.html
>>>
>>> regarding multiple test declarations, jtreg supports that, but in the current version it's not fully/officially supported and hence it's not documented. Jon has fixed a few jtreg bugs[1-3] related to this and AFAIK is working towards full support, so I hope it will be supported and documented in the next promoted build of jtreg.
>>>
>>> [1] https://bugs.openjdk.java.net/browse/CODETOOLS-7900144 : Implement multiple tests in one file in jtreg
>>> [2] https://bugs.openjdk.java.net/browse/CODETOOLS-7901914 : Failure to find multiple @test in a single file
>>> [3] https://bugs.openjdk.java.net/browse/CODETOOLS-7901940 : support multiple @test in one test file
>>>
>>> Thanks,
>>> -- Igor
>>>
>>>> On Apr 18, 2017, at 6:28 PM, David Holmes <[hidden email]> wrote:
>>>>
>>>> Hi Igor,
>>>>
>>>> On 19/04/2017 8:12 AM, Igor Ignatyev wrote:
>>>>> http://cr.openjdk.java.net/~iignatyev//8178788/webrev.00/index.html
>>>>>> 69903 lines changed: 69903 ins; 0 del; 0 mod;
>>>>> (69524 lines are generated)
>>>>>
>>>>> Hi all,
>>>>>
>>>>> could you please review this patch which adds a jtreg test wrapper for jcstress test suite and jtreg tests which run jsctress tests thru this wrapper?
>>>>>
>>>>> webrev: http://cr.openjdk.java.net/~iignatyev//8178788/webrev.00/index.html
>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8178788
>>>>> testing: applications/jcstress
>>>>
>>>> This looks really good! Only concern is that according to the docs jtreg doesn't seem to support multiple test declarations in a single file ?? Maybe the docs are out of date?
>>>
>>>>
>>>> http://openjdk.java.net/jtreg/tag-spec.html
>>>>
>>>> A few grammatical nits in the README:
>>>>
>>>> 23 The tests located under this directory run tests from Java Concurrency Stress
>>>>
>>>> from Java -> from the Java
>>>>
>>>> 24 [1] aka jcstress test suite. This suite aims to verify the correctness of
>>>>
>>>> Suggest:
>>>>
>>>> test suite [1] (a.k.a. jcstress). This suite ...
>>>>
>>>> 25 concurrency support in JDK.
>>>>
>>>> in JDK -> in the JDK
>>>>
>>>> 27 All the tests are run thought test driver class -- JcstressRunner, which
>>>>
>>>> s/thought/through the/
>>>>
>>>> 29 spawns a new JVM to run one jsctress test and checks that it is finished
>>>>
>>>> is finished -> finishes
>>>>
>>>> 33 changed, one should make corresponding changes in the artifact description in
>>>>
>>>> in the -> to the
>>>>
>>>> 36 */Test.java files should never be modified directly, because they are
>>>>
>>>> Insert "The" at start.
>>>>
>>>> ---
>>>>
>>>> General Java style nits re long ling wrapping. You seem to be using a consistent "indent by 8" style when wrapping long lines. The "usual" style guidelines suggest a more structured indentation style, for example:
>>>>
>>>> - when wrapping the arguments to a call align after the opening ( of the call ie:
>>>>
>>>>      throw new Error("TESTBUG: Can not resolve artifacts for "
>>>>                      + JcstressRunner.class.getName(), e);
>>>>
>>>> - when you have chained method calls, try to align at the dots ie:
>>>>
>>>>   return artifacts.get("org.openjdk.jcstress.jcstress-tests-all-0.3")
>>>>                   .toAbsolutePath();
>>>>
>>>> and
>>>>
>>>> ProcessBuilder pb = ProcessTools.createJavaProcessBuilder(getCmd(args))
>>>>                               .redirectErrorStream(true)
>>>>                               .redirectOutput(out.toFile());
>>>>
>>>> ---
>>>>
>>>> test/applications/jcstress/TestGenerator.java
>>>>
>>>> 44  * This is a test generator, should be run only when jsctress is changed
>>>>
>>>> Typo: jsctress -> jcstress
>>>>
>>>> 51  * Use jcstress test suite to generates
>>>>
>>>> generates -> generate
>>>>
>>>> Strictly speaking the copyright years should only be updated when there is a substantive change to the text of the file, so updating whenever the files are regenerated is not really correct. This may need to be manually adjusted when commiting updated files ... though if there is no change other than to copyright year then the changes can simply be ignored (hg revert) and no update pushed.
>>>>
>>>> Thanks,
>>>> David
>>>> ------
>>>>
>>>>> Thanks,
>>>>> -- Igor
>>>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: RFR(XL/M) : 8178788: wrap JCStress test suite as jtreg tests

Frank Yuan
In reply to this post by Igor Ignatyev
Understand. Alright, let’s talk off this thread for what out of scope of this RFE.

 

Thanks

Frank

 

From: Igor Ignatyev [mailto:[hidden email]]
Sent: Thursday, April 20, 2017 2:06 AM
To: Frank Yuan
Cc: HotSpot Open Source Developers
Subject: Re: RFR(XL/M) : 8178788: wrap JCStress test suite as jtreg tests

 

Hi Felix,

 

I understand your point and use-case and believe it is important and useful, however I don't think we should do that now. 1st of all, current jcstress wrapper is able to run precompiled tests from jsctress-tests-all, so if people write their own jcstress based tests, they will still need to use jcstress to build and run them. 2nd, hotspot/test/applications/jcstress tests depend on the version of jcstress, and currently the version is defined in wrapper, so I'd prefer to have the wrapper as much closer to these tests as possible. that is to say, if we have high need for running custom jcstress tests, then we should invest more in jcstress driver/wrapper (and maybe infra) so it will be possible to have tests in jcstress format as part of our test bases, but this is out of scope of this RFE.

 

Thanks,

-- Igor

 

On Apr 18, 2017, at 11:39 PM, Frank Yuan <[hidden email] <mailto:[hidden email]> > wrote:

 

Hi Igor

 

The artifact which I talked about is the wrapper(JcstressRunner.java), I supposed there would be more people to create their tests with jcstress facility if the wrapper is put to a common place, there have been e.g. failure_handler, lib in  <http://hg.openjdk.java.net/jdk10/hs/test> hg.openjdk.java.net/jdk10/hs/test.

 

If we put the wrapper into hotspot repo, even after JEP 296, it would be under test/hotspot/…

 

Certainly, it’s up to you:)

 

 

Thanks

Frank

 

From: Igor Ignatyev [mailto:[hidden email]]
Sent: Wednesday, April 19, 2017 1:47 PM
To: Frank Yuan
Cc: HotSpot Open Source Developers
Subject: Re: RFR(XL/M) : 8178788: wrap JCStress test suite as jtreg tests

 

Hi Felix,

 

currently, <root>/test is not a jtreg testbase and it is not supported by make and the rest of infra, therefore it's not possible to have tests there. I don't want to mix up this fix w/ infra/make related work, and would prefer to think about this movement after (or as a part of) JEP 296 Consolidate the JDK Forest into a Single Repository.

 

actually, corelibs people are more than welcome to run hotspot/test, we (hotspot team) run jdk/test quite often and would really love to see other people running our tests before pushing changes which can affect hotspot and/or its tests.

 

-- Igor

 

 

On Apr 18, 2017, at 8:32 PM, Frank Yuan < <mailto:[hidden email]> [hidden email]> wrote:

 

Hi Igor

 

This is really a valuable work!

 

Would you like to pull your artifacts up to the common repo  <http://hg.openjdk.java.net/jdk10/hs/test?> hg.openjdk.java.net/jdk10/hs/test? That more people(e.g. corelibs :)) can benefit from this.

 

Thanks

Frank

 

 


Subject:

RFR(XL/M) : 8178788: wrap JCStress test suite as jtreg tests


Date:

Tue, 18 Apr 2017 15:12:48 -0700


From:

Igor Ignatyev  <mailto:[hidden email]> <[hidden email]>


To:

 <mailto:[hidden email]> [hidden email] compiler  <mailto:[hidden email]> <[hidden email]>, HotSpot Open Source Developers  <mailto:[hidden email]> <[hidden email]>

 

 <http://cr.openjdk.java.net/~iignatyev/8178788/webrev.00/index.html> http://cr.openjdk.java.net/~iignatyev//8178788/webrev.00/index.html
> 69903 lines changed: 69903 ins; 0 del; 0 mod;
(69524 lines are generated)
 
Hi all,
 
could you please review this patch which adds a jtreg test wrapper for jcstress test suite and jtreg tests which run jsctress tests thru this wrapper?
 
webrev:  <http://cr.openjdk.java.net/~iignatyev/8178788/webrev.00/index.html> http://cr.openjdk.java.net/~iignatyev//8178788/webrev.00/index.html
JBS:  <https://bugs.openjdk.java.net/browse/JDK-8178788> https://bugs.openjdk.java.net/browse/JDK-8178788
testing: applications/jcstress
 
Thanks,
-- Igor

 

Loading...