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

Hamlin Li huaming.li at oracle.com
Sat May 27 01:14:02 UTC 2017


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