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

[mikhailo.seledtsov at oracle.com]

Here is the updated webrev: 
http://cr.openjdk.java.net/~mseledtsov/8179317.02/

I have addressed comments from Igor, as well as a concern from David 
about testing.

I have added a native test: || 
test/hotspot/jtreg/testlibrary_tests/process/TestNativeProcessBuilder.java


Misha

*
*

*
*

On 1/31/20 4:34 PM, mikhailo.seledtsov at oracle.com wrote:
> 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
>>>>>>>>
>>>