Skip to content

RATIS-1977. Remove Junit 4 dependencies. #1269

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
szetszwo merged 6 commits into from
May 23, 2025
Merged

RATIS-1977. Remove Junit 4 dependencies. #1269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5ec6bcc
RATIS-1977. Remove Junit 4 dependencies.
slfan1989 May 22, 2025
c8b6f2c
RATIS-1977. Remove Junit 4 dependencies.
slfan1989 May 22, 2025
77e8c0b
RATIS-1977. Improve Code.
slfan1989 May 23, 2025
8749e5f
RATIS-1977. Improve Code.
slfan1989 May 23, 2025
c00a0aa
RATIS-1977. Improve Code.
slfan1989 May 23, 2025
4801472
RATIS-1977. Use jakarta annotation.
slfan1989 May 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 7 additions & 12 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,8 +221,10 @@
<testsThreadCount>4</testsThreadCount>

<slf4j.version>2.0.7</slf4j.version>
<junit-bom.version>5.11.2</junit-bom.version>
<junit-bom.version>5.12.2</junit-bom.version>
<mockito.version>4.11.0</mockito.version>
<jacoco.version>0.8.12</jacoco.version>
<jakarta.annotation.version>1.3.5</jakarta.annotation.version>
<flaky-test-groups>flaky | org.apache.ratis.test.tag.FlakyTest</flaky-test-groups>
</properties>

Expand Down Expand Up @@ -417,12 +419,6 @@
<scope>test</scope>
<version>${slf4j.version}</version>
</dependency>

<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
<version>4.13.2</version>
</dependency>
<dependency>
<groupId>org.junit</groupId>
<artifactId>junit-bom</artifactId>
Expand All @@ -433,13 +429,12 @@
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
<version>4.3.1</version>
<version>${mockito.version}</version>
</dependency>
<dependency>
<groupId>org.apache.tomcat</groupId>
<artifactId>annotations-api</artifactId>
<version>6.0.53</version>
<scope>provided</scope>
<groupId>jakarta.annotation</groupId>
<artifactId>jakarta.annotation-api</artifactId>
<version>${jakarta.annotation.version}</version>
</dependency>
</dependencies>
</dependencyManagement>
Expand Down
11 changes: 0 additions & 11 deletions ratis-common/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,6 @@
<artifactId>slf4j-api</artifactId>
</dependency>

<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
<scope>test</scope>
</dependency>

<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-api</artifactId>
Expand All @@ -54,11 +48,6 @@
<artifactId>junit-jupiter-engine</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.junit.vintage</groupId>
<artifactId>junit-vintage-engine</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.junit.platform</groupId>
<artifactId>junit-platform-launcher</artifactId>
Expand Down
9 changes: 0 additions & 9 deletions ratis-common/src/test/java/org/apache/ratis/BaseTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,6 @@
import org.apache.ratis.util.StringUtils;
import org.apache.ratis.util.TimeDuration;
import org.apache.ratis.util.function.CheckedRunnable;
import org.junit.After;
import org.junit.Before;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Assumptions;
Expand Down Expand Up @@ -72,8 +70,6 @@ public void setFirstException(Throwable e) {
}
}

// TODO: Junit 4 reference should be removed once all the unit tests are migrated to Junit 5.

private String testCaseName;

@BeforeEach
Expand All @@ -85,8 +81,6 @@ public void setup(TestInfo testInfo) {
+ "." + (method == null? null : method.getName());
}

// @Before annotation is retained to support junit 4 tests.
Copy link
Contributor

@szetszwo szetszwo May 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also remove the other two junit 4 comments:

+++ b/ratis-common/src/test/java/org/apache/ratis/BaseTest.java
@@ -72,8 +72,6 @@ public abstract class BaseTest {
     }
   }
 
-  // TODO: Junit 4 reference should be removed once all the unit tests are migrated to Junit 5.
-
   private String testCaseName;
 
   @BeforeEach
@@ -133,7 +131,6 @@ public abstract class BaseTest {
   }
 
   public File getTestDir() {
-    // This will work for both junit 4 and 5.
     return new File(getClassTestDir(), testCaseName);
   }

Copy link
Contributor Author

@slfan1989 slfan1989 May 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for pointing out these issues! I have made the changes as suggested.

@Before
@BeforeEach
public void checkAssumptions() {
final int leaks = ReferenceCountedLeakDetector.getLeakDetector().getLeakCount();
Expand All @@ -99,8 +93,6 @@ public void checkAssumptions() {
Assumptions.assumeTrue(exited == null, () -> "Already exited with " + exited);
}

// @After annotation is retained to support junit 4 tests.
@After
@AfterEach
public void assertNoFailures() {
final Throwable e = firstException.get();
Expand Down Expand Up @@ -133,7 +125,6 @@ public File getClassTestDir() {
}

public File getTestDir() {
// This will work for both junit 4 and 5.
return new File(getClassTestDir(), testCaseName);
}

Expand Down
5 changes: 0 additions & 5 deletions ratis-examples/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,11 +117,6 @@
<scope>runtime</scope>
</dependency>

<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-engine</artifactId>
Expand Down
4 changes: 2 additions & 2 deletions ratis-proto/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,8 +176,8 @@
<artifactId>ratis-thirdparty-misc</artifactId>
</dependency>
<dependency>
<groupId>org.apache.tomcat</groupId>
<artifactId>annotations-api</artifactId>
<groupId>jakarta.annotation</groupId>
<artifactId>jakarta.annotation-api</artifactId>
</dependency>
</dependencies>
</project>
10 changes: 0 additions & 10 deletions ratis-server/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,6 @@
<artifactId>slf4j-api</artifactId>
</dependency>

<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-api</artifactId>
Expand All @@ -80,11 +75,6 @@
<artifactId>junit-jupiter-engine</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.junit.vintage</groupId>
<artifactId>junit-vintage-engine</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.junit.platform</groupId>
<artifactId>junit-platform-launcher</artifactId>
Expand Down
4 changes: 2 additions & 2 deletions ratis-server/src/test/java/org/apache/ratis/RaftExceptionBaseTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
import org.apache.ratis.util.Slf4jUtils;
import org.apache.ratis.util.SizeInBytes;
import org.junit.jupiter.api.Assertions;
import org.junit.Assume;
import org.junit.jupiter.api.Assumptions;
import org.junit.jupiter.api.Test;
import org.slf4j.event.Level;

Expand Down Expand Up @@ -84,7 +84,7 @@ RaftClientReply assertNotLeaderException(RaftPeerId expectedSuggestedLeader,
final SimpleMessage message = new SimpleMessage(messageId);
final RaftClientReply reply = rpc.sendRequest(cluster.newRaftClientRequest(ClientId.randomId(), server, message));
Assertions.assertNotNull(reply);
Assume.assumeFalse(reply.isSuccess());
Assumptions.assumeFalse(reply.isSuccess());
final NotLeaderException nle = reply.getNotLeaderException();
Objects.requireNonNull(nle);
Assertions.assertEquals(expectedSuggestedLeader, nle.getSuggestedLeader().getId());
Expand Down
11 changes: 6 additions & 5 deletions ratis-server/src/test/java/org/apache/ratis/RaftTestUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@
import org.apache.ratis.util.Preconditions;
import org.apache.ratis.util.ProtoUtils;
import org.apache.ratis.util.TimeDuration;
import org.junit.AssumptionViolatedException;
import org.apache.ratis.util.function.CheckedConsumer;
import org.junit.jupiter.api.Assumptions;
import org.junit.jupiter.api.Assertions;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -479,18 +480,18 @@ static List<RaftPeer> getPeersWithPriority(List<RaftPeer> peers, RaftPeer sugges

static RaftPeerId changeLeader(MiniRaftCluster cluster, RaftPeerId oldLeader)
throws Exception {
return changeLeader(cluster, oldLeader, AssumptionViolatedException::new);
Copy link
Contributor Author

@slfan1989 slfan1989 May 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part needs some clarification: we've replaced org.junit.AssumptionViolatedException with org.opentest4j.TestAbortedException. The reason for this change is that JUnit 5 internally uses TestAbortedException when handling assumptions via org.junit.jupiter.api.Assumptions.assumeTrue. Given this, I believe it's appropriate to adopt TestAbortedException directly in our implementation.

Copy link
Contributor

@szetszwo szetszwo May 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use Assumptions.abort(..) since it is a public API.

diff --git a/ratis-server/src/test/java/org/apache/ratis/RaftTestUtil.java b/ratis-server/src/test/java/org/apache/ratis/RaftTestUtil.java
index c9db9933b2..d0641e39c4 100644
--- a/ratis-server/src/test/java/org/apache/ratis/RaftTestUtil.java
+++ b/ratis-server/src/test/java/org/apache/ratis/RaftTestUtil.java
@@ -44,7 +44,8 @@ import org.apache.ratis.util.JavaUtils;
 import org.apache.ratis.util.Preconditions;
 import org.apache.ratis.util.ProtoUtils;
 import org.apache.ratis.util.TimeDuration;
-import org.opentest4j.TestAbortedException;
+import org.apache.ratis.util.function.CheckedConsumer;
+import org.junit.jupiter.api.Assumptions;
 import org.junit.jupiter.api.Assertions;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -479,18 +480,18 @@ public interface RaftTestUtil {
 
   static RaftPeerId changeLeader(MiniRaftCluster cluster, RaftPeerId oldLeader)
       throws Exception {
-    return changeLeader(cluster, oldLeader, TestAbortedException::new);
+    return changeLeader(cluster, oldLeader, Assumptions::abort);
   }
 
-  static RaftPeerId changeLeader(MiniRaftCluster cluster, RaftPeerId oldLeader, Function<String, Exception> constructor)
-      throws Exception {
+  static RaftPeerId changeLeader(MiniRaftCluster cluster, RaftPeerId oldLeader,
+      CheckedConsumer<String, Exception> failToChangeLeaderHandler) throws Exception {
     final String name = JavaUtils.getCallerStackTraceElement().getMethodName() + "-changeLeader";
     cluster.setBlockRequestsFrom(oldLeader.toString(), true);
     try {
       return JavaUtils.attemptRepeatedly(() -> {
         final RaftPeerId newLeader = waitForLeader(cluster).getId();
         if (newLeader.equals(oldLeader)) {
-          throw constructor.apply("Failed to change leader: newLeader == oldLeader == " + oldLeader);
+          failToChangeLeaderHandler.accept("Failed to change leader: newLeader == oldLeader == " + oldLeader);
         }
         LOG.info("Changed leader from " + oldLeader + " to " + newLeader);
         return newLeader;
diff --git a/ratis-server/src/test/java/org/apache/ratis/server/impl/LeaderElectionTests.java b/ratis-server/src/test/java/org/apache/ratis/server/impl/LeaderElectionTests.java
index f35626894f..25caa9d06e 100644
--- a/ratis-server/src/test/java/org/apache/ratis/server/impl/LeaderElectionTests.java
+++ b/ratis-server/src/test/java/org/apache/ratis/server/impl/LeaderElectionTests.java
@@ -184,7 +184,7 @@ public abstract class LeaderElectionTests<CLUSTER extends MiniRaftCluster>
   void runTestChangeLeader(MiniRaftCluster cluster) throws Exception {
     RaftPeerId leader = RaftTestUtil.waitForLeader(cluster).getId();
     for(int i = 0; i < 10; i++) {
-      leader = RaftTestUtil.changeLeader(cluster, leader, IllegalStateException::new);
+      leader = RaftTestUtil.changeLeader(cluster, leader, Assertions::fail);
     }
   }

Copy link
Contributor Author

@slfan1989 slfan1989 May 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@szetszwo Thanks for the review! I’ve made the updates as suggested.

return changeLeader(cluster, oldLeader, Assumptions::abort);
}

static RaftPeerId changeLeader(MiniRaftCluster cluster, RaftPeerId oldLeader, Function<String, Exception> constructor)
throws Exception {
static RaftPeerId changeLeader(MiniRaftCluster cluster, RaftPeerId oldLeader,
CheckedConsumer<String, Exception> failToChangeLeaderHandler) throws Exception {
final String name = JavaUtils.getCallerStackTraceElement().getMethodName() + "-changeLeader";
cluster.setBlockRequestsFrom(oldLeader.toString(), true);
try {
return JavaUtils.attemptRepeatedly(() -> {
final RaftPeerId newLeader = waitForLeader(cluster).getId();
if (newLeader.equals(oldLeader)) {
throw constructor.apply("Failed to change leader: newLeader == oldLeader == " + oldLeader);
failToChangeLeaderHandler.accept("Failed to change leader: newLeader == oldLeader == " + oldLeader);
}
LOG.info("Changed leader from " + oldLeader + " to " + newLeader);
return newLeader;
Expand Down
2 changes: 1 addition & 1 deletion ratis-server/src/test/java/org/apache/ratis/server/impl/LeaderElectionTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ public void testChangeLeader() throws Exception {
void runTestChangeLeader(MiniRaftCluster cluster) throws Exception {
RaftPeerId leader = RaftTestUtil.waitForLeader(cluster).getId();
for(int i = 0; i < 10; i++) {
leader = RaftTestUtil.changeLeader(cluster, leader, IllegalStateException::new);
leader = RaftTestUtil.changeLeader(cluster, leader, Assertions::fail);
}
}

Expand Down
25 changes: 14 additions & 11 deletions ratis-server/src/test/java/org/apache/ratis/server/impl/StateMachineShutdownTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,10 @@
import org.apache.ratis.statemachine.impl.SimpleStateMachine4Testing;
import org.apache.ratis.statemachine.StateMachine;
import org.apache.ratis.statemachine.TransactionContext;
import org.junit.*;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.mockito.MockedStatic;
import org.mockito.Mockito;
import org.slf4j.Logger;
Expand Down Expand Up @@ -121,12 +124,12 @@ public void unblockAllTxns() {
}
}

@Before
@BeforeEach
public void setup() {
mocked = Mockito.mockStatic(CompletableFuture.class, Mockito.CALLS_REAL_METHODS);
}

@After
@AfterEach
public void tearDownClass() {
if (mocked != null) {
mocked.close();
Expand Down Expand Up @@ -162,10 +165,10 @@ public void testStateMachineShutdownWaitsForApplyTxn() throws Exception {
RaftClientReply watchReply = client.io().watch(
logIndex, RaftProtos.ReplicationLevel.ALL_COMMITTED);
watchReply.getCommitInfos().forEach(
val -> Assert.assertTrue(val.getCommitIndex() >= logIndex));
val -> Assertions.assertTrue(val.getCommitIndex() >= logIndex));
final RaftServer.Division secondFollower = cluster.getFollowers().get(1);
// Second follower is blocked in apply transaction
Assert.assertTrue(secondFollower.getInfo().getLastAppliedIndex() < logIndex);
Assertions.assertTrue(secondFollower.getInfo().getLastAppliedIndex() < logIndex);

// Now shutdown the follower in a separate thread
final Thread t = new Thread(secondFollower::close);
Expand All @@ -176,24 +179,24 @@ public void testStateMachineShutdownWaitsForApplyTxn() throws Exception {
// Now unblock the second follower
long minIndex = ((StateMachineWithConditionalWait) secondFollower.getStateMachine()).blockTxns.stream()
.min(Comparator.naturalOrder()).get();
Assert.assertEquals(2, StateMachineWithConditionalWait.numTxns.values().stream()
Assertions.assertEquals(2, StateMachineWithConditionalWait.numTxns.values().stream()
.filter(val -> val.get() == 3).count());
// The second follower should still be blocked in apply transaction
Assert.assertTrue(secondFollower.getInfo().getLastAppliedIndex() < minIndex);
Assertions.assertTrue(secondFollower.getInfo().getLastAppliedIndex() < minIndex);
for (long index : ((StateMachineWithConditionalWait) secondFollower.getStateMachine()).blockTxns) {
if (minIndex != index) {
((StateMachineWithConditionalWait) secondFollower.getStateMachine()).unBlockApplyTxn(index);
}
}
Assert.assertEquals(2, StateMachineWithConditionalWait.numTxns.values().stream()
Assertions.assertEquals(2, StateMachineWithConditionalWait.numTxns.values().stream()
.filter(val -> val.get() == 3).count());
Assert.assertTrue(secondFollower.getInfo().getLastAppliedIndex() < minIndex);
Assertions.assertTrue(secondFollower.getInfo().getLastAppliedIndex() < minIndex);
((StateMachineWithConditionalWait) secondFollower.getStateMachine()).unBlockApplyTxn(minIndex);

// Now wait for the thread
t.join(5000);
Assert.assertEquals(logIndex, secondFollower.getInfo().getLastAppliedIndex());
Assert.assertEquals(3, StateMachineWithConditionalWait.numTxns.values().stream()
Assertions.assertEquals(logIndex, secondFollower.getInfo().getLastAppliedIndex());
Assertions.assertEquals(3, StateMachineWithConditionalWait.numTxns.values().stream()
.filter(val -> val.get() == 3).count());

cluster.shutdown();
Expand Down
10 changes: 0 additions & 10 deletions ratis-test/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,11 +131,6 @@
<scope>test</scope>
</dependency>

<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-engine</artifactId>
Expand All @@ -151,11 +146,6 @@
<artifactId>junit-jupiter-params</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.junit.vintage</groupId>
<artifactId>junit-vintage-engine</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.junit.platform</groupId>
<artifactId>junit-platform-launcher</artifactId>
Expand Down
Loading