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
Thu Jan 8 22:54:58 UTC 2015
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> 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