Hi Chris,
looks fine now. +1
On 8/8/18 11:35 PM, Chris Yin wrote:
Hi, Roger
Many thanks for your review and comments, inline and updated new webrev as
below, thanks
http://cr.openjdk.java.net/~xyin/8208483/webrev.05/
On 8 Aug 2018, at 9:52 PM, Roger Riggs <roger.ri...@oracle.com> wrote:
Hi Chris,
Each of these tests is going to spawn a new process (/othervm). That's a lot of
overhead for a short test.
Thank you for checking this, actually used /othervm here is intentional to make
object factories working correctly, I’m not sure why yet, just guess jtreg
agentvm mode will affect object factory loader which caused object factory not
working. For example, if I removed /othervm from LookupWithAnyAttrProp.java
then run 'jtreg -agentvm -v:summary -testjdk:xxx -a
LookupWithAnyAttrProp.java’, test will failed to complain lookup returned
object is com.sun.jndi.dns.DnsContext instead of TestDnsObject, that was
unexpected. (I used latest jtreg version with jdk12, same result)
yes, /othervm is needed to avoid interference with other tests
Can these be written as TestNG tests so they run quicker and more tests be in a
single source file
and run in single VM?
That's a good idea, but per current tests situation, guess it’s hard, 2 points
as below
1. As above mentioned, for factory related tests, we have to specify /othervm
to make object factory working, that may not suitable for TestNG
/othervm can be used with testng
2. Each test along with a .dns dump file, even the test logic are very close,
the dumped messages maybe very different. If using TestNG 1 test file with
multiple tests, it will be more complex to manage/maintain those dump files
(like create/update dump file per test, search/load dump file when run each
test etc.)
As a thought experiment, and perhaps worth a prototype and especially if
there are more tests to write.
The testname/dns might need to be refactored out from the test class
name, but if I understand
the local server case, it creates a new local server for each test with
a new port and environment.
With no overlap between the test data, a clean separation between the
tests at runtime may be possible
with not too much work.
Thanks, Roger
@Override should be on its own line so the method signature is on a line by
itself too.
Setup your IDE to use the formatting conventions and let it reformat.
Sure, fixed, changed my IDE to use that style
LookupFactoryBase.java: 67 / 84 - Be consistent about the form of messages,
I prefer the Expected xxx but found yyy form. (no need to say it failed, it
threw an exception).
Fixed as you suggested form
The imports should be before the comment with the jtreg test description.
Fixed, thanks
Regards,
Chris
Thanks, Roger
On 8/7/18 11:41 PM, Chris Yin wrote:
Just one more minor revision to expand initContext() into test method for
easier read, new webrev as below, thanks
http://cr.openjdk.java.net/~xyin/8208483/webrev.04/
Regards,
Chris
On 7 Aug 2018, at 5:41 PM, Chris Yin <xu.y....@oracle.com> wrote:
Hi, Vyom
Thanks a lot for your comment, I didn’t realize it, sure, I moved this constant
to LookupFactoryBase as you suggested and also abstract the same verify logic
into base class as ‘verifyLookupObjectAndValue’, hope that looks better, update
new webrev as below, thanks
http://cr.openjdk.java.net/~xyin/8208483/webrev.03/
Regards,
Chris
On 7 Aug 2018, at 4:35 PM, vyom tewari <vyom.tew...@oracle.com> wrote:
Hi Chris,
Overall latest code looks good to me, one minor comment did you consider moving "private static final String
ATTRIBUTE_VALUE = "1.2.4.1";" in "LookupFactoryBase" ? as both LookupWithAnyAttrProp &
LookupWithAttrProp inherit "LookupFactoryBase".
.Thanks,
Vyom
On Monday 06 August 2018 03:02 PM, Chris Yin wrote:
Hi, Vyom
Many thanks for your review and comments, inline and updated webrev as below,
thanks
http://cr.openjdk.java.net/~xyin/8208483/webrev.01/
On 6 Aug 2018, at 4:12 PM, vyom tewari <vyom.tew...@oracle.com> wrote:
Hi Chris,
1-> DirAFactory.java, "getIbjectInstance" returns "null" if it fails to construct object. I will
suggest you to throw some "RuntimeException" instead returning null. If you return null then caller of
"getObjectInstance" had to check for null and it will end in lots of boilerplate code.
‘getObjectInstance’ methods are override from DirObjectFactory and
ObjectFactory, per my understanding on javadoc, return null should indicate
object cannot be created and allow other factories can be tried, feel free to
let me know if I misunderstand something on the api doc, thanks
Paste a few javadoc here against ‘getObjectInstance’ method as below
* @return The object created; null if an object cannot be created.
* @exception Exception If this object factory encountered an exception
* while attempting to create an object, and no other object factories are
* to be tried.
2-> DirTxtFactory.java same as "DirFactory.java”.
Same as above
3-> In LookupWithAnyAttrProp/LookupWithAttrProp java create a constant for
"1.2.4.1" and put one liner comment if possible. This is will help future
maintainer to understand why we are comparing with this value.
Sure, created constant for “1.2.4.1” and put comment to explain that it’s pre
defined attribute value of ‘A’, thanks
One generic comment, in most of the places i can see, you declared functions to throw
generic exception "Exception", please change it to specific exception or list
of specific exceptions if possible.
Fixed, those generic exception are generated automatically through override
method, I removed those unused throws Exception from xxxFactory.
Thanks & Regards,
Chris
Thanks,
Vyom
On Monday 30 July 2018 02:08 PM, Chris Yin wrote:
Please review the changes to add 5 JNDI tests to com/sun/jndi/dns/FactoryTests/
in OpenJDK, thanks
bug: https://bugs.openjdk.java.net/browse/JDK-8208483
<https://bugs.openjdk.java.net/browse/JDK-8208483>
webrev: http://cr.openjdk.java.net/~xyin/8208483/webrev.00/
<http://cr.openjdk.java.net/~xyin/8208483/webrev.00/>
Regards,
Chris
