On Fri, 22 Jan 2021 17:59:33 GMT, Mandy Chung <mch...@openjdk.org> wrote:
>> 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 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/
Implemented in next patch
> 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.
Now this file is deleted.
> 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.
Removed in next patch.
> 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.
Using FileUtils in next patch
> 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
Thanks, Corrected in next patch.
> 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?
Thanks, Removed in next patch. Trying to keep maximum line length 120.
-------------
PR: https://git.openjdk.java.net/jdk/pull/2170
