[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-core-libs-dev
Subject: Re: Review request for JDK-8051709: Convert JAXP function tests: javax.xml.datatype to jtreg (testng
From: Lance Andersen <lance.andersen () oracle ! com>
Date: 2015-01-30 21:25:55
Message-ID: F6525BFE-FF72-4639-97AA-FC705BFC6FBE () oracle ! com
[Download RAW message or body]
Hi Frank
Looks fine
best
lance
On Jan 30, 2015, at 12:59 AM, Frank Yuan <frank.yuan@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@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@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@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@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@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@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@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@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@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@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@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@oracle.com
>
>
>
Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
Lance.Andersen@oracle.com
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic