On Jan 6, 2015, at 4:31 PM, Tristan Yan <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> 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> 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/ >>>> >>>> Thank you >>>> Tristan >>>> >>>> >>>>> On Jan 2, 2015, at 10:39 AM, huizhe wang <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 >>>>>> >>>>>> >>>>>> Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 >>>>>> Oracle Java Engineering >>>>>> 1 Network Drive >>>>>> Burlington, MA 01803 >>>>>> lance.ander...@oracle.com >>>>>> Sent from my iPad >>>>>> >>>>>> On Dec 30, 2014, at 5:28 PM, Tristan Yan <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/~tyan/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> >>> >>> Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 >>> Oracle Java Engineering >>> 1 Network Drive >>> Burlington, MA 01803 >>> lance.ander...@oracle.com >>> >>> >>> >> > Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
