[12] RFR 8208279: Add 8 JNDI tests to com/sun/jndi/dns/EnvTests/
vyom tewari
vyom.tewari at oracle.com
Fri Aug 3 07:45:25 UTC 2018
Hi Chris,
Overall looks good to me, couple of minor comment.
1-> ProviderUrlGen.java, Line:100 Exception e can be replaced with
specific exceptions.
2-> In couple of places we are calling "removeFromEnvironment" on
context and spec says it will return "null" if the env is not set. I
see that we are setting the required env in "initContext" so it will
never be "null" in our tests, but i suggest you to put just one liner
comment that "val" can not be null. This is just to improve code
readability, please feel free to ignore, if you think it is not worth
enough.
Thanks,
Vyom
On Monday 30 July 2018 08:41 AM, Chris Yin wrote:
> Please find the new webrev as below, it addressed some similar issues which mentioned in review comments from another thread RFR 8200151, thanks
>
> webrev: http://cr.openjdk.java.net/~xyin/8208279/webrev.01/ <http://cr.openjdk.java.net/~xyin/8208279/webrev.01/>
>
> Regards,
> Chris
>
>> On 26 Jul 2018, at 3:37 PM, Chris Yin <xu.y.yin at oracle.com> wrote:
>>
>> Please review the changes to add another 8 JNDI tests to com/sun/jndi/dns/EnvTests/ in OpenJDK, thanks
>>
>> bug: https://bugs.openjdk.java.net/browse/JDK-8208279 <https://bugs.openjdk.java.net/browse/JDK-8208279>
>> webrev: http://cr.openjdk.java.net/~xyin/8208279/webrev.00/ <http://cr.openjdk.java.net/~xyin/8208279/webrev.00/>
>>
>> Regards,
>> Chris
More information about the core-libs-dev
mailing list