ctubbsii commented on code in PR #15:
URL:
https://github.com/apache/accumulo-classloaders/pull/15#discussion_r1063794600
##########
modules/vfs-class-loader/src/test/java/org/apache/accumulo/classloader/vfs/AccumuloVFSClassLoaderTest.java:
##########
@@ -18,37 +18,36 @@
*/
package org.apache.accumulo.classloader.vfs;
-import static org.junit.Assert.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
import java.io.File;
+import java.util.Objects;
import org.apache.commons.io.FileUtils;
import org.apache.commons.vfs2.FileObject;
import org.apache.commons.vfs2.FileSystemException;
-import org.junit.Before;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.TemporaryFolder;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
@SuppressFBWarnings(value = "PATH_TRAVERSAL_IN", justification = "paths not
set by user input")
public class AccumuloVFSClassLoaderTest {
- @Rule
- public TemporaryFolder folder1 =
- new TemporaryFolder(new File(System.getProperty("user.dir") +
"/target"));
+ @TempDir
+ private static File folder1;
Review Comment:
It'd be good to rename this variable to something like `tempDir` rather than
`folder1`. Also, this is `static`. I assume that this means one temporary
directory will be used for all tests (like `@BeforeAll` instead of
`@BeforeEach`). If it were non-static, I think it would create a new temporary
directory for each test. In the `setup()` method below, a file is placed into
this directory with the same name each time. If these tests use the same
temporary directory, and the same file name, it seems like there's a chance of
the tests interfering with each other.
If I'm understanding this correctly, this should either be non-static, so
the tests have independent and unique temporary directories. Or, the setup
method should create a unique folder underneath this directory, or use a unique
file name for the jar.
In the old code, the `TemporaryFolder` was non-static, and so each test had
a unique value for `folder1`, so this didn't matter.
##########
pom.xml:
##########
@@ -340,11 +350,15 @@
</module>
<module name="RegexpSinglelineJava">
<property name="format"
value="junit[.]framework[.]TestCase" />
- <property name="message" value="Use JUnit4+ @Test
annotation instead of TestCase" />
+ <property name="message" value="Use JUnit5+ @Test
annotation instead of TestCase" />
Review Comment:
This detects the JUnit3 usages, but not the JUnit4 usages. A new pattern is
needed to detect JUnit4 now.
##########
modules/vfs-class-loader/src/test/java/org/apache/accumulo/classloader/vfs/ClassPathPrinterTest.java:
##########
@@ -18,40 +18,39 @@
*/
package org.apache.accumulo.classloader.vfs;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
import java.io.File;
import java.net.MalformedURLException;
-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 edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
@SuppressFBWarnings(value = "PATH_TRAVERSAL_IN", justification = "paths not
set by user input")
public class ClassPathPrinterTest {
- @Rule
- public TemporaryFolder folder1 =
- new TemporaryFolder(new File(System.getProperty("user.dir") +
"/target"));
+ @TempDir
+ private static File folder1;
Review Comment:
same comment as above
--
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]
