Hi Frank, On Apr 1, 2015, at 6:46 AM, Frank Yuan <frank.y...@oracle.com> wrote:
> Hi Lance > > Thank you for your comments! > > I have already added/updated the comments as your suggestion. I see them in the webrev below but were not in webrev.01 which is what I looked at yesterday > For USER_DIR, I am not sure if I understood your question, USER_DIR is a > public member in JAXPTestUtilities, it's shared by all jaxp functional > co-location tests, and assigned with System.getProperty("user.dir"), jtreg > affords this property, but may be different dir under different mode. sorry if this was not clear. jtreg has a concept of a working directory, which I believe jtreg sets user.dir to. Because of that, there really is not a reason to have to specify user.dir again in your tests. jtreg makes sure that the working directory gets cleaned up after a test run Best Lance > > The new webrev is at: http://cr.openjdk.java.net/~fyuan/8051560/webrev.02/ > > Best Regards > Frank > > From: Lance Andersen [mailto:lance.ander...@oracle.com] > Sent: Tuesday, March 31, 2015 9:45 PM > To: Frank Yuan > Cc: 'huizhe wang'; 'Core-Libs-Dev'; 'Gustavo Galimberti' > Subject: Re: Review request for JDK-8051560: JAXP function astro tests > conversion > > Hi Frank, > > Overall, its OK. > > A couple of suggestions but they should not prevent you from pushing your > changes: > > I still think you should consider adding a basic comment to tests which are > missing one such as in DocumentLSTest.java & NamespaceContextTest.java. > > In AstroTest.java, you probably do not need the USER.DIR field as jtreg and > just reference the file/directory. > > Best > Lance > On Mar 30, 2015, at 11:14 PM, Frank Yuan <frank.y...@oracle.com> wrote: > > > Hi Joe > > I am glad to explain your questions! > > For test number, the original AstroApp has 10 test, XPath API, SAX 2.0.1, > Schema Validation, Namespace, DocumentLS and Astro Test mode 1~5, one test > method is reported as one test case, so there are 39. Current astro test > suite has 6 tests, besides the first 5, AstroTest includes mode1 ~5. So they > are same in deed. There are totally 371 test, 365 are already in repo, the 6 > are astro. And 70 of them are functional test, because functional and unit > test use separate TEST.properties files, you see the functional number at > last, the unit number is scrolled up on the screen. > > For test report, current process means original query, one original test mode > connects one factories permutation. I also noticed it's not easy to identify > the test mode/factories permutation, so I added > "System.out.println(fFactClass.getName() +" : " + isFactClass.getName());" > after I sent last webrev to you, you can check it now still at > http://cr.openjdk.java.net/~fyuan/8051560/webrev.01/ > > I am not sure which is other insight you mentioned, would you like to explain > more? > > Best Regareds > Frank > > From: huizhe wang [mailto:huizhe.w...@oracle.com] > Sent: Tuesday, March 31, 2015 10:14 AM > To: Frank Yuan > Cc: 'Lance Andersen'; 'Core-Libs-Dev'; 'Gustavo Galimberti' > Subject: Re: Review request for JDK-8051560: JAXP function astro tests > conversion > > Hi Frank, > > Looks good. > > Since you've gone through the original test/the astro application, and > refactored it into a TestNG test, you probably have other insight too. If so, > it would be nice to add them as well. > > In terms of report, yes, I can see the output files displayed along with > process id. Thanks for doing that. Not as good as the old test report, which > lists test mode/query ids with status, but acceptable since it prints out the > process and query ids that helps identifying the exact test. > > I have always wondered, but thought not so important, that in its current > (TestNG) form, the test report will show test cases (thus fewer tests than > the original test suite). For example, the AstroApp reports 10 tests and 39 > test cases, while the new 5. Console display is quite different from the html > report as well. For example, on the console, the test results are: passed: > 70, but the html report shows 371 tests passed. What are your thoughts on > this? > > Thanks, > Joe > > On 3/27/2015 2:43 AM, Frank Yuan wrote: > Hi Joe and Lance > > Thank you very much for your review! It's very good comment! > > According to your comments, I made the following changes: > 1. Rename the output file name to be easier understanding. > 2. Print the output file name to be easier debugging. > 3. Add comment for AstroProcessor class > 4. Add comment in AstroTest to describe the test, some are from astro > application design document. > > Besides these new adding comments, there are many comments in astro > classes(in libs/test/astro), which are from the original source files. And > the current tests prints information to the standard out, which is redirected > to .jtr file by jtreg, so there is not any other trace file now. USER_DIR is > the scratch dir in deed, it is property user.dir in jtreg. The new webrev is > at http://cr.openjdk.java.net/~fyuan/8051560/webrev.01/ > > Btw, I also sent out review request for JDK-8051559: JAXP function dom tests > minutes later than this review request, I am afraid you may miss it, reminder > here:) > > Best Regards > Frank > > From: huizhe wang [mailto:huizhe.w...@oracle.com] > Sent: Friday, March 27, 2015 9:02 AM > To: Lance Andersen > Cc: Frank Yuan; 'Core-Libs-Dev'; 'Gustavo Galimberti' > Subject: Re: Review request for JDK-8051560: JAXP function astro tests > conversion > > I second Lance. The Main of the original astro had Javadocs and developer > comments. Probably more important is that you've completely changed the main > classes (TestDriver and Main), which looks good, however, the original > classes contained a lot of information on what each test does and how it > works that seem to have all been lost. > > The suite's README and build.xml may also contain information that is worth > keeping. Some of them may be no longer valid, in which case, it may be > helpful to describe the change. For example, the log files described in > README were useful for debugging. It would be good to explain where to find > the debug info in your new design. > > While scanning the test, I see that you are creating temporary output files > under USER_DIR. Is that intended? JAXP processors do not necessarily open > them with the DELETE_ON_CLOSE option. I thought in previous tests, you were > creating them in the scratch directory. > > Thanks, > Joe > > On 3/26/2015 12:08 PM, Lance Andersen wrote: > Hi Frank, > > Overall these look fine. I would suggest adding a simple comment to describe > the tests that do not have one to give a basic intent of the test to make it > easier for someone to understand if they are new. > > Best > Lance > On Mar 25, 2015, at 5:34 AM, Frank Yuan <frank.y...@oracle.com> wrote: > > > > > Hi, Joe and All > > > > We are working on moving internal jaxp functional tests to open jdk repo. > > This is the astro suite. Would you please review these test? Any comment > will be appreciated. > > > > bug: https://bugs.openjdk.java.net/browse/JDK-8051560 > > webrev: http://cr.openjdk.java.net/~fyuan/8051560/webrev.00/ > > > > AstroTest is the primary test in this suite, it transforms an xml file(which > includes astro data) with several xsl files, sets different filtering > condition by these xsl files and different filtering range, finally compares > the result with golden files. > > And there are 5 permutations of InputSourceFactory and FilterFactory(I uses > template method pattern for the variant FilterFactoryImpls), each > permutation will be applied to above transforming processes. > > > > Thanks, > > > > Frank > > > <image001.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 > > > > > > > > > <image001.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