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.
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.
GetAttrsBase: 53: The exception message would be more readable as:
"Attributes to be verified is null"
GetAttrsNotFound: 50: throw the fail exception here; hiding it in the
verifyAttributes
method is harder to understand
GetNonstandard: 87, 93: Printing the expected and actual arrays on
failure (regardless of debug) will help diagnose failures more quickly
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.
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).
:153: cleanupClosableRes does not appear to be used; if it is not
needed, remove it
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.
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/~xyin/8198882/webrev.02/>
On 13 Jul 2018, at 1:46 PM, vyom tewari <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/>
Regards,
Chris
On 22 Mar 2018, at 11:16 AM, Chris Yin <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/>
Regards,
Chris
