Thank you Roger, sure, I will expand initContext() inline in the test method for future tests
Regards, Chris > On 8 Aug 2018, at 3:11 AM, Roger Riggs <roger.ri...@oracle.com> wrote: > > Hi Chris, > > Looks ok. > > For future tests, I think I would put the contents of the initContext() > methods inline in the test method so the setup and the testing of the > conditions were in the same method, making it easier to read. > > Thanks, Roger > > > On 8/6/2018 11:53 PM, Chris Yin wrote: >> Hi, Roger >> >> Thanks a lot for your review and comments, inline and updated webrev as >> below, thanks >> >> http://cr.openjdk.java.net/~xyin/8208279/webrev.03/ >> <http://cr.openjdk.java.net/~xyin/8208279/webrev.03/> >> >>> On 6 Aug 2018, at 10:58 PM, Roger Riggs <roger.ri...@oracle.com> >>> <mailto:roger.ri...@oracle.com> wrote: >>> >>> Hi Chris, >>> >>> EnvTestBase.java: The class and each of the methods should have javadoc >>> with a descriptive comment >>> to make the test construction easier to understand for future maintenance >>> especially since >>> they are shared/overridden and used by many of the test cases. >>> For example, verifyEnvProperty should say what is true about a valid >>> property. >>> With many layers of inherited test methods it needs easier to understand >>> what comes from where. >>> Build good habits with writing complete sentences with capitalization and >>> punctuation in comments. >> Sure, I added javadoc to EnvTestBase class and shared/override methods >> (except getter/setter), will also complement javadoc to test base class in >> other review thread, thanks >> >>> initContext() belongs in DNSTestBase along with context() and setContext() >>> to avoid splitting that >>> function across multiple classes. >>> Or remove initContext() since it is overridden in several tests (without >>> calling super) and >>> just call setContext() with new InitialDirContext(env()) consistently in >>> each test. >> Yep, I hesitated at first whether put initContext() to DNSTestBase, after >> look through many dns test cases, I found there are a lot of tests may need >> to override it and some case to use parameterized method will be easier >> (such as initContext(String, String) used in Factory Tests), so I decided to >> leave it to each sub test base or tests to customize. Anyway we could >> refactor it later if we found that way better. >> For now I removed initContext() from EnvTestBase as you suggested in 2nd and >> used private initContext() for complex initialization or just call >> setContext with new InitialDirContext(env()) for simple one, thanks >> >>> ProviderUrlGen.java should not mention SWAN by name as it is an internal >>> resource >>> (and that's not the current name). >> Sure, removed that name >> >>> RemoveInherited.java: for strings that need to be consistent across the >>> test, they should be defined as >>> final statics so there is only one declaration. >> Fixed as your suggested >> >> Thanks & Regards, >> Chris >> >>> Thanks, Roger >>> >>> On 8/6/2018 3:09 AM, Chris Yin wrote: >>>> Thank you, Vyom >>>> >>>> Regards, >>>> Chris >>>> >>>>> On 6 Aug 2018, at 2:02 PM, vyom tewari <vyom.tew...@oracle.com> >>>>> <mailto:vyom.tew...@oracle.com> wrote: >>>>> >>>>> Hi Chris, >>>>> >>>>> Latest webrev looks good to me. >>>>> >>>>> Thanks, >>>>> >>>>> Vyom >>>>> >>>>> >>>>> On Friday 03 August 2018 02:46 PM, Chris Yin wrote: >>>>>> Hi, Vyom >>>>>> >>>>>> Thank a lot for your review and comments, inline and update new webrev >>>>>> as below >>>>>> >>>>>> http://cr.openjdk.java.net/~xyin/8208279/webrev.02/ >>>>>> <http://cr.openjdk.java.net/~xyin/8208279/webrev.02/> >>>>>> >>>>>>> On 3 Aug 2018, at 3:45 PM, vyom tewari <vyom.tew...@oracle.com> >>>>>>> <mailto:vyom.tew...@oracle.com> wrote: >>>>>>> >>>>>>> Hi Chris, >>>>>>> >>>>>>> Overall looks good to me, couple of minor comment. >>>>>>> >>>>>>> 1-> ProviderUrlGen.java, Line:100 Exception e can be replaced with >>>>>>> specific exceptions. >>>>>> Fixed, thanks >>>>>> >>>>>>> 2-> In couple of places we are calling "removeFromEnvironment" on >>>>>>> context and spec says it will return "null" if the env is not set. I >>>>>>> see that we are setting the required env in "initContext" so it will >>>>>>> never be "null" in our tests, but i suggest you to put just one liner >>>>>>> comment that "val" can not be null. This is just to improve code >>>>>>> readability, please feel free to ignore, if you think it is not worth >>>>>>> enough. >>>>>> Sure, added one line comment before each “removeFromEnvironment” call as >>>>>> you suggested >>>>>> >>>>>> Thanks, >>>>>> Chris >>>>>> >>>>>>> Thanks, >>>>>>> >>>>>>> Vyom >>>>>>> >>>>>>> >>>>>>> On Monday 30 July 2018 08:41 AM, Chris Yin wrote: >>>>>>>> Please find the new webrev as below, it addressed some similar issues >>>>>>>> which mentioned in review comments from another thread RFR 8200151, >>>>>>>> thanks >>>>>>>> >>>>>>>> webrev: http://cr.openjdk.java.net/~xyin/8208279/webrev.01/ >>>>>>>> <http://cr.openjdk.java.net/~xyin/8208279/webrev.01/> >>>>>>>> <http://cr.openjdk.java.net/~xyin/8208279/webrev.01/> >>>>>>>> <http://cr.openjdk.java.net/~xyin/8208279/webrev.01/> >>>>>>>> >>>>>>>> Regards, >>>>>>>> Chris >>>>>>>> >>>>>>>>> On 26 Jul 2018, at 3:37 PM, Chris Yin <xu.y....@oracle.com> >>>>>>>>> <mailto:xu.y....@oracle.com> wrote: >>>>>>>>> >>>>>>>>> Please review the changes to add another 8 JNDI tests to >>>>>>>>> com/sun/jndi/dns/EnvTests/ in OpenJDK, thanks >>>>>>>>> >>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8208279 >>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8208279> >>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8208279> >>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8208279> >>>>>>>>> webrev: http://cr.openjdk.java.net/~xyin/8208279/webrev.00/ >>>>>>>>> <http://cr.openjdk.java.net/~xyin/8208279/webrev.00/> >>>>>>>>> <http://cr.openjdk.java.net/~xyin/8208279/webrev.00/> >>>>>>>>> <http://cr.openjdk.java.net/~xyin/8208279/webrev.00/> >>>>>>>>> >>>>>>>>> Regards, >>>>>>>>> Chris >
