Thank you, Roger Chris
> On 19 Oct 2018, at 2:51 AM, Roger Riggs <roger.ri...@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.ri...@oracle.com> >>> <mailto:roger.ri...@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....@oracle.com> >>>>> <mailto:xu.y....@oracle.com> wrote: >>>>> >>>>> Thank you, Vyom >>>>> >>>>> Regards, >>>>> Chris >>>>> >>>>>> On 30 Jul 2018, at 5:06 PM, vyom tewari <vyom.tew...@oracle.com> >>>>>> <mailto: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/~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.tew...@oracle.com >>>>>>>> <mailto:vyom.tew...@oracle.com> <mailto: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> >>>>>>>>> <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
