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.Riggs@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.yin@oracle.com> wrote:
Thank you, Vyom
Regards, Chris
On 30 Jul 2018, at 5:06 PM, vyom tewari <vyom.tewari@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.tewari@oracle.com <mailto:vyom.tewari@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