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/ > On 6 Aug 2018, at 10:58 PM, Roger Riggs <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> 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/ >>>> >>>>> On 3 Aug 2018, at 3:45 PM, vyom tewari <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/> >>>>>> >>>>>> Regards, >>>>>> Chris >>>>>> >>>>>>> On 26 Jul 2018, at 3:37 PM, Chris Yin <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> >>>>>>> webrev: http://cr.openjdk.java.net/~xyin/8208279/webrev.00/ >>>>>>> <http://cr.openjdk.java.net/~xyin/8208279/webrev.00/> >>>>>>> >>>>>>> Regards, >>>>>>> Chris >
