Review request for JDK-8051710: Convert JAXP function tests: javax.xml.jaxp14.* to jtreg (testng) tests

[Lance Andersen]

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.yuan at 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.wang at oracle.com] 
> Sent: Wednesday, January 28, 2015 12:44 PM
> To: Frank Yuan
> Cc: 'Lance Andersen'; 'Core-Libs-Dev'; 'jibing chen'; 'Gustavo Galimberti';
> sandeep.konchady at 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.wang at oracle.com]
>> Sent: Wednesday, January 28, 2015 10:27 AM
>> To: Frank Yuan
>> Cc: 'Lance Andersen'; 'Core-Libs-Dev'; 'jibing chen'; 'Gustavo
> Galimberti';
>> sandeep.konchady at 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.Andersen at oracle.com