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/~tyan/8051563/webrev.01/> Thank you Tristan > On Jan 6, 2015, at 2:27 PM, huizhe wang <[email protected] > <mailto:[email protected]>> 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 <[email protected] >> <mailto:[email protected]>> wrote: >> >>> >>> On Jan 6, 2015, at 4:31 PM, Tristan Yan <[email protected] >>> <mailto:[email protected]>> 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 <[email protected] >>>>> <mailto:[email protected]>> 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 <[email protected] >>>>>> <mailto:[email protected]>> 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 <[email protected] >>>>>>>> <mailto:[email protected]>> 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> >>>>>>>>> [email protected] <mailto:[email protected]> >>>>>>>>> Sent from my iPad >>>>>>>>> >>>>>>>>> On Dec 30, 2014, at 5:28 PM, Tristan Yan <[email protected] >>>>>>>>> <mailto:[email protected]>> 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 >>>>>> [email protected] <mailto:[email protected]> >>>>>> >>>>>> >>>>>> >>>>> >>>> >>> >>> <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 >>> [email protected] <mailto:[email protected]> >>> >>> >>> >> >> <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 >> [email protected] <mailto:[email protected]> >> >> >> >
