[12] RFR 8200151: Add 8 JNDI tests to com/sun/jndi/dns/ConfigTests/

Chris Yin xu.y.yin at oracle.com
Fri Aug 10 09:15:54 UTC 2018


Minor revision to address testbase javadoc, initContext() expansion, @Override line and imports place, new webrev as below, thanks

http://cr.openjdk.java.net/~xyin/8200151/webrev.02/

Regards,
Chris

> On 30 Jul 2018, at 5:08 PM, Chris Yin <xu.y.yin at oracle.com> wrote:
> 
> Thank you, Vyom
> 
> Regards,
> Chris
> 
>> On 30 Jul 2018, at 5:06 PM, vyom tewari <vyom.tewari at oracle.com> wrote:
>> 
>> Hi Chris,
>> 
>> Latest code looks good to me.
>> 
>> Thanks,
>> 
>> Vyom
>> 
>> On Friday 27 July 2018 01:12 PM, Chris Yin wrote:
>>> Hi, Vyom
>>> 
>>> Thank a lot for your review and comments, inline and update new webrev as below, thanks
>>> 
>>> http://cr.openjdk.java.net/~xyin/8200151/webrev.01/ <http://cr.openjdk.java.net/%7Exyin/8200151/webrev.01/>
>>> 
>>> 
>>>> On 26 Jul 2018, at 5:27 PM, vyom tewari <vyom.tewari at oracle.com <mailto:vyom.tewari at oracle.com>> wrote:
>>>> 
>>>> Hi Chris,
>>>> 
>>>> Thanks for writing the tests overall code looks good to me, below are few minor comments.
>>>> 
>>>> 1`-> Fix tag order, my Netbeans always complains :) .
>>> 
>>> Fixed, thanks
>>> 
>>>> 
>>>> 2->  you make AuthRecursiveBase class as public but i can not see it is being used outside,  i will suggest you to give the default access if possible.
>>> 
>>> Sure, changed it to default access now
>>> 
>>>> 
>>>> 3-> AuthTrue.java, change the message as follows
>>>> 
>>>> "Got exception as expected : " -> "Got expected exception: “;
>>> 
>>> Fixed
>>> 
>>>> 
>>>> 
>>>> 4-> PortUnreachable.java , any specific reason for 1000ms or you just choose random value ? Please don't hard-code this specific value  create a  constant and use it . If possible put some comment why we choose 1 second, this will help in debugging if this test fails intermittently in future.
>>> 
>>> Sure, I created a constant ’THRESHOLD' for it, 1000ms will be used as a threshold value for this test, normally the request should fail very quick (in a few ms), but consider to different platform and test machine performance, we used an acceptable threshold here, some comments added to it in the code too, thanks
>>> 
>>>> 
>>>> 5-> Timeout.java, do you really need to set the env using "DNSTestUtils.initEnv" ?
>>>> 
>>>> I see, this test is not using the DNSServer and other environments variables which were set by "DNSTestUtils.initEnv()" or i am missing something.
>>> 
>>> Actually, it’s indeed not necessary, just set very few default value such as ‘Context.INITIAL_CONTEXT_FACTORY’ in the test code should be fine, but I may still want to keep the test consistency, then we could have better maintainability for those tests. Sorry, I forgot to refactor this test with testbase (actually, its not necessary too, but guess the consistent style and shared base/methods will make the test easy to read and maintain, refactored this test and PortUnreachable.java now)
>>> 
>>> Regards,
>>> Chris
>>> 
>>>> 
>>>> Thanks,
>>>> Vyom
>>>> 
>>>> On Wednesday 25 July 2018 02:41 PM, Chris Yin wrote:
>>>>> Please review the changes to add 8 JNDI tests to com/sun/jndi/dns/ConfigTests/ in OpenJDK, due to known issue 7164518, PortUnreachable.java will be problem list for 'macosx-all', thanks
>>>>> 
>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8200151 <https://bugs.openjdk.java.net/browse/JDK-8200151> <https://bugs.openjdk.java.net/browse/JDK-8200151 <https://bugs.openjdk.java.net/browse/JDK-8200151>>
>>>>> webrev: http://cr.openjdk.java.net/~xyin/8200151/webrev.00/ <http://cr.openjdk.java.net/%7Exyin/8200151/webrev.00/> <http://cr.openjdk.java.net/~xyin/8200151/webrev.00/ <http://cr.openjdk.java.net/%7Exyin/8200151/webrev.00/>>
>>>>> 
>>>>> Regards,
>>>>> Chris
>>>> 
>>> 
>> 
> 



More information about the core-libs-dev mailing list