RFR: 8166026: Refactor java/lang shell tests to java

classic Classic list List threaded Threaded
24 messages Options
12
Reply | Threaded
Open this post in threaded view
|

RFR: 8166026: Refactor java/lang shell tests to java

Ivan Šipka
Refactor `test/jdk/java/lang/annotation/loaderLeak/LoaderLeak.sh` as java test.

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

Commit messages:
 - 8166026: Refactor java/lang shell tests to java

Changes: https://git.openjdk.java.net/jdk/pull/1577/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1577&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8166026
  Stats: 501 lines in 6 files changed: 196 ins; 305 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1577.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1577/head:pull/1577

PR: https://git.openjdk.java.net/jdk/pull/1577
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8166026: Refactor java/lang shell tests to java [v2]

Ivan Šipka
> 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:
  - all: https://git.openjdk.java.net/jdk/pull/1577/files
  - new: https://git.openjdk.java.net/jdk/pull/1577/files/bc56a637..3a832a4d

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1577&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1577&range=00-01

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1577.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1577/head:pull/1577

PR: https://git.openjdk.java.net/jdk/pull/1577
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8166026: Refactor java/lang shell tests to java [v2]

Roger Riggs-2
On Thu, 3 Dec 2020 19:33:14 GMT, Ivan Šipka <[hidden email]> 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 rriggs (Reviewer).

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

> 63:     @Test
> 64:     public void testWithoutReadingAnnotations() throws Throwable {
> 65:         runJavaProcessExpectSuccesExitCode("Main");

Succes -> Success typo.  *Everywhere*

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

> 131: class SimpleClassLoader extends ClassLoader {
> 132:
> 133:     private Hashtable classes = new Hashtable();

A good upgrade would be to use HashMap<String, Class> instead of the ancient HashTable.
Also, avoid Raw types and cleanup other compile time warnings.

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

PR: https://git.openjdk.java.net/jdk/pull/1577
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8166026: Refactor java/lang shell tests to java [v3]

Ivan Šipka
In reply to this post by Ivan Šipka
> 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:
  - all: https://git.openjdk.java.net/jdk/pull/1577/files
  - new: https://git.openjdk.java.net/jdk/pull/1577/files/3a832a4d..fc0af133

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1577&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1577&range=01-02

  Stats: 14 lines in 1 file changed: 2 ins; 1 del; 11 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1577.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1577/head:pull/1577

PR: https://git.openjdk.java.net/jdk/pull/1577
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8166026: Refactor java/lang shell tests to java [v3]

Roger Riggs-2
On Wed, 9 Dec 2020 15:19:58 GMT, Ivan Šipka <[hidden email]> 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

Marked as reviewed by rriggs (Reviewer).

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

PR: https://git.openjdk.java.net/jdk/pull/1577
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8166026: Refactor java/lang shell tests to java [v3]

Igor Ignatyev-2
In reply to this post by Ivan Šipka
On Wed, 9 Dec 2020 15:19:58 GMT, Ivan Šipka <[hidden email]> 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 79:

> 77:                                 .directory(Paths.get(Utils.TEST_CLASSES).toFile()
> 78:                         )
> 79:                 ).shouldHaveExitValue(0);

indentation here looks wierd

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

> 52:         List<String> classes = List.of("A.class", "B.class", "C.class");
> 53:         for (String fileName : classes) {
> 54:             Files.move(

I don't think it's a good idea to move files created and managed by `jtreg`. I'd recommend you copying them here and, in `runJavaProcess...` constructing `ProcessBuilder` youself:

var args = new ArrayList<String>(command.length + 1);
args.add(JDKToolFinder.getJDKTool("java"));
Collections.addAll(args, command);
var pb = new ProcessBuilder(args).directory(Paths.get(Utils.TEST_CLASSES).toFile());

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

PR: https://git.openjdk.java.net/jdk/pull/1577
Reply | Threaded
Open this post in threaded view
|

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

Ivan Šipka
On Fri, 11 Dec 2020 13:47:54 GMT, Igor Ignatyev <[hidden email]> wrote:

>> Ivan Šipka has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   8166026: Refactor java/lang shell tests to java
>
> test/jdk/java/lang/annotation/LoaderLeakTest.java line 54:
>
>> 52:         List<String> classes = List.of("A.class", "B.class", "C.class");
>> 53:         for (String fileName : classes) {
>> 54:             Files.move(
>
> I don't think it's a good idea to move files created and managed by `jtreg`. I'd recommend you copying them here and, in `runJavaProcess...` constructing `ProcessBuilder` youself:
>
> var args = new ArrayList<String>(command.length + 1);
> args.add(JDKToolFinder.getJDKTool("java"));
> Collections.addAll(args, command);
> var pb = new ProcessBuilder(args).directory(Paths.get(Utils.TEST_CLASSES).toFile());

They are intentionally moved out of class path so that the application class loader can not find them. If they are not moved they will be loaded by the application class loader and they need to be [loaded](https://github.com/openjdk/jdk/blob/bc56a63702b8730abc1d0aebee133a5884145fa1/test/jdk/java/lang/annotation/LoaderLeakTest.java#L96) by the tests very own `SimpleClassLoader` in order to be able to test for the leakage.

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

PR: https://git.openjdk.java.net/jdk/pull/1577
Reply | Threaded
Open this post in threaded view
|

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

Igor Ignatyev-2
On Tue, 5 Jan 2021 20:58:37 GMT, Ivan Šipka <[hidden email]> wrote:

>> test/jdk/java/lang/annotation/LoaderLeakTest.java line 54:
>>
>>> 52:         List<String> classes = List.of("A.class", "B.class", "C.class");
>>> 53:         for (String fileName : classes) {
>>> 54:             Files.move(
>>
>> I don't think it's a good idea to move files created and managed by `jtreg`. I'd recommend you copying them here and, in `runJavaProcess...` constructing `ProcessBuilder` youself:
>>
>> var args = new ArrayList<String>(command.length + 1);
>> args.add(JDKToolFinder.getJDKTool("java"));
>> Collections.addAll(args, command);
>> var pb = new ProcessBuilder(args).directory(Paths.get(Utils.TEST_CLASSES).toFile());
>
> They are intentionally moved out of class path so that the application class loader can not find them. If they are not moved they will be loaded by the application class loader and they need to be [loaded](https://github.com/openjdk/jdk/blob/bc56a63702b8730abc1d0aebee133a5884145fa1/test/jdk/java/lang/annotation/LoaderLeakTest.java#L96) by the tests very own `SimpleClassLoader` in order to be able to test for the leakage.

I understand the intention, but it can be achieved w/o messing around w/ jtreg managed directories, for example by changing `SimpleClassLoader` to not delegate the loading of `A`, `B`, `C` classes to the application CL.

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

PR: https://git.openjdk.java.net/jdk/pull/1577
Reply | Threaded
Open this post in threaded view
|

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

Ivan Šipka
In reply to this post by Ivan Šipka
> 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:

  trying to manipulate effective jtreg classpath

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1577/files
  - new: https://git.openjdk.java.net/jdk/pull/1577/files/fc0af133..a5ef036d

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1577&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1577&range=02-03

  Stats: 397 lines in 2 files changed: 200 ins; 197 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1577.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1577/head:pull/1577

PR: https://git.openjdk.java.net/jdk/pull/1577
Reply | Threaded
Open this post in threaded view
|

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

Ivan Šipka
In reply to this post by Ivan Šipka
> 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: https://git.openjdk.java.net/jdk/pull/1577/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1577&range=04
  Stats: 444 lines in 6 files changed: 139 ins; 305 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1577.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1577/head:pull/1577

PR: https://git.openjdk.java.net/jdk/pull/1577
Reply | Threaded
Open this post in threaded view
|

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

Igor Ignatyev-2
On Mon, 8 Feb 2021 20:12:03 GMT, Ivan Šipka <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Igor Ignatyev-2
In reply to this post by Igor Ignatyev-2
On Fri, 11 Dec 2020 13:39:04 GMT, Igor Ignatyev <[hidden email]> wrote:

>> Ivan Šipka has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   8166026: Refactor java/lang shell tests to java
>
> test/jdk/java/lang/annotation/LoaderLeakTest.java line 64:
>
>> 62:     @Test
>> 63:     public void testWithoutReadingAnnotations() throws Throwable {
>> 64:         runJavaProcessExpectSuccessExitCode("Main");
>
> indentation here looks wierd

indentation still looks weird here.

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

PR: https://git.openjdk.java.net/jdk/pull/1577
Reply | Threaded
Open this post in threaded view
|

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

Ivan Šipka
In reply to this post by Igor Ignatyev-2
On Tue, 9 Feb 2021 22:09:33 GMT, Igor Ignatyev <[hidden email]> wrote:

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

That indentation is intended to make the code more readable, at least I find it so. Changed now to more "traditional style".

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

PR: https://git.openjdk.java.net/jdk/pull/1577
Reply | Threaded
Open this post in threaded view
|

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

Ivan Šipka
In reply to this post by Ivan Šipka
> 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 five 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
 - 8166026: Refactor java/lang shell tests to java

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

Changes: https://git.openjdk.java.net/jdk/pull/1577/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1577&range=05
  Stats: 435 lines in 6 files changed: 130 ins; 305 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1577.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1577/head:pull/1577

PR: https://git.openjdk.java.net/jdk/pull/1577
Reply | Threaded
Open this post in threaded view
|

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

Ivan Šipka
In reply to this post by Ivan Šipka
> 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:
  - all: https://git.openjdk.java.net/jdk/pull/1577/files
  - new: https://git.openjdk.java.net/jdk/pull/1577/files/d9d41ebd..23025452

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1577&range=06
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1577&range=05-06

  Stats: 3 lines in 1 file changed: 1 ins; 1 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1577.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1577/head:pull/1577

PR: https://git.openjdk.java.net/jdk/pull/1577
Reply | Threaded
Open this post in threaded view
|

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

Ivan Šipka
In reply to this post by Ivan Šipka
> 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:
  - all: https://git.openjdk.java.net/jdk/pull/1577/files
  - new: https://git.openjdk.java.net/jdk/pull/1577/files/23025452..7133fe98

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1577&range=07
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1577&range=06-07

  Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1577.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1577/head:pull/1577

PR: https://git.openjdk.java.net/jdk/pull/1577
Reply | Threaded
Open this post in threaded view
|

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

Igor Ignatyev-2
On Mon, 22 Feb 2021 15:24:19 GMT, Ivan Šipka <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Igor Ignatyev-2
On Mon, 22 Feb 2021 15:33:30 GMT, Igor Ignatyev <[hidden email]> wrote:

>> Ivan Šipka has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   8166026: Refactor java/lang shell tests to java
>
> 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.

nit 2: `=` and `<` should be surrounded by a space.

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

PR: https://git.openjdk.java.net/jdk/pull/1577
Reply | Threaded
Open this post in threaded view
|

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

Ivan Šipka
In reply to this post by Ivan Šipka
> 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:
  - all: https://git.openjdk.java.net/jdk/pull/1577/files
  - new: https://git.openjdk.java.net/jdk/pull/1577/files/7133fe98..9e99a760

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1577&range=08
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1577&range=07-08

  Stats: 12 lines in 1 file changed: 5 ins; 1 del; 6 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1577.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1577/head:pull/1577

PR: https://git.openjdk.java.net/jdk/pull/1577
Reply | Threaded
Open this post in threaded view
|

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

Daniel Fuchs-2
On Mon, 22 Feb 2021 15:50:03 GMT, Ivan Šipka <[hidden email]> 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

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

> 75:         var c = new WeakReference<Class<?>>(loader.loadClass("C"));
> 76:         if (c.get() == null) throw new AssertionError("class missing after loadClass");
> 77:         if (c.get().getClassLoader() != loader) throw new AssertionError("wrong classloader");

This can throw NPE as there is no guarantee that two subsequent calls will return the same value: c.get() could very well be null the second time around.

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

> 92:             System.gc();
> 93:             Thread.sleep(20);
> 94:             if (c.get() == null) {

I'd suggest using the new `if (c.refersTo(null)) {` here.

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

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