Hi Chris,
please review the updated webrev below. Comments inline.
http://cr.openjdk.java.net/~xiaofeya/8166139/webrev.03/
-Felix
On 2017/5/29 18:19, Chris Hegarty wrote:
Felix,
Thanks for taking this one. A few comments:
1) test/java/net/URLConnection/6212146/TestDriver.java
Please check indentation around L81:
80 Files.copy(testJar,
___________targetDir.resolve(ARCHIVE_NAME),
81 ___________StandardCopyOption.REPLACE_EXISTING);
Also, and more importantly, the ulimit that was executed in the shell
is supposed to affect the potential file descriptor usage of the
following java process. I believe that is not captured correctly
in the TestDriver.java, since there is no parent/child relationship
between the processes, no?
Fixed, thanks
Felix
2) test/java/net/URLClassLoader/getresourceasstream/policy
Please add a copyright/header.
Checked existing test policy files, all of them have no
copyright/header. So I hesitated to add here. Please confirm.
-Felix
3) getresourceasstream/TestDriver.java, CheckSealedTest.java,
CloseTest.java
If you statically import StandardCopyOption.REPLACE_EXISTING in a
few places it may lead to a few shorter lines.
Changed, thanks
Felix
4) Why has the test for 5077773 simply been removed?
As priorly commented, the test is probably not necessary.
Because it actually quits immediately after prerequisite checking for
"javax/swing/text/rtf/charsets/mac.txt". According with JDK-5077773, it
is to cover endorsed scenarios, while endorsed mechanism has been
removed in JDK 9.
-Felix
-Chris.
On 27/05/17 02:15, Felix Yang wrote:
Hi Roger,
thanks for the comments. Please see the new webrev
http://cr.openjdk.java.net/~xiaofeya/8166139/webrev.02/
-Felix
On 2017/5/27 3:50, Roger Riggs wrote:
Hi Felix,
fyi, there is a new version of webrev.ksh that provides convenient
next and previous links.
http://hg.openjdk.java.net/code-tools/webrev/raw-file/d26c194772db/webrev.ksh
CloseTest.java: 66; checking WORK_DIR *after* calling setup() does not
make sense.
Ditto: closetest/GetResourceAsStream.java
Removed such check, since the test depends on several system
properties("test.src", "user.dir"). It is not expected to run outside
jtreg.
-Felix
URLConnection/6212146/Test.java:47 typo: ULR -> URL
Regards, Roger
On 5/25/2017 11:14 PM, Felix Yang wrote:
Hi Roger,
please review the updated webrev:
http://cr.openjdk.java.net/~xiaofeya/8166139/webrev.01/
Thanks,
Felix
On 2017/5/26 3:24, Roger Riggs wrote:
Hi Felix,
closetest/CloseTest:
32: Please put the @modules directives together (and not repeat)
Fixed
-Felix
44: do not use wildcard imports (reconfigure your IDE if necessary
to avoid accidents).
Fixed and also organized imports in other test files
-Felix
63: setup() is invoked twice... remove 1 or explain why 2 calls
Fixed
-Felix
69, 78,90,91,... : space in method call is not proper style
Fixed and also formatted other history codes. Just format only
without logic change.
Not restricted to this test. I only formatted history codes "nearby",
to avoid hiding real logic changes in the patch.
-Felix
sealing/CheckSealed.java:
- Why is @test block removed?
Should be converted to @run main CheckSealed
There was a checksealed.sh. I replaced it with CheckSealedTest.java
and declared @test there.
http://cr.openjdk.java.net/~xiaofeya/8166139/webrev.01/test/java/net/URLClassLoader/sealing/CheckSealedTest.java.html
BTW, you may noticed test/java/net/URLClassLoader/B5077773.* were
removed. That is because corresponding prerequisite was removed even
in JDK 8.
Perhaps then add a comment to CheckSealed.java indicating it is
compiled and executed by CheckSealedTest.java.
Roger
-Felix
Regards, Roger
On 5/25/2017 4:08 AM, Felix Yang wrote:
Hi there,
please review following patch to convert all shell cases under
java/net to plain java codes.
Webrev:
http://cr.openjdk.java.net/~xiaofeya/8166139/webrev.00/
Bug:
https://bugs.openjdk.java.net/browse/JDK-8166139
Thanks,
Felix
