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

Tue Feb 4 17:50:16 UTC 2020

Hi David,

   Thank you for review. Please see my comments inline:

On 2/3/20 8:50 PM, David Holmes wrote:
> 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.
Fixed.
>
> When do we run the testlibrary_tests tests?
It is part of hotspot_misc group.
>
> ---
>
>
>    /**
>      * 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.
Done
>
> No need to see updated webrev.

Thank you,

Misha

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