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

[Roger Riggs]

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.

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.

Consider using java.time.Instant and Duration for the Timeout test;
it will print nicer and has some handy methods.

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