(10) RFR of JDK-8166142: Refactor java.io.serialization shell tests to java

Hamlin Li huaming.li at oracle.com
Thu Jun 1 01:52:58 UTC 2017


Hi Roger,

Thank you for detailed reviewing, fixed as you suggested except below 
comment:

     81-83/93/95:  use  SHARE.resolve(xxx).toString to compute the paths.

As the code is constructing a class path with 2 paths rather than 
constructing a path.

new webrev: http://cr.openjdk.java.net/~mli/8166142/webrev.03/

Thank you

-Hamlin


On 2017/6/1 0:47, Roger Riggs wrote:
> Hi Hamlin,
>
> RenamePackageTest.java:
>   - 48,61:  rename "sutup" -> "setup"
>
> 81-83/93/95:  use  SHARE.resolve(xxx).toString to compute the paths.
>
> ClassPathTest.java:
> 42: add a space before "{"
>
> 56:  Generally, exception messages should not end in "."; they are phrases
> so they could be printed with context.
> In this specific case it is not important, just a pattern to follow 
> consistently.
>
> Thanks, Roger
>
> On 5/26/2017 9:14 PM, Hamlin Li wrote:
>> Hi Roger,
>>
>> Thank you for catching it, new webrev: 
>> http://cr.openjdk.java.net/~mli/8166142/webrev.02/
>>
>> Thank you
>>
>> -Hamlin
>>
>>
>> On 2017/5/27 3:30, Roger Riggs wrote:
>>> Hi Hamlin,
>>>
>>> SubclassTest.java:37: typo" excepiton" -> exception
>>>
>>>
>>> SubclassDataLossTest.java:
>>> 103/104:  Adding the class loader close() calls isn't very effective 
>>> since if there is an exception they are not
>>> closed and the creation in a static initializer is mismatched with 
>>> main() code.
>>>
>>> It would be cleaner to open in main() using try-with-resources and 
>>> close them in a finally clause.
>>> And pass the loaders to the test function.
>>> But for simplicity, perhaps just leave out the new calls to 
>>> ldr1/2.close.
>>>
>>> (There is something odd about the webrev links, the nagivation links 
>>> don't work as expected. but that's transient)
>>>
>>> Thanks, Roger
>>>
>>>
>>>
>>> On 5/26/2017 12:26 AM, Hamlin Li wrote:
>>>> Hi Roger,
>>>>
>>>> Thank you for detailed review. Fixed as you suggested, new webrev: 
>>>> http://cr.openjdk.java.net/~mli/8166142/webrev.01/
>>>>
>>>> Thank you
>>>>
>>>> -Hamlin
>>>>
>>>>
>>>> On 2017/5/26 2:54, Roger Riggs wrote:
>>>>> Hi Hamlin,
>>>>>
>>>>> For imports they should import specific classes, wildcards are not 
>>>>> used.
>>>>>
>>>>> maskSyntheticModifier/Test.java
>>>>> consTest/Test.java
>>>>> deserializeButton/Test.java
>>>>>
>>>>>  test/java/io/Serializable/packageAccess/Driver.java: the name 
>>>>> "Driver" is not descriptive and should be.
>>>>>
>>>>> Each Driver.java should have a different and 
>>>>> functional/descriptive name.
>>>>>
>>>>> Better yet, there should be a single program that creates jar 
>>>>> files using command line arguments
>>>>> that can be invoked depending on the test.
>>>>>
>>>>> There is a mix of Driver's copying files vs the test program 
>>>>> copying; can that be made more uniform.
>>>>>
>>>>> Test.java files should have descriptive/functional names. (Even if 
>>>>> the duplicating the directory)
>>>>>
>>>>> Regards, Roger
>>>>>
>>>>>
>>>>> On 5/24/2017 5:09 AM, Hamlin Li wrote:
>>>>>> Would you please review the below patch?
>>>>>>
>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8166142
>>>>>>
>>>>>> webrev: http://cr.openjdk.java.net/~mli/8166142/webrev.00/
>>>>>>
>>>>>>
>>>>>> Thank you
>>>>>>
>>>>>> -Hamlin
>>>>>>
>>>>>
>>>>
>>>
>>
>



More information about the core-libs-dev mailing list