Thanks Joe and Lance. do you mind sponsor this for me.
Tristan
> On Jan 8, 2015, at 2:54 PM, huizhe wang <huizhe.w...@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.w...@oracle.com
>>> <mailto:huizhe.w...@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.ander...@oracle.com
>>>> <mailto:lance.ander...@oracle.com>> wrote:
>>>>
>>>>>
>>>>> On Jan 6, 2015, at 4:31 PM, Tristan Yan <tristan....@oracle.com
>>>>> <mailto:tristan....@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.w...@oracle.com
>>>>>>> <mailto:huizhe.w...@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....@oracle.com
>>>>>>>> <mailto:tristan....@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.w...@oracle.com
>>>>>>>>>> <mailto:huizhe.w...@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.ander...@oracle.com <mailto:lance.ander...@oracle.com>
>>>>>>>>>>> Sent from my iPad
>>>>>>>>>>>
>>>>>>>>>>> On Dec 30, 2014, at 5:28 PM, Tristan Yan <tristan....@oracle.com
>>>>>>>>>>> <mailto:tristan....@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.ander...@oracle.com <mailto:lance.ander...@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.ander...@oracle.com <mailto:lance.ander...@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.ander...@oracle.com <mailto:lance.ander...@oracle.com>
>>>>
>>>>
>>>>
>>>
>>
>
