RFR: JDK-8051563: Convert JAXP function tests in xslt components: : org.apache.qetest.dtm package, org.apache.qetest.trax package to openjdk

huizhe wang huizhe.wang at oracle.com
Tue Jan 6 22:27:45 UTC 2015 

On 1/6/2015 2:25 PM, Lance Andersen wrote:
> One more thing :-)
>
> Remember to update your copyright year to 2015 also

Indeed, that applies to my webrevs as well :-)

Joe

>
> Best
> Lance
> On Jan 6, 2015, at 5:04 PM, Lance Andersen <lance.andersen at oracle.com 
> <mailto:lance.andersen at oracle.com>> wrote:
>
>>
>> On Jan 6, 2015, at 4:31 PM, Tristan Yan <tristan.yan at oracle.com 
>> <mailto:tristan.yan at oracle.com>> wrote:
>>
>>> Thank you Lance and Joe.
>>
>> You are very welcome.
>>> I am working on the fix.
>>
>> No rush from my perspective, have a lot to keep me busy in the 
>> interim before your next webrev . :-)
>>> it may take a couple of days that the code needs some refactor.
>> Understand,  as I have been working on tests for RowSets, I have 
>> continued to play the refactor dance myself.
>>
>> Best,
>> Lance
>>> I will send out next review once I finish it.
>>> Thanks
>>> Tristan
>>>
>>>> On Jan 6, 2015, at 1:22 PM, huizhe wang <huizhe.wang at oracle.com 
>>>> <mailto:huizhe.wang at oracle.com>> wrote:
>>>>
>>>> Thanks for taking the initiative and effort to refactor and clean 
>>>> up all of the Functional tests in previous changeset!
>>>>
>>>> We've gone through several iterations for the new ones (xslt 
>>>> tests). I totally agree with Lance, it looks very good overall, a 
>>>> lot better than its original form.
>>>>
>>>> It's nice to group all of the tests that required file access, that 
>>>> prepares all other tests to run with minimal permission for a 
>>>> (future) secure-test-target. Unit tests might need a similar 
>>>> treatment, but no pressure to do now :-)
>>>>
>>>> A minor comment on utility classes: there's JAXPTestUtilities and 
>>>> TestUtils. The former is an util for all tests while the later 
>>>> contained a couple of SAX handlers, it may make sense to move them 
>>>> into their own classes just as the other Handlers. The constants 
>>>> (XML_DIR and GOLDEN_DIR) then can be declared in a base class for 
>>>> the SAX tests.
>>>>
>>>> I understand each group of tests have their own XML source and 
>>>> golden files, thus XML_DIR and GOLDEN_DIR. It may be possible to 
>>>> add a base for each group while put test.src and test.classes into 
>>>> the very base class for all tests. So in general, we would have:
>>>>               TestBase    FileTestBase
>>>>               Base for each group (e.g. SAXTestBase, DOMTestBase, 
>>>> XSLTTestBase...) that extends either TestBase or FileTestBase
>>>>
>>>> I remember there were a few tests that required a http server. So 
>>>> then we may need a HttpTestBase as well.
>>>>
>>>> I know this is not what we've discussed (or planned) previously. 
>>>> But since you've done a great job to incorporate what were 
>>>> previously quite separate test suites into one whole test suite. I 
>>>> can't resist to ask a bit more. Don't get me wrong, what you've 
>>>> done exceeded my expectation in a big way (only a month ago, we 
>>>> were still talking about quick/straight conversion so that you can 
>>>> start to take care of the new features)!
>>>>
>>>> SAXParserTest: I noticed Old: testParse11, testParse27 --> New: 
>>>> testParse05.  So is testParse03 a new test? I can see SAXException 
>>>> is expected, but not IOE. In fact, this shows that the JAXP spec 
>>>> missed defining how empty string shall be handled (should have been 
>>>> an IAE).
>>>>
>>>> Best,
>>>> Joe
>>>>
>>>> On 1/6/2015 11:33 AM, Lance Andersen wrote:
>>>>> Hi Tristan,
>>>>>
>>>>> Sorry for the delay but with the holidays and the number of files, 
>>>>> it took a while to go through :-)
>>>>>
>>>>> Overall, it looks pretty good.
>>>>>
>>>>> A couple of suggestions, but I would not necessarily hold back 
>>>>> from committing:
>>>>>
>>>>> - For tests where you are not caring about the actual exception 
>>>>>  that is thrown to indicate a failure, such as 
>>>>> in DocumentBuilderFactory01.java and testCheckSchemaSupport1, I 
>>>>> would just have the method declaration  use "throws Exception" vs 
>>>>>  list all of the  possible individual Exceptions,  as it keeps it 
>>>>> more compact.  Glad to see you are not using failUnexcepted() now.
>>>>>
>>>>> - In test classes such as in testCheckSchemaSupport3. java 
>>>>> and DocumentBuilderImpl01.java, some tests do not use assertXXX or 
>>>>> expect an Exception.  Would be good at least to document what 
>>>>> could cause a failure or make it clear the expected behavior of 
>>>>> the test for passing.
>>>>>
>>>>> -SAXParserTest02.java and other tests where you get a 
>>>>> reader/parser such as testXMLReader01, I would at least assert 
>>>>> that null is not returned where as it is now, you only validate 
>>>>> that an exception is not returned
>>>>>
>>>>> - I know you are porting existing tests, but I would consider 
>>>>> consolidating tests as it seems like there is not a real reason to 
>>>>> have a testNG  class with just 1 test.  I would group the like 
>>>>> tests (such as SAXTFactoryTest ) in one testNG  test class as that 
>>>>> allows you to streamline further.
>>>>>
>>>>> - TfClearParamTest.java (as and example) minor nit that you have 
>>>>> your @Before/AfterGroups method in between tests.  I would suggest 
>>>>> grouping all methods such as this DataProviders before or after 
>>>>> the actual tests for better organization
>>>>>
>>>>> - TraxSAXWrapper.java, not sure I understand the "Sorry I could 
>>>>> not resist comment"
>>>>>
>>>>> - TransformerHandlerAPITest.java has typos in comments: 
>>>>> "IllegalArgumentExceptionis thrown…."
>>>>>
>>>>> - Minitest.java, I would add a comment for your Data Provider
>>>>>
>>>>> Best,
>>>>> Lance
>>>>>
>>>>> On Jan 2, 2015, at 1:49 PM, Tristan Yan <tristan.yan at oracle.com 
>>>>> <mailto:tristan.yan at oracle.com>> wrote:
>>>>>
>>>>>> Hi Joe and Lance
>>>>>> Sorry for my late reply. I just uploaded the new webrev which is 
>>>>>> trying to limit minimal permissions for most of tests. The new 
>>>>>> changeset has two base classes; class JAXPBaseTest has only 
>>>>>> minimal set permissions; class JAXPFileBaseTest adds two 
>>>>>> permissions that need access local files in the directory 
>>>>>> directory test.src and test.classes. Most of tests only 
>>>>>> inherit JAXPBaseTest that provides only minimal set permissions. 
>>>>>> Some tests will inherit JAXPFileBaseTest because tests need 
>>>>>> access local files.
>>>>>> Please help to review the code.
>>>>>> http://cr.openjdk.java.net/~tyan/8051563/webrev.00/ 
>>>>>> <http://cr.openjdk.java.net/%7Etyan/8051563/webrev.00/>
>>>>>>
>>>>>> Thank you
>>>>>> Tristan
>>>>>>
>>>>>>
>>>>>>> On Jan 2, 2015, at 10:39 AM, huizhe wang <huizhe.wang at oracle.com 
>>>>>>> <mailto:huizhe.wang at oracle.com>> wrote:
>>>>>>>
>>>>>>> Lance,
>>>>>>>
>>>>>>> Tristan is looking into adding an extension base class for about 
>>>>>>> 60 tests that require file permission, then the current base 
>>>>>>> class would indeed set "minimal" permission. So please wait for 
>>>>>>> his update :-)
>>>>>>>
>>>>>>> Best,
>>>>>>> Joe
>>>>>>>
>>>>>>> On 12/30/2014 3:07 PM, Lance @ Oracle wrote:
>>>>>>>> Hi Tristan,
>>>>>>>>
>>>>>>>> I will look at this but doubt I will get to this tomorrow
>>>>>>>>
>>>>>>>>
>>>>>>>> Best,
>>>>>>>> Lance
>>>>>>>>
>>>>>>>>
>>>>>>>> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance 
>>>>>>>> Andersen| Principal Member of Technical Staff | +1.781.442.2037 
>>>>>>>> <tel:+1.781.442.2037>
>>>>>>>> Oracle Java Engineering
>>>>>>>> 1 Network Drive <x-apple-data-detectors://34/0>
>>>>>>>> Burlington, MA 01803 <x-apple-data-detectors://34/0>
>>>>>>>> Lance.Andersen at oracle.com <mailto:Lance.Andersen at oracle.com>
>>>>>>>> Sent from my iPad
>>>>>>>>
>>>>>>>> On Dec 30, 2014, at 5:28 PM, Tristan Yan 
>>>>>>>> <tristan.yan at oracle.com <mailto:tristan.yan at oracle.com>> wrote:
>>>>>>>>
>>>>>>>>> Hi All
>>>>>>>>>
>>>>>>>>> Can I get your review on this change.
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~tyan/8051563/webrev.00/ 
>>>>>>>>> <http://cr.openjdk.java.net/%7Etyan/8051563/webrev.00/> 
>>>>>>>>> <http://cr.openjdk.java.net/~tyan/8051563/webrev.00/ 
>>>>>>>>> <http://cr.openjdk.java.net/%7Etyan/8051563/webrev.00/>>
>>>>>>>>>
>>>>>>>>> These fixes originally come from bug 
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8051563 
>>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8051563> as part of 
>>>>>>>>> our XML test colocation work. ThIS change-set mainly covers 
>>>>>>>>> tests for package org.apache.qetest.dtm and 
>>>>>>>>> org.apache.qetest.trax.
>>>>>>>>>
>>>>>>>>> In the meantime I took steps at fixing some of our existed 
>>>>>>>>> test code on below issues:
>>>>>>>>>
>>>>>>>>> 1. Add a base test class for all functional tests that enable 
>>>>>>>>> security manager running. A limited minimal permissions set 
>>>>>>>>> have been set for every test.
>>>>>>>>> 2. Remove all unnecessary exception capture for functional 
>>>>>>>>> tests that we’re using testng to handle all the exceptions.
>>>>>>>>> 3. Use try-resource block to solve all possible resource leaks 
>>>>>>>>> (including InputStream, OutputStream, Writer, Reader).
>>>>>>>>>
>>>>>>>>> Thanks a lot.
>>>>>>>>> Tristan
>>>>>>>
>>>>>>
>>>>>
>>>>> <Mail Attachment.gif> 
>>>>> <http://oracle.com/us/design/oracle-email-sig-198324.gif>
>>>>> <http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif>
>>>>> <http://oracle.com/us/design/oracle-email-sig-198324.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 <mailto:Lance.Andersen at oracle.com>
>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>> <oracle_sig_logo.gif> 
>> <http://oracle.com/us/design/oracle-email-sig-198324.gif>
>> <http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif>
>> <http://oracle.com/us/design/oracle-email-sig-198324.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 <mailto:Lance.Andersen at oracle.com>
>>
>>
>>
>
> <http://oracle.com/us/design/oracle-email-sig-198324.gif>
> <http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif>
> <http://oracle.com/us/design/oracle-email-sig-198324.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 <mailto:Lance.Andersen at oracle.com>
>
>
>

More information about the core-libs-dev mailing list