You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2020/07/30 07:35:52 UTC
[GitHub] [zookeeper] tamaashu opened a new pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
tamaashu opened a new pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417
Sub-task of ZOOKEEPER-3732, updating jUnit to 5.6.2 in zookeeper-server sub-component.
Change-Id: I1fc2a7f860eae0f1d285cb278f6eca7a3b947695
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tamaashu commented on pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
tamaashu commented on pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#issuecomment-666262917
checkstyle issues fixed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r462938070
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/common/BaseX509ParameterizedTestCase.java
##########
@@ -106,4 +108,24 @@ protected BaseX509ParameterizedTestCase(
}
}
+ public void init(
Review comment:
do we need this method to be public? protected wouldn't be more appropriate?
maybe we do need public, I might not see all the usage (it's not a small PR :p). but AFAICS we only call this from child classes.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tamaashu commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
tamaashu commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r463287118
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/test/MultiOperationTest.java
##########
@@ -52,42 +54,33 @@
import org.apache.zookeeper.Transaction;
import org.apache.zookeeper.WatchedEvent;
import org.apache.zookeeper.Watcher;
-import org.apache.zookeeper.ZKParameterized;
import org.apache.zookeeper.ZooDefs;
import org.apache.zookeeper.ZooDefs.Ids;
import org.apache.zookeeper.ZooKeeper;
import org.apache.zookeeper.data.ACL;
import org.apache.zookeeper.data.Id;
import org.apache.zookeeper.data.Stat;
import org.apache.zookeeper.server.SyncRequestProcessor;
-import org.junit.Before;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-import org.junit.runners.Parameterized.Parameters;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-@RunWith(Parameterized.class)
-@Parameterized.UseParametersRunnerFactory(ZKParameterized.RunnerFactory.class)
public class MultiOperationTest extends ClientBase {
private static final Logger LOG = LoggerFactory.getLogger(MultiOperationTest.class);
private ZooKeeper zk;
private ZooKeeper zk_chroot;
- private final boolean useAsync;
-
- public MultiOperationTest(boolean useAsync) {
- this.useAsync = useAsync;
- }
-
- @Parameters
- public static Collection<Object[]> configs() {
- return Arrays.asList(new Object[][]{{false}, {true}});
+ public static Stream<Arguments> data() throws Exception {
Review comment:
Fixed
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/test/ObserverMasterTest.java
##########
@@ -60,30 +59,20 @@
import org.apache.zookeeper.server.admin.Commands;
import org.apache.zookeeper.server.quorum.QuorumPeerConfig;
import org.apache.zookeeper.server.util.PortForwarder;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-@RunWith(Parameterized.class)
public class ObserverMasterTest extends ObserverMasterTestBase {
protected static final Logger LOG = LoggerFactory.getLogger(ObserverMasterTest.class);
- public ObserverMasterTest(Boolean testObserverMaster) {
- this.testObserverMaster = testObserverMaster;
- }
-
- @Parameterized.Parameters
- public static List<Object[]> data() {
- return Arrays.asList(new Object[][]{{Boolean.TRUE}, {Boolean.FALSE}});
- }
-
- private Boolean testObserverMaster;
-
- private PortForwarder setUp(final int omProxyPort) throws IOException {
- return setUp(omProxyPort, testObserverMaster);
+ public static Stream<Arguments> data() {
Review comment:
Fixed
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/test/ObserverMasterTest.java
##########
@@ -416,8 +398,9 @@ private ZooKeeperAdmin createAdmin(int clientPort) throws IOException {
// This test is known to be flaky and fail due to "reconfig already in progress".
// TODO: Investigate intermittent testDynamicReconfig failures.
- // @Test
- public void testDynamicReconfig() throws InterruptedException, IOException, KeeperException {
+ // @ParameterizedTest
Review comment:
Fixed
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/test/QuorumBase.java
##########
@@ -70,11 +71,12 @@
protected boolean localSessionsEnabled = false;
protected boolean localSessionsUpgradingEnabled = false;
- @Test
+ //@Test
// This just avoids complaints by junit
public void testNull() {
Review comment:
Fixed
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/test/ThrottledOpQuorumTest.java
##########
@@ -23,15 +23,22 @@
import org.apache.zookeeper.ZooKeeper;
import org.apache.zookeeper.server.ZooKeeperServer;
import org.apache.zookeeper.server.quorum.QuorumPeer;
-import org.junit.BeforeClass;
-import org.junit.Test;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
public class ThrottledOpQuorumTest extends QuorumBase {
- @BeforeClass
+ @BeforeAll
public static void applyMockUps() {
ThrottledOpHelper.applyMockUps();
}
+ @BeforeEach
+ @Override
+ public void setUp() throws Exception {
Review comment:
Fixed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r462968923
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/RemoveWatchesTest.java
##########
@@ -82,15 +84,10 @@ public void tearDown() throws Exception {
super.tearDown();
}
- private final boolean useAsync;
-
- public RemoveWatchesTest(boolean useAsync) {
- this.useAsync = useAsync;
- }
-
- @Parameters
- public static Collection<Object[]> configs() {
- return Arrays.asList(new Object[][]{{false}, {true}});
+ public static Stream<Arguments> data() throws Exception {
Review comment:
the page above doesn't list `booleans` as annotation parameter, but according to the javadoc, it works too: https://junit.org/junit5/docs/current/api/org.junit.jupiter.params/org/junit/jupiter/params/provider/ValueSource.html
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r462960775
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/ReconfigDuringLeaderSyncTest.java
##########
@@ -39,48 +38,40 @@
import org.apache.zookeeper.server.quorum.flexible.QuorumMaj;
import org.apache.zookeeper.test.ClientBase;
import org.apache.zookeeper.test.ClientBase.CountdownWatcher;
-import org.junit.After;
-import org.junit.AfterClass;
-import org.junit.Before;
-import org.junit.BeforeClass;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-@RunWith(Parameterized.class)
public class ReconfigDuringLeaderSyncTest extends QuorumPeerTestBase {
private static final Logger LOG = LoggerFactory.getLogger(ReconfigDuringLeaderSyncTest.class);
private static int SERVER_COUNT = 3;
private MainThread[] mt;
private static boolean bakAsyncSending;
- private boolean asyncSending;
-
- public ReconfigDuringLeaderSyncTest(boolean asyncSending) {
- this.asyncSending = asyncSending;
- }
-
- @Parameterized.Parameters
- public static Collection sendingModes() {
- return Arrays.asList(new Object[][]{{true}, {false}});
+ public static Stream<Arguments> data() {
Review comment:
`@ValueSource` here too
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tamaashu commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
tamaashu commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r463286585
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/RemoveWatchesTest.java
##########
@@ -82,15 +84,10 @@ public void tearDown() throws Exception {
super.tearDown();
}
- private final boolean useAsync;
-
- public RemoveWatchesTest(boolean useAsync) {
- this.useAsync = useAsync;
- }
-
- @Parameters
- public static Collection<Object[]> configs() {
- return Arrays.asList(new Object[][]{{false}, {true}});
+ public static Stream<Arguments> data() throws Exception {
Review comment:
Fixed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r462928657
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/ClientRequestTimeoutTest.java
##########
@@ -19,78 +19,84 @@
package org.apache.zookeeper;
import static org.apache.zookeeper.test.ClientBase.CONNECTION_TIMEOUT;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTimeout;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.fail;
import java.io.IOException;
+import java.time.Duration;
import org.apache.zookeeper.ZooDefs.Ids;
import org.apache.zookeeper.client.HostProvider;
import org.apache.zookeeper.client.ZKClientConfig;
import org.apache.zookeeper.server.quorum.QuorumPeerTestBase;
import org.apache.zookeeper.test.ClientBase;
import org.apache.zookeeper.test.ClientBase.CountdownWatcher;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
public class ClientRequestTimeoutTest extends QuorumPeerTestBase {
private static final int SERVER_COUNT = 3;
private boolean dropPacket = false;
private int dropPacketType = ZooDefs.OpCode.create;
- @Test(timeout = 120000)
+ @Test
Review comment:
I see there is a @Timeout annotation since 5.5. Although it is experimental. Did you try it maybe?
https://junit.org/junit5/docs/5.5.0-RC2/api/org/junit/jupiter/api/Timeout.html
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r462934183
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/ZKTestCase.java
##########
@@ -54,54 +52,35 @@ protected String getTestName() {
return testName;
}
- @BeforeClass
+ @BeforeAll
public static void before() {
if (!testBaseDir.exists()) {
- assertTrue(
- "Cannot properly create test base directory " + testBaseDir.getAbsolutePath(),
- testBaseDir.mkdirs());
+ assertTrue(testBaseDir.mkdirs(),
+ "Cannot properly create test base directory " + testBaseDir.getAbsolutePath());
} else if (!testBaseDir.isDirectory()) {
- assertTrue(
- "Cannot properly delete file with duplicate name of test base directory " + testBaseDir.getAbsolutePath(),
- testBaseDir.delete());
- assertTrue(
- "Cannot properly create test base directory " + testBaseDir.getAbsolutePath(),
- testBaseDir.mkdirs());
+ assertTrue(testBaseDir.delete(),
+ "Cannot properly delete file with duplicate name of test base directory " + testBaseDir.getAbsolutePath());
+ assertTrue(testBaseDir.mkdirs(),
+ "Cannot properly create test base directory " + testBaseDir.getAbsolutePath());
}
}
- @Rule
- public TestWatcher watchman = new TestWatcher() {
-
- @Override
- public void starting(Description method) {
- // By default, disable starting a JettyAdminServer in tests to avoid
- // accidentally attempting to start multiple admin servers on the
- // same port.
- System.setProperty("zookeeper.admin.enableServer", "false");
- // ZOOKEEPER-2693 disables all 4lw by default.
- // Here we enable the 4lw which ZooKeeper tests depends.
- System.setProperty("zookeeper.4lw.commands.whitelist", "*");
- testName = method.getMethodName();
- LOG.info("STARTING {}", testName);
- }
-
- @Override
- public void finished(Description method) {
- LOG.info("FINISHED {}", testName);
- }
-
- @Override
- public void succeeded(Description method) {
- LOG.info("SUCCEEDED {}", testName);
- }
-
- @Override
- public void failed(Throwable e, Description method) {
- LOG.error("FAILED {}", testName, e);
- }
+ @BeforeEach
+ public void starting(TestInfo testInfo) {
Review comment:
just for curiosity: is it guaranteed, that the ZKTestCase.starting will be executed before any @BeforeEach annotated setup method in the child classes?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r462934183
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/ZKTestCase.java
##########
@@ -54,54 +52,35 @@ protected String getTestName() {
return testName;
}
- @BeforeClass
+ @BeforeAll
public static void before() {
if (!testBaseDir.exists()) {
- assertTrue(
- "Cannot properly create test base directory " + testBaseDir.getAbsolutePath(),
- testBaseDir.mkdirs());
+ assertTrue(testBaseDir.mkdirs(),
+ "Cannot properly create test base directory " + testBaseDir.getAbsolutePath());
} else if (!testBaseDir.isDirectory()) {
- assertTrue(
- "Cannot properly delete file with duplicate name of test base directory " + testBaseDir.getAbsolutePath(),
- testBaseDir.delete());
- assertTrue(
- "Cannot properly create test base directory " + testBaseDir.getAbsolutePath(),
- testBaseDir.mkdirs());
+ assertTrue(testBaseDir.delete(),
+ "Cannot properly delete file with duplicate name of test base directory " + testBaseDir.getAbsolutePath());
+ assertTrue(testBaseDir.mkdirs(),
+ "Cannot properly create test base directory " + testBaseDir.getAbsolutePath());
}
}
- @Rule
- public TestWatcher watchman = new TestWatcher() {
-
- @Override
- public void starting(Description method) {
- // By default, disable starting a JettyAdminServer in tests to avoid
- // accidentally attempting to start multiple admin servers on the
- // same port.
- System.setProperty("zookeeper.admin.enableServer", "false");
- // ZOOKEEPER-2693 disables all 4lw by default.
- // Here we enable the 4lw which ZooKeeper tests depends.
- System.setProperty("zookeeper.4lw.commands.whitelist", "*");
- testName = method.getMethodName();
- LOG.info("STARTING {}", testName);
- }
-
- @Override
- public void finished(Description method) {
- LOG.info("FINISHED {}", testName);
- }
-
- @Override
- public void succeeded(Description method) {
- LOG.info("SUCCEEDED {}", testName);
- }
-
- @Override
- public void failed(Throwable e, Description method) {
- LOG.error("FAILED {}", testName, e);
- }
+ @BeforeEach
+ public void starting(TestInfo testInfo) {
Review comment:
just for curiosity: is it guaranteed, that the ZKTestCase.starting will be executed before any `@BeforeEach` annotated setup method in the child classes?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r462961955
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/UnifiedServerSocketModeDetectionTest.java
##########
@@ -61,23 +59,19 @@
* trigger blocking mode detection. This is necessary to ensure that the
* Leader's accept() thread doesn't get blocked.
*/
-@RunWith(Parameterized.class)
public class UnifiedServerSocketModeDetectionTest extends ZKTestCase {
private static final Logger LOG = LoggerFactory.getLogger(UnifiedServerSocketModeDetectionTest.class);
- @Parameterized.Parameters
- public static Collection<Object[]> params() {
- ArrayList<Object[]> result = new ArrayList<>();
- result.add(new Object[]{true});
- result.add(new Object[]{false});
- return result;
+ public static Stream<Arguments> data() throws Exception {
Review comment:
`@ValueSource` here too
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r462939705
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/ClientRequestTimeoutTest.java
##########
@@ -19,78 +19,84 @@
package org.apache.zookeeper;
import static org.apache.zookeeper.test.ClientBase.CONNECTION_TIMEOUT;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTimeout;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.fail;
import java.io.IOException;
+import java.time.Duration;
import org.apache.zookeeper.ZooDefs.Ids;
import org.apache.zookeeper.client.HostProvider;
import org.apache.zookeeper.client.ZKClientConfig;
import org.apache.zookeeper.server.quorum.QuorumPeerTestBase;
import org.apache.zookeeper.test.ClientBase;
import org.apache.zookeeper.test.ClientBase.CountdownWatcher;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
public class ClientRequestTimeoutTest extends QuorumPeerTestBase {
private static final int SERVER_COUNT = 3;
private boolean dropPacket = false;
private int dropPacketType = ZooDefs.OpCode.create;
- @Test(timeout = 120000)
+ @Test
Review comment:
I would much prefer to use the annotation (for readability / clean code), if the timeout is really about the whole test and not only some small part of it. But only if it is stable. I wonder why is this experimental still. It was introduced in 5.5 and we are using 5.6 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tamaashu commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
tamaashu commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r463287568
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/util/PemReaderTest.java
##########
@@ -18,50 +18,35 @@
package org.apache.zookeeper.util;
-import static org.junit.Assert.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
import java.io.IOException;
import java.security.GeneralSecurityException;
import java.security.KeyStoreException;
import java.security.PrivateKey;
import java.security.cert.X509Certificate;
-import java.util.Collection;
import java.util.List;
import java.util.Optional;
+import java.util.stream.Stream;
import org.apache.zookeeper.common.BaseX509ParameterizedTestCase;
import org.apache.zookeeper.common.KeyStoreFileType;
import org.apache.zookeeper.common.X509KeyType;
-import org.apache.zookeeper.common.X509TestContext;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
-@RunWith(Parameterized.class)
public class PemReaderTest extends BaseX509ParameterizedTestCase {
- @Parameterized.Parameters
- public static Collection<Object[]> params() {
+ public static Stream<Arguments> data() {
return BaseX509ParameterizedTestCase.defaultParams();
}
- public PemReaderTest(
- X509KeyType caKeyType, X509KeyType certKeyType, String keyPassword, Integer paramIndex) {
- super(paramIndex, () -> {
- try {
- return X509TestContext.newBuilder()
- .setTempDir(tempDir)
- .setKeyStorePassword(keyPassword)
- .setKeyStoreKeyType(certKeyType)
- .setTrustStorePassword(keyPassword)
- .setTrustStoreKeyType(caKeyType)
- .build();
- } catch (Exception e) {
- throw new RuntimeException(e);
- }
- });
- }
-
- @Test
- public void testLoadPrivateKeyFromKeyStore() throws IOException, GeneralSecurityException {
+ @ParameterizedTest
+ @MethodSource("data")
Review comment:
Fixed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tamaashu commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
tamaashu commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r463812544
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/test/ThrottledOpQuorumTest.java
##########
@@ -23,15 +23,22 @@
import org.apache.zookeeper.ZooKeeper;
import org.apache.zookeeper.server.ZooKeeperServer;
import org.apache.zookeeper.server.quorum.QuorumPeer;
-import org.junit.BeforeClass;
-import org.junit.Test;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
public class ThrottledOpQuorumTest extends QuorumBase {
- @BeforeClass
+ @BeforeAll
public static void applyMockUps() {
ThrottledOpHelper.applyMockUps();
}
+ @BeforeEach
+ @Override
+ public void setUp() throws Exception {
Review comment:
This time it should be really fixed. Sorry, I haven't recognized that I forgot them.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
symat commented on pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#issuecomment-666356034
@anmolnar I'm not sure how our new Jenkins tasks are working now. Are they integrated with github? It would be nice to see that this patch is running with all JDK versions.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tamaashu commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
tamaashu commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r463286199
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/util/PemReaderTest.java
##########
@@ -18,50 +18,35 @@
package org.apache.zookeeper.util;
-import static org.junit.Assert.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
import java.io.IOException;
import java.security.GeneralSecurityException;
import java.security.KeyStoreException;
import java.security.PrivateKey;
import java.security.cert.X509Certificate;
-import java.util.Collection;
import java.util.List;
import java.util.Optional;
+import java.util.stream.Stream;
import org.apache.zookeeper.common.BaseX509ParameterizedTestCase;
import org.apache.zookeeper.common.KeyStoreFileType;
import org.apache.zookeeper.common.X509KeyType;
-import org.apache.zookeeper.common.X509TestContext;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
-@RunWith(Parameterized.class)
public class PemReaderTest extends BaseX509ParameterizedTestCase {
- @Parameterized.Parameters
- public static Collection<Object[]> params() {
+ public static Stream<Arguments> data() {
return BaseX509ParameterizedTestCase.defaultParams();
}
- public PemReaderTest(
- X509KeyType caKeyType, X509KeyType certKeyType, String keyPassword, Integer paramIndex) {
- super(paramIndex, () -> {
- try {
- return X509TestContext.newBuilder()
- .setTempDir(tempDir)
- .setKeyStorePassword(keyPassword)
- .setKeyStoreKeyType(certKeyType)
- .setTrustStorePassword(keyPassword)
- .setTrustStoreKeyType(caKeyType)
- .build();
- } catch (Exception e) {
- throw new RuntimeException(e);
- }
- });
- }
-
- @Test
- public void testLoadPrivateKeyFromKeyStore() throws IOException, GeneralSecurityException {
+ @ParameterizedTest
+ @MethodSource("data")
Review comment:
Done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tamaashu commented on pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
tamaashu commented on pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#issuecomment-666733907
> wow, this was a huge amount of work, thank you @tamaashu !
> I really like JUnit5 (although I miss the `expected` parameter to declare exceptions).
> I left a couple of comments, please check them.
Thanks for the review. I've fixed all issues found. Could you please re-review them?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r462972088
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/test/ObserverMasterTest.java
##########
@@ -416,8 +398,9 @@ private ZooKeeperAdmin createAdmin(int clientPort) throws IOException {
// This test is known to be flaky and fail due to "reconfig already in progress".
// TODO: Investigate intermittent testDynamicReconfig failures.
- // @Test
- public void testDynamicReconfig() throws InterruptedException, IOException, KeeperException {
+ // @ParameterizedTest
Review comment:
I would prefer to properly ignore the test (`@Disabled`), so it is listed among the ignored tests.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tamaashu commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
tamaashu commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r463289800
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/common/BaseX509ParameterizedTestCase.java
##########
@@ -106,4 +108,24 @@ protected BaseX509ParameterizedTestCase(
}
}
+ public void init(
Review comment:
Fixed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r462977496
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/test/ThrottledOpQuorumTest.java
##########
@@ -23,15 +23,22 @@
import org.apache.zookeeper.ZooKeeper;
import org.apache.zookeeper.server.ZooKeeperServer;
import org.apache.zookeeper.server.quorum.QuorumPeer;
-import org.junit.BeforeClass;
-import org.junit.Test;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
public class ThrottledOpQuorumTest extends QuorumBase {
- @BeforeClass
+ @BeforeAll
public static void applyMockUps() {
ThrottledOpHelper.applyMockUps();
}
+ @BeforeEach
+ @Override
+ public void setUp() throws Exception {
Review comment:
why do we need this method? (maybe I just miss something)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r462972629
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/test/QuorumBase.java
##########
@@ -70,11 +71,12 @@
protected boolean localSessionsEnabled = false;
protected boolean localSessionsUpgradingEnabled = false;
- @Test
+ //@Test
// This just avoids complaints by junit
public void testNull() {
Review comment:
do we still need this method?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] nkalmar commented on pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
nkalmar commented on pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#issuecomment-668501329
Started reviewing... :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] asfgit closed pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tamaashu commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
tamaashu commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r462944728
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/ClientRequestTimeoutTest.java
##########
@@ -19,78 +19,84 @@
package org.apache.zookeeper;
import static org.apache.zookeeper.test.ClientBase.CONNECTION_TIMEOUT;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTimeout;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.fail;
import java.io.IOException;
+import java.time.Duration;
import org.apache.zookeeper.ZooDefs.Ids;
import org.apache.zookeeper.client.HostProvider;
import org.apache.zookeeper.client.ZKClientConfig;
import org.apache.zookeeper.server.quorum.QuorumPeerTestBase;
import org.apache.zookeeper.test.ClientBase;
import org.apache.zookeeper.test.ClientBase.CountdownWatcher;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
public class ClientRequestTimeoutTest extends QuorumPeerTestBase {
private static final int SERVER_COUNT = 3;
private boolean dropPacket = false;
private int dropPacketType = ZooDefs.OpCode.create;
- @Test(timeout = 120000)
+ @Test
Review comment:
Hmm, tried it out with a few cases, looks working.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] anmolnar commented on pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
anmolnar commented on pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#issuecomment-666276915
Thanks @tamaashu
Travis doesn't like your patch:
```
/jersey/Base.java:[42,10] cannot find symbol
symbol: class JUnit4ZKTestRunner
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] anmolnar commented on pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
anmolnar commented on pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#issuecomment-666391863
@symat I'm working with INFRA to get a github user which is able to update build status of Pull Requests and you will see updates from the new job.
However I set up the PreCommit job to run against JDK8 only to make it more stable, but looks like a multi-JDK job would be more beneficial. :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r462946600
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/RemoveWatchesTest.java
##########
@@ -82,15 +84,10 @@ public void tearDown() throws Exception {
super.tearDown();
}
- private final boolean useAsync;
-
- public RemoveWatchesTest(boolean useAsync) {
- this.useAsync = useAsync;
- }
-
- @Parameters
- public static Collection<Object[]> configs() {
- return Arrays.asList(new Object[][]{{false}, {true}});
+ public static Stream<Arguments> data() throws Exception {
Review comment:
for these trivial test parameter wouldn't be nicer to use `@ValueSource` instead of `@MethodSource` ?
see: https://www.baeldung.com/parameterized-tests-junit-5#1-simple-values
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r462969330
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/test/MultiOperationTest.java
##########
@@ -52,42 +54,33 @@
import org.apache.zookeeper.Transaction;
import org.apache.zookeeper.WatchedEvent;
import org.apache.zookeeper.Watcher;
-import org.apache.zookeeper.ZKParameterized;
import org.apache.zookeeper.ZooDefs;
import org.apache.zookeeper.ZooDefs.Ids;
import org.apache.zookeeper.ZooKeeper;
import org.apache.zookeeper.data.ACL;
import org.apache.zookeeper.data.Id;
import org.apache.zookeeper.data.Stat;
import org.apache.zookeeper.server.SyncRequestProcessor;
-import org.junit.Before;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-import org.junit.runners.Parameterized.Parameters;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-@RunWith(Parameterized.class)
-@Parameterized.UseParametersRunnerFactory(ZKParameterized.RunnerFactory.class)
public class MultiOperationTest extends ClientBase {
private static final Logger LOG = LoggerFactory.getLogger(MultiOperationTest.class);
private ZooKeeper zk;
private ZooKeeper zk_chroot;
- private final boolean useAsync;
-
- public MultiOperationTest(boolean useAsync) {
- this.useAsync = useAsync;
- }
-
- @Parameters
- public static Collection<Object[]> configs() {
- return Arrays.asList(new Object[][]{{false}, {true}});
+ public static Stream<Arguments> data() throws Exception {
Review comment:
`@ValueSource(booleans={true,false})` here too
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r462955008
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/ZKTestCase.java
##########
@@ -54,54 +52,35 @@ protected String getTestName() {
return testName;
}
- @BeforeClass
+ @BeforeAll
public static void before() {
if (!testBaseDir.exists()) {
- assertTrue(
- "Cannot properly create test base directory " + testBaseDir.getAbsolutePath(),
- testBaseDir.mkdirs());
+ assertTrue(testBaseDir.mkdirs(),
+ "Cannot properly create test base directory " + testBaseDir.getAbsolutePath());
} else if (!testBaseDir.isDirectory()) {
- assertTrue(
- "Cannot properly delete file with duplicate name of test base directory " + testBaseDir.getAbsolutePath(),
- testBaseDir.delete());
- assertTrue(
- "Cannot properly create test base directory " + testBaseDir.getAbsolutePath(),
- testBaseDir.mkdirs());
+ assertTrue(testBaseDir.delete(),
+ "Cannot properly delete file with duplicate name of test base directory " + testBaseDir.getAbsolutePath());
+ assertTrue(testBaseDir.mkdirs(),
+ "Cannot properly create test base directory " + testBaseDir.getAbsolutePath());
}
}
- @Rule
- public TestWatcher watchman = new TestWatcher() {
-
- @Override
- public void starting(Description method) {
- // By default, disable starting a JettyAdminServer in tests to avoid
- // accidentally attempting to start multiple admin servers on the
- // same port.
- System.setProperty("zookeeper.admin.enableServer", "false");
- // ZOOKEEPER-2693 disables all 4lw by default.
- // Here we enable the 4lw which ZooKeeper tests depends.
- System.setProperty("zookeeper.4lw.commands.whitelist", "*");
- testName = method.getMethodName();
- LOG.info("STARTING {}", testName);
- }
-
- @Override
- public void finished(Description method) {
- LOG.info("FINISHED {}", testName);
- }
-
- @Override
- public void succeeded(Description method) {
- LOG.info("SUCCEEDED {}", testName);
- }
-
- @Override
- public void failed(Throwable e, Description method) {
- LOG.error("FAILED {}", testName, e);
- }
+ @BeforeEach
+ public void starting(TestInfo testInfo) {
Review comment:
cool
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r462959101
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/LearnerSyncThrottlerTest.java
##########
@@ -18,63 +18,66 @@
package org.apache.zookeeper.server.quorum;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.fail;
import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.Collection;
import java.util.List;
import java.util.concurrent.Callable;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
+import java.util.stream.Stream;
import org.apache.zookeeper.ZKTestCase;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-@RunWith(Parameterized.class)
public class LearnerSyncThrottlerTest extends ZKTestCase {
private static final Logger LOG = LoggerFactory.getLogger(LearnerSyncThrottlerTest.class);
- private LearnerSyncThrottler.SyncType syncType;
- public LearnerSyncThrottlerTest(LearnerSyncThrottler.SyncType syncType) {
- this.syncType = syncType;
+ public static Stream<Arguments> data() {
Review comment:
instead of data(), we can use `@EnumSource`
https://www.baeldung.com/parameterized-tests-junit-5#3-enum
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tamaashu edited a comment on pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
tamaashu edited a comment on pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#issuecomment-666733907
> wow, this was a huge amount of work, thank you @tamaashu !
> I really like JUnit5 (although I miss the `expected` parameter to declare exceptions).
> I left a couple of comments, please check them.
@symat Thanks for the review. I've fixed all issues found. Could you please re-review them?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r462957948
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/LearnerMetricsTest.java
##########
@@ -32,50 +31,41 @@
import org.apache.zookeeper.server.ServerMetrics;
import org.apache.zookeeper.test.ClientBase;
import org.hamcrest.Matcher;
-import org.junit.After;
-import org.junit.AfterClass;
-import org.junit.Before;
-import org.junit.BeforeClass;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-
-@RunWith(Parameterized.class)
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+
public class LearnerMetricsTest extends QuorumPeerTestBase {
private static final int TIMEOUT_SECONDS = 30;
private static final int SERVER_COUNT = 4; // 1 observer, 3 participants
private final QuorumPeerTestBase.MainThread[] mt = new QuorumPeerTestBase.MainThread[SERVER_COUNT];
private ZooKeeper zk_client;
- private boolean asyncSending;
private static boolean bakAsyncSending;
- public LearnerMetricsTest(boolean asyncSending) {
- this.asyncSending = asyncSending;
- }
-
- @Parameterized.Parameters
- public static Collection sendingModes() {
- return Arrays.asList(new Object[][]{{true}, {false}});
+ public static Stream<Arguments> data() {
Review comment:
maybe `@ValueSource(booleans={true,false})` here too?
https://junit.org/junit5/docs/current/api/org.junit.jupiter.params/org/junit/jupiter/params/provider/ValueSource.html
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tamaashu commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
tamaashu commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r463286843
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/LearnerMetricsTest.java
##########
@@ -32,50 +31,41 @@
import org.apache.zookeeper.server.ServerMetrics;
import org.apache.zookeeper.test.ClientBase;
import org.hamcrest.Matcher;
-import org.junit.After;
-import org.junit.AfterClass;
-import org.junit.Before;
-import org.junit.BeforeClass;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-
-@RunWith(Parameterized.class)
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+
public class LearnerMetricsTest extends QuorumPeerTestBase {
private static final int TIMEOUT_SECONDS = 30;
private static final int SERVER_COUNT = 4; // 1 observer, 3 participants
private final QuorumPeerTestBase.MainThread[] mt = new QuorumPeerTestBase.MainThread[SERVER_COUNT];
private ZooKeeper zk_client;
- private boolean asyncSending;
private static boolean bakAsyncSending;
- public LearnerMetricsTest(boolean asyncSending) {
- this.asyncSending = asyncSending;
- }
-
- @Parameterized.Parameters
- public static Collection sendingModes() {
- return Arrays.asList(new Object[][]{{true}, {false}});
+ public static Stream<Arguments> data() {
Review comment:
Fixed
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/ReconfigDuringLeaderSyncTest.java
##########
@@ -39,48 +38,40 @@
import org.apache.zookeeper.server.quorum.flexible.QuorumMaj;
import org.apache.zookeeper.test.ClientBase;
import org.apache.zookeeper.test.ClientBase.CountdownWatcher;
-import org.junit.After;
-import org.junit.AfterClass;
-import org.junit.Before;
-import org.junit.BeforeClass;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-@RunWith(Parameterized.class)
public class ReconfigDuringLeaderSyncTest extends QuorumPeerTestBase {
private static final Logger LOG = LoggerFactory.getLogger(ReconfigDuringLeaderSyncTest.class);
private static int SERVER_COUNT = 3;
private MainThread[] mt;
private static boolean bakAsyncSending;
- private boolean asyncSending;
-
- public ReconfigDuringLeaderSyncTest(boolean asyncSending) {
- this.asyncSending = asyncSending;
- }
-
- @Parameterized.Parameters
- public static Collection sendingModes() {
- return Arrays.asList(new Object[][]{{true}, {false}});
+ public static Stream<Arguments> data() {
Review comment:
Fixed
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/UnifiedServerSocketModeDetectionTest.java
##########
@@ -61,23 +59,19 @@
* trigger blocking mode detection. This is necessary to ensure that the
* Leader's accept() thread doesn't get blocked.
*/
-@RunWith(Parameterized.class)
public class UnifiedServerSocketModeDetectionTest extends ZKTestCase {
private static final Logger LOG = LoggerFactory.getLogger(UnifiedServerSocketModeDetectionTest.class);
- @Parameterized.Parameters
- public static Collection<Object[]> params() {
- ArrayList<Object[]> result = new ArrayList<>();
- result.add(new Object[]{true});
- result.add(new Object[]{false});
- return result;
+ public static Stream<Arguments> data() throws Exception {
Review comment:
Fixed
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/WatchLeakTest.java
##########
@@ -70,36 +67,30 @@
/**
* Demonstrate ZOOKEEPER-1382 : Watches leak on expired session
*/
-@RunWith(Parameterized.class)
-@Parameterized.UseParametersRunnerFactory(ZKParameterized.RunnerFactory.class)
public class WatchLeakTest {
protected static final Logger LOG = LoggerFactory.getLogger(WatchLeakTest.class);
final long SESSION_ID = 0xBABEL;
- private final boolean sessionTimedout;
-
- @Before
+ @BeforeEach
public void setUp() {
System.setProperty("zookeeper.admin.enableServer", "false");
}
- public WatchLeakTest(boolean sessionTimedout) {
- this.sessionTimedout = sessionTimedout;
- }
-
- @Parameters
- public static Collection<Object[]> configs() {
- return Arrays.asList(new Object[][]{{false}, {true}});
+ public static Stream<Arguments> data() throws Exception {
Review comment:
Fixed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tamaashu commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
tamaashu commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r464367188
##########
File path: pom.xml
##########
@@ -554,11 +555,28 @@
<artifactId>jmockit</artifactId>
<version>${jmockit.version}</version>
</dependency>
+ <dependency>
+ <groupId>org.junit.jupiter</groupId>
+ <artifactId>junit-jupiter-api</artifactId>
+ <version>${junit.version}</version>
+ </dependency>
Review comment:
Sorry, I've misunderstood you. I have updated the scopes of the artifacts in zookeeper-server.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r462962456
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/WatchLeakTest.java
##########
@@ -70,36 +67,30 @@
/**
* Demonstrate ZOOKEEPER-1382 : Watches leak on expired session
*/
-@RunWith(Parameterized.class)
-@Parameterized.UseParametersRunnerFactory(ZKParameterized.RunnerFactory.class)
public class WatchLeakTest {
protected static final Logger LOG = LoggerFactory.getLogger(WatchLeakTest.class);
final long SESSION_ID = 0xBABEL;
- private final boolean sessionTimedout;
-
- @Before
+ @BeforeEach
public void setUp() {
System.setProperty("zookeeper.admin.enableServer", "false");
}
- public WatchLeakTest(boolean sessionTimedout) {
- this.sessionTimedout = sessionTimedout;
- }
-
- @Parameters
- public static Collection<Object[]> configs() {
- return Arrays.asList(new Object[][]{{false}, {true}});
+ public static Stream<Arguments> data() throws Exception {
Review comment:
`@ValueSource` here too
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tamaashu commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
tamaashu commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r463286676
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/LearnerSyncThrottlerTest.java
##########
@@ -18,63 +18,66 @@
package org.apache.zookeeper.server.quorum;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.fail;
import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.Collection;
import java.util.List;
import java.util.concurrent.Callable;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
+import java.util.stream.Stream;
import org.apache.zookeeper.ZKTestCase;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-@RunWith(Parameterized.class)
public class LearnerSyncThrottlerTest extends ZKTestCase {
private static final Logger LOG = LoggerFactory.getLogger(LearnerSyncThrottlerTest.class);
- private LearnerSyncThrottler.SyncType syncType;
- public LearnerSyncThrottlerTest(LearnerSyncThrottler.SyncType syncType) {
- this.syncType = syncType;
+ public static Stream<Arguments> data() {
Review comment:
Fixed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r462914486
##########
File path: zookeeper-server/pom.xml
##########
@@ -152,8 +152,22 @@
<scope>test</scope>
</dependency>
<dependency>
- <groupId>org.junit.vintage</groupId>
- <artifactId>junit-vintage-engine</artifactId>
+ <groupId>org.junit.jupiter</groupId>
+ <artifactId>junit-jupiter-api</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.junit.jupiter</groupId>
+ <artifactId>junit-jupiter-engine</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.junit.platform</groupId>
+ <artifactId>junit-platform-runner</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.junit.jupiter</groupId>
+ <artifactId>junit-jupiter</artifactId>
+ <version>5.4.2</version>
Review comment:
I guess we don't need this version here. (also it differs from the {junit.version} defined in the parent pom)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tamaashu commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
tamaashu commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r463043678
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/ClientRequestTimeoutTest.java
##########
@@ -19,78 +19,84 @@
package org.apache.zookeeper;
import static org.apache.zookeeper.test.ClientBase.CONNECTION_TIMEOUT;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTimeout;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.fail;
import java.io.IOException;
+import java.time.Duration;
import org.apache.zookeeper.ZooDefs.Ids;
import org.apache.zookeeper.client.HostProvider;
import org.apache.zookeeper.client.ZKClientConfig;
import org.apache.zookeeper.server.quorum.QuorumPeerTestBase;
import org.apache.zookeeper.test.ClientBase;
import org.apache.zookeeper.test.ClientBase.CountdownWatcher;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
public class ClientRequestTimeoutTest extends QuorumPeerTestBase {
private static final int SERVER_COUNT = 3;
private boolean dropPacket = false;
private int dropPacketType = ZooDefs.OpCode.create;
- @Test(timeout = 120000)
+ @Test
Review comment:
@symat I've uploaded a change to use Timeout annotation. It looks working for me.
This way we need much less code changes which helps backportability.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r463584028
##########
File path: pom.xml
##########
@@ -554,11 +555,28 @@
<artifactId>jmockit</artifactId>
<version>${jmockit.version}</version>
</dependency>
+ <dependency>
+ <groupId>org.junit.jupiter</groupId>
+ <artifactId>junit-jupiter-api</artifactId>
+ <version>${junit.version}</version>
+ </dependency>
Review comment:
don't we miss the `<scope>test</scope>` from some of the junit dependencies?
I see that in the child module (zookeeper-server/pom.xml) we define test scope for some, but e.g. `junit-jupiter-api` and `junit-jupiter-engine` is missing test scope in both pom files.
Maybe I just miss something, but can you doublecheck that we don't leak any junit dependencies for upstream projects using ZooKeeper?
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/test/StandaloneTest.java
##########
@@ -48,12 +49,18 @@
protected static final Logger LOG = LoggerFactory.getLogger(StandaloneTest.class);
- @Before
+ @BeforeEach
public void setup() {
System.setProperty("zookeeper.DigestAuthenticationProvider.superDigest", "super:D/InIHSb7yEEbrWz8b9l71RjZJU="/* password is 'test'*/);
QuorumPeerConfig.setReconfigEnabled(true);
}
+ @AfterEach
+ @Override
+ public void tearDown() throws Exception{
Review comment:
please check if we really need this method here
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/test/ThrottledOpQuorumTest.java
##########
@@ -23,15 +23,22 @@
import org.apache.zookeeper.ZooKeeper;
import org.apache.zookeeper.server.ZooKeeperServer;
import org.apache.zookeeper.server.quorum.QuorumPeer;
-import org.junit.BeforeClass;
-import org.junit.Test;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
public class ThrottledOpQuorumTest extends QuorumBase {
- @BeforeClass
+ @BeforeAll
public static void applyMockUps() {
ThrottledOpHelper.applyMockUps();
}
+ @BeforeEach
+ @Override
+ public void setUp() throws Exception {
Review comment:
I still see this method here. Did you fix this in the parent class somehow? I still see a `setUp()` method in the `QuorumBase` class with `@BeforeEach` annotation, so this method doesn't have any benefit (unleess if I miss something).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tamaashu commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
tamaashu commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r462937676
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/ClientRequestTimeoutTest.java
##########
@@ -19,78 +19,84 @@
package org.apache.zookeeper;
import static org.apache.zookeeper.test.ClientBase.CONNECTION_TIMEOUT;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTimeout;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.fail;
import java.io.IOException;
+import java.time.Duration;
import org.apache.zookeeper.ZooDefs.Ids;
import org.apache.zookeeper.client.HostProvider;
import org.apache.zookeeper.client.ZKClientConfig;
import org.apache.zookeeper.server.quorum.QuorumPeerTestBase;
import org.apache.zookeeper.test.ClientBase;
import org.apache.zookeeper.test.ClientBase.CountdownWatcher;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
public class ClientRequestTimeoutTest extends QuorumPeerTestBase {
private static final int SERVER_COUNT = 3;
private boolean dropPacket = false;
private int dropPacketType = ZooDefs.OpCode.create;
- @Test(timeout = 120000)
+ @Test
Review comment:
assertTimeout is stable, so I preferred to use it.
But I can try it out.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tamaashu commented on pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
tamaashu commented on pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#issuecomment-666312333
> Thanks @tamaashu
> Travis doesn't like your patch:
>
> ```
> /jersey/Base.java:[42,10] cannot find symbol
> symbol: class JUnit4ZKTestRunner
> ```
fixed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tamaashu commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
tamaashu commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r463812676
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/test/StandaloneTest.java
##########
@@ -48,12 +49,18 @@
protected static final Logger LOG = LoggerFactory.getLogger(StandaloneTest.class);
- @Before
+ @BeforeEach
public void setup() {
System.setProperty("zookeeper.DigestAuthenticationProvider.superDigest", "super:D/InIHSb7yEEbrWz8b9l71RjZJU="/* password is 'test'*/);
QuorumPeerConfig.setReconfigEnabled(true);
}
+ @AfterEach
+ @Override
+ public void tearDown() throws Exception{
Review comment:
Done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] nkalmar commented on pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
nkalmar commented on pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#issuecomment-668583287
merged to master, thanks @tamaashu
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r462980267
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/util/PemReaderTest.java
##########
@@ -18,50 +18,35 @@
package org.apache.zookeeper.util;
-import static org.junit.Assert.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
import java.io.IOException;
import java.security.GeneralSecurityException;
import java.security.KeyStoreException;
import java.security.PrivateKey;
import java.security.cert.X509Certificate;
-import java.util.Collection;
import java.util.List;
import java.util.Optional;
+import java.util.stream.Stream;
import org.apache.zookeeper.common.BaseX509ParameterizedTestCase;
import org.apache.zookeeper.common.KeyStoreFileType;
import org.apache.zookeeper.common.X509KeyType;
-import org.apache.zookeeper.common.X509TestContext;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
-@RunWith(Parameterized.class)
public class PemReaderTest extends BaseX509ParameterizedTestCase {
- @Parameterized.Parameters
- public static Collection<Object[]> params() {
+ public static Stream<Arguments> data() {
return BaseX509ParameterizedTestCase.defaultParams();
}
- public PemReaderTest(
- X509KeyType caKeyType, X509KeyType certKeyType, String keyPassword, Integer paramIndex) {
- super(paramIndex, () -> {
- try {
- return X509TestContext.newBuilder()
- .setTempDir(tempDir)
- .setKeyStorePassword(keyPassword)
- .setKeyStoreKeyType(certKeyType)
- .setTrustStorePassword(keyPassword)
- .setTrustStoreKeyType(caKeyType)
- .build();
- } catch (Exception e) {
- throw new RuntimeException(e);
- }
- });
- }
-
- @Test
- public void testLoadPrivateKeyFromKeyStore() throws IOException, GeneralSecurityException {
+ @ParameterizedTest
+ @MethodSource("data")
Review comment:
just for curiosity... wouldn't `@MethodSource("defaultParams")` or `@MethodSource("org.apache.zookeeper.common.BaseX509ParameterizedTestCase#defaultParams")` work in all the child classes of `BaseX509ParameterizedTestCase`? then we wouldn't need to define the `data()` method in any of these, making the code a bit more compact.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r462954823
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/ClientRequestTimeoutTest.java
##########
@@ -19,78 +19,84 @@
package org.apache.zookeeper;
import static org.apache.zookeeper.test.ClientBase.CONNECTION_TIMEOUT;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTimeout;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.fail;
import java.io.IOException;
+import java.time.Duration;
import org.apache.zookeeper.ZooDefs.Ids;
import org.apache.zookeeper.client.HostProvider;
import org.apache.zookeeper.client.ZKClientConfig;
import org.apache.zookeeper.server.quorum.QuorumPeerTestBase;
import org.apache.zookeeper.test.ClientBase;
import org.apache.zookeeper.test.ClientBase.CountdownWatcher;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
public class ClientRequestTimeoutTest extends QuorumPeerTestBase {
private static final int SERVER_COUNT = 3;
private boolean dropPacket = false;
private int dropPacketType = ZooDefs.OpCode.create;
- @Test(timeout = 120000)
+ @Test
Review comment:
it is a huge work to change it back to annotations for all test cases... let's wait for the opinion of others, before you would change it
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tamaashu commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
tamaashu commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r463804083
##########
File path: pom.xml
##########
@@ -554,11 +555,28 @@
<artifactId>jmockit</artifactId>
<version>${jmockit.version}</version>
</dependency>
+ <dependency>
+ <groupId>org.junit.jupiter</groupId>
+ <artifactId>junit-jupiter-api</artifactId>
+ <version>${junit.version}</version>
+ </dependency>
Review comment:
ZooKeeper-IT component needs some jUnit dependencies with compile scope, others are only needed for unit tests with test scope, that's why we have the differences.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ctubbsii commented on pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#issuecomment-666455975
> I really like JUnit5 (although I miss the `expected` parameter to declare exceptions).
`.assertThrows()` is way better than `expected`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tamaashu commented on pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
tamaashu commented on pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#issuecomment-665952705
okay, so I'll have to fix a few checkstyle issues :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r462928657
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/ClientRequestTimeoutTest.java
##########
@@ -19,78 +19,84 @@
package org.apache.zookeeper;
import static org.apache.zookeeper.test.ClientBase.CONNECTION_TIMEOUT;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTimeout;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.fail;
import java.io.IOException;
+import java.time.Duration;
import org.apache.zookeeper.ZooDefs.Ids;
import org.apache.zookeeper.client.HostProvider;
import org.apache.zookeeper.client.ZKClientConfig;
import org.apache.zookeeper.server.quorum.QuorumPeerTestBase;
import org.apache.zookeeper.test.ClientBase;
import org.apache.zookeeper.test.ClientBase.CountdownWatcher;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
public class ClientRequestTimeoutTest extends QuorumPeerTestBase {
private static final int SERVER_COUNT = 3;
private boolean dropPacket = false;
private int dropPacketType = ZooDefs.OpCode.create;
- @Test(timeout = 120000)
+ @Test
Review comment:
I see there is a @Timeout annotation since 5.5. Although it is experimental. Did you try it maybe?
https://junit.org/junit5/docs/current/api/org.junit.jupiter.api/org/junit/jupiter/api/Timeout.html
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r462970916
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/test/ObserverMasterTest.java
##########
@@ -60,30 +59,20 @@
import org.apache.zookeeper.server.admin.Commands;
import org.apache.zookeeper.server.quorum.QuorumPeerConfig;
import org.apache.zookeeper.server.util.PortForwarder;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-@RunWith(Parameterized.class)
public class ObserverMasterTest extends ObserverMasterTestBase {
protected static final Logger LOG = LoggerFactory.getLogger(ObserverMasterTest.class);
- public ObserverMasterTest(Boolean testObserverMaster) {
- this.testObserverMaster = testObserverMaster;
- }
-
- @Parameterized.Parameters
- public static List<Object[]> data() {
- return Arrays.asList(new Object[][]{{Boolean.TRUE}, {Boolean.FALSE}});
- }
-
- private Boolean testObserverMaster;
-
- private PortForwarder setUp(final int omProxyPort) throws IOException {
- return setUp(omProxyPort, testObserverMaster);
+ public static Stream<Arguments> data() {
Review comment:
`@ValueSource(booleans={true,false})` here too
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tamaashu commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
tamaashu commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r463286416
##########
File path: zookeeper-server/pom.xml
##########
@@ -152,8 +152,22 @@
<scope>test</scope>
</dependency>
<dependency>
- <groupId>org.junit.vintage</groupId>
- <artifactId>junit-vintage-engine</artifactId>
+ <groupId>org.junit.jupiter</groupId>
+ <artifactId>junit-jupiter-api</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.junit.jupiter</groupId>
+ <artifactId>junit-jupiter-engine</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.junit.platform</groupId>
+ <artifactId>junit-platform-runner</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.junit.jupiter</groupId>
+ <artifactId>junit-jupiter</artifactId>
+ <version>5.4.2</version>
Review comment:
Fixed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tamaashu commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
tamaashu commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r462939237
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/ZKTestCase.java
##########
@@ -54,54 +52,35 @@ protected String getTestName() {
return testName;
}
- @BeforeClass
+ @BeforeAll
public static void before() {
if (!testBaseDir.exists()) {
- assertTrue(
- "Cannot properly create test base directory " + testBaseDir.getAbsolutePath(),
- testBaseDir.mkdirs());
+ assertTrue(testBaseDir.mkdirs(),
+ "Cannot properly create test base directory " + testBaseDir.getAbsolutePath());
} else if (!testBaseDir.isDirectory()) {
- assertTrue(
- "Cannot properly delete file with duplicate name of test base directory " + testBaseDir.getAbsolutePath(),
- testBaseDir.delete());
- assertTrue(
- "Cannot properly create test base directory " + testBaseDir.getAbsolutePath(),
- testBaseDir.mkdirs());
+ assertTrue(testBaseDir.delete(),
+ "Cannot properly delete file with duplicate name of test base directory " + testBaseDir.getAbsolutePath());
+ assertTrue(testBaseDir.mkdirs(),
+ "Cannot properly create test base directory " + testBaseDir.getAbsolutePath());
}
}
- @Rule
- public TestWatcher watchman = new TestWatcher() {
-
- @Override
- public void starting(Description method) {
- // By default, disable starting a JettyAdminServer in tests to avoid
- // accidentally attempting to start multiple admin servers on the
- // same port.
- System.setProperty("zookeeper.admin.enableServer", "false");
- // ZOOKEEPER-2693 disables all 4lw by default.
- // Here we enable the 4lw which ZooKeeper tests depends.
- System.setProperty("zookeeper.4lw.commands.whitelist", "*");
- testName = method.getMethodName();
- LOG.info("STARTING {}", testName);
- }
-
- @Override
- public void finished(Description method) {
- LOG.info("FINISHED {}", testName);
- }
-
- @Override
- public void succeeded(Description method) {
- LOG.info("SUCCEEDED {}", testName);
- }
-
- @Override
- public void failed(Throwable e, Description method) {
- LOG.error("FAILED {}", testName, e);
- }
+ @BeforeEach
+ public void starting(TestInfo testInfo) {
Review comment:
According to the documentation yes, except they get overridden: https://junit.org/junit5/docs/5.0.1/api/org/junit/jupiter/api/BeforeEach.html
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on a change in pull request #1417: ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1417:
URL: https://github.com/apache/zookeeper/pull/1417#discussion_r463610221
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/test/ThrottledOpQuorumTest.java
##########
@@ -23,15 +23,22 @@
import org.apache.zookeeper.ZooKeeper;
import org.apache.zookeeper.server.ZooKeeperServer;
import org.apache.zookeeper.server.quorum.QuorumPeer;
-import org.junit.BeforeClass;
-import org.junit.Test;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
public class ThrottledOpQuorumTest extends QuorumBase {
- @BeforeClass
+ @BeforeAll
public static void applyMockUps() {
ThrottledOpHelper.applyMockUps();
}
+ @BeforeEach
+ @Override
+ public void setUp() throws Exception {
Review comment:
You marked this as fixed, but I still see this method here. Did you fix this in the parent class somehow? I still see a `setUp()` method in the `QuorumBase` class with `@BeforeEach` annotation, so this method doesn't have any benefit (unleess if I miss something).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org