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

[David Holmes]

Hi Christian,

First, thanks for attempting this, it is a huge effort!

Second, as this is changing the <root>/test/lib files then it needs to 
be reviewed by more than just the hotspot group.

I won't claim to have examined every file in detail - there is a lot 
that has to be taken on face-value here. I have skimmed the webrevs and 
patch files.

A few comments in-lined below ...

On 13/08/2016 1:32 AM, Christian Tornqvist 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.

Do the jtreg authors agree with that rule? I'm happy to have such a 
simple rule as long as it is agreed upon by all.

> 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

Are these only: Platform.jdkLibPath(), Platform.sharedObjectName ?

They seem general-purpose to me even if mosts tests don't need to care. 
So they seem suitable inclusions into the Platform class to me.

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

Related to that I see a number of entries like:

- * @library /testlibrary /test/lib /
+ * @library /test/lib /

this is not part of your change by why is / listed as a library ??

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

Just to be crystal clear, did you also run all the @ignore'd tests?

> Webrev:
>
> http://cr.openjdk.java.net/~ctornqvi/webrev/8157957/webrev.00/

The jdk/test/* changes only seem to be for the @library changes in the 
path, I don't see any changes to @build tags. Is that because all the 
@builds are necessary or are you simply not taking on the task of also 
updating the jdk tests?

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

Looks fine.

Thanks,
David
-----

> Bug:
>
> https://bugs.openjdk.java.net/browse/JDK-8157957
>
> Thanks,
>
> Christian
>
>
>
>
>