RFR(S): 8179317: [TESTBUG] rewrite runtime shell tests in java
igor.ignatyev at oracle.com
Fri Jan 31 04:58:17 UTC 2020
I have a two comments regarding ProcessTools.java:
1. in createNativeTestProcessBuilder, shouldn't ".exe" be added to executableName if we are on windows?
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.
> 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
> - 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.
> - 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
> - 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,
> 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.
>>> And I agree that it would be better to rename TestDescription.java.
>>> 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
>>>> -- 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/
>>>>> Ran the affected tests
>>>>> Thank you,
More information about the hotspot-runtime-dev