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

Christian Tornqvist christian.tornqvist at oracle.com
Mon Sep 16 14:47:37 PDT 2013 

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

More information about the hotspot-dev mailing list