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....@oracle.com> wrote: > > 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 >>>> >>> >> >
