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

[Erik Helin]

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