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> 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/>
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>> 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>
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/>
Regards,
Chris
