Hi Frank, Overall they look fine. Minor comments that you can address when you have time but should not stop you pushing your changes/additions
TransformTest - just have setup throw Exception as you really don't need the granularity because your are not doing anything with it XPathFactoryTest - Please be consistent as to whether you are using javadoc comments or not to introduce methods (i.e. clean up old or new tests) DataProvider invalid-parameterss - seems to be the same in multiple classes so I would move it to the base class and override when needed SchemaFactoryTest, ValidateHandlerTests, ValidatorTest - When time permits, I would add a brief comment for each test Best Lance On Jan 28, 2015, at 6:18 AM, Frank Yuan <frank.y...@oracle.com> wrote: > Hi Joe > > I have moved the jaxp14 tests to the corresponding packages, would you like > to have a check? http://cr.openjdk.java.net/~fyuan/8051710/webrev.01/ > > There is an issue, SchemaFactoryTest is merged to > validation.SchemaFactoryTest, but the validation package(Lance reviewed it) > is waiting to push. I am not sure how to handle for your convenience, so > this webrev also includes validation package. Maybe we can wait for pushing > validation, or push this patch for both jaxp14 and validation if you think > these code change is ok. > > Best Regards > Frank > > -----Original Message----- > From: huizhe wang [mailto:huizhe.w...@oracle.com] > Sent: Wednesday, January 28, 2015 12:44 PM > To: Frank Yuan > Cc: 'Lance Andersen'; 'Core-Libs-Dev'; 'jibing chen'; 'Gustavo Galimberti'; > sandeep.konch...@oracle.com; 'Alexandre (Shura) Iline' > Subject: Re: Review request for JDK-8051710: Convert JAXP function tests: > javax.xml.jaxp14.* to jtreg (testng) tests > > > On 1/27/2015 7:06 PM, Frank Yuan wrote: >> Thank you, Joe. >> >> I will sort the tests as your suggestion, and have 3 questions to > confirm >> with you: >> 1. Should I also sort the gap tests? >> 2. Should I put astro suite at side of auctionportal? > > Ah, in that case, you may put gap tests alongside the auctionportal as well. >> 3. If a package has very few test, e.g. I may put XMLEventFactoryTest > and >> something else in javax.xml.stream.ptest, is it ok? (I would rename >> XMLEventFactoryTest as its small coverage) > > Sounds good to me. > > Thanks, > Joe > >> >> Best Regards >> Frank >> >> >> -----Original Message----- >> From: huizhe wang [mailto:huizhe.w...@oracle.com] >> Sent: Wednesday, January 28, 2015 10:27 AM >> To: Frank Yuan >> Cc: 'Lance Andersen'; 'Core-Libs-Dev'; 'jibing chen'; 'Gustavo > Galimberti'; >> sandeep.konch...@oracle.com; 'Alexandre (Shura) Iline' >> Subject: Re: Review request for JDK-8051710: Convert JAXP function > tests: >> javax.xml.jaxp14.* to jtreg (testng) tests >> >> Hi Frank, >> >> Nice refactoring the original tests, esp. the TransformerTest! >> >> jaxp14 is legacy in the jaxp standalone world. While we are at this, >> you > may >> want to move these tests to their relevant packages since in the JDK > world, >> jaxp14 is no longer relevant (jaxp 1.4 was integrated into JDK 6). As >> you've already split FactoryTest into various Factory tests, you may > find >> them a bit thin in terms of test coverage now that they are named >> *FactoryTest since they cover just one of the two newInstance methods. >> I would think it makes sense to move them into / combine with the >> Factory tests of their own packages. >> >> Thanks, >> Joe >> >> On 1/27/2015 1:09 AM, Frank Yuan wrote: >>> Hi, Joe, Lance and All >>> >>> We are working on moving internal jaxp functional tests to open jdk >> repo. >>> This is the jaxp14 suite. Would you please review these test? Any >> comment >>> will be appreciated. >>> >>> bug: https://bugs.openjdk.java.net/browse/JDK-8051710 >>> webrev: http://cr.openjdk.java.net/~fyuan/8051710/webrev.00/ >>> >>> >>> Thanks, >>> >>> Frank >>> >> > > Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
