Skip to content

YARN-11258. [JDK17] Upgrade JUnit from 4 to 5 in hadoop-yarn-server-common. #7437

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
merged 1 commit into from
Mar 2, 2025
Merged

YARN-11258. [JDK17] Upgrade JUnit from 4 to 5 in hadoop-yarn-server-common. #7437

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 6 additions & 0 deletions ...rg/apache/hadoop/yarn/server/federation/store/impl/ZKFederationStateStoreOpDurations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import org.apache.hadoop.classification.InterfaceAudience;
import org.apache.hadoop.classification.InterfaceStability;
import org.apache.hadoop.classification.VisibleForTesting;
import org.apache.hadoop.metrics2.MetricsCollector;
import org.apache.hadoop.metrics2.MetricsInfo;
import org.apache.hadoop.metrics2.MetricsSource;
Expand Down Expand Up @@ -236,4 +237,9 @@ public void removeStoredTokenDuration(long startTime, long endTime) {
public void getTokenByRouterStoreTokenDuration(long startTime, long endTime) {
getTokenByRouterStoreToken.add(endTime - startTime);
}

@VisibleForTesting
protected ZKFederationStateStoreOpDurations resetOpDurations() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I understand why the resetOpDurations changes were made. (There's a singleton instance holding state across test cases.) However, did something about JUnit 5 trigger the need for this? Is it just an unrelated bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you very much for your question! it's a very good one. Apologies for the delayed response, as I've been caught up with some work in the past 1-2 days.

I have noticed a difference in behavior between Junit4 and Junit5 when running unit tests with inheritance. In Junit4, the test methods of the parent and child classes are executed separately, while in Junit5, the test methods of the parent and child classes are coupled together and executed in the child class.

  • Junit4: The test methods of the parent and child classes are generally executed separately. If we define a test method in the parent class, the child class can inherit these methods and also define its own test methods. During execution, Junit4 will first execute the parent class's test methods, then the child class's test methods.

  • Junit5: In Junit5, the test methods in the parent class and the child class are executed together. The inheritance mechanism in Junit5 causes the parent class's test methods to be inherited into the child class, and by default, both the parent and child test methods are executed, with a different execution order compared to Junit4.

This difference leads to an issue: in the TestZookeeperFederationStateStore#testMetricsInited test, my goal is to check whether the metric accumulation behaves as expected.

In Junit4, ZKFederationStateStoreOpDurations is recreated when executing the child class's tests, so the data it contains is the initial value, which matches the expected result.

However, in Junit5, due to the singleton nature of ZKFederationStateStoreOpDurations, it accumulates metric data from all methods in the unit test, leading to a result that does not meet expectations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To ensure that the unit test passes in Junit5, I added a reset method here to make the execution result match the expected outcome.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand it now. Thanks for the detailed explanation!

return new ZKFederationStateStoreOpDurations();
}
}
5 changes: 5 additions & 0 deletions ...va/org/apache/hadoop/yarn/server/federation/store/impl/ZookeeperFederationStateStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2095,4 +2095,9 @@ private List<ApplicationHomeSubCluster> loadRouterApplications() throws Exceptio
}
return applicationHomeSubClusters;
}

@VisibleForTesting
public void resetOpDurations() {
opDurations = opDurations.resetOpDurations();
}
}
56 changes: 30 additions & 26 deletions ...p-yarn-server/hadoop-yarn-server-common/src/test/java/org/apache/hadoop/yarn/TestRPC.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,12 @@
import org.apache.hadoop.yarn.server.api.protocolrecords.ReportNewCollectorInfoResponse;
import org.apache.hadoop.yarn.server.api.records.AppCollectorData;
import org.apache.hadoop.yarn.util.Records;
import org.junit.Assert;
import org.junit.Test;
import org.junit.jupiter.api.Test;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;

public class TestRPC {

Expand Down Expand Up @@ -136,9 +140,9 @@ public void testUnknownCall() {
try {
proxy.getNewApplication(Records
.newRecord(GetNewApplicationRequest.class));
Assert.fail("Excepted RPC call to fail with unknown method.");
fail("Excepted RPC call to fail with unknown method.");
} catch (YarnException e) {
Assert.assertTrue(e.getMessage().matches(
assertTrue(e.getMessage().matches(
"Unknown method getNewApplication called on.*"
+ "org.apache.hadoop.yarn.proto.ApplicationClientProtocol"
+ "\\$ApplicationClientProtocolService\\$BlockingInterface "
Expand Down Expand Up @@ -171,9 +175,9 @@ public void testRPCOnCollectorNodeManagerProtocol() throws IOException {
try {
unknownProxy.getNewApplication(Records
.newRecord(GetNewApplicationRequest.class));
Assert.fail("Excepted RPC call to fail with unknown method.");
fail("Excepted RPC call to fail with unknown method.");
} catch (YarnException e) {
Assert.assertTrue(e.getMessage().matches(
assertTrue(e.getMessage().matches(
"Unknown method getNewApplication called on.*"
+ "org.apache.hadoop.yarn.proto.ApplicationClientProtocol"
+ "\\$ApplicationClientProtocolService\\$BlockingInterface "
Expand All @@ -195,7 +199,7 @@ public void testRPCOnCollectorNodeManagerProtocol() throws IOException {
DEFAULT_APP_ID, DEFAULT_COLLECTOR_ADDR, null);
proxy.reportNewCollectorInfo(request);
} catch (YarnException e) {
Assert.fail("RPC call failured is not expected here.");
fail("RPC call failured is not expected here.");
}

try {
Expand All @@ -204,17 +208,17 @@ public void testRPCOnCollectorNodeManagerProtocol() throws IOException {
DEFAULT_APP_ID, DEFAULT_COLLECTOR_ADDR, DEFAULT_COLLECTOR_TOKEN);
proxy.reportNewCollectorInfo(request);
} catch (YarnException e) {
Assert.fail("RPC call failured is not expected here.");
fail("RPC call failured is not expected here.");
}

// Verify empty request get YarnException back (by design in
// DummyNMCollectorService)
try {
proxy.reportNewCollectorInfo(Records
.newRecord(ReportNewCollectorInfoRequest.class));
Assert.fail("Excepted RPC call to fail with YarnException.");
fail("Excepted RPC call to fail with YarnException.");
} catch (YarnException e) {
Assert.assertTrue(e.getMessage().contains(ILLEGAL_NUMBER_MESSAGE));
assertTrue(e.getMessage().contains(ILLEGAL_NUMBER_MESSAGE));
}

// Verify request with a valid app ID
Expand All @@ -224,12 +228,12 @@ public void testRPCOnCollectorNodeManagerProtocol() throws IOException {
ApplicationId.newInstance(0, 1));
GetTimelineCollectorContextResponse response =
proxy.getTimelineCollectorContext(request);
Assert.assertEquals("test_user_id", response.getUserId());
Assert.assertEquals("test_flow_name", response.getFlowName());
Assert.assertEquals("test_flow_version", response.getFlowVersion());
Assert.assertEquals(12345678L, response.getFlowRunId());
assertEquals("test_user_id", response.getUserId());
assertEquals("test_flow_name", response.getFlowName());
assertEquals("test_flow_version", response.getFlowVersion());
assertEquals(12345678L, response.getFlowRunId());
} catch (YarnException | IOException e) {
Assert.fail("RPC call failured is not expected here.");
fail("RPC call failured is not expected here.");
}

// Verify request with an invalid app ID
Expand All @@ -238,10 +242,10 @@ public void testRPCOnCollectorNodeManagerProtocol() throws IOException {
GetTimelineCollectorContextRequest.newInstance(
ApplicationId.newInstance(0, 2));
proxy.getTimelineCollectorContext(request);
Assert.fail("RPC call failured is expected here.");
fail("RPC call failured is expected here.");
} catch (YarnException | IOException e) {
Assert.assertTrue(e instanceof YarnException);
Assert.assertTrue(e.getMessage().contains(
assertTrue(e instanceof YarnException);
assertTrue(e.getMessage().contains(
"The application is not found."));
}
server.stop();
Expand Down Expand Up @@ -309,17 +313,17 @@ private void test(String rpcClass) throws Exception {
proxy.stopContainers(stopRequest);
} catch (YarnException e) {
exception = true;
Assert.assertTrue(e.getMessage().contains(EXCEPTION_MSG));
Assert.assertTrue(e.getMessage().contains(EXCEPTION_CAUSE));
assertTrue(e.getMessage().contains(EXCEPTION_MSG));
assertTrue(e.getMessage().contains(EXCEPTION_CAUSE));
System.out.println("Test Exception is " + e.getMessage());
} catch (Exception ex) {
ex.printStackTrace();
} finally {
server.stop();
}
Assert.assertTrue(exception);
Assert.assertNotNull(statuses.get(0));
Assert.assertEquals(ContainerState.RUNNING, statuses.get(0).getState());
assertTrue(exception);
assertNotNull(statuses.get(0));
assertEquals(ContainerState.RUNNING, statuses.get(0).getState());
}

public class DummyContainerManager implements ContainerManagementProtocol {
Expand Down Expand Up @@ -468,11 +472,11 @@ public ReportNewCollectorInfoResponse reportNewCollectorInfo(
if (appCollectors.size() == 1) {
// check default appID and collectorAddr
AppCollectorData appCollector = appCollectors.get(0);
Assert.assertEquals(appCollector.getApplicationId(),
assertEquals(appCollector.getApplicationId(),
DEFAULT_APP_ID);
Assert.assertEquals(appCollector.getCollectorAddr(),
assertEquals(appCollector.getCollectorAddr(),
DEFAULT_COLLECTOR_ADDR);
Assert.assertTrue(appCollector.getCollectorToken() == null ||
assertTrue(appCollector.getCollectorToken() == null ||
appCollector.getCollectorToken().equals(DEFAULT_COLLECTOR_TOKEN));
} else {
throw new YarnException(ILLEGAL_NUMBER_MESSAGE);
Expand Down
14 changes: 8 additions & 6 deletions ...n-server-common/src/test/java/org/apache/hadoop/yarn/TestResourceTrackerPBClientImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,13 @@
import org.apache.hadoop.yarn.server.api.protocolrecords.RegisterNodeManagerResponse;
import org.apache.hadoop.yarn.server.api.protocolrecords.UnRegisterNodeManagerRequest;
import org.apache.hadoop.yarn.server.api.protocolrecords.UnRegisterNodeManagerResponse;
import org.junit.AfterClass;
import org.junit.BeforeClass;
import org.junit.Test;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;

import static org.junit.Assert.*;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;

/**
* Test ResourceTrackerPBClientImpl. this class should have methods
Expand All @@ -52,7 +54,7 @@ public class TestResourceTrackerPBClientImpl {
private final static org.apache.hadoop.yarn.factories.RecordFactory recordFactory = RecordFactoryProvider
.getRecordFactory(null);

@BeforeClass
@BeforeAll
public static void start() {
InetSocketAddress address = new InetSocketAddress(0);
Configuration configuration = new Configuration();
Expand All @@ -67,7 +69,7 @@ public static void start() {

}

@AfterClass
@AfterAll
public static void stop() {
if (server != null) {
server.stop();
Expand Down
12 changes: 6 additions & 6 deletions ...r/hadoop-yarn-server-common/src/test/java/org/apache/hadoop/yarn/TestYSCRPCFactories.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,6 @@
import java.io.IOException;
import java.net.InetSocketAddress;

import org.junit.Assert;

import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.ipc.Server;
import org.apache.hadoop.net.NetUtils;
Expand All @@ -37,7 +35,9 @@
import org.apache.hadoop.yarn.server.api.protocolrecords.RegisterNodeManagerResponse;
import org.apache.hadoop.yarn.server.api.protocolrecords.UnRegisterNodeManagerRequest;
import org.apache.hadoop.yarn.server.api.protocolrecords.UnRegisterNodeManagerResponse;
import org.junit.Test;
import org.junit.jupiter.api.Test;

import static org.junit.jupiter.api.Assertions.fail;

public class TestYSCRPCFactories {

Expand All @@ -64,7 +64,7 @@ private void testPbServerFactory() {
server.start();
} catch (YarnRuntimeException e) {
e.printStackTrace();
Assert.fail("Failed to create server");
fail("Failed to create server");
} finally {
server.stop();
}
Expand All @@ -90,12 +90,12 @@ private void testPbClientFactory() {
client = (ResourceTracker) RpcClientFactoryPBImpl.get().getClient(ResourceTracker.class, 1, NetUtils.getConnectAddress(server), conf);
} catch (YarnRuntimeException e) {
e.printStackTrace();
Assert.fail("Failed to create client");
fail("Failed to create client");
}

} catch (YarnRuntimeException e) {
e.printStackTrace();
Assert.fail("Failed to create server");
fail("Failed to create server");
} finally {
server.stop();
}
Expand Down
11 changes: 6 additions & 5 deletions .../hadoop-yarn-server-common/src/test/java/org/apache/hadoop/yarn/TestYSCRecordFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,15 @@

package org.apache.hadoop.yarn;

import org.junit.Assert;

import org.apache.hadoop.yarn.exceptions.YarnRuntimeException;
import org.apache.hadoop.yarn.factories.RecordFactory;
import org.apache.hadoop.yarn.factories.impl.pb.RecordFactoryPBImpl;
import org.apache.hadoop.yarn.server.api.protocolrecords.NodeHeartbeatRequest;
import org.apache.hadoop.yarn.server.api.protocolrecords.impl.pb.NodeHeartbeatRequestPBImpl;
import org.junit.Test;
import org.junit.jupiter.api.Test;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.fail;

public class TestYSCRecordFactory {

Expand All @@ -34,10 +35,10 @@ public void testPbRecordFactory() {
RecordFactory pbRecordFactory = RecordFactoryPBImpl.get();
try {
NodeHeartbeatRequest request = pbRecordFactory.newRecordInstance(NodeHeartbeatRequest.class);
Assert.assertEquals(NodeHeartbeatRequestPBImpl.class, request.getClass());
assertEquals(NodeHeartbeatRequestPBImpl.class, request.getClass());
} catch (YarnRuntimeException e) {
e.printStackTrace();
Assert.fail("Failed to crete record");
fail("Failed to crete record");
}

}
Expand Down
31 changes: 16 additions & 15 deletions ...oop-yarn-server-common/src/test/java/org/apache/hadoop/yarn/TestYarnServerApiClasses.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,11 @@

package org.apache.hadoop.yarn;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertTrue;

import java.io.IOException;
import java.nio.ByteBuffer;
Expand Down Expand Up @@ -63,8 +65,7 @@
import org.apache.hadoop.yarn.server.api.records.impl.pb.MasterKeyPBImpl;
import org.apache.hadoop.yarn.server.api.records.impl.pb.NodeStatusPBImpl;
import org.apache.hadoop.yarn.server.utils.YarnServerBuilderUtils;
import org.junit.Assert;
import org.junit.Test;
import org.junit.jupiter.api.Test;

/**
* Simple test classes from org.apache.hadoop.yarn.server.api
Expand Down Expand Up @@ -127,14 +128,14 @@ public void testNodeHeartbeatRequestPBImpl() {
assertEquals("localhost", copy.getNodeStatus().getNodeId().getHost());
assertEquals(collectors, copy.getRegisteringCollectors());
// check labels are coming with valid values
Assert.assertTrue(original.getNodeLabels()
assertTrue(original.getNodeLabels()
.containsAll(copy.getNodeLabels()));
// check for empty labels
original.setNodeLabels(new HashSet<NodeLabel> ());
copy = new NodeHeartbeatRequestPBImpl(
original.getProto());
Assert.assertNotNull(copy.getNodeLabels());
Assert.assertEquals(0, copy.getNodeLabels().size());
assertNotNull(copy.getNodeLabels());
assertEquals(0, copy.getNodeLabels().size());
}

@Test
Expand All @@ -155,7 +156,7 @@ public void testNodeHeartbeatRequestPBImplWithNullLabels() {
NodeHeartbeatRequestPBImpl original = new NodeHeartbeatRequestPBImpl();
NodeHeartbeatRequestPBImpl copy =
new NodeHeartbeatRequestPBImpl(original.getProto());
Assert.assertNull(copy.getNodeLabels());
assertNull(copy.getNodeLabels());
}

/**
Expand Down Expand Up @@ -218,7 +219,7 @@ public void testNodeHeartbeatResponsePBImpl() throws IOException {
YarnServerBuilderUtils
.convertFromProtoFormat(copy.getSystemCredentialsForApps())
.get(getApplicationId(1));
Assert.assertNotNull(buffer);
assertNotNull(buffer);
buffer.rewind();
buf.reset(buffer);
credentials1Out.readTokenStorageStream(buf);
Expand Down Expand Up @@ -370,7 +371,7 @@ public void testRegisterNodeManagerRequestWithNullLabels() {
((RegisterNodeManagerRequestPBImpl) request).getProto());

// check labels are coming with no values
Assert.assertNull(request1.getNodeLabels());
assertNull(request1.getNodeLabels());
}

@Test
Expand All @@ -387,14 +388,14 @@ public void testRegisterNodeManagerRequestWithValidLabels() {
((RegisterNodeManagerRequestPBImpl) request).getProto());

// check labels are coming with valid values
Assert.assertEquals(true, nodeLabels.containsAll(copy.getNodeLabels()));
assertEquals(true, nodeLabels.containsAll(copy.getNodeLabels()));

// check for empty labels
request.setNodeLabels(new HashSet<NodeLabel> ());
copy = new RegisterNodeManagerRequestPBImpl(
((RegisterNodeManagerRequestPBImpl) request).getProto());
Assert.assertNotNull(copy.getNodeLabels());
Assert.assertEquals(0, copy.getNodeLabels().size());
assertNotNull(copy.getNodeLabels());
assertEquals(0, copy.getNodeLabels().size());
}

@Test
Expand All @@ -405,7 +406,7 @@ public void testUnRegisterNodeManagerRequestPBImpl() throws Exception {

UnRegisterNodeManagerRequestPBImpl copy = new UnRegisterNodeManagerRequestPBImpl(
request.getProto());
Assert.assertEquals(nodeId, copy.getNodeId());
assertEquals(nodeId, copy.getNodeId());
}

private HashSet<NodeLabel> getValidNodeLabels() {
Expand Down
Loading