RFR (M) : JDK-8016029 : test runtime/6878713/Test6878713.sh failed - round 2

[Christian Tornqvist]

Hi David,

>1. Missing @run tag

The default action (when there is no @compile or @run tag ) of jtreg is '@run main Test.java'

>3. Not new with your change, but the unpacked files don't get cleaned up.

Jtreg will clean up the scratch directory when the test has passed, this not something the tests should do themselves

Thanks,
Christian

-----Original Message-----
From: hotspot-runtime-dev-bounces at openjdk.java.net [mailto:hotspot-runtime-dev-bounces at openjdk.java.net] On Behalf Of David Holmes
Sent: Monday, August 19, 2013 10:16 PM
To: Mikhailo Seledtsov
Cc: hotspot-runtime-dev
Subject: Re: RFR (M) : JDK-8016029 : test runtime/6878713/Test6878713.sh failed - round 2

Hi Misha,

I have a couple of issues with this.

1. Missing @run tag

2. The shell script uses the COMPILEJDK to find the jar tool, but you are using JDKToolFinder which uses the TESTJDK - which means your test will be limited to running only on a full JDK (not a JRE). I thought the testlibrary supported using either the test-jdk or the compile-jdk but I don't see that now. :(

3. Not new with your change, but the unpacked files don't get cleaned up.

Thanks,
David

On 20/08/2013 3:50 AM, Mikhailo Seledtsov wrote:
> This is a round 2 review for this fix.
> Please review.
>
> Original review request (dated 16-July-2013) with the same subject was:
> "
> Please review this test fix. The change is a fix for the original bug, 
> plus an update to the test condition to reflect new JVM behavior and a 
> rewrite of an sh script test in Java.
> JBS: https://jbs.oracle.com/bugs/browse/JDK-8016029
> (Old) Webrev: 
> http://cr.openjdk.java.net/~mseledtsov/6878713/webrev.00/
> <http://cr.openjdk.java.net/%7Emseledtsov/6878713/webrev.00/>
> "
>
>
> Change since last round of review:
>      JVM run-time technical leadership has made a decision to keep the 
> testcase.jar until a long-term solution is in place for handling "Java 
> assembler"-based and byte-code-based test cases.
>
> Here is the updated webrev and comments:
>      Webrev: http://cr.openjdk.java.net/~mseledtsov/8016029/webrev.01/
> <http://cr.openjdk.java.net/%7Emseledtsov/8016029/webrev.01/>
>      Testing: JPRT run for this test
> (2013-08-19-154813.mseledtsov.bugFixing01-hotspot-rt)
>
> Here are original notes:
> - the original behavior of the VM in this test case has changed, and 
> the expected error message has changed as well. There is an extensive 
> amount of information about this in the original bug, posted by Dan. 
> The change was introduced in change-set: 4876:f75faf51e8c4 by Harold. 
> Checking for the old expected messages has been removed from the test. 
> This change is for JDK8 and forward, and no back-ports planned.
>
> - the flag -XX:+IgnoreUnrecognizedVMOptions has been removed to avoid 
> masking a potential problem. If VM drops support for any of XX flags 
> used in this test, it is better to know about it right away rather 
> than mask it and silently pass/ignore.
>
> - the flag -XX:+PrintCommandLineFlags has been removed; if this 
> information is desired, IMO it should be triggered either by a verbose 
> mode of JTREG tools, or by hand, not by individual test(s).
>
> Round 2 notes:
> - the shell script has been replaced by Java
>
> - test has been moved to a new location, moving away from using bug 
> numbers to using functional categories for test sub-folders
>
>
> Misha