On Fri, 22 Jan 2021 16:52:02 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: > > NonJavaName Tests updated > Used newInstance() method to create the different EnclosingClasses at > runtime test/jdk/java/lang/Class/forName/NonJavaNameTest.java line 43: > 41: * @run testng/othervm NonJavaNameTest > 42: * @summary Verify names that aren't legal Java names are accepted by > forName. > 43: * @author Joseph D. Darcy we can drop this `@author` in particular this is a new file. test/jdk/java/lang/Class/forName/NonJavaNameTest.java line 49: > 47: private static final String SRC_DIR = System.getProperty("test.src"); > 48: private static final String NONJAVA_NAMES_SRC = SRC_DIR + "/classes/"; > 49: private static final String NONJAVA_NAMES_CLASSES = > System.getProperty("test.classes", "."); I was at first confused with `NON_NAMES_CLASSES` of what it means. For simplicity and clarity, better to rename these variables: s/NONJAVA_NAMES_SRC/TEST_SRC/ s/NONJAVA_NAMES_CLASSES/TEST_CLASSES/ test/jdk/java/lang/Class/forName/NonJavaNameTest.java line 96: > 94: > 95: @AfterClass > 96: public void deleteInvalidNameClasses() throws IOException { jtreg will do the clean up and this is not necessary. test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 95: > 93: Files.deleteIfExists(pkg2Dir); > 94: Files.deleteIfExists(pkg1File); > 95: Files.deleteIfExists(pkg1Dir); You can consider using `jdk.test.lib.util.FileUtils` test library to remove the entire directory. test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 97: > 95: Files.deleteIfExists(pkg1Dir); > 96: Files.createDirectories(pkg2Dir); > 97: } catch (IOException ex) { The test should fail when running into any error. Using the test library will do the retry test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 176: > 174: } > 175: > 176: private void check(final Class<?> c, final Class<?> enc, I see that you make all parameters in all methods final. It just adds noise. Can you leave it out? test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 197: > 195: c.getSimpleName(), annotation.simple(), > 196: c.getCanonicalName(), > 197: annotation.hasCanonical() ? annotation.canonical() : > null); I am guessing this whitespace in line 195-197 is not intentional? Same for line 203-206? test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 103: > 101: createAndWriteEnclosingClasses(enclosingPath, pkg2File, > "pkg1.pkg2"); > 102: > 103: String javacPath = JDKToolFinder.getJDKTool("javac"); You can use `jdk.test.lib.compiler.CompilerUtils` test library to compile the file in process. test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 71: > 69: /* > 70: * @test > 71: * @bug 4992173 4992170 this needs `@modules jdk.compiler` test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 141: > 139: } > 140: > 141: private void createAndWriteEnclosingClasses(final Path source, final > Path target, final String packagePath) { s/packagePath/packageName/ no need to declare parameters as `final` test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 147: > 145: while ((line = br.readLine()) != null) { > 146: if (line.contains("canonical=\"EnclosingClass")) { > 147: line = line.replaceAll("canonical=\"EnclosingClass", > "canonical=\"" + packagePath + ".EnclosingClass"); suggestion: declare `var className = packageName + ".EnclosingClass";` and replace those occurrance. test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 155: > 153: bw.println(line); > 154: } > 155: } catch (IOException ex) { Should not swallow the error and instead, let it propagate and the test should fail. ------------- PR: https://git.openjdk.java.net/jdk/pull/2170
