Thank you, Vyom Regards, Chris
> On 30 Jul 2018, at 5:06 PM, vyom tewari <vyom.tew...@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.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> >>>> <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 >>> >> >
