Review request for JDK-8051559: JAXP function dom tests conversion
huizhe wang
huizhe.wang at oracle.com
Fri Apr 3 21:41:51 UTC 2015
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.wang at 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.wang at 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.wang at 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.andersen at oracle.com
> <mailto:lance.andersen at 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.Andersen at oracle.com
> <mailto:Lance.Andersen at oracle.com>
>
> Sent from my iPad
>
>
> On Mar 31, 2015, at 4:15 AM, Frank Yuan
> <frank.yuan at oracle.com <mailto:frank.yuan at oracle.com>>
> wrote:
>
> Hi Joe
>
>
>
> Do you have any comment for dom suite co-location?
>
>
>
> Best Regards
>
> Frank
>
>
>
> From: Frank Yuan [mailto:frank.yuan at oracle.com]
> Sent: Wednesday, March 25, 2015 5:46 PM
> To: 'huizhe wang'; 'Core-Libs-Dev'
> Cc: 'jibing chen'; 'Gustavo Galimberti';
> sandeep.konchady at oracle.com
> <mailto:sandeep.konchady at 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.Andersen at oracle.com <mailto:Lance.Andersen at oracle.com>
>
>
>
>
>
More information about the core-libs-dev
mailing list