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

[Mikhailo Seledtsov]

Gary, David, Christian,
    Thank you for the review.

Here is my feedback:
  On (1), the missing @run tag, I will make the change before the 
check-in. I assume the change is minor enough not to require posting new 
webrev.

  On (3), I will follow Christian's recommendation, since David says he 
will not push it. My personal view on this is to keep the test code to 
the substance of the test, and leave cleanup to the framework. As for 
running  JTREG tests outside of the framework, I ofter do that; I 
usually wrap the tests into a script that deletes or renames JT* folders 
before running the tests.

  On (2), the use of 'jar' tool from compile vs test JDK 
(JDKToolFinder.getJDKTool("jar")). I have discussed this with Christian. 
Christian says that there is a change on the way to fix the gedJdkTool() 
API. The new  behavior of  this method will attempt to find the tool in 
the Test JDK first; if not found, it will use the tool from the compile 
JDK. Given this, and if no one has additional objections, I am inclined 
not to make changes to this section of the test.

To recap, if I do not receive additional comments or objections, I will 
make change (1) and send it on the way to check in.

Thank you,
Misha

On 8/20/2013 12:16 AM, David Holmes wrote:
> 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
>>