Review request for JDK-8051709: Convert JAXP function tests: javax.xml.datatype to jtreg (testng) tests

[Lance Andersen]

Hi Frank

Overall, it is fine, a couple minor nits:

I thinking some of the comments in both class should be clearer as the comments can be confusing as to what the test should return
---------------
 332 
 333     /**
 334      * Test XMLGregorianCalendar#toString() Bug # 5049528:
 335      * XMLGregorianCalendar.toString throws IllegalStateException
 336      *
 337      */
 338     @Test(dataProvider = "calendar")
 339     public void checkToStringPos(final int year, final int month, final int day, final int hour, final int minute, final int second) {
 340         XMLGregorianCalendar calendar = datatypeFactory.newXMLGregorianCalendar(year, month, day, hour, minute, second, undef, undef);
 341         calendar.toString();
 342     }

-------------

This test is not expecting an IllegalStateException.  I would also probably validate that the value from toString() is valid

another example:

---------
 552     }
 553 
 554     /**
 555      * Test Duration#getXMLSchemaType() throws UnsupportedOperationException.
 556      *
 557      * <p>
 558      * Bug # 5049544: Duration.getXMLSchemaType throws
 559      * UnsupportedOperationException
 560      *
 561      */


--------

I would just review your comments to make them clearer.


The comment below has a typo:
----------
255     /**
 256      * Test XMLGregorianCalendar#toGregorianCalendar( TimeZone timezone, Locale
 257      * aLocale, XMLGregorianCalendar defaults)
 258      *
 259      * <p>
 260      * Bug # 5040542: toGregorianCalendar(...) does not use the defaults
 261      * XMLGregorianCalendar paramete
 262      *
 263      */

---------


no need for another review, I would just tweak the comments and push

Best
Lance

On Jan 28, 2015, at 5:56 AM, Frank Yuan <frank.yuan at oracle.com> wrote:

> Hi Lance
>  
> I have updated the code as your suggestions, would you like to review it again?http://cr.openjdk.java.net/~fyuan/8051709/webrev.01/
>  
> Best Regards
> Frank
>  
>  
> From: Lance Andersen [mailto:lance.andersen at oracle.com] 
> Sent: Wednesday, January 28, 2015 3:57 AM
> To: Frank Yuan
> Cc: 'huizhe wang'; 'Core-Libs-Dev'; 'Gustavo Galimberti'
> Subject: Re: Review request for JDK-8051709: Convert JAXP function tests: javax.xml.datatype to jtreg (testng) tests
>  
> Hi Frank,
>  
> On Jan 27, 2015, at 4:40 AM, Frank Yuan <frank.yuan at oracle.com> wrote:
> 
> 
> Thank you, Lance!
>  
> I applied some experience from your previous comments:)
>  
> I a glad they were useful :-)
> 
>  
> I have a question for your comment, could you check it below in line?
>  
> See below
> 
>  
> Best Regards
> Frank
>  
> From: Lance Andersen [mailto:lance.andersen at oracle.com] 
> Sent: Tuesday, January 27, 2015 3:24 AM
> To: Frank Yuan
> Cc: 'huizhe wang'; 'Core-Libs-Dev'; 'Gustavo Galimberti'
> Subject: Re: Review request for JDK-8051709: Convert JAXP function tests: javax.xml.datatype to jtreg (testng) tests
>  
> Hi Frank,
>  
> I think this looks good. 
>  
>  
> Not sure if you are going to add more tests in the future, but would be good to have tests such as
>  
> new Duration(x.toString()).equals(x)
>  
>  
> Perhaps a few more checks on expected toString() output….
>  
> //Frank: I will do it
>  
>  
> For XMLGregorianCalendarTest.java,  I would consider at some point adding more permutations of some of the tests that are validating a bugs(now that you are adding this as a new test suite to openjdk)
>  
> //Frank: I am not sure what you mean, which bug do you want me to add test for?
>  
> For example checkIsValid()
>  
> I would add a DataProvider and add more permutations to test so that you can reduce other potential errors..
>  
> Again, a nice to have for a next update.
>  
> The problem I always have when we add one off tests, it becomes very hard to manage your test suite and really understand the quality of your coverage.  Better to enhance existing tests to fill in weaknesses as this helps keep your test suite from getting out of control…
>  
>  
> 
> 
>  
> Best
> Lance
>  
> On Jan 26, 2015, at 1:42 AM, Frank Yuan <frank.yuan at oracle.com> wrote:
> 
> 
> 
> Hi, Joe and All
> 
> We are working on moving internal jaxp functional tests to open jdk repo.
> This is the datatype suite. Would you please review these test?  Any comment
> will be appreciated.
> 
> bug: https://bugs.openjdk.java.net/browse/JDK-8051709
> webrev: http://cr.openjdk.java.net/~fyuan/8051709/webrev.00/
> 
> 
> Thanks,
> 
> Frank
> 
>  
> <image001.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
>  
> 
> 
> 
>  
> <image001.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