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

Fri, 10 Aug 2018 10:48:01 -0700 

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.
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. 

Consider using java.time.Instant and Duration for the Timeout test;
it will print nicer and has some handy methods.

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

Reply via email to