On Mon, 8 Feb 2021 20:12:03 GMT, Ivan Šipka <isi...@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
