RFR 8166139/10, Refactor java/net shell cases to java

Felix Yang felix.yang at oracle.com
Mon May 29 15:17:20 UTC 2017


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
>>>>>>
>>>>>
>>>>
>>>
>>



More information about the core-libs-dev mailing list