slawekjaranowski commented on code in PR #56: URL: https://github.com/apache/maven-invoker/pull/56#discussion_r997721215
########## src/test/java/org/apache/maven/shared/invoker/MavenCommandLineBuilderTest.java: ########## @@ -19,57 +19,46 @@ * under the License. */ +import org.apache.maven.shared.utils.Os; +import org.apache.maven.shared.utils.cli.Commandline; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + import java.io.File; import java.io.FileWriter; import java.io.IOException; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collections; -import java.util.HashSet; -import java.util.List; -import java.util.Properties; -import java.util.Set; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.*; Review Comment: Incorrect order. ########## src/test/java/org/apache/maven/shared/invoker/MavenCommandLineBuilderTest.java: ########## @@ -19,57 +19,46 @@ * under the License. */ +import org.apache.maven.shared.utils.Os; +import org.apache.maven.shared.utils.cli.Commandline; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + import java.io.File; import java.io.FileWriter; import java.io.IOException; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collections; -import java.util.HashSet; -import java.util.List; -import java.util.Properties; -import java.util.Set; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.*; -import org.apache.maven.shared.utils.Os; -import org.apache.maven.shared.utils.cli.Commandline; -import org.junit.After; -import org.junit.Before; -import org.junit.Rule; -import org.junit.Test; -import org.junit.rules.TemporaryFolder; - -import static org.hamcrest.CoreMatchers.is; -import static org.hamcrest.CoreMatchers.notNullValue; -import static org.junit.Assert.assertArrayEquals; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; -import static org.junit.Assume.assumeThat; +import static org.junit.jupiter.api.Assertions.*; Review Comment: Don't use wildcard imports. ########## src/test/java/org/apache/maven/shared/invoker/DefaultInvokerTest.java: ########## @@ -19,20 +19,19 @@ * under the License. */ +import org.apache.maven.shared.utils.Os; +import org.apache.maven.shared.utils.StringUtils; +import org.junit.jupiter.api.Test; + import java.io.File; -import java.io.IOException; import java.net.URI; import java.net.URISyntaxException; import java.net.URL; import java.util.Arrays; import java.util.Collections; import java.util.Properties; -import org.apache.maven.shared.utils.Os; -import org.apache.maven.shared.utils.StringUtils; -import org.junit.Test; - -import static org.junit.Assert.assertEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; Review Comment: Please follow: https://maven.apache.org/developers/conventions/code.html#java-code-convention-import-layouts ########## src/test/java/org/apache/maven/shared/invoker/MavenCommandLineBuilderTest.java: ########## @@ -109,7 +98,7 @@ public void testShouldFailToSetLocalRepoLocationFromRequestWhenItIsAFile() @Test public void testShouldSetLocalRepoLocationGlobally() throws IOException { - File lrd = temporaryFolder.newFolder( "workdir" ).getCanonicalFile(); + File lrd = Files.createDirectory(temporaryFolder.resolve("workdir")).toFile().getCanonicalFile(); Review Comment: Please follow: https://maven.apache.org/developers/conventions/code.html in whole file. Should be: ```suggestion File lrd = Files.createDirectory( temporaryFolder.resolve( "workdir" ) ).toFile().getCanonicalFile(); ``` missing spaces ########## src/test/java/org/apache/maven/shared/invoker/MavenCommandLineBuilderTest.java: ########## @@ -857,46 +846,46 @@ public void testShouldInsertActivatedProfiles() public void testMvnExecutableFromInvoker() throws Exception { - assumeThat( "Test only works when maven.home is set", - System.getProperty( "maven.home" ), is(notNullValue())); + assumingThat( Objects.nonNull(System.getProperty( "maven.home" )), + () -> System.out.println("Test only works when maven.home is set")); Review Comment: `assumingThat` doesn't break a test ... https://junit.org/junit5/docs/current/api/org.junit.jupiter.api/org/junit/jupiter/api/Assumptions.html#assumingThat(boolean,org.junit.jupiter.api.function.Executable) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
