> On 17 Jan 2017, at 15:33, Andrey Nazarov <andrey.x.naza...@oracle.com> wrote: > > Thanks for review. I’ve update patch > http://cr.openjdk.java.net/~anazarov/8071566/webrev.01/
I think this is ok. -Chris. > > —Andrey >> On 3 Jan 2017, at 14:25, Chris Hegarty <chris.hega...@oracle.com> wrote: >> >> 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. >
