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

classic Classic list List threaded Threaded
8 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

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
|  
Report Content as Inappropriate

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

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