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

Fri Oct 19 01:21:29 UTC 2018

Thank you, Roger

Chris

> On 19 Oct 2018, at 2:51 AM, Roger Riggs <Roger.Riggs at Oracle.com> wrote:
> 
> Hi Chris,
> 
> Sorry, for the delay. I'd lost track of this review.
> 
> Looks fine.
> 
> Roger
> 
> 
> On 08/20/2018 05:28 AM, Chris Yin wrote:
>> Hi, Roger
>> 
>> Sorry for the late response since I was on vacation and thank you for the comments, inline and update webrev as below
>> 
>> http://cr.openjdk.java.net/~xyin/8200151/webrev.03/ <http://cr.openjdk.java.net/~xyin/8200151/webrev.03/>
>> 
>>> On 11 Aug 2018, at 1:47 AM, Roger Riggs <Roger.Riggs at Oracle.com> <mailto:Roger.Riggs at Oracle.com> wrote:
>>> 
>>> Hi Chris,
>>> 
>>> There seems to be a lot of repetition in tests that could be combined.
>>> For example, the RecursiveDefault, RecursiveTrue, and RecursiveFalse tests should be a
>>> single test that is invoked 3 times, (multiple @run lines) with an argument to say which case to test.
>>> There would be fewer files and less code to maintain.
>>> 
>>> The same goes for AuthDefault, AuthTrue and AuthFalse.
>> Thanks, fixed as you suggested, minor change in DNSTestUtils to support it
>> 
>>> Why is PortUnreachable added to the ProblemList and also included in the Change set.
>>> It would cleaner to treat it separately if it can't be fixed as part of adding these new tests.
>> Thank you for checking this, there is a platform specified known issue on macOS which caused PortUnreachable failure, but it’s unrelated to JNDI feature, and test working well on other platforms, so I add it to problemlist to exclude the test for macOS only, feel free to let me know if it’s not suitable, I could just remove the test here for now
>> 
>>> Consider using java.time.Instant and Duration for the Timeout test;
>>> it will print nicer and has some handy methods.
>> Sure, changed as you suggested, thanks
>> 
>> Regards,
>> Chris
>> 
>>> Regards, Roger
>>> 
>>> 
>>> On 8/10/18 5:15 AM, Chris Yin wrote:
>>>> 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/ <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> <mailto: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> <mailto: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/~xyin/8200151/webrev.01/> <http://cr.openjdk.java.net/%7Exyin/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> <mailto: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> <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/~xyin/8200151/webrev.00/> <http://cr.openjdk.java.net/%7Exyin/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/~xyin/8200151/webrev.00/> <http://cr.openjdk.java.net/%7Exyin/8200151/webrev.00/> <http://cr.openjdk.java.net/%7Exyin/8200151/webrev.00/>>
>>>>>>>>> 
>>>>>>>>> Regards,
>>>>>>>>> Chris
> 
> -- 
> Thanks, Roger