On Mon, 25 Jan 2021 20:51:06 GMT, Mahendra Chhipa <github.com+34924738+mahendrachh...@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
