Hi Frank On Apr 3, 2015, at 5:41 PM, huizhe wang <huizhe.w...@oracle.com> wrote:
> > 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. As I mentioned earlier, I do not have an issue with you pushing your tests but looking at the webrev.02 a couple of minor points of feedback which you can choose to ignore. There are a few tests in AbstractCharacterDataTest.java, AttrTest.java, DocumentTest.java, DocumentTypeTest.java, DomImplementationTest.java, NotationTest.java that could use a comment still Same comment as previously about the use of user.dir in NodeTest.java, really not necessary but no harm Again, feel free to ignore Best Lance > > 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/ >> 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 DomExcpTest as 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/ >> >> 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> >> 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 >> >> >> Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 >> Oracle Java Engineering >> 1 Network Drive >> Burlington, MA 01803 >> lance.ander...@oracle.com >> Sent from my iPad >> >> On Mar 31, 2015, at 4:15 AM, Frank Yuan <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; >> '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/ >> >> >> >> Thanks, >> >> >> >> Frank >> >> >> <Mail Attachment.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 >> >> >> >> >> >> >> >> >> > Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
