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

Igor Ignatyev igor.ignatyev at oracle.com
Wed Apr 19 02:51:42 UTC 2017


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 <david.holmes at oracle.com> 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
>> 



More information about the hotspot-compiler-dev mailing list