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

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

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
|

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
|

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
|

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
|

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

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

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

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
|

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
|

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

Aleksey Shipilev-4
In reply to this post by Igor Ignatyev
On 04/19/2017 12: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:
TL;DR: This patch introduces more problems than it solves. Just run the jcstress
tests-all JAR against the tested runtime.

Wrapping jcstress tests with jtreg defies the purpose of jcstress harness --
that is, running lots of tests as fast as it possibly could without affecting
testing quality. For example, by cleverly reusing VMs between the tests, using
Whitebox to deoptimize without restarting the VMs, etc. It really wastes CPU
time to run each test in isolation.

Also, it does not "automatically" work, which defies "easy to run" goal:

Caused by: java.io.FileNotFoundException: Couldn't automatically resolve
dependency for jcstress-tests-all , revision 0.3
Please specify the location using jdk.test.lib.artifacts.jcstress-tests-all
        at
jdk.test.lib.artifacts.DefaultArtifactManager.resolve(DefaultArtifactManager.java:37)
        at jdk.test.lib.artifacts.ArtifactResolver.resolve(ArtifactResolver.java:54)
        at applications.jcstress.JcstressRunner.pathToArtifact(JcstressRunner.java:53)
        ... 8 more

Okay, brilliant! How do I configure this, if I run "make test"?

CONF=linux-x86_64-normal-server-release LOG=info make test TEST="hotspot_all"


-Aleksey


signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Stuart Monteith
Hello,
  I've spent some time on this,  and I have to admit that I'm stumped. I get exactly the same errors on x86 on jdk10/hs and jdk10/jdk10 with arecent build of JTReg and JT_HOME set appropriately.

Are there any pointers on how this is supposed to be run?

Thanks,
   Stuart

On 25 April 2017 at 11:47, Aleksey Shipilev <[hidden email]> wrote:
On 04/19/2017 12: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:

TL;DR: This patch introduces more problems than it solves. Just run the jcstress
tests-all JAR against the tested runtime.

Wrapping jcstress tests with jtreg defies the purpose of jcstress harness --
that is, running lots of tests as fast as it possibly could without affecting
testing quality. For example, by cleverly reusing VMs between the tests, using
Whitebox to deoptimize without restarting the VMs, etc. It really wastes CPU
time to run each test in isolation.

Also, it does not "automatically" work, which defies "easy to run" goal:

Caused by: java.io.FileNotFoundException: Couldn't automatically resolve
dependency for jcstress-tests-all , revision 0.3
Please specify the location using jdk.test.lib.artifacts.jcstress-tests-all
        at
jdk.test.lib.artifacts.DefaultArtifactManager.resolve(DefaultArtifactManager.java:37)
        at jdk.test.lib.artifacts.ArtifactResolver.resolve(ArtifactResolver.java:54)
        at applications.jcstress.JcstressRunner.pathToArtifact(JcstressRunner.java:53)
        ... 8 more

Okay, brilliant! How do I configure this, if I run "make test"?

CONF=linux-x86_64-normal-server-release LOG=info make test TEST="hotspot_all"


-Aleksey


Reply | Threaded
Open this post in threaded view
|

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

Stuart Monteith
For the record, if I put my build of jcstress.jar in $HOME, the following allows the jcstress tests to run:

    make test TEST="hotspot_all" EXTRA_JTREG_OPTIONS="-Djdk.test.lib.artifacts.jcstress-tests-all=$HOME/jcstress.jar"

From "make help" and the makefiles themselves, I had expected the follow to work:
    make  TEST="hotspot_all" JTREG="VM_OPTIONS=-Djdk.test.lib.artifacts.jcstress-tests-all=$HOME/jcstress.jar" test

but it does not - the JTREG parameter is apparently ignored. This is unfortunate as there is a warning as it is a non-control variable.

Am I wrong in thinking that this was written for testing internall within Oracle? I can not find an instance of  "com.oracle.jib.api.JibServiceFactory" in the OpenJDK project or elsewhere.

BR,
    Stuart

On 8 September 2017 at 16:53, Stuart Monteith <[hidden email]> wrote:
Hello,
  I've spent some time on this,  and I have to admit that I'm stumped. I get exactly the same errors on x86 on jdk10/hs and jdk10/jdk10 with arecent build of JTReg and JT_HOME set appropriately.

Are there any pointers on how this is supposed to be run?

Thanks,
   Stuart

On 25 April 2017 at 11:47, Aleksey Shipilev <[hidden email]> wrote:
On 04/19/2017 12: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:

TL;DR: This patch introduces more problems than it solves. Just run the jcstress
tests-all JAR against the tested runtime.

Wrapping jcstress tests with jtreg defies the purpose of jcstress harness --
that is, running lots of tests as fast as it possibly could without affecting
testing quality. For example, by cleverly reusing VMs between the tests, using
Whitebox to deoptimize without restarting the VMs, etc. It really wastes CPU
time to run each test in isolation.

Also, it does not "automatically" work, which defies "easy to run" goal:

Caused by: java.io.FileNotFoundException: Couldn't automatically resolve
dependency for jcstress-tests-all , revision 0.3
Please specify the location using jdk.test.lib.artifacts.jcstress-tests-all
        at
jdk.test.lib.artifacts.DefaultArtifactManager.resolve(DefaultArtifactManager.java:37)
        at jdk.test.lib.artifacts.ArtifactResolver.resolve(ArtifactResolver.java:54)
        at applications.jcstress.JcstressRunner.pathToArtifact(JcstressRunner.java:53)
        ... 8 more

Okay, brilliant! How do I configure this, if I run "make test"?

CONF=linux-x86_64-normal-server-release LOG=info make test TEST="hotspot_all"


-Aleksey



Reply | Threaded
Open this post in threaded view
|

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

Magnus Ihse Bursie
Stuart,

On 2017-09-13 20:36, Stuart Monteith wrote:
For the record, if I put my build of jcstress.jar in $HOME, the following allows the jcstress tests to run:

    make test TEST="hotspot_all" EXTRA_JTREG_OPTIONS="-Djdk.test.lib.artifacts.jcstress-tests-all=$HOME/jcstress.jar"

From "make help" and the makefiles themselves, I had expected the follow to work:
    make  TEST="hotspot_all" JTREG="VM_OPTIONS=-Djdk.test.lib.artifacts.jcstress-tests-all=$HOME/jcstress.jar" test

but it does not - the JTREG parameter is apparently ignored. This is unfortunate as there is a warning as it is a non-control variable.

You are using the "test" target, but the JTREG option is only available for the new "run-test" target. Using "test" will invoke the old testing framework, which is about to be replaced by a more modern and integrated one. In the long term, "test" will invoke the new framework, but during a transition period, "run-test" needs to be used.

For what it's worth, I tried your command line (but without the patch applied) and verified using LOG=cmdlines that the -D option was indeed passed to jtreg.

/Magnus


Am I wrong in thinking that this was written for testing internall within Oracle? I can not find an instance of  "com.oracle.jib.api.JibServiceFactory" in the OpenJDK project or elsewhere.

BR,
    Stuart

On 8 September 2017 at 16:53, Stuart Monteith <[hidden email]> wrote:
Hello,
  I've spent some time on this,  and I have to admit that I'm stumped. I get exactly the same errors on x86 on jdk10/hs and jdk10/jdk10 with arecent build of JTReg and JT_HOME set appropriately.

Are there any pointers on how this is supposed to be run?

Thanks,
   Stuart

On 25 April 2017 at 11:47, Aleksey Shipilev <[hidden email]> wrote:
On 04/19/2017 12: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:

TL;DR: This patch introduces more problems than it solves. Just run the jcstress
tests-all JAR against the tested runtime.

Wrapping jcstress tests with jtreg defies the purpose of jcstress harness --
that is, running lots of tests as fast as it possibly could without affecting
testing quality. For example, by cleverly reusing VMs between the tests, using
Whitebox to deoptimize without restarting the VMs, etc. It really wastes CPU
time to run each test in isolation.

Also, it does not "automatically" work, which defies "easy to run" goal:

Caused by: java.io.FileNotFoundException: Couldn't automatically resolve
dependency for jcstress-tests-all , revision 0.3
Please specify the location using jdk.test.lib.artifacts.jcstress-tests-all
        at
jdk.test.lib.artifacts.DefaultArtifactManager.resolve(DefaultArtifactManager.java:37)
        at jdk.test.lib.artifacts.ArtifactResolver.resolve(ArtifactResolver.java:54)
        at applications.jcstress.JcstressRunner.pathToArtifact(JcstressRunner.java:53)
        ... 8 more

Okay, brilliant! How do I configure this, if I run "make test"?

CONF=linux-x86_64-normal-server-release LOG=info make test TEST="hotspot_all"


-Aleksey




Reply | Threaded
Open this post in threaded view
|

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

Stuart Monteith
Thank you Magnus, that's useful. 

My working command line is:
    make run-test TEST="hotspot_all" JTREG="VM_OPTIONS=-Djdk.test.lib.artifacts.jcstress-tests-all=$HOME/jcstress.jar"

but it takes an excessively long time to run.  Are there plans for a means to cleanly disable the JCStress JTreg tests so we can efficiently run them with the JCStress harness?


Thanks,
   Stuart

On 14 September 2017 at 08:40, Magnus Ihse Bursie <[hidden email]> wrote:
Stuart,

On 2017-09-13 20:36, Stuart Monteith wrote:
For the record, if I put my build of jcstress.jar in $HOME, the following allows the jcstress tests to run:

    make test TEST="hotspot_all" EXTRA_JTREG_OPTIONS="-Djdk.test.lib.artifacts.jcstress-tests-all=$HOME/jcstress.jar"

From "make help" and the makefiles themselves, I had expected the follow to work:
    make  TEST="hotspot_all" JTREG="VM_OPTIONS=-Djdk.test.lib.artifacts.jcstress-tests-all=$HOME/jcstress.jar" test

but it does not - the JTREG parameter is apparently ignored. This is unfortunate as there is a warning as it is a non-control variable.

You are using the "test" target, but the JTREG option is only available for the new "run-test" target. Using "test" will invoke the old testing framework, which is about to be replaced by a more modern and integrated one. In the long term, "test" will invoke the new framework, but during a transition period, "run-test" needs to be used.

For what it's worth, I tried your command line (but without the patch applied) and verified using LOG=cmdlines that the -D option was indeed passed to jtreg.

/Magnus



Am I wrong in thinking that this was written for testing internall within Oracle? I can not find an instance of  "com.oracle.jib.api.JibServiceFactory" in the OpenJDK project or elsewhere.

BR,
    Stuart

On 8 September 2017 at 16:53, Stuart Monteith <[hidden email]> wrote:
Hello,
  I've spent some time on this,  and I have to admit that I'm stumped. I get exactly the same errors on x86 on jdk10/hs and jdk10/jdk10 with arecent build of JTReg and JT_HOME set appropriately.

Are there any pointers on how this is supposed to be run?

Thanks,
   Stuart

On 25 April 2017 at 11:47, Aleksey Shipilev <[hidden email]> wrote:
On 04/19/2017 12: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:

TL;DR: This patch introduces more problems than it solves. Just run the jcstress
tests-all JAR against the tested runtime.

Wrapping jcstress tests with jtreg defies the purpose of jcstress harness --
that is, running lots of tests as fast as it possibly could without affecting
testing quality. For example, by cleverly reusing VMs between the tests, using
Whitebox to deoptimize without restarting the VMs, etc. It really wastes CPU
time to run each test in isolation.

Also, it does not "automatically" work, which defies "easy to run" goal:

Caused by: java.io.FileNotFoundException: Couldn't automatically resolve
dependency for jcstress-tests-all , revision 0.3
Please specify the location using jdk.test.lib.artifacts.jcstress-tests-all
        at
jdk.test.lib.artifacts.DefaultArtifactManager.resolve(DefaultArtifactManager.java:37)
        at jdk.test.lib.artifacts.ArtifactResolver.resolve(ArtifactResolver.java:54)
        at applications.jcstress.JcstressRunner.pathToArtifact(JcstressRunner.java:53)
        ... 8 more

Okay, brilliant! How do I configure this, if I run "make test"?

CONF=linux-x86_64-normal-server-release LOG=info make test TEST="hotspot_all"


-Aleksey