Re: [12] RFR 8208279: Add 8 JNDI tests to com/sun/jndi/dns/EnvTests/

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 wrote: > > Hi Chris, > > Looks ok. > > For future tests, I think I would put the contents of the initContext() > methods inline in the test

Re: [12] RFR 8208279: Add 8 JNDI tests to com/sun/jndi/dns/EnvTests/

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

Re: [12] RFR 8208279: Add 8 JNDI tests to com/sun/jndi/dns/EnvTests/

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 wrote: > > Hi Chris, > > EnvTestBase.java: The class and each of the methods should have javadoc

Re: [12] RFR 8208279: Add 8 JNDI tests to com/sun/jndi/dns/EnvTests/

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,

Re: [12] RFR 8208279: Add 8 JNDI tests to com/sun/jndi/dns/EnvTests/

Thank you, Vyom Regards, Chris > On 6 Aug 2018, at 2:02 PM, vyom tewari 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

Re: [12] RFR 8208279: Add 8 JNDI tests to com/sun/jndi/dns/EnvTests/

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

Re: [12] RFR 8208279: Add 8 JNDI tests to com/sun/jndi/dns/EnvTests/

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 wrote: > > Hi Chris, > > Overall looks good to me, couple of minor comment. > > 1-> ProviderUrlGen.java,

Re: [12] RFR 8208279: Add 8 JNDI tests to com/sun/jndi/dns/EnvTests/

Hi Chris, Overall looks good to me, couple of minor comment. 1-> ProviderUrlGen.java, Line:100 Exception e can be replaced with specific  exceptions. 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

Re: [12] RFR 8208279: Add 8 JNDI tests to com/sun/jndi/dns/EnvTests/

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/ Regards, Chris > On 26 Jul 2018,