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?
2) test/java/net/URLClassLoader/getresourceasstream/policy
Please add a copyright/header.
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.
4) Why has the test for 5077773 simply been removed?
-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
