[GitHub] [maven-invoker] slawekjaranowski commented on a diff in pull request #56: [MSHARED-1152] - Migrate tests to JUnit5

[GitBox <gi...@apache.org>]

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: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org