Hi Frank,
On Apr 1, 2015, at 6:46 AM, Frank Yuan <[email protected]> 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:[email protected]]
> 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 <[email protected]> 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:[email protected]]
> 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:[email protected]]
> 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 <[email protected]> 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
> [email protected]
>
>
>
>
>
>
>
>
> <image001.gif>
>
> Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
> Oracle Java Engineering
> 1 Network Drive
> Burlington, MA 01803
> [email protected]
>
>
>
Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
[email protected]
