Hi Frank Looks fine
best lance On Jan 30, 2015, at 12:59 AM, Frank Yuan <frank.y...@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.ander...@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.y...@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.ander...@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.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 > > > > > > > <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
