ztzg commented on code in PR #2100:
URL: https://github.com/apache/zookeeper/pull/2100#discussion_r1441508757
##########
zookeeper-server/src/test/java/org/apache/zookeeper/server/PrepRequestProcessorTest.java:
##########
@@ -80,9 +81,11 @@ public class PrepRequestProcessorTest extends ClientBase {
private boolean isReconfigEnabledPreviously;
private boolean isStandaloneEnabledPreviously;
+ @TempDir
+ static File tmpDir;
Review Comment:
This `static` attribute
[seems](https://junit.org/junit5/docs/5.6.2/api/org.junit.jupiter.api/org/junit/jupiter/api/io/TempDir.html)
to change the scope:
> The temporary directory will be shared by all tests in a class when the
annotation is present on a static field or on a parameter of a
[@BeforeAll](https://junit.org/junit5/docs/5.6.2/api/org.junit.jupiter.api/org/junit/jupiter/api/BeforeAll.html)
method.
(The suite was previously creating a temporary directory "BeforeEach" test,
providing each `ZooKeeperServer` with empty snap/log paths.)
##########
zookeeper-server/src/test/java/org/apache/zookeeper/server/admin/JettyAdminServerTest.java:
##########
@@ -65,6 +66,9 @@ public class JettyAdminServerTest extends ZKTestCase {
static final String HTTPS_URL_FORMAT = "https://localhost:%d/commands";;
private final int jettyAdminPort = PortAssignment.unique();
+ @TempDir
+ static File tempDir;
Review Comment:
This `static` attribute also
[seems](https://junit.org/junit5/docs/5.6.2/api/org.junit.jupiter.api/org/junit/jupiter/api/io/TempDir.html)
to change the scope:
> The temporary directory will be shared by all tests in a class when the
annotation is present on a static field or on a parameter of a
[@BeforeAll](https://junit.org/junit5/docs/5.6.2/api/org.junit.jupiter.api/org/junit/jupiter/api/BeforeAll.html)
method.
but it should not matter as the `X509TestContext` only uses it as a location
for temporary files with non-deterministic names.
##########
zookeeper-server/src/test/java/org/apache/zookeeper/server/controller/ControllerTestBase.java:
##########
@@ -18,31 +18,28 @@
package org.apache.zookeeper.server.controller;
-import static org.junit.jupiter.api.Assertions.assertFalse;
-import static org.junit.jupiter.api.Assertions.assertTrue;
import java.io.File;
import java.util.List;
import java.util.concurrent.TimeoutException;
import org.apache.zookeeper.ZKTestCase;
import org.junit.After;
import org.junit.Before;
+import org.junit.jupiter.api.io.TempDir;
public class ControllerTestBase extends ZKTestCase {
protected ControllerService controllerService;
protected CommandClient commandClient;
- private File tempDirectory;
protected ControllerServerConfig config;
+ @TempDir
+ static File tempDir;
+
@Before
Review Comment:
This test seems to mix JUnit 4.x and 5.x bits; not sure what the impact is
on the scope and/or effect of `@TempDir`.
##########
zookeeper-server/src/test/java/org/apache/zookeeper/server/persistence/SnapStreamTest.java:
##########
@@ -122,18 +120,18 @@ private void checkInvalidSnapshot(String filename,
boolean fsync) throws IOExcep
assertFalse(SnapStream.isValidSnapshot(file));
}
- private void checkInvalidSnapshot(String filename) throws IOException {
- checkInvalidSnapshot(filename, false);
- checkInvalidSnapshot(filename, true);
+ private void checkInvalidSnapshot(String filename, File tmpDir) throws
IOException {
Review Comment:
Scope: the `@TempDir` annotations on tests create *per-test* temporary
directories, but these directories are then used *twice*--which is a change in
scope.
I don't think it matters in practice for these tests, but it might be better
to make the scope and/or the fact that some files are rewritten multiple times
explicit. Or perhaps a comment is sufficient.
##########
zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/Zab1_0Test.java:
##########
@@ -69,14 +69,16 @@
import org.apache.zookeeper.txn.TxnHeader;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
public class Zab1_0Test extends ZKTestCase {
private static final Logger LOG =
LoggerFactory.getLogger(Zab1_0Test.class);
- private static final File testData = new
File(System.getProperty("test.data.dir", "src/test/resources/data"));
+ @TempDir
+ static File testData;
Review Comment:
There doesn't seem to be a reason to reference `test.data.dir`, indeed. But
`testData` is only used for creating further test directories; it might be a
good idea to switch the individual tests to use `@TempDir` parameter
annotations.
##########
zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/CommitProcessorTest.java:
##########
@@ -80,14 +81,14 @@ public class CommitProcessorTest extends ZKTestCase {
boolean stopped;
TestZooKeeperServer zks;
- File tmpDir;
+ @TempDir
+ static File tmpDir;
Review Comment:
This static attribute
[seems](https://junit.org/junit5/docs/5.6.2/api/org.junit.jupiter.api/org/junit/jupiter/api/io/TempDir.html)
to change the scope:
The temporary directory will be shared by all tests in a class when the
annotation is present on a static field or on a parameter of a
[@BeforeAll](https://junit.org/junit5/docs/5.6.2/api/org.junit.jupiter.api/org/junit/jupiter/api/BeforeAll.html)
method.
(`setUp` is called by each test, and was previously providing each
TestZooKeeperServer with empty snap/log paths.)
##########
zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/auth/QuorumAuthTestBase.java:
##########
@@ -44,11 +44,11 @@ public class QuorumAuthTestBase extends ZKTestCase {
protected static final Logger LOG =
LoggerFactory.getLogger(QuorumAuthTestBase.class);
protected List<MainThread> mt = new ArrayList<>();
+ @TempDir
protected static File jaasConfigDir;
Review Comment:
Interesting. Unless I am missing something, this value is used by a static
method, which is itself called by `static { }` blocks in derived classes. How
do we know the JUnit "engine" is going to be able to initialize the field
before such blocks are executed?
--
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: notifications-unsubscr...@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
