Hi Chris,
Looks ok.
For future tests, I think I would put the contents of the initContext()
methods inline in the test method so the setup and the testing of the
conditions were in the same method, making it easier to read.
Thanks, Roger
On 8/6/2018 11:53 PM, Chris Yin wrote:
Hi, Roger
Thanks a lot for your review and comments, inline and updated webrev as below,
thanks
http://cr.openjdk.java.net/~xyin/8208279/webrev.03/
On 6 Aug 2018, at 10:58 PM, Roger Riggs <roger.ri...@oracle.com> wrote:
Hi Chris,
EnvTestBase.java: The class and each of the methods should have javadoc with a
descriptive comment
to make the test construction easier to understand for future maintenance
especially since
they are shared/overridden and used by many of the test cases.
For example, verifyEnvProperty should say what is true about a valid property.
With many layers of inherited test methods it needs easier to understand what
comes from where.
Build good habits with writing complete sentences with capitalization and
punctuation in comments.
Sure, I added javadoc to EnvTestBase class and shared/override methods (except
getter/setter), will also complement javadoc to test base class in other review
thread, thanks
initContext() belongs in DNSTestBase along with context() and setContext() to
avoid splitting that
function across multiple classes.
Or remove initContext() since it is overridden in several tests (without
calling super) and
just call setContext() with new InitialDirContext(env()) consistently in each
test.
Yep, I hesitated at first whether put initContext() to DNSTestBase, after look
through many dns test cases, I found there are a lot of tests may need to
override it and some case to use parameterized method will be easier (such as
initContext(String, String) used in Factory Tests), so I decided to leave it to
each sub test base or tests to customize. Anyway we could refactor it later if
we found that way better.
For now I removed initContext() from EnvTestBase as you suggested in 2nd and
used private initContext() for complex initialization or just call setContext
with new InitialDirContext(env()) for simple one, thanks
ProviderUrlGen.java should not mention SWAN by name as it is an internal
resource
(and that's not the current name).
Sure, removed that name
RemoveInherited.java: for strings that need to be consistent across the test,
they should be defined as
final statics so there is only one declaration.
Fixed as your suggested
Thanks & Regards,
Chris
Thanks, Roger
On 8/6/2018 3:09 AM, Chris Yin wrote:
Thank you, Vyom
Regards,
Chris
On 6 Aug 2018, at 2:02 PM, vyom tewari <vyom.tew...@oracle.com> wrote:
Hi Chris,
Latest webrev looks good to me.
Thanks,
Vyom
On Friday 03 August 2018 02:46 PM, Chris Yin wrote:
Hi, Vyom
Thank a lot for your review and comments, inline and update new webrev as below
http://cr.openjdk.java.net/~xyin/8208279/webrev.02/
On 3 Aug 2018, at 3:45 PM, vyom tewari <vyom.tew...@oracle.com> wrote:
Hi Chris,
Overall looks good to me, couple of minor comment.
1-> ProviderUrlGen.java, Line:100 Exception e can be replaced with specific
exceptions.
Fixed, thanks
2-> In couple of places we are calling "removeFromEnvironment" on context and spec says it will return
"null" if the env is not set. I see that we are setting the required env in "initContext" so it will never be
"null" in our tests, but i suggest you to put just one liner comment that "val" can not be null. This is just
to improve code readability, please feel free to ignore, if you think it is not worth enough.
Sure, added one line comment before each “removeFromEnvironment” call as you
suggested
Thanks,
Chris
Thanks,
Vyom
On Monday 30 July 2018 08:41 AM, Chris Yin wrote:
Please find the new webrev as below, it addressed some similar issues which
mentioned in review comments from another thread RFR 8200151, thanks
webrev: http://cr.openjdk.java.net/~xyin/8208279/webrev.01/
<http://cr.openjdk.java.net/~xyin/8208279/webrev.01/>
Regards,
Chris
On 26 Jul 2018, at 3:37 PM, Chris Yin <xu.y....@oracle.com> wrote:
Please review the changes to add another 8 JNDI tests to
com/sun/jndi/dns/EnvTests/ in OpenJDK, thanks
bug: https://bugs.openjdk.java.net/browse/JDK-8208279
<https://bugs.openjdk.java.net/browse/JDK-8208279>
webrev: http://cr.openjdk.java.net/~xyin/8208279/webrev.00/
<http://cr.openjdk.java.net/~xyin/8208279/webrev.00/>
Regards,
Chris
