-
Notifications
You must be signed in to change notification settings - Fork 6.1k
JDK-8183372 : Refactor java/lang/Class shell tests to java #2170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
👋 Welcome back mahendrachhipa! A progress list of the required criteria for merging this PR into |
@mahendrachhipa The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
Can this be done all in Adding the
|
Used newInstance() method to create the different EnclosingClasses at runtime
Review comments implemented. |
* @build NonJavaNames | ||
* @run testng/othervm NonJavaNameTest | ||
* @summary Verify names that aren't legal Java names are accepted by forName. | ||
* @author Joseph D. Darcy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can drop this @author
in particular this is a new file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now this file is deleted.
public class NonJavaNameTest { | ||
private static final String SRC_DIR = System.getProperty("test.src"); | ||
private static final String NONJAVA_NAMES_SRC = SRC_DIR + "/classes/"; | ||
private static final String NONJAVA_NAMES_CLASSES = System.getProperty("test.classes", "."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented in next patch
} | ||
|
||
@AfterClass | ||
public void deleteInvalidNameClasses() throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jtreg will do the clean up and this is not necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed in next patch.
Files.deleteIfExists(pkg2File); | ||
Files.deleteIfExists(pkg2Dir); | ||
Files.deleteIfExists(pkg1File); | ||
Files.deleteIfExists(pkg1Dir); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can consider using jdk.test.lib.util.FileUtils
test library to remove the entire directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Using FileUtils in next patch
Files.deleteIfExists(pkg1File); | ||
Files.deleteIfExists(pkg1Dir); | ||
Files.createDirectories(pkg2Dir); | ||
} catch (IOException ex) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test should fail when running into any error. Using the test library will do the retry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Corrected in next patch.
createAndWriteEnclosingClasses(enclosingPath, pkg1File, "pkg1"); | ||
createAndWriteEnclosingClasses(enclosingPath, pkg2File, "pkg1.pkg2"); | ||
|
||
String javacPath = JDKToolFinder.getJDKTool("javac"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use jdk.test.lib.compiler.CompilerUtils
test library to compile the file in process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to use the CopmilerUtils to compile the file, But this utility is not able to the dependencies of the class. Example it is not able to find the common.TestMe class while try to compile the EnclosingClass.java.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to add --source-path
where it can find the dependent source files.
|
||
/* | ||
* @test | ||
* @bug 4992173 4992170 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs @modules jdk.compiler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Added in next patch
} | ||
} | ||
|
||
private void createAndWriteEnclosingClasses(final Path source, final Path target, final String packagePath) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/packagePath/packageName/
no need to declare parameters as final
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed the packagePath to packageName in next patch. To avoid the Checkstyle warning added the final key word with parameters.
String line; | ||
while ((line = br.readLine()) != null) { | ||
if (line.contains("canonical=\"EnclosingClass")) { | ||
line = line.replaceAll("canonical=\"EnclosingClass", "canonical=\"" + packagePath + ".EnclosingClass"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: declare var className = packageName + ".EnclosingClass";
and replace those occurrance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Implemented the comment in next patch.
} | ||
bw.println(line); | ||
} | ||
} catch (IOException ex) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not swallow the error and instead, let it propagate and the test should fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Corrected this in next patch.
Can the new code be added to the existing |
* @test | ||
* @bug 4952558 | ||
* @library /test/lib | ||
* @build NonJavaNames |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This @build
can be dropped as this is done implicitly.
private static final String SRC_DIR = System.getProperty("test.src"); | ||
private static final String TEST_SRC = SRC_DIR + "/classes/"; | ||
private static final String TEST_CLASSES = System.getProperty("test.classes", "."); | ||
Path dhyphenPath, dcommaPath, dperiodPath, dleftsquarePath, drightsquarePath, dplusPath, dsemicolonPath, dzeroPath, dthreePath, dzadePath; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These variables should belong to the createInvalidNameClasses
method. You can simply add Path
type declaration in line 78-87.
public void createEnclosingClasses() throws Throwable { | ||
Path enclosingPath = Paths.get(ENCLOSING_CLASS_SRC); | ||
Path pkg1Dir = Paths.get(SRC_DIR + "/pkg1"); | ||
Path pkg2Dir = Paths.get(SRC_DIR+"/pkg1/pkg2"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: space before and after +
is missing
createAndWriteEnclosingClasses(enclosingPath, pkg1File, "pkg1"); | ||
createAndWriteEnclosingClasses(enclosingPath, pkg2File, "pkg1.pkg2"); | ||
|
||
String javacPath = JDKToolFinder.getJDKTool("javac"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to add --source-path
where it can find the dependent source files.
@BeforeClass | ||
public void createEnclosingClasses() throws Throwable { | ||
Path enclosingPath = Paths.get(ENCLOSING_CLASS_SRC); | ||
Path pkg1Dir = Paths.get(SRC_DIR + "/pkg1"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively:
Path pkg1Dir = Paths.get(SRC_DIR, pkg1);
Path pkg2Dir = Paths.get(SRC_DIR, pkg1, pkg2);
Path pkg1File = pkg1Dir.resolve("EnclosingClass.java");
Path pkg2File = pkg2Dir.resolve("EnclosingClass.java");
|
||
String javacPath = JDKToolFinder.getJDKTool("javac"); | ||
OutputAnalyzer outputAnalyzer = ProcessTools.executeCommand(javacPath, "-d", System.getProperty("test.classes", "."), | ||
SRC_DIR + "/EnclosingClass.java", SRC_DIR + "/pkg1/EnclosingClass.java", SRC_DIR + "/pkg1/pkg2/EnclosingClass.java"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File.separator
is different on Unix and Windows. Either replace /
with File.separator
. Or you can use java.nio.file.Path
and call Path::toString
to get the string representation.
See above suggestion of declaring static final Path ENCLOSING_CLASS_SRC
. So the above should use ENCLOSING_CLASS_SRC
and pkg2File
instead (calling toString)
String encName, String encNameExpected, | ||
String simpleName, String simpleNameExpected, | ||
String canonicalName, String canonicalNameExpected) { | ||
private void check(final Class<?> c, final Class<?> enc, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it from your IDE configurations? You can turn off Checkstyle warnings. This just adds noise.
} | ||
|
||
@Test | ||
public void testEnclosingClasses() throws Throwable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better to throw specific exceptions for example ReflectiveOperationException
. Same for the @Test
below.
String encName, String encNameExpected, | ||
String simpleName, String simpleNameExpected, | ||
String canonicalName, String canonicalNameExpected) { | ||
private void check(final Class<?> c, final Class<?> enc, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also assume some of the formatting changes are changed by your IDE suggestion. Can you please revert the formatting changes (there are quite a few of them). This will help the reviewers.
|
||
@AfterClass | ||
public void deleteEnclosingClasses() throws IOException { | ||
Path pkg1Dir = Paths.get(SRC_DIR + "/pkg1"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Path pkg1Dir = Paths.get(SRC_DIR + "/pkg1"); | |
Path pkg1Dir = Paths.get(SRC_DIR, "pkg1"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like keeping the changes within the existing .java files, thanks. I have a few more suggestions.
* @library /test/lib | ||
* @modules jdk.compiler | ||
* @build jdk.test.lib.process.* EnclosingClassTest | ||
* @run testng/othervm EnclosingClassTest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test should still be run with -esa -ea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, while running from command line providing the -esa and -a option to run the tests.
|
||
@BeforeClass | ||
public void createEnclosingClasses() throws Throwable { | ||
Path enclosingPath = Paths.get(ENCLOSING_CLASS_SRC); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 'enclosingPath' Path could be the constant. Then ENCLOSING_CLASS_SRC is not needed.
@AfterClass | ||
public void deleteEnclosingClasses() throws IOException { | ||
Path pkg1Dir = Paths.get(SRC_DIR + "/pkg1"); | ||
FileUtils.deleteFileTreeWithRetry(pkg1Dir); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To keep the source code repo clean from temporary generated files, these files are removed after test. Test logs have the information about the generated files for the Postmortem analysis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test files shouldn't be written into the repository (repos are sometimes tested on read-only filesystems). Files should be written under JTwork/scratch/.
expected + "'"); | ||
private void match(final String actual, final String expected) { | ||
System.out.println("actual:" + actual + "expected:" + expected); | ||
assert ((actual == null && expected == null) || actual.trim().equals(expected.trim())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiousity, why is the trim() now needed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expected string having one space as prefixed in some class names incase of pkg1 and pkg2 packages. So trimmig before comparing with actual name:
Current package no space
nested class within top level class:
class EnclosingClass$Nested
is enclosed by: class EnclosingClass
has simple name: Nested' has canonical name:
EnclosingClass.Nested'
actual:class EnclosingClassexpected:class EnclosingClass
pkg1.pkg2 package have expected class name with one space prefixed
nested class within top level class:
class pkg1.pkg2.EnclosingClass$Nested
is enclosed by: class pkg1.pkg2.EnclosingClass
has simple name: Nested' has canonical name:
pkg1.pkg2.EnclosingClass.Nested'
actual:class pkg1.pkg2.EnclosingClassexpected: class pkg1.pkg2.EnclosingClass
pkg1 package have expected class name with one space prefixed
nested class within top level class:
class pkg1.EnclosingClass$Nested
is enclosed by: class pkg1.EnclosingClass
has simple name: Nested' has canonical name:
pkg1.EnclosingClass.Nested'
actual:class pkg1.EnclosingClassexpected: class pkg1.EnclosingClass
check(c, encClass, "" + encClass, annotation.encl(), c.getSimpleName(), annotation.simple(), | ||
c.getCanonicalName(), annotation.hasCanonical() ? annotation.canonical() : null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like an unneeded formatting change
check(array, array.getEnclosingClass(), "", "", array.getSimpleName(), | ||
annotation.simple() + "[]", array.getCanonicalName(), annotation.hasCanonical() | ||
? annotation.canonical() + "[]" : null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like an unneeded formatting change
System.err.println("Error in " + | ||
tests.getClass().getName() + | ||
"." + f.getName()); | ||
System.err.println("Error in " + tests.getClass().getName() + "." + f.getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like an unneeded formatting change
|
||
@BeforeClass | ||
public void createInvalidNameClasses() throws IOException { | ||
Path hyphenPath = Paths.get(TEST_SRC + "hyphen.class"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Building a Path
with string concatenation is an anti-pattern.
Also Path.of
is probably preferred instead of Paths.get
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now not doing the concatenation of the strings to create Path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update. This is much cleaner.
} | ||
@DataProvider(name = "badNonJavaClassNames") | ||
Object[][] getBadNonJavaClassNames() { | ||
return new Object[][] {{";"}, {"["}, {"."}}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: one line per test (i.e. make it multiple lines)
import org.testng.annotations.AfterClass; | ||
import org.testng.annotations.BeforeClass; | ||
import org.testng.annotations.Test; | ||
|
||
/* | ||
* @test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: suggest to move this comment block to the top before all imports.
@mahendrachhipa This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 692 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@bchristi-git, @mlchung) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
"]" | ||
}; | ||
@Test(dataProvider = "goodNonJavaClassNames") | ||
public void testGoodNonJavaClassNames(String name) throws Throwable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/Throwable/ClassFileNotFoundException/
System.out.println("Testing bad class name ``" + name + "''"); | ||
try { | ||
Class.forName(name); | ||
} catch (Exception e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/Exception/ClassNotFoundException/
/integrate |
@mahendrachhipa |
I think Brent wants to review it. I will let Brent to sponsor you. |
actual + "' matches expected `" + | ||
expected + "'"); | ||
private void match(final String actual, final String expected) { | ||
System.out.println("actual:" + actual + "expected:" + expected); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to insert a space before expected:
expected + "'"); | ||
private void match(final String actual, final String expected) { | ||
System.out.println("actual:" + actual + "expected:" + expected); | ||
assert ((actual == null && expected == null) || actual.equals(expected.trim())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to figure out where the extra space is coming from, if the test worked before, but now fails without adding the trim()
.
Also, I think it would be better to throw an exception instead of using assert
, so the test works with or without assertions being enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I missed your comment about this change.
assert((actual == null && expected == null) || actual.equals(expected)); | ||
private void match(String actual, String expected) { | ||
System.out.println("actual:" + actual + "expected:" + expected); | ||
assert((actual == null && expected == null) || actual.equals(expected.trim())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this check is done, because assertions aren't enabled. This should be changed to explicitly throw an exception.
(I think my comments may no have been visible because I didn't "submit" my review. Hopefully they are now.) |
/integrate |
@mahendrachhipa |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks for the changes.
/sponsor |
@bchristi-git The PR has been updated since the change author (@mahendrachhipa) issued the |
/integrate |
@mahendrachhipa |
/sponsor |
@bchristi-git @mahendrachhipa Since your change was applied there have been 692 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 6dc3c6d. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
https://bugs.openjdk.java.net/browse/JDK-8183372
Progress
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/2170/head:pull/2170
$ git checkout pull/2170