Andrey, > On 30 Dec 2016, at 14:11, Andrey Nazarov <andrey.x.naza...@oracle.com> wrote: > > Hi, > > Following tests for Jar tool were added: > > Tests for API validator. Jar tool should detect API changes between releases. > Test for custom manifest file. > Test for version format in --release option > > Also refactoring of existing tests was made: > 1. Common base class “MRTestBase.java” was extracted. > 2. Result and process builders were substituted by existing library classes > jdk/test/lib/process/* > > Please review. > > Review: http://cr.openjdk.java.net/~anazarov/8071566/webrev.00/ > Issue: https://bugs.openjdk.java.net/browse/JDK-8071566 > <https://bugs.openjdk.java.net/browse/JDK-8071566>
I think the changes are generally ok. A few comments: 1) The webrev shows ApiValidatorTest.java (was test/tools/jar/multiRelease/Basic.java). I assume ApiValidatorTest.java is just a new file, and not Basic.java copied. These files should have no relationship in mercurial. 2) Style. The existing style is a little confused to me. Please check line length, <= 80 chars where possible Align dots ( fluid method invocations ) where possible Align method arguments where possible, and makes sense 3) Where deleting files/directories, please use FileUtils from the test library. -Chris.