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/ > On 11 Aug 2018, at 1:47 AM, Roger Riggs <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/ >> >> 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 >
