Thank you, Roger > On 9 Aug 2018, at 10:01 PM, Roger Riggs <roger.ri...@oracle.com> wrote: > > Hi Chris, > > looks fine now. +1 > > On 8/8/18 11:35 PM, Chris Yin wrote: >> Hi, Roger >> >> Many thanks for your review and comments, inline and updated new webrev as >> below, thanks >> >> http://cr.openjdk.java.net/~xyin/8208483/webrev.05/ >> >>> On 8 Aug 2018, at 9:52 PM, Roger Riggs <roger.ri...@oracle.com> wrote: >>> >>> Hi Chris, >>> >>> Each of these tests is going to spawn a new process (/othervm). That's a >>> lot of overhead for a short test. >> Thank you for checking this, actually used /othervm here is intentional to >> make object factories working correctly, I’m not sure why yet, just guess >> jtreg agentvm mode will affect object factory loader which caused object >> factory not working. For example, if I removed /othervm from >> LookupWithAnyAttrProp.java then run 'jtreg -agentvm -v:summary -testjdk:xxx >> -a LookupWithAnyAttrProp.java’, test will failed to complain lookup returned >> object is com.sun.jndi.dns.DnsContext instead of TestDnsObject, that was >> unexpected. (I used latest jtreg version with jdk12, same result) > yes, /othervm is needed to avoid interference with other tests >>> 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? >> That's a good idea, but per current tests situation, guess it’s hard, 2 >> points as below >> 1. As above mentioned, for factory related tests, we have to specify >> /othervm to make object factory working, that may not suitable for TestNG > /othervm can be used with testng
Cool, thank you for the info, I will have a try, guess the only concern is whether /othervm could be applied to test method to avoid interference with others in same test class >> 2. Each test along with a .dns dump file, even the test logic are very >> close, the dumped messages maybe very different. If using TestNG 1 test file >> with multiple tests, it will be more complex to manage/maintain those dump >> files (like create/update dump file per test, search/load dump file when run >> each test etc.) > As a thought experiment, and perhaps worth a prototype and especially if > there are more tests to write. Sure, I will try to make some prototype at the same time to see whether that way will be better > > The testname/dns might need to be refactored out from the test class name, > but if I understand > the local server case, it creates a new local server for each test with a new > port and environment. > With no overlap between the test data, a clean separation between the tests > at runtime may be possible > with not too much work. Yes, your understanding is correct, just the real case seems a little complex to sort out Thanks & Regards, Chris > > Thanks, Roger > > >> >>> @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. >> Sure, fixed, changed my IDE to use that style >> >>> 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). >> Fixed as you suggested form >> >>> The imports should be before the comment with the jtreg test description. >> Fixed, thanks >> >> Regards, >> Chris >> >>> 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....@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 >
