Re: [12] RFR 8200151: Add 8 JNDI tests to com/sun/jndi/dns/ConfigTests/

[Roger Riggs]

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/

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

--
Thanks, Roger