Thanks, I will update it.
Tristan
> On Jan 6, 2015, at 2:27 PM, huizhe wang <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>
>>
>>
>>
>
