Review request for JDK-8051560: JAXP function astro tests conversion

Tue Mar 31 13:45:01 UTC 2015

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.yuan at 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.wang at 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.wang at 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.yuan at 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.Andersen at oracle.com
>  
> 
> 
> 
>  
>  

Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
Lance.Andersen at oracle.com