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

[Mandy Chung]

On Fri, 22 Jan 2021 16:52:02 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:
> 
>   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