RFR 8198882: Add 10 JNDI tests to com/sun/jndi/dns/AttributeTests/
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/~xyin/8198882/webrev.00/> Regards, Chris
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/~xyin/8198882/webrev.01/> Regards, Chris
On 22 Mar 2018, at 11:16 AM, Chris Yin <xu.y.yin@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/~xyin/8198882/webrev.00/>
Regards, Chris
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(); 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, 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.yin@oracle.com <mailto:xu.y.yin@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 webrev: http://cr.openjdk.java.net/~xyin/8198882/webrev.00/ <http://cr.openjdk.java.net/%7Exyin/8198882/webrev.00/>
Regards, Chris
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.tewari@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.yin@oracle.com <mailto:xu.y.yin@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
Hi Chris, latest webrev looks good to me, thanks for explanation about copyright date. Thanks, Vyom On Friday 13 July 2018 11:44 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/>
On 13 Jul 2018, at 1:46 PM, vyom tewari <vyom.tewari@oracle.com <mailto:vyom.tewari@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.yin@oracle.com <mailto:xu.y.yin@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 webrev: http://cr.openjdk.java.net/~xyin/8198882/webrev.00/ <http://cr.openjdk.java.net/%7Exyin/8198882/webrev.00/>
Regards, Chris
Thank you, Vyom Regards, Chris
On 13 Jul 2018, at 5:15 PM, vyom tewari <vyom.tewari@oracle.com> wrote:
Hi Chris,
latest webrev looks good to me, thanks for explanation about copyright date.
Thanks,
Vyom
On Friday 13 July 2018 11:44 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/>
On 13 Jul 2018, at 1:46 PM, vyom tewari <vyom.tewari@oracle.com <mailto:vyom.tewari@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.yin@oracle.com <mailto:xu.y.yin@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
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.tewari@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.yin@oracle.com <mailto:xu.y.yin@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
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/~xyin/8198882/webrev.03/>
On 24 Jul 2018, at 1:05 AM, Roger Riggs <Roger.Riggs@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/~xyin/8198882/webrev.02/>
On 13 Jul 2018, at 1:46 PM, vyom tewari <vyom.tewari@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.yin@oracle.com <mailto:xu.y.yin@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
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@Oracle.com <mailto:Roger.Riggs@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@oracle.com <mailto:vyom.tewari@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.yin@oracle.com <mailto:xu.y.yin@oracle.com> <mailto:xu.y.yin@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
Thank you Roger, I will fix them before push. Regards, Chris
On 24 Jul 2018, at 10:12 PM, Roger Riggs <Roger.Riggs@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@Oracle.com <mailto:Roger.Riggs@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@oracle.com <mailto:vyom.tewari@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@oracle.com <mailto:xu.y.yin@oracle.com> <mailto:xu.y.yin@oracle.com <mailto:xu.y.yin@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
participants (3)
-
Chris Yin
-
Roger Riggs
-
vyom tewari