[GitHub] [maven-verifier] michael-o commented on a diff in pull request #31: [MSHARED-1089] Upgrade maven-verifier to JDK 8 / Junit 5

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

michael-o commented on code in PR #31:
URL: https://github.com/apache/maven-verifier/pull/31#discussion_r912207466


##########
src/test/java/org/apache/maven/it/Embedded3xLauncherTest.java:
##########
@@ -19,22 +19,22 @@
  * under the License.
  */
 
-import java.io.File;
 import java.nio.file.Files;
+import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.Properties;
 
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.TemporaryFolder;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.is;
 
+
 public class Embedded3xLauncherTest
 {
-    @Rule
-    public TemporaryFolder temporaryFolder = new TemporaryFolder();
+    @TempDir
+    public Path temporaryFolder;

Review Comment:
   Please make it finally proper: `temporaryDir`, not folder.



##########
src/test/java/org/apache/maven/it/ForkedLauncherTest.java:
##########
@@ -92,7 +93,7 @@ static void expectFileLine( File file, String expectedline ) throws IOException
             }
 
             String message = "%s doesn't contain '%s', was:\n%s";
-            fail( String.format( message, file.getName(), expectedline, text ) );
+            fail( String.format( message, file.getFileName().toString(), expectedline, text ) );

Review Comment:
   `toString()` is redudant, it will be autoinvoked.



##########
src/test/java/org/apache/maven/it/ForkedLauncherTest.java:
##########
@@ -22,26 +22,28 @@
  */
 
 import static org.hamcrest.Matchers.is;
-import static org.junit.Assert.fail;
+import static org.junit.jupiter.api.Assertions.fail;
 
 import java.io.BufferedReader;
-import java.io.File;
-import java.io.FileReader;
+import java.nio.file.Files;
 import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Properties;
 
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.TemporaryFolder;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
 
 public class ForkedLauncherTest
 {
-    @Rule
-    public TemporaryFolder temporaryFolder = new TemporaryFolder();
+    @TempDir
+    public Path temporaryFolder;

Review Comment:
   Same here



##########
src/test/java/org/apache/maven/it/ForkedLauncherTest.java:
##########
@@ -92,7 +93,7 @@ static void expectFileLine( File file, String expectedline ) throws IOException
             }
 
             String message = "%s doesn't contain '%s', was:\n%s";

Review Comment:
   Change `\n` to `%n`



##########
src/test/java/org/apache/maven/it/VerifierTest.java:
##########
@@ -105,19 +101,20 @@ public void testStripAnsi()
     @Test
     public void testLoadPropertiesFNFE() throws VerificationException
     {
-        exception.expectCause( isA( FileNotFoundException.class ) );
-
-        Verifier verifier = new Verifier( "src/test/resources" );
-        verifier.loadProperties( "unknown.properties" );
+        VerificationException exception = assertThrows( VerificationException.class, () -> {
+            Verifier verifier = new Verifier( "src/test/resources" );
+            verifier.loadProperties( "unknown.properties" );
+        } );
+        assertInstanceOf( FileNotFoundException.class, exception.getCause() );
     }
 
     @Test
     public void testDedicatedMavenHome() throws VerificationException, IOException
     {
         String mavenHome = Paths.get( "src/test/resources/maven-home" ).toAbsolutePath().toString();
-        Verifier verifier = new Verifier( temporaryFolder.getRoot().toString(), null, false, mavenHome );
+        Verifier verifier = new Verifier( temporaryFolder.toString(), null, false, mavenHome );

Review Comment:
   That's ugly, the ctor should rather accept `Path`



##########
src/test/java/org/apache/maven/it/VerifierTest.java:
##########
@@ -19,28 +19,24 @@
  * under the License.
  */
 
-import java.io.File;
 import java.io.FileNotFoundException;
 import java.io.IOException;
+import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.Arrays;
 import java.util.Properties;
 
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.ExpectedException;
-import org.junit.rules.TemporaryFolder;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
 
-import static org.hamcrest.CoreMatchers.isA;
-import static org.junit.Assert.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertInstanceOf;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 
 public class VerifierTest
 {
-    @Rule
-    public final ExpectedException exception = ExpectedException.none();
-
-    @Rule
-    public TemporaryFolder temporaryFolder = new TemporaryFolder();
+    @TempDir
+    public Path temporaryFolder;

Review Comment:
   same here



-- 
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