Thank you Lance and Joe. I am working on the fix. it may take a couple of days that the code needs some refactor. 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> 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> >> >> >> >
