RFR(XL): 8157957 - ClassNotFoundException: jdk.test.lib.JDKToolFinder

[Igor Ignatyev]

Hi Christian,

thank you for your tremendous work and sorry that you had to redo it ‘cuz of me )

I’ve not finished review yet, but in order to speed up things a little bit, here is my questions/comments so far:

- http://cr.openjdk.java.net/~ctornqvi/webrev/8157957/webrev.00/root
1. why have you put InfiniteLoop and TimeLimitedRunner into apps package? it looks like this package contained classes which are application: LingeredApp and LingeredAppWithDeadlock, neither TimeLimitedRunner nor InfiniteLoop is an application. ClassFileInstaller, on contrary, is an application, but it’s in default package.
2. AYK, we have tests for our testlibraries, I think those tests should be moved from hotspot/test/testlibrary_tests/ to root/test/testlibrary_tests/ together w/ libraries. btw, RedefineClassHelper has a reference to test/testlibrary_tests/RedefineClassTest.java

- http://cr.openjdk.java.net/~ctornqvi/webrev/8157957/webrev.00/hotspot
while I was looking thru the changeset I've noticed that
1. in some tests you haven’t replaced @build <test> w/ @buid s.h.WhiteBox, e.g. test/compiler/calls/fromInterpreted/InterpretedInvokeSpecial2NativeTest.java still has '@build compiler.calls.common.InvokeSpecial' however the test uses the same code as test/compiler/calls/fromInterpreted/InterpretedInvokeSpecial2InterpretedTest and you made replacement in latter.
2. in some tests you haven’t removed ‘@build s.h.WhiteBox’ when it should be possible, e.g. test/compiler/jsr292/NonInlinedCall/InvokeTest.java. the test doesn’t use ClassFileInstaller or reflection to get s.h.WhiteBox, so its dependency on WhiteBox class is statically computable and hence there is no need to have s.h.WhiteBox in @build action.

what I’m saying is it’s hard to correct all @build in all our tests and I don’t think it’s safe to do it now given the size, intermittent nature of possible bugs and how close we are to RDP. so I’d prefer to postpone @build part of change till jdk 10.

Thanks,
— Igor

> On Aug 12, 2016, at 6:32 PM, Christian Tornqvist <christian.tornqvist at oracle.com> wrote:
> 
> Hi everyone,
> 
> 
> 
> Please review this fix for the CNFE issue we've had in the Hotspot jtreg
> tests. The two big culprits for the intermittent CNFE are: Duplicate classes
> on classpath with different content and @build tags causing class files to
> be written in bad places.
> 
> 
> 
> There has been a lot of confusion around when there's a need to add @build
> to a test. In general it turns out it has been overused, which can lead to
> side effects. Here's an easy rule:
> 
> 
> 
> If you run only that test in a clean jtwork folder and it passes, then
> there's no need for @build. 
> 
> 
> 
> If it doesn't pass, then it might need an explicit @build, here are some
> examples when it might be needed:
> 
> 
> 
> * If the class is used by ClassFileInstaller and this is invoked by @run
> main ClassFileInstaller (sun.hotspot.Whitebox is an example of this)
> 
> * If it's a class that that doesn't have reference from the Test itself (if
> "javac Test" wouldn't compile it, you might need to explicitly @build it)
> 
> 
> 
> The change includes:
> 
> 
> 
> * Removing all unnecessary @build tags, some of the CNFE was due to use of
> it to compile classes in /test/lib or /testlibrary when having different
> classpath (@library) set.
> 
> * Changed @build <testname> to @build sun.hotspot.Whitebox
> 
> * Moved /test/lib/share/classes/jdk/test/lib to /test/lib/jdk/test/lib 
> 
> * Some of the classes in /hotspot/test/testlibrary was only used by one or
> two tests, these classes shouldn't be part of the shared testlibrary code
> and they were moved to the location of the test
> 
> * Merged/moved the classes in /hotspot/test/testlibrary to /test/lib
> 
> * Moved some of the /test/lib classes into packages
> 
> * Changed @library /testlibrary to @library /test/lib
> 
> * Changed imports from jdk.test.lib.* to explicit imports to help in future
> refactoring
> 
> 
> 
> Almost all of the changes in here are in the jtreg @build and @library tags,
> very little code has been touched. Copyright headers will be updated before
> push.
> 
> 
> 
> Testing done: Ran the entire hotspot/test/ folder multiple times locally and
> in RBT
> 
> 
> 
> Webrev:
> 
> http://cr.openjdk.java.net/~ctornqvi/webrev/8157957/webrev.00/
> 
> 
> 
> A recently added test required me to fix that as well, generating and
> uploading a new webrev of the Hotspot repo changes takes about 3h though, so
> here's the diff for that file:
> 
> 
> 
> diff -r f37577c20a6b test/serviceability/sa/sadebugd/SADebugDTest.java
> 
> --- a/test/serviceability/sa/sadebugd/SADebugDTest.java Wed Aug 10 21:02:14
> 2016 -0400
> 
> +++ b/test/serviceability/sa/sadebugd/SADebugDTest.java Fri Aug 12 11:25:54
> 2016 -0400
> 
> @@ -26,7 +26,7 @@
> 
>  * @summary Checks that the jshdb debugd utility sucessfully starts
> 
>  *          and tries to attach to a running process
> 
>  * @modules java.base/jdk.internal.misc
> 
> - * @library /test/lib/share/classes
> 
> + * @library /test/lib
> 
>  *
> 
>  * @run main/othervm SADebugDTest
> 
>  */
> 
> 
> 
> Bug:
> 
> https://bugs.openjdk.java.net/browse/JDK-8157957
> 
> 
> 
> Thanks,
> 
> Christian
> 
> 
> 
> 
>