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

[David Holmes]

Hi Christian,

On 20/08/2013 2:10 PM, Christian Tornqvist wrote:
> Hi David,
>
>> 1. Missing @run tag
>
> The default action (when there is no @compile or @run tag ) of jtreg is '@run main Test.java'

Okay, but there have been a few issues lately with misconfigured tags so 
I think it preferable to be explicit.

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

I see we are relying on jtreg to do the cleanup here, but I like tests 
that can be run outside of jtreg too and not cause problems and/or a 
mess. But I won't push it. :)

Thanks,
David

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