Thank you Roger, I will fix them before push. Regards, Chris
> On 24 Jul 2018, at 10:12 PM, Roger Riggs <roger.ri...@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.ri...@oracle.com >>> <mailto:roger.ri...@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.tew...@oracle.com >>>>> <mailto:vyom.tew...@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....@oracle.com >>>>>>> <mailto:xu.y....@oracle.com> <mailto:xu.y....@oracle.com >>>>>>> <mailto:xu.y....@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 >>> >> >
