RFR(S): 8179317: [TESTBUG] rewrite runtime shell tests in java

[mikhailo.seledtsov at oracle.com]

David,

   Thank you for your feedback.

On 1/30/20 9:25 PM, David Holmes wrote:
> Hi Misha,
>
> Overall this is looking quite good.
>
> On 31/01/2020 2:58 pm, Igor Ignatyev wrote:
>> Hi Misha,
>>
>> I have a two comments regarding ProcessTools.java:
>> 1. in createNativeTestProcessBuilder, shouldn't ".exe" be added to 
>> executableName if we are on windows?
>
> More generally the only tests that use these new APIs are limited to 
> Linux, so AFAICS these new test library APIs are untested on other 
> platforms - is that the case?

I am looking into creating a simple test for 
createNativeTestProcessBuilder(), which will also indirectly test its 
dependency methods in Platform.java.

The test I have in mind will launch a native executable, which will in 
turn launch a simple Test class. The main test will check for success 
exit code.

> One query below ...
>
>> 2. making addJvmLib to return the instance of ProcessBuilder might 
>> make this API more akin to the builder pattern, which ProcessBuilder 
>> follows, so it will seem more natural
>>
>> the rest looks good to me.
>>
>> -- Igor
>>
>>
>>> On Jan 30, 2020, at 8:41 PM, mikhailo.seledtsov at oracle.com wrote:
>>>
>>> Here is the updated webrev: 
>>> http://cr.openjdk.java.net/~mseledtsov/8179317.01/
>>>
>>> I did the following since last webrev:
>>>
>>>    - addressed feedback from Igor and Leonid
>>>
>>>    - tests StackGap, StackGuard and TLS launch a native executable, 
>>> which in turn loads and starts JVM via JNI.
>>>      This requires additional setup, such as native library path and 
>>> full path to test executable, all platform
>>>      dependent.
>>>
>>>    - I have found similar code in runtime/signal. I have identified 
>>> common code and methods, and placed them
>>>      with the test library. The code identifying a path to JVM 
>>> shared lib now lives in Platform.java, and
>>>      helper methods for native process builder went to 
>>> ProcessTools.java
>>>
>>>    - these are new methods in the test library, no changes to 
>>> existing API or behavior, which limits the
>>>      risk for the users of the test libraries.
>
> That all seems fine modulo the cross-platform testing.
>
>>>    - I have retrofitted runtime/signal/SigTestDriver.java to use 
>>> these new library methods.
>>>
>>>    - Also I had to explicitly set the CLASSPATH to 
>>> Utils.TEST_CLASS_PATH for StackGap, StackGuard and TLS tests
>>>      W/o this, each test will fail in a following manner:
>>>
>>>      #  Internal Error 
>>> (open/src/hotspot/share/runtime/jniHandles.inline.hpp:91), 
>>> pid=13075, tid=13075
>>>      #  assert(handle != __null) failed: JNI handle should not be null
>>>      # V  [libjvm.so+0x8944e5] 
>>> JNIHandles::resolve_non_null(_jobject*)+0x1c5
>
> I'm curious about this. Can you send me a hs_err file via email. This 
> looks like a bug to me.

Will do.


Thanks,

Misha

>
> Thanks,
> David
> -----
>
>>>
>>>
>>> Testing:
>>>    - run the affected tests on Linux-x64: All PASS
>>>    - run the affected tests on other platforms: in progress...
>>>    - run tier1,2,3: in progress
>>>
>>>
>>> Thank you,
>>> Misha
>>>
>>> On 1/27/20 12:08 PM, mikhailo.seledtsov at oracle.com wrote:
>>>>
>>>> On 1/24/20 10:33 PM, Leonid Mesnik wrote:
>>>>> Looks good for me also.
>>>>
>>>> Thank you Leonid.
>>>>
>>>> I will make changes recommended by Igor, and post an updated webrev.
>>>>
>>>>
>>>> Misha
>>>>
>>>>>
>>>>> And I agree that it would be better to rename TestDescription.java.
>>>>>
>>>>> Leonid
>>>>>
>>>>> On 1/24/20 7:14 PM, Igor Ignatyev wrote:
>>>>>> Hi Misha,
>>>>>>
>>>>>> in general looks good to me, I have two comments though:
>>>>>> - why did you put 'new OutputAnalyzer(pb.start())' into parentheses?
>>>>>> - in vmTestbase, TestDescription.java was originally used to be 
>>>>>> just that a test description w/o any test logic inside, and I'd 
>>>>>> prefer it stays that way. in other words, I'd recommend you to 
>>>>>> rename 
>>>>>> test/hotspot/jtreg/vmTestbase/metaspace/flags/maxMetaspaceSize/TestDescription.java 
>>>>>> to 
>>>>>> test/hotspot/jtreg/vmTestbase/metaspace/flags/maxMetaspaceSize/Test.java 
>>>>>> for example. this will require the corresponding change in 
>>>>>> test/hotspot/jtreg/TEST.groups
>>>>>>
>>>>>> Thanks,
>>>>>> -- Igor
>>>>>>
>>>>>>> On Jan 24, 2020, at 5:56 PM, mikhailo.seledtsov at oracle.com wrote:
>>>>>>>
>>>>>>> Please review this change that converts HotSpot/Runtime shell 
>>>>>>> tests to Java
>>>>>>>
>>>>>>>       JBS: https://bugs.openjdk.java.net/browse/JDK-8179317
>>>>>>>       Webrev: http://cr.openjdk.java.net/~mseledtsov/8179317.00/
>>>>>>>       Testing:
>>>>>>>          Ran the affected tests
>>>>>>>
>>>>>>>
>>>>>>> Thank you,
>>>>>>> Misha
>>>>>>>
>>