On 4/2/2015 10:11 PM, Frank Yuan wrote:
Yes, understand, thank you very much! I will follow this rule you
talked in future!
Well, since current code is ok for you, would you like to be my
sponsor to push the code?
Will do once Lance finishes the review.
Best,
Joe
Best Regards
Frank
*From:*huizhe wang [mailto:huizhe.w...@oracle.com]
*Sent:* Friday, April 03, 2015 12:16 PM
*To:* Frank Yuan
*Cc:* 'Lance Andersen'; 'Core-Libs-Dev'; 'Gustavo Galimberti'
*Subject:* Re: Review request for JDK-8051559: JAXP function dom tests
conversion
Hi Frank,
Looks good. With regard to a test being too simple or
straightforward, it can be subjective. I mean, what are easy to the
author/you may not for others who don't have the knowledge on the
particular subject matter. So it's always good to leave comments when
you are there. It's also helpful for the author/you to remember what's
done and why should you revisit them.
Cheers,
Joe
On 4/2/2015 8:30 PM, Frank Yuan wrote:
Hi Joe
Thank you for your comments, I have updated at
http://cr.openjdk.java.net/~fyuan/8051559/webrev.02/
<http://cr.openjdk.java.net/%7Efyuan/8051559/webrev.02/>
Please also check my comments in line below
Best Regards
Frank
*From:*huizhe wang [mailto:huizhe.w...@oracle.com]
*Sent:* Friday, April 03, 2015 10:02 AM
*To:* Frank Yuan
*Cc:* 'Lance Andersen'; 'Core-Libs-Dev'; 'Gustavo Galimberti'
*Subject:* Re: Review request for JDK-8051559: JAXP function dom
tests conversion
DocumentTest->testCreateElementNeg, the conversion seems to be
incorrect. The original test, as the Javadoc stated, "This checks
for the constuctor of DOMException ". The new test however, only
verifies that an Exception is thrown.
//I moved a part of original DomExcpTestas testCreateElementNeg
into DocumentTest, and write some code for DOMException, but as
you known, class DOMException is so simple, JCK test has covered
what I/original DomExcpTest test, Shura suggested me to remove
this test from ours, then I did as his comment.
Comments for tests such as testCreateAttributeNSNeg can be more
descriptive, for example, "Test for createAttributeNS method:
verifies that DOMException is thrown if reserved prefixes are used
with an arbitrary namespace name."
//Modified the comments to be more descriptive as your example on
the similar cases.
The original tests that made up DocumentTypeTest had comments that
you may want to copy over or rewrite as you did other tests.
//Because I thought this test is so straightforward, I didn't add
comment for it. However, I added comments as your suggestion.
The comment for EntityChildTest can be "Test DOM Parser: parsing
an xml file that contains external entities."
//changed the comment
NodeListTest: @summary Verifies a bug found in jaxp1.0.1 and
1.1FCS. After going out of bound, the last element of a NodeList
returns null. The bug has been fixed in jaxp 1.1.1 build.
//changed the comment
Thanks,
Joe
On 4/1/2015 4:01 AM, Frank Yuan wrote:
Hi Joe and Lance
Many thanks for your review and comments!
I have added the comments to the tests as your suggestions
except some test method is very simple, the method name/code
can present all information, e.g. method testHasFeature in
DomImplementationTest.java. Please check my update in
http://cr.openjdk.java.net/~fyuan/8051559/webrev.01/
<http://cr.openjdk.java.net/%7Efyuan/8051559/webrev.01/>
As well, I understand your suggestion about the code comment,
I will think about how to add the comment on the viewpoint of
the other code reader in my future work, thank you!
Best Regards
Frank
*From:*huizhe wang [mailto:huizhe.w...@oracle.com]
*Sent:* Wednesday, April 01, 2015 1:14 PM
*To:* Lance Andersen
*Cc:* Frank Yuan; Core-Libs-Dev; Gustavo Galimberti
*Subject:* Re: Review request for JDK-8051559: JAXP function
dom tests conversion
Hi Frank,
I did see the request, just didn't have time to look at it.
I again agree with Lance, that these tests were written over
10 years ago, it would be valuable to write down whatever
understanding you gained while converting the tests, same as
the Astro application/test, the goal of each test and how it
works would all be helpful. Basically, it would be nice to add
some comment on @Test.
I tried the tests. They worked fine with my current build
(with some changes).
Thanks,
Joe
On 3/31/2015 7:10 AM, Lance Andersen wrote:
Hi Frank
On Mar 31, 2015, at 7:24 AM, Lance @ Oracle
<lance.ander...@oracle.com
<mailto:lance.ander...@oracle.com>> wrote:
Hi frank
Can you forward the other review request as I think I
thought they were the same and deleted it
Ignore this comment, the subjects were too similar but
this is what needed reviewed.
I will look at this again today
The tests overall look fine.
I still have the same comment WRT providing a simple
comment describing each test. The key point to remember
is we want to make it easier for someone to look at the
test, understand what you are trying to validate, and
understand the coverage of the tests. This will help
future maintainers of the code. Comments are just as
important in test code as it is in implementation IMHO.
Best
Lance
Best
Lance
<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance
Andersen| Principal Member of Technical Staff |
+1.781.442.2037 <tel:+1.781.442.2037>
Oracle Java Engineering
1 Network Drive <x-apple-data-detectors://34/0>
Burlington, MA 01803 <x-apple-data-detectors://34/0>
lance.ander...@oracle.com
<mailto:lance.ander...@oracle.com>
Sent from my iPad
On Mar 31, 2015, at 4:15 AM, Frank Yuan
<frank.y...@oracle.com <mailto:frank.y...@oracle.com>>
wrote:
Hi Joe
Do you have any comment for dom suite co-location?
Best Regards
Frank
From: Frank Yuan [mailto:frank.y...@oracle.com]
Sent: Wednesday, March 25, 2015 5:46 PM
To: 'huizhe wang'; 'Core-Libs-Dev'
Cc: 'jibing chen'; 'Gustavo Galimberti';
sandeep.konch...@oracle.com
<mailto:sandeep.konch...@oracle.com>;
'Alexandre (Shura) Iline'
Subject: RE: Review request for JDK-8051559: JAXP
function dom tests
conversion
Hi, Joe and All
We are working on moving internal jaxp functional
tests to open jdk repo.
This is the dom suite. Would you please review
these test? Any comment will
be appreciated.
bug: https://bugs.openjdk.java.net/browse/JDK-8051559
webrev:
http://cr.openjdk.java.net/~fyuan/8051559/webrev.00/
<http://cr.openjdk.java.net/%7Efyuan/8051559/webrev.00/>
Thanks,
Frank
<http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance
Andersen| Principal Member of Technical Staff |
+1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>
