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

[vyom tewari]

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