Review request for JDK-8051559: JAXP function dom tests conversion

Sat Apr 4 01:49:04 UTC 2015

On Apr 3, 2015, at 5:41 PM, huizhe wang <huizhe.wang at 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.

I may not get to this until Monday given it is Easter Weekend (and Joe I will get to your other review as well).  Just ran out of cycles today

Best
Lance
> 
> 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/
>> 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 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.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> 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.Andersen at oracle.com
>> Sent from my iPad
>> 
>> On Mar 31, 2015, at 4:15 AM, Frank Yuan <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;
>> '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.Andersen at oracle.com
>>  
>> 
>> 
>> 
>> 
>>  
>>  
>>  
>>  
> 

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