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

[David Holmes]

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?

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.

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