On Mon, 22 Feb 2021 15:24:19 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 incrementally with one additional
> commit since the last revision:
>
> 8166026: Refactor java/lang shell tests to java
Changes requested by iignatyev (Reviewer).
test/jdk/java/lang/annotation/LoaderLeakTest.java line 80:
> 78: System.gc();
> 79: System.gc();
> 80: if (c.get() == null) throw new AssertionError("class missing");
it would be better to use a different message here, so it would uniquely
identify a failed check, e.g. `class missing after GC but before loader is
unreachable`
test/jdk/java/lang/annotation/LoaderLeakTest.java line 74:
> 72: ClassLoader loader = new SimpleClassLoader();
> 73: var c = new WeakReference<Class<?>>(loader.loadClass("C"));
> 74: if (c.get() == null) throw new AssertionError();
please add an exception message here as well.
test/jdk/java/lang/annotation/LoaderLeakTest.java line 68:
> 66: public static void main(String[] args) throws Exception {
> 67: for (int i=0; i<100; i++)
> 68: doTest(args.length != 0);
nit: it's usually better to use `{`/`}` even for a one-line loop block.
test/jdk/java/lang/annotation/LoaderLeakTest.java line 99:
> 97: }
> 98:
> 99: @java.lang.annotation.Retention(RUNTIME)
nit: `import java.lang.annotation.Retention;`
test/jdk/java/lang/annotation/LoaderLeakTest.java line 59:
> 57: private void runJavaProcessExpectSuccessExitCode(String ... command)
> throws Throwable {
> 58: var processBuilder =
> ProcessTools.createJavaProcessBuilder(command)
> 59:
> .directory(Paths.get(Utils.TEST_CLASSES).toFile());
nit: in the case of chain calls, lines should be aligned by `.`
test/jdk/java/lang/annotation/LoaderLeakTest.java line 28:
> 26: * @bug 5040740
> 27: * @summary annotations cause memory leak
> 28: * @author gafter
not sure about etiquette in core-libs testsuite, but in hotspot testsuite, we
tend not to use `@author` tag.
-------------
PR: https://git.openjdk.java.net/jdk/pull/1577
