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

[David Holmes]

Hi Misha,

On 4/02/2020 2:35 pm, mikhailo.seledtsov at oracle.com wrote:
> 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

  26  * @summary Linux kernel stack guard should not cause segfaults on 
x86-32
   27  * @modules java.base/jdk.internal.misc

Neither of those lines apply to the new test.

When do we run the testlibrary_tests tests?

---


    /**
      * Adds JVM library path to the native library path.
      *
      * @param pb ProcessBuilder to be updated with JVM library path.
      */
     public static ProcessBuilder addJvmLib(ProcessBuilder pb) throws 
Exception {

Please add @return doc comment.

No need to see updated webrev.

Thanks,
David

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