Review request for JDK-8051709: Convert JAXP function tests: javax.xml.datatype to jtreg (testng) tests
Lance Andersen
lance.andersen at oracle.com
Fri Jan 30 21:25:55 UTC 2015
Hi Frank
Looks fine
best
lance
On Jan 30, 2015, at 12:59 AM, Frank Yuan <frank.yuan at oracle.com> wrote:
> Hi Lance
>
> Changed the comment to '/*', could you have a check? http://cr.openjdk.java.net/~fyuan/8051709/webrev.03/
>
> Best Regards
> Frank
>
> From: Lance Andersen [mailto:lance.andersen at oracle.com]
> Sent: Thursday, January 29, 2015 10:22 PM
> 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 see you decided to use documentation comments (/** vs /*) I would use '/*' otherwise you need to at missing javadoc tags (param, exception) to quiet IDEs and -Xdoclint
>
> The update comments are OK
>
> Best
> Lance
> On Jan 29, 2015, at 3:37 AM, Frank Yuan <frank.yuan at oracle.com> wrote:
>
>
> Hi Lance
>
> I have corrected these comments, would you like to have a look?http://cr.openjdk.java.net/~fyuan/8051709/webrev.02/
>
> Best Regards
> Frank
>
> From: Lance Andersen [mailto:lance.andersen at oracle.com]
> Sent: Thursday, January 29, 2015 4:55 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
>
> 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
>
>
>
>
>
>
> <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
More information about the core-libs-dev
mailing list