[12] RFR 8208279: Add 8 JNDI tests to com/sun/jndi/dns/EnvTests/
Roger Riggs
Roger.Riggs at Oracle.com
Mon Aug 6 14:58:49 UTC 2018
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.
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.
ProviderUrlGen.java should not mention SWAN by name as it is an internal
resource
(and that's not the current 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.
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.tewari at 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.tewari at 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.yin at 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
More information about the core-libs-dev
mailing list