One more thing :-) Remember to update your copyright year to 2015 also
Best Lance On Jan 6, 2015, at 5:04 PM, Lance Andersen <lance.ander...@oracle.com> wrote: > > 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 >>>> >>>> >>>> >>> >> > > <oracle_sig_logo.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
