[12] RFR 8200151: Add 8 JNDI tests to com/sun/jndi/dns/ConfigTests/ (original) (raw)

Chris Yin xu.y.yin at oracle.com
Mon Aug 20 09:28:58 UTC 2018

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

Thank you, Vyom Regards, Chris

On 30 Jul 2018, at 5:06 PM, vyom tewari <vyom.tewari at 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 at oracle.com <mailto:vyom.tewari at 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.INITIALCONTEXTFACTORY’ 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

More information about the core-libs-dev mailing list