RFR(S): 8014905 - [TESTBUG] Some hotspot tests should be updated to divide test jdk and compile jdk

[David Holmes]

Thumbs up from me!

Thanks,
David

On 17/09/2013 7:47 AM, Christian Tornqvist wrote:
> Updated webrev where I've removed
>
> runtime/NMT/MallocTestType.java
> runtime/NMT/ThreadedMallocTestType.java
> runtime/NMT/ThreadedVirtualAllocTestType.java
> runtime/NMT/VirtualAllocTestType.java
> runtime/NMT/BaselineWithParameter.java
> runtime/NMT/JcmdScale.java
> runtime/NMT/JcmdWithNMTDisabled.java
> runtime/NMT/ShutdownTwice.java
> runtime/NMT/SummaryAfterShutdown.java
> runtime/NMT/SummarySanityCheck.java
> runtime/RedefineObject/TestRedefineObject.java
>
> from the needs_jdk group in TEST.groups based on David's comment in the bug.
>
> Webrev can be found at:
> http://cr.openjdk.java.net/~ctornqvi/webrev/8014905/webrev.04/
>
> Thanks,
> Christian
>
> -----Original Message-----
> From: David Holmes [mailto:david.holmes at oracle.com]
> Sent: Sunday, September 15, 2013 9:45 PM
> To: Christian Tornqvist
> Cc: 'hotspot-dev developers'
> Subject: Re: RFR(S): 8014905 - [TESTBUG] Some hotspot tests should be updated to divide test jdk and compile jdk
>
> On 16/09/2013 11:41 AM, David Holmes wrote:
>> On 14/09/2013 4:52 AM, Christian Tornqvist wrote:
>>> I've added getCompileJDKTool() and getTestJDKTool(), the common
>>> function getJDKTool() will still look in test.jdk first and
>>> compile.jdk as a fallback.
>>>
>>> Changed the TestVerifyDuringStartup.java that Igor changed as part of
>>> his change to use getJDKTool() since this is what
>>> ProcessTools.createJavaProcessBuilder() does.
>>>
>>> Verified my change locally by running test/runtime and test/gc jtreg
>>> tests to verify that they work correctly when test.jdk is set to a JRE.
>>>
>>> New webrev can be found at:
>>> http://cr.openjdk.java.net/~ctornqvi/webrev/8014905/webrev.03/
>>
>> Looks good to me!
>>
>> Thanks,
>> Chris
>
> That should be:
>
> Thanks, Chris.
>
> David
> -----
>
> David
> -----
>
> :)
>
>>> Thanks,
>>> Christian
>>>
>>> -----Original Message-----
>>> From: hotspot-runtime-dev-bounces at openjdk.java.net
>>> [mailto:hotspot-runtime-dev-bounces at openjdk.java.net] On Behalf Of
>>> David Holmes
>>> Sent: Friday, September 13, 2013 7:11 AM
>>> To: Erik Helin
>>> Cc: hotspot-runtime-dev at openjdk.java.net
>>> Subject: Re: RFR(S): 8014905 - [TESTBUG] Some hotspot tests should be
>>> updated to divide test jdk and compile jdk
>>>
>>> Just to cc the list what I said in other email ...
>>>
>>> I think this is a good compromise. Having an API that allows the test
>>> to select which JDK to use is important; but maintaining
>>> compatability with existing test behaviour is also important.
>>>
>>> Thanks,
>>> David
>>>
>>> On 12/09/2013 11:22 PM, Erik Helin wrote:
>>>> On 2013-09-12, David Holmes wrote:
>>>>> On 12/09/2013 9:27 PM, Igor Ignatyev wrote:
>>>>>> On 09/12/2013 02:13 PM, David Holmes wrote:
>>>>>>> On 12/09/2013 8:07 PM, Igor Ignatyev wrote:
>>>>>>>> On 09/12/2013 01:54 PM, David Holmes wrote:
>>>>>>>>> On 12/09/2013 4:20 PM, Igor Ignatyev wrote:
>>>>>>>>>> Christian,
>>>>>>>>>>
>>>>>>>>>> I have made some changes in JDKToolFinder w/ patch for
>>>>>>>>>> JDK-8012447[*]:
>>>>>>>>>>     - getJDKTool uses 'compile.jdk'
>>>>>>>>>>     - getCurrentJDKTool uses 'test.jdk'
>>>>>>>>>>
>>>>>>>>>> So, I'm not sure that your change is necessary.
>>>>>>>>>
>>>>>>>>> I prefer Christian's approach as it is behaviour preserving.
>>>>>>>>> With your change any test that actually wants/needs to use the
>>>>>>>>> test JDK will have to be modified.
>>>>>>>> I agree, but w/ Christian's approach we could get situation that
>>>>>>>> we test binary from another JDK, e.g.
>>>>>>>> 'test/gc/TestVerifyDuringStartup.java'
>>>>>>>> which used 'java' from 'compile.jdk' instead of 'test.jdk'.
>>>>>>>
>>>>>>> I don't see how as the test JDK is checked first. The gap in
>>>>>>> Christian's approach is the more rare case of where the test JDKs
>>>>>>> tool must not be used for that part of the test.
>>>>>> Oh sorry, I misread the code. Anyway, I don't think that this code
>>>>>> is good, because if I want to get one of jdk tool, I expect that I
>>>>>> will get it from '-compilejdk' if this option's specified, and
>>>>>> from '-jdk', otherwise. But w/  Christian's patch I will get tool
>>>>>> from '-jdk' if tool exists there, that may confuse.
>>>>>
>>>>> Anyone using this function has to know what it returns regardless
>>>>> of whether that is from compile-jdk or test-jdk. With your change a
>>>>> bunch of tests that were using the test-jdk tools will now be using
>>>>> the compile-jdk tools, and while that might not cause a test
>>>>> failure it might negate the purpose of the test. So unless you've
>>>>> examined all the tests that use this API I can't agree that your
>>>>> change was appropriate.
>>>>
>>>> I, Igor and Christian discussed this on IM and the solution that we
>>>> came up with was that JDKToolFinder should have three methods:
>>>> getJDKTool, getCompilerJDKTool and getTestJDKTool. getJDKTool
>>>> should, as in Christian's change, first try to make use of
>>>> getTestJDKTool and then, if that fails, use getCompilerJDKTool.
>>>>
>>>> getCompilerJDKTool and getTestJDKTool should be like the ones Igor
>>>> has written, but the should use make use of a private function to
>>>> avoid code duplication.
>>>>
>>>> This way, writing an simple test will still be easy, using
>>>> getJDKTool should in most cases be what you want. However, if you
>>>> are writing a more tricky test, getTestJDKTool and
>>>> getCompilerJDKTool are still there for those cases.
>>>>
>>>> David, what do think about this suggestion?
>>>>
>>>>>>> So I think
>>>>>>>> that any test that actually wants/needs to use the test JDK must
>>>>>>>> do it explicitly.
>>>>>>>>>
>>>>>>>>>> [*]
>>>>>>>>>> http://hg.openjdk.java.net/hsx/hotspot-comp/hotspot/rev/ceda33
>>>>>>>>>> ff
>>>>>>>>>> 54a3
>>>>>>>>>
>>>>>>>>> Was this reviewed on hotspot-dev/hotspot-runtime-dev or only
>>>>>>>>> hotspot-compiler-dev?
>>>>>>>>
>>>>>>>> It was reviewed only on hotspot-compiler-dev.
>>>>>>>
>>>>>>> These kinds of changes really have to be reviewed more broadly as
>>>>>>> they affect everyone. We wouldn't even have known about your
>>>>>>> change if Christian hadn't done this one. Plus the testlibrary
>>>>>>> really should have a nominal owner to coordinate things - and we
>>>>>>> need to make sure the hotspot and JDK versions are kept in sync.
>>>>
>>>> I guess that I and Christian usually try to monitor the changes to
>>>> the hotspot testlibrary, and I think Igor will start do it as well
>>>> :) This way, we at least have one from runtime, one from compiler
>>>> and one from gc.
>>>>
>>>> As for keeping the JDK and HotSpot versions in sync, well that is a
>>>> problem. One of the issues is that just because we in the hotspot
>>>> team reviewed and approved changes to HotSpot's testlibrary does not
>>>> automatically mean that the JDK team wants these patches. Therefore,
>>>> keeping the JDK and the HotSpot test libraries in sync will require
>>>> more work than just "moving patches".
>>>>
>>>> Unfortunately, I do not have any good solution for this at the
>>>> moment :(
>>>>
>>>>>> I fully agree w/ you and I'm really sorry about the situation.
>>>>>> But I don't see big difference between 'hotspot-runtime-dev' and
>>>>>> 'hotspot-compiler-dev', so *this* change "really have to be
>>>>>> reviewed more broadly as they affect everyone".
>>>>>
>>>>> I meant reviewing on hotspot-runtime-dev as well as
>>>>> hotspot-compiler-dev, not in place of. hotspot-dev could have been
>>>>> in place.
>>>>
>>>> I think hotspot-dev is better for changes to the hotspot testlibrary
>>>> since the changes affect all the hotspot devevelopers.
>>>>
>>>> Thanks,
>>>> Erik
>>>>
>>>>> Cheers,
>>>>> David
>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> David
>>>>>>>>>
>>>>>>>>>> Best regards,
>>>>>>>>>> Igor Ignatyev
>>>>>>>>>>
>>>>>>>>>> On 09/12/2013 06:38 AM, Christian Tornqvist wrote:
>>>>>>>>>>> Hi everyone,
>>>>>>>>>>>
>>>>>>>>>>> Small change in JDKToolFinder so that it will now look in
>>>>>>>>>>> compile.jdk if the tool is not found in test.jdk. I’ve tested
>>>>>>>>>>> it locally by running tests with test.jdk set to a JRE and
>>>>>>>>>>> compile.jdk set to JDK to see that they work correctly.
>>>>>>>>>>>
>>>>>>>>>>> Webrev:
>>>>>>>>>>>
>>>>>>>>>>> http://cr.openjdk.java.net/~ctornqvi/webrev/8014905/webrev.02
>>>>>>>>>>> /
>>>>>>>>>>>
>>>>>>>>>>> Bug is unfortunately not visible externally at this time L
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>>
>>>>>>>>>>> Christian
>>>>>>>>>>>
>>>
>