RFR 8198882: Add 10 JNDI tests to com/sun/jndi/dns/AttributeTests/

Wed Jul 25 02:14:47 UTC 2018

Thank you Roger, I will fix them before push.

Regards,
Chris

> On 24 Jul 2018, at 10:12 PM, Roger Riggs <Roger.Riggs at Oracle.com> wrote:
> 
> Hi Chris,
> 
> Thanks for the improvements and javadoc.
> 
> Conventionally, the first sentence javadoc comments end in "." (period). It makes it more natural to read.
> 
> The closing ")" in method calls looks odd to be on a line by itself.
> For example, GetAttrBase.java: line 35
> 
> Please fix these but no further review is needed.
> 
> Thanks, Roger
> 
> 
> 
> 
> On 7/24/18 2:12 AM, Chris Yin wrote:
>> Hi, Roger
>> 
>> Thanks a lot for your review and comments, inline and update new webrev as below, please have a review
>> 
>> http://cr.openjdk.java.net/~xyin/8198882/webrev.03/ <http://cr.openjdk.java.net/%7Exyin/8198882/webrev.03/>
>> 
>> 
>>> On 24 Jul 2018, at 1:05 AM, Roger Riggs <Roger.Riggs at Oracle.com <mailto:Roger.Riggs at Oracle.com>> wrote:
>>> 
>>> Hi Chris,
>>> 
>>> Sorry to be late to the review cycle.
>>> 
>>> The use of initializer blocks instead of constructors reduces the readability of the code.
>>> A simple fix is to add the constructor declarations ahead of the initializer to make the code more readable.
>> 
>> Fixed, use constructor now instead of initialization block, thanks
>> 
>>> 
>>> GetAttrsBase: 69: The signature of setMandatoryAttrs could be setMandatoryAttrs(String...) would allow
>>> the mandatory strings to be passed without explicit array creation and would still allow
>>> String[] to be passed where more convenient.
>> 
>> Changed with your suggested style
>> 
>>> 
>>> GetAttrsBase: 53: The exception message would be more readable as: "Attributes to be verified is null”
>> 
>> Sure, changed the exception msg as you suggested
>> 
>>> 
>>> GetAttrsNotFound: 50: throw the fail exception here; hiding it in the verifyAttributes
>>> method is harder to understand
>> 
>> Fixed, removed verifyAttributes override method and throw exception directly in runTest()
>> 
>>> 
>>> GetNonstandard: 87, 93: Printing the expected and actual arrays on failure (regardless of debug) will help diagnose failures more quickly
>> 
>> Sure, put expected and actual arrays in exception msg and simplify condition check 
>> 
>>> 
>>> GetNumericIRRs/GetNumericRRs: Tests like this are so abstract that is it difficult to see what
>>> is being tested. For example, where does getIndex() get its value?
>>> Either more comments are needed or more expressive method names.
>> 
>> Yep, let me try to expand the key test method into test directly, there will be some redundancy, but should be more clear to see what we want to test
>> 
>>> 
>>> DNSTestUtils: 94:  Don't encourage the usage allowing separation of -D from the argument.
>>> Its not allowed by the normal command line parsing and should not be supported.
>>> (Yes, I know it is/was not changed by this review).
>> 
>> Sure, removed this kind of usage support
>> 
>>> 
>>> :153:  cleanupClosableRes does not appear to be used;  if it is not needed, remove it
>> 
>> Yes, removed
>> 
>>> 
>>> A lot of the test framework is embodied in these DNSTestUtils without explanation or comment.
>>> It will help future maintenance, if there is a class level description (prose) describing
>>> the sequence of events for tests.
>> 
>> I add some javadoc to those DNSTestUtils, base sequence in TestBase class description, hope that will be helpful
>> 
>> Thanks & Regards,
>> Chris
>> 
>>> 
>>> Regards, Roger
>>> 
>>> 
>>> On 7/13/18 2:14 AM, Chris Yin wrote:
>>>> Hi, Vyom
>>>> 
>>>> Thank you for the review and comments, update webrev as below and comment inline
>>>> 
>>>> webrev: http://cr.openjdk.java.net/~xyin/8198882/webrev.02/ <http://cr.openjdk.java.net/%7Exyin/8198882/webrev.02/> <http://cr.openjdk.java.net/~xyin/8198882/webrev.02/ <http://cr.openjdk.java.net/%7Exyin/8198882/webrev.02/>>
>>>> 
>>>> 
>>>>> On 13 Jul 2018, at 1:46 PM, vyom tewari <vyom.tewari at oracle.com <mailto:vyom.tewari at oracle.com>> wrote:
>>>>> 
>>>>> Hi Chris,
>>>>> 
>>>>> Thanks for doing this overall looks good to me, few minor comments.
>>>>> 1->  DNSTestUtils.java, please start the server first and then set the "TEST_DNS_SERVER_THREAD". This will not make much difference but we will avoid setting "TEST_DNS_SERVER_THREAD" env variable if server fails to start.
>>>>> 129             env.put(TEST_DNS_SERVER_THREAD, inst);
>>>>>  130             inst.start();
>>>> Fixed, thanks
>>>> 
>>>>> 2->  I noticed that  copyright date (Copyright (c) 2000, 2018,) but webrev tells all the tests are new, please fix copyright date as well.
>>>> Thanks for checking this. Since this task is part of umbrella enhancement JDK-8191011 <https://bugs.openjdk.java.net/browse/JDK-8191011 <https://bugs.openjdk.java.net/browse/JDK-8191011>> JNDI SQE tests co-location, for those added tests which are migrated from SQE tests, the copyright date will follow the guidance SQE Test copyright year + migration copyright year if the 2 year are not same, for dump files (like *.dns) are new added under our new framework so just use current copyright year, hope that explains :), thanks
>>>> 
>>>> Regards,
>>>> Chris
>>>> 
>>>>> Thanks,
>>>>> Vyom
>>>>> 
>>>>> On Thursday 12 July 2018 02:08 PM, Chris Yin wrote:
>>>>>> Please have a review to new webrev as below, some code refactoring on lib parts and enhanced DNSServer to handle retry request which will make the tests more stable, thanks
>>>>>> 
>>>>>> http://cr.openjdk.java.net/~xyin/8198882/webrev.01/ <http://cr.openjdk.java.net/%7Exyin/8198882/webrev.01/> <http://cr.openjdk.java.net/%7Exyin/8198882/webrev.01/ <http://cr.openjdk.java.net/%7Exyin/8198882/webrev.01/>>
>>>>>> 
>>>>>> Regards,
>>>>>> Chris
>>>>>> 
>>>>>>> On 22 Mar 2018, at 11:16 AM, Chris Yin <xu.y.yin at oracle.com <mailto:xu.y.yin at oracle.com> <mailto:xu.y.yin at oracle.com <mailto:xu.y.yin at oracle.com>>> wrote:
>>>>>>> 
>>>>>>> Please review the changes to add 10 JNDI tests to com/sun/jndi/dns/AttributeTests/, thanks
>>>>>>> 
>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8198882 <https://bugs.openjdk.java.net/browse/JDK-8198882> <https://bugs.openjdk.java.net/browse/JDK-8198882 <https://bugs.openjdk.java.net/browse/JDK-8198882>>
>>>>>>> webrev: http://cr.openjdk.java.net/~xyin/8198882/webrev.00/ <http://cr.openjdk.java.net/%7Exyin/8198882/webrev.00/> <http://cr.openjdk.java.net/%7Exyin/8198882/webrev.00/ <http://cr.openjdk.java.net/%7Exyin/8198882/webrev.00/>>
>>>>>>> 
>>>>>>> Regards,
>>>>>>> Chris
>>> 
>> 
> 