RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v3]

[Brent Christian]

On Mon, 25 Jan 2021 20:51:06 GMT, Mahendra Chhipa <github.com+34924738+mahendrachhipa at openjdk.org> wrote:

>> https://bugs.openjdk.java.net/browse/JDK-8183372
>
> Mahendra Chhipa has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Implemented the review comments.

I like keeping the changes within the existing .java files, thanks.  I have a few more suggestions.

test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 76:

> 74: * @modules jdk.compiler
> 75: * @build jdk.test.lib.process.* EnclosingClassTest
> 76: * @run testng/othervm EnclosingClassTest

The test should still be run with -esa -ea

test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 88:

> 86:     @BeforeClass
> 87:     public void createEnclosingClasses() throws Throwable {
> 88:         Path enclosingPath = Paths.get(ENCLOSING_CLASS_SRC);

The 'enclosingPath' Path could be the constant.  Then ENCLOSING_CLASS_SRC is not needed.

test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 126:

> 124:         Path pkg1Dir = Paths.get(SRC_DIR + "/pkg1");
> 125:         FileUtils.deleteFileTreeWithRetry(pkg1Dir);
> 126:     }

I'm not convinced the `@AfterClass` method is necessary.  Pre-cleanup is already done on LL94-95.  And occasionally, test-generated artifacts are helpful in post-mortem analysis.

test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 158:

> 156:     private void match(final String actual, final String expected) {
> 157:         System.out.println("actual:" + actual + "expected:" + expected);
> 158:         assert ((actual == null && expected == null) || actual.trim().equals(expected.trim()));

Out of curiousity, why is the trim() now needed ?

test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 180:

> 178:         info(c, encClass, annotation.desc());
> 179:         check(c, encClass, "" + encClass, annotation.encl(), c.getSimpleName(), annotation.simple(),
> 180:                 c.getCanonicalName(), annotation.hasCanonical() ? annotation.canonical() : null);

This looks like an unneeded formatting change

test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 187:

> 185:         check(array, array.getEnclosingClass(), "", "", array.getSimpleName(),
> 186:                 annotation.simple() + "[]", array.getCanonicalName(), annotation.hasCanonical()
> 187:                         ? annotation.canonical() + "[]" : null);

This looks like an unneeded formatting change

test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 197:

> 195:                     testClass((Class<?>) f.get(tests), annotation, f);
> 196:                 } catch (AssertionError ex) {
> 197:                     System.err.println("Error in " + tests.getClass().getName() + "." + f.getName());

This looks like an unneeded formatting change

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

Changes requested by bchristi (Reviewer).

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