Re: [12] RFR 8208483: Add 5 JNDI tests to com/sun/jndi/dns/FactoryTests/

[vyom tewari]

Hi Chris,

Latest code webrev(.03) looks good to me, expending initContext() makes 
tests more readable :) .

Thanks,

Vyom


On Wednesday 08 August 2018 09:11 AM, Chris Yin wrote:
Just one more minor revision to expand initContext() into test method for 
easier read, new webrev as below, thanks

http://cr.openjdk.java.net/~xyin/8208483/webrev.04/

Regards,
Chris

On 7 Aug 2018, at 5:41 PM, Chris Yin <xu.y....@oracle.com> wrote:

Hi, Vyom

Thanks a lot for your comment, I didn’t realize it, sure, I moved this constant 
to LookupFactoryBase as you suggested and also abstract the same verify logic 
into base class as ‘verifyLookupObjectAndValue’, hope that looks better, update 
new webrev as below, thanks

http://cr.openjdk.java.net/~xyin/8208483/webrev.03/

Regards,
Chris

On 7 Aug 2018, at 4:35 PM, vyom tewari <vyom.tew...@oracle.com> wrote:

Hi Chris,

Overall latest code looks good to me, one minor comment did you consider moving  "private static final String 
ATTRIBUTE_VALUE = "1.2.4.1";" in "LookupFactoryBase" ? as both LookupWithAnyAttrProp & 
LookupWithAttrProp inherit "LookupFactoryBase".

.Thanks,

Vyom


On Monday 06 August 2018 03:02 PM, Chris Yin wrote:
Hi, Vyom

Many thanks for your review and comments, inline and updated webrev as below, 
thanks

http://cr.openjdk.java.net/~xyin/8208483/webrev.01/

On 6 Aug 2018, at 4:12 PM, vyom tewari <vyom.tew...@oracle.com> wrote:

Hi Chris,

1-> DirAFactory.java, "getIbjectInstance" returns "null"  if it fails to construct object. I will 
suggest you to throw  some "RuntimeException" instead returning null. If you return null then caller of 
"getObjectInstance" had to check for null and it will end in lots of boilerplate code.
‘getObjectInstance’ methods are override from DirObjectFactory and 
ObjectFactory, per my understanding on javadoc, return null should indicate 
object cannot be created and allow other factories can be tried, feel free to 
let me know if I misunderstand something on the api doc, thanks

Paste a few javadoc here against ‘getObjectInstance’ method as below

* @return The object created; null if an object cannot be created.
* @exception Exception If this object factory encountered an exception
* while attempting to create an object, and no other object factories are
* to be tried.

2-> DirTxtFactory.java same as "DirFactory.java”.
Same as above

3-> In LookupWithAnyAttrProp/LookupWithAttrProp java create a constant for 
"1.2.4.1" and put one liner comment if possible. This is will help future 
maintainer to understand why we are comparing with this value.
Sure, created constant for “1.2.4.1” and put comment to explain that it’s pre 
defined attribute value of ‘A’, thanks

One generic comment, in most of the places i can see, you declared functions to throw 
generic exception "Exception",  please change it to specific  exception or list 
of specific exceptions if possible.
Fixed, those generic exception are generated automatically through override 
method, I removed those unused throws Exception from xxxFactory.

Thanks & Regards,
Chris

Thanks,

Vyom


On Monday 30 July 2018 02:08 PM, Chris Yin wrote:
Please review the changes to add 5 JNDI tests to com/sun/jndi/dns/FactoryTests/ 
in OpenJDK, thanks

bug: https://bugs.openjdk.java.net/browse/JDK-8208483 
<https://bugs.openjdk.java.net/browse/JDK-8208483>
webrev: http://cr.openjdk.java.net/~xyin/8208483/webrev.00/ 
<http://cr.openjdk.java.net/~xyin/8208483/webrev.00/>

Regards,
Chris