RFR: 8259267: Refactor LoaderLeak shell test as java test. [v5]

[Igor Ignatyev]

On Mon, 8 Feb 2021 20:12:03 GMT, Ivan Šipka <isipka at openjdk.org> wrote:

>> Refactor `test/jdk/java/lang/annotation/loaderLeak/LoaderLeak.sh` as java test.
>
> Ivan Šipka has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains four commits:
> 
>  - 8166026: Refactor java/lang shell tests to java
>  - 8166026: Refactor java/lang shell tests to java
>  - 8166026: Refactor java/lang shell tests to java
>  - 8166026: Refactor java/lang shell tests to java

Changes requested by iignatyev (Reviewer).

test/jdk/java/lang/annotation/LoaderLeakTest.java line 2:

> 1: /*
> 2:  * Copyright (c) 2004, 2020, Oracle and/or its affiliates. All rights reserved.

it's 2021

test/jdk/java/lang/annotation/LoaderLeakTest.java line 78:

> 76:         // URL classes = new URL("file://" + System.getProperty("user.dir") + "/classes");
> 77:         // URL[] path = { classes };
> 78:         // URLClassLoader loader = new URLClassLoader(path);

please remove the commented out lines (L76-78)

test/jdk/java/lang/annotation/LoaderLeakTest.java line 128:

> 126: 
> 127:     @Override
> 128:     public synchronized Class<?> loadClass(String className, boolean resolveIt)

does it need to be `synchronized` ?

test/jdk/java/lang/annotation/LoaderLeakTest.java line 123:

> 121:             return Files.readAllBytes(Paths.get(className + ".class"));
> 122:         } catch (Exception e) {
> 123:             throw new Error(e);

could you please use a descriptive message here (and include `className`  value into it), so it will be easier to analyze test failures?

test/jdk/java/lang/annotation/LoaderLeakTest.java line 119:

> 117: 
> 118:     private byte[] getClassImplFromDataBase(String className) {
> 119:         byte result[];

unused

test/jdk/java/lang/annotation/LoaderLeakTest.java line 80:

> 78:         // URLClassLoader loader = new URLClassLoader(path);
> 79:         ClassLoader loader = new SimpleClassLoader();
> 80:         WeakReference<Class<?>> c = new WeakReference<Class<?>>(loader.loadClass("C"));

nit: I'd use `var` here.

test/jdk/java/lang/annotation/LoaderLeakTest.java line 82:

> 80:         WeakReference<Class<?>> c = new WeakReference<Class<?>>(loader.loadClass("C"));
> 81:         if (c.get() == null) throw new AssertionError();
> 82:         if (c.get().getClassLoader() != loader) throw new AssertionError();

please use the messages which explain what went wrong, so when the failure happens, the person who analyze it won't have to open test code every time to just understand which of assertions was hit.

test/jdk/java/lang/annotation/LoaderLeakTest.java line 106:

> 104: }
> 105: 
> 106: @java.lang.annotation.Retention(java.lang.annotation.RetentionPolicy.RUNTIME)

nit: I'd import these types from j.l.a

-------------

PR: https://git.openjdk.java.net/jdk/pull/1577