Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v12]
On Tue, 2 Feb 2021 21:00:00 GMT, Mahendra Chhipa 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: > > Generate the source files in JTWork directory. Looks good. Thanks for the changes. - Marked as reviewed by bchristi (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v12]
> https://bugs.openjdk.java.net/browse/JDK-8183372 Mahendra Chhipa has updated the pull request incrementally with one additional commit since the last revision: Generate the source files in JTWork directory. - Changes: - all: https://git.openjdk.java.net/jdk/pull/2170/files - new: https://git.openjdk.java.net/jdk/pull/2170/files/224a104a..3c235ab0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2170=11 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2170=10-11 Stats: 16 lines in 1 file changed: 1 ins; 7 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/2170.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2170/head:pull/2170 PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v3]
On Wed, 27 Jan 2021 23:03:55 GMT, Mahendra Chhipa wrote: >> 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. > > 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. Test files shouldn't be written into the repository (repos are sometimes tested on read-only filesystems). Files should be written under JTwork/scratch/. - PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v11]
> https://bugs.openjdk.java.net/browse/JDK-8183372 Mahendra Chhipa has updated the pull request incrementally with one additional commit since the last revision: Using testNg Assert instead of assert. - Changes: - all: https://git.openjdk.java.net/jdk/pull/2170/files - new: https://git.openjdk.java.net/jdk/pull/2170/files/320b3054..224a104a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2170=10 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2170=09-10 Stats: 4 lines in 1 file changed: 1 ins; 1 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2170.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2170/head:pull/2170 PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v4]
On Thu, 28 Jan 2021 00:34:08 GMT, Brent Christian wrote:
>> Mahendra Chhipa has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Implemented the review comments.
>
> 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.equals(expected.trim()));
>
> 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.
Good catch. I missed your comment about this change.
-
PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v4]
On Mon, 1 Feb 2021 23:43:25 GMT, Brent Christian wrote: >> Mahendra Chhipa has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Implemented the review comments. > > Changes requested by bchristi (Reviewer). (I think my comments may no have been visible because I didn't "submit" my review. Hopefully they are now.) - PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v10]
On Mon, 1 Feb 2021 22:23:02 GMT, Mahendra Chhipa
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:
>
> throwing the specific exceptions.
test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 159:
> 157: private void match(String actual, String expected) {
> 158: System.out.println("actual:" + actual + "expected:" + expected);
> 159: assert((actual == null && expected == null) ||
> actual.equals(expected.trim()));
I don't think this check is done, because assertions aren't enabled. This
should be changed to explicitly throw an exception.
-
PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v4]
On Wed, 27 Jan 2021 22:44:00 GMT, Mahendra Chhipa
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.
Changes requested by bchristi (Reviewer).
test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 157:
> 155:
> 156: private void match(final String actual, final String expected) {
> 157: System.out.println("actual:" + actual + "expected:" + expected);
It would be good to insert a space before `expected:`
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.equals(expected.trim()));
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.
-
PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v9]
On Mon, 1 Feb 2021 21:54:50 GMT, Mandy Chung wrote: >> Mahendra Chhipa has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Implemented the review comments. > > Marked as reviewed by mchung (Reviewer). I think Brent wants to review it. I will let Brent to sponsor you. - PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v10]
> https://bugs.openjdk.java.net/browse/JDK-8183372 Mahendra Chhipa has updated the pull request incrementally with one additional commit since the last revision: throwing the specific exceptions. - Changes: - all: https://git.openjdk.java.net/jdk/pull/2170/files - new: https://git.openjdk.java.net/jdk/pull/2170/files/9ff23ef5..320b3054 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2170=09 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2170=08-09 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/2170.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2170/head:pull/2170 PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v9]
On Mon, 1 Feb 2021 21:46:00 GMT, Mahendra Chhipa
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.
Marked as reviewed by mchung (Reviewer).
test/jdk/java/lang/Class/forName/NonJavaNames.java line 119:
> 117:
> 118: @Test(dataProvider = "goodNonJavaClassNames")
> 119: public void testGoodNonJavaClassNames(String name) throws Throwable {
s/Throwable/ClassFileNotFoundException/
test/jdk/java/lang/Class/forName/NonJavaNames.java line 129:
> 127: try {
> 128: Class.forName(name);
> 129: } catch (Exception e) {
s/Exception/ClassNotFoundException/
-
PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v9]
> 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. - Changes: - all: https://git.openjdk.java.net/jdk/pull/2170/files - new: https://git.openjdk.java.net/jdk/pull/2170/files/9a280259..9ff23ef5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2170=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2170=07-08 Stats: 28 lines in 2 files changed: 14 ins; 11 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/2170.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2170/head:pull/2170 PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v8]
On Mon, 1 Feb 2021 20:27:00 GMT, Mahendra Chhipa
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.
Thanks for the update. This is much cleaner.
test/jdk/java/lang/Class/forName/NonJavaNames.java line 154:
> 152: @DataProvider(name = "badNonJavaClassNames")
> 153: Object[][] getBadNonJavaClassNames() {
> 154: return new Object[][] {{";"}, {"["}, {"."}};
Nit: one line per test (i.e. make it multiple lines)
test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 42:
> 40:
> 41: /*
> 42: * @test
Nit: suggest to move this comment block to the top before all imports.
-
Marked as reviewed by mchung (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v8]
> 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. - Changes: - all: https://git.openjdk.java.net/jdk/pull/2170/files - new: https://git.openjdk.java.net/jdk/pull/2170/files/0cbee711..9a280259 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2170=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2170=06-07 Stats: 4 lines in 1 file changed: 2 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2170.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2170/head:pull/2170 PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v7]
> 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. - Changes: - all: https://git.openjdk.java.net/jdk/pull/2170/files - new: https://git.openjdk.java.net/jdk/pull/2170/files/40ba2b3b..0cbee711 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2170=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2170=05-06 Stats: 138 lines in 2 files changed: 51 ins; 46 del; 41 mod Patch: https://git.openjdk.java.net/jdk/pull/2170.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2170/head:pull/2170 PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v6]
> https://bugs.openjdk.java.net/browse/JDK-8183372 Mahendra Chhipa has updated the pull request incrementally with one additional commit since the last revision: Reset the formatting. - Changes: - all: https://git.openjdk.java.net/jdk/pull/2170/files - new: https://git.openjdk.java.net/jdk/pull/2170/files/d8ce7caa..40ba2b3b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2170=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2170=04-05 Stats: 24 lines in 1 file changed: 7 ins; 2 del; 15 mod Patch: https://git.openjdk.java.net/jdk/pull/2170.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2170/head:pull/2170 PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v5]
> 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 review comment. - Changes: - all: https://git.openjdk.java.net/jdk/pull/2170/files - new: https://git.openjdk.java.net/jdk/pull/2170/files/cd57d8f5..d8ce7caa Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2170=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2170=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2170.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2170/head:pull/2170 PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v3]
On Tue, 26 Jan 2021 22:55:32 GMT, Brent Christian wrote: >> Mahendra Chhipa has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Implemented the review comments. > > 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. 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. - PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v4]
> 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. - Changes: - all: https://git.openjdk.java.net/jdk/pull/2170/files - new: https://git.openjdk.java.net/jdk/pull/2170/files/9517404b..cd57d8f5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2170=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2170=02-03 Stats: 58 lines in 2 files changed: 6 ins; 6 del; 46 mod Patch: https://git.openjdk.java.net/jdk/pull/2170.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2170/head:pull/2170 PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v3]
On Tue, 26 Jan 2021 22:52:15 GMT, Brent Christian wrote: >> Mahendra Chhipa has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Implemented the review comments. > > 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 Yes, while running from command line providing the -esa and -a option to run the tests. - PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v3]
On Tue, 26 Jan 2021 23:25:03 GMT, Florent Guillaume
wrote:
>> Mahendra Chhipa has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Implemented the review comments.
>
> test/jdk/java/lang/Class/forName/NonJavaNames.java line 67:
>
>> 65: @BeforeClass
>> 66: public void createInvalidNameClasses() throws IOException {
>> 67: Path hyphenPath = Paths.get(TEST_SRC + "hyphen.class");
>
> Building a `Path` with string concatenation is an anti-pattern.
> Also `Path.of` is probably preferred instead of `Paths.get`.
Now not doing the concatenation of the strings to create Path.
-
PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v3]
On Tue, 26 Jan 2021 22:56:25 GMT, Brent Christian wrote:
>> Mahendra Chhipa has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Implemented the review comments.
>
> 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 ?
expected string having one space as prefixed in some class names incase of pkg1
and pkg2 packages. So trimmig before comparing with actual name:
=
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
=
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.EnclosingClass**expected: class
pkg1.pkg2.EnclosingClass**
==
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.EnclosingClass**expected: class pkg1.EnclosingClass**
-
PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v3]
On Mon, 25 Jan 2021 20:51:06 GMT, Mahendra Chhipa
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.
test/jdk/java/lang/Class/forName/NonJavaNames.java line 67:
> 65: @BeforeClass
> 66: public void createInvalidNameClasses() throws IOException {
> 67: Path hyphenPath = Paths.get(TEST_SRC + "hyphen.class");
Building a `Path` with string concatenation is an anti-pattern.
Also `Path.of` is probably preferred instead of `Paths.get`.
-
PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v3]
On Mon, 25 Jan 2021 20:51:06 GMT, Mahendra Chhipa
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
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v2]
On Tue, 26 Jan 2021 22:30:03 GMT, Mandy Chung wrote: >> To avoid the Checkstyle warnings, I added them. > > Is it from your IDE configurations? You can turn off Checkstyle warnings. > This just adds noise. 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. - PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v2]
On Mon, 25 Jan 2021 19:33:08 GMT, Mahendra Chhipa
wrote:
>> 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.
>
> 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.
You need to add `--source-path` where it can find the dependent source files.
>> 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?
>
> To avoid the Checkstyle warnings, I added them.
Is it from your IDE configurations? You can turn off Checkstyle warnings.
This just adds noise.
-
PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v3]
On Mon, 25 Jan 2021 20:51:06 GMT, Mahendra Chhipa
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.
test/jdk/java/lang/Class/forName/NonJavaNames.java line 37:
> 35: * @bug 4952558
> 36: * @library /test/lib
> 37: * @build NonJavaNames
This `@build` can be dropped as this is done implicitly.
test/jdk/java/lang/Class/forName/NonJavaNames.java line 63:
> 61: private static final String TEST_SRC = SRC_DIR + "/classes/";
> 62: private static final String TEST_CLASSES =
> System.getProperty("test.classes", ".");
> 63: Path dhyphenPath, dcommaPath, dperiodPath, dleftsquarePath,
> drightsquarePath, dplusPath, dsemicolonPath, dzeroPath, dthreePath, dzadePath;
These variables should belong to the `createInvalidNameClasses` method. You
can simply add `Path` type declaration in line 78-87.
test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 90:
> 88: Path enclosingPath = Paths.get(ENCLOSING_CLASS_SRC);
> 89: Path pkg1Dir = Paths.get(SRC_DIR + "/pkg1");
> 90: Path pkg2Dir = Paths.get(SRC_DIR+"/pkg1/pkg2");
nit: space before and after `+` is missing
test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 89:
> 87: public void createEnclosingClasses() throws Throwable {
> 88: Path enclosingPath = Paths.get(ENCLOSING_CLASS_SRC);
> 89: Path pkg1Dir = Paths.get(SRC_DIR + "/pkg1");
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");
test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 84:
> 82: public class EnclosingClassTest {
> 83: private static final String SRC_DIR = System.getProperty("test.src");
> 84: private static final String ENCLOSING_CLASS_SRC = SRC_DIR +
> "/EnclosingClass.java";
Suggestion:
private static final Path ENCLOSING_CLASS_SRC = Paths.get(SRC_DIR,
"EnclosingClass.java");
Use Path::toString to convert to a String where needed.
test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 103:
> 101: String javacPath = JDKToolFinder.getJDKTool("javac");
> 102: OutputAnalyzer outputAnalyzer =
> ProcessTools.executeCommand(javacPath, "-d",
> System.getProperty("test.classes", "."),
> 103: SRC_DIR + "/EnclosingClass.java", SRC_DIR +
> "/pkg1/EnclosingClass.java", SRC_DIR + "/pkg1/pkg2/EnclosingClass.java");
`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)
test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 108:
> 106:
> 107: @Test
> 108: public void testEnclosingClasses() throws Throwable {
better to throw specific exceptions for example `ReflectiveOperationException`.
Same for the `@Test` below.
test/jdk/java/lang/Class/getEnclosingClass/EnclosingClassTest.java line 124:
> 122: @AfterClass
> 123: public void deleteEnclosingClasses() throws IOException {
> 124: Path pkg1Dir = Paths.get(SRC_DIR + "/pkg1");
Suggestion:
Path pkg1Dir = Paths.get(SRC_DIR, "pkg1");
-
PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v3]
> 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. - Changes: - all: https://git.openjdk.java.net/jdk/pull/2170/files - new: https://git.openjdk.java.net/jdk/pull/2170/files/e664bd73..9517404b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2170=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2170=01-02 Stats: 226 lines in 3 files changed: 64 ins; 132 del; 30 mod Patch: https://git.openjdk.java.net/jdk/pull/2170.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2170/head:pull/2170 PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v2]
On Fri, 22 Jan 2021 18:17:33 GMT, Mandy Chung 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/getEnclosingClass/EnclosingClassTest.java line 71: > >> 69: /* >> 70: * @test >> 71: * @bug 4992173 4992170 > > this needs `@modules jdk.compiler` Thanks. Added in next patch - PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v2]
On Fri, 22 Jan 2021 18:09:43 GMT, Mandy Chung 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/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? To avoid the Checkstyle warnings, I added them. - PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v2]
On Fri, 22 Jan 2021 18:16:54 GMT, Mandy Chung 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/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.
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.
> 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`
Renamed the packagePath to packageName in next patch. To avoid the Checkstyle
warning added the final key word with parameters.
> 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.
Thanks. Implemented the comment in next patch.
> 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.
Thanks. Corrected this in next patch.
-
PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v2]
On Fri, 22 Jan 2021 17:59:33 GMT, Mandy Chung 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
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java
On Fri, 22 Jan 2021 16:57:41 GMT, Mahendra Chhipa
wrote:
>> Can this be done all in `EnclosingClassTest.java`, without a new
>> `RunEnclosingClassTest.java`?
>>
>> Adding the `@BeforeClass` and `@AfterClass` methods to what's there, you may
>> just need to
>> change the `test()` calls to use reflection - something along the lines of:
>>
>>> test(`Class.forName("EnclosingClass").getDeclaredConstructor().newInstance());`
>
> Review comments implemented.
Can the new code be added to the existing `NonJavaNames.java` instead of adding
a new `NonJavaNameTest.java` file?
-
PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v2]
On Fri, 22 Jan 2021 16:52:02 GMT, Mahendra Chhipa
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
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java
On Thu, 21 Jan 2021 05:19:00 GMT, Brent Christian wrote:
>> https://bugs.openjdk.java.net/browse/JDK-8183372
>
> Can this be done all in `EnclosingClassTest.java`, without a new
> `RunEnclosingClassTest.java`?
>
> Adding the `@BeforeClass` and `@AfterClass` methods to what's there, you may
> just need to
> change the `test()` calls to use reflection - something along the lines of:
>
>> test(`Class.forName("EnclosingClass").getDeclaredConstructor().newInstance());`
Review comments implemented.
-
PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java [v2]
> 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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/2170/files - new: https://git.openjdk.java.net/jdk/pull/2170/files/a02e66a5..e664bd73 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2170=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2170=00-01 Stats: 404 lines in 5 files changed: 186 ins; 210 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/2170.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2170/head:pull/2170 PR: https://git.openjdk.java.net/jdk/pull/2170
Re: RFR: JDK-8183372 : Refactor java/lang/Class shell tests to java
On Wed, 20 Jan 2021 17:27:43 GMT, Mahendra Chhipa
wrote:
> https://bugs.openjdk.java.net/browse/JDK-8183372
Can this be done all in `EnclosingClassTest.java`, without a new
`RunEnclosingClassTest.java`?
Adding the `@BeforeClass` and `@AfterClass` methods to what's there, you may
just need to
change the `test()` calls to use reflection - something along the lines of:
> test(`Class.forName("EnclosingClass").getDeclaredConstructor().newInstance());`
-
PR: https://git.openjdk.java.net/jdk/pull/2170
