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

[Roger Riggs]

Hi Chris,

Each of these tests is going to spawn a new process (/othervm). That's a 
lot of overhead for a short test.

Can these be written as TestNG tests so they run quicker and more tests 
be in a single source file
and run in single VM?

@Override should be on its own line so the method signature is on a line 
by itself too.
Setup your IDE to use the formatting conventions and let it reformat.

LookupFactoryBase.java: 67 / 84  - Be consistent about the form of 
messages,
  I prefer the Expected xxx but found yyy form.   (no need to say it 
failed, it threw an exception).

The imports should be before the comment with the jtreg test description.

Thanks, Roger

On 8/7/18 11:41 PM, 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.yin at 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.tewari at 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.tewari at 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