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.y...@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.ander...@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.y...@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.ander...@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.y...@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.ander...@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.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
