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.tew...@oracle.com
<mailto:vyom.tew...@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
