RFR: JDK-8051563: Convert JAXP function tests in xslt components: : org.apache.qetest.dtm package, org.apache.qetest.trax package to openjdk
Tristan Yan
tristan.yan at oracle.com
Fri Jan 9 04:50:34 UTC 2015
Thanks Joe and Lance. do you mind sponsor this for me.
Tristan
> On Jan 8, 2015, at 2:54 PM, huizhe wang <huizhe.wang at oracle.com> wrote:
>
> Thanks for the update. I think the webrev is ready for putback.
>
> Best,
> Joe
>
> On 1/7/2015 9:41 PM, Tristan Yan wrote:
>> Hi Joe/Lance and others.
>>
>> Please review updated version for this one.
>>
>> http://cr.openjdk.java.net/~tyan/8051563/webrev.01/ <http://cr.openjdk.java.net/%7Etyan/8051563/webrev.01/>
>>
>> Thank you
>> Tristan
>>
>>> On Jan 6, 2015, at 2:27 PM, huizhe wang <huizhe.wang at oracle.com <mailto:huizhe.wang at oracle.com>> wrote:
>>>
>>>
>>> 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> <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>
>>>>>
>>>>>
>>>>>
>>>>
>>>> <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>
>>>>
>>>>
>>>>
>>>
>>
>
More information about the core-libs-dev
mailing list