Copilot commented on code in PR #8112:
URL: https://github.com/apache/incubator-seata/pull/8112#discussion_r3328736614
##########
server/pom.xml:
##########
@@ -445,6 +445,35 @@
</execution>
</executions>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-surefire-plugin</artifactId>
+ <configuration>
+ <forkCount>2</forkCount>
+ <reuseForks>true</reuseForks>
+ <excludes>
+ <exclude>**/cluster/raft/execute/*Test.java</exclude>
+ <exclude>**/cluster/raft/RaftServerTest.java</exclude>
+ </excludes>
+ </configuration>
Review Comment:
Inheriting from the parent POM already configures surefire <excludes> as a
scalar property. Defining <excludes> here without combine.self="override" can
cause Maven to merge the parent value with this list, yielding an unexpected
exclude set (and in some Maven versions, an invalid config shape). Override the
parent excludes explicitly so only the raft patterns are excluded in the
default execution.
##########
test-suite/test-new-version/src/test/java/org/apache/seata/core/rpc/netty/BaseNettyClientTest.java:
##########
@@ -119,36 +123,66 @@ protected ServerInstance startServerSimple(int port)
throws Exception {
serverConfig.setServerListenPort(port);
NettyRemotingServer nettyRemotingServer = new
NettyRemotingServer(workingThreads, serverConfig);
+ AtomicBoolean serverStatus = new AtomicBoolean();
new Thread(() -> {
- SessionHolder.init(null);
-
nettyRemotingServer.setHandler(DefaultCoordinator.getInstance(nettyRemotingServer));
- // set registry
- XID.setIpAddress(NetUtil.getLocalIp());
- XID.setPort(port);
- // init snowflake for transactionId, branchId
- UUIDGenerator.init(1L);
- nettyRemotingServer.init();
+ try {
+ SessionHolder.init(null);
+
nettyRemotingServer.setHandler(DefaultCoordinator.getInstance(nettyRemotingServer));
Review Comment:
startServerSimple() polls up to 10s but only tracks a boolean "started"
state; if init() fails quickly (e.g., bind error because the port was taken),
the method will still sleep/poll until timeout and then throws a generic
message. Capturing the failure (Throwable) and/or using a latch to signal
completion would fail fast and surface the root cause, reducing CI time and
flakiness diagnostics.
##########
test-suite/test-new-version/src/test/java/org/apache/seata/core/rpc/netty/BaseNettyClientTest.java:
##########
@@ -42,6 +42,10 @@ public abstract class BaseNettyClientTest {
private static final Logger LOGGER =
LoggerFactory.getLogger(BaseNettyClientTest.class);
+ private String originalGroupList;
+ private String originalServerPort;
+ private String originalShutdownWait;
+
Review Comment:
BaseNettyClientTest.cleanupAfterTest() always calls cleanupClientConfig(),
but the original property values are only captured when configureClient() is
called. For tests that don't call configureClient(), cleanupClientConfig() will
clear system properties (and SHUTDOWN_WAIT) unconditionally, which can cause
cross-test contamination in the same fork. Track whether configureClient() was
applied before attempting to restore/clear.
##########
test-suite/test-new-version/src/test/java/org/apache/seata/saga/engine/db/AbstractServerTest.java:
##########
@@ -38,10 +42,36 @@
*/
public abstract class AbstractServerTest {
+ private static final int SERVER_PORT = findAvailablePort();
+
+ private static String originalConfigType;
+ private static String originalConfigFileName;
+ private static String originalGroupList;
+
+ static {
+ originalConfigType = System.getProperty("config.type");
+ originalConfigFileName = System.getProperty("config.file.name");
+ originalGroupList = System.getProperty("service.default.grouplist");
+ System.setProperty("config.type", "file");
+ System.setProperty("config.file.name", "file.conf");
+ System.setProperty("service.default.grouplist", "127.0.0.1:" +
SERVER_PORT);
+ ConfigurationFactory.reload();
+ ConfigurationCache.clear();
+ }
Review Comment:
This class modifies global configuration (config.type / config.file.name /
service.default.grouplist) in a static initializer. With surefire reuseForks +
multiple tests per fork, any failure that prevents stopSeataServer() from
running will leave the fork JVM in a modified configuration state and can break
unrelated tests. Prefer moving this property setup into a dedicated
`@BeforeAll` (and restoration into @AfterAll) or into
startSeataServer()/stopSeataServer() with try/finally in subclasses, so config
changes are scoped to the tests that actually start the server.
##########
test-suite/test-new-version/src/test/java/org/apache/seata/core/rpc/netty/BaseNettyClientTest.java:
##########
@@ -119,36 +123,66 @@ protected ServerInstance startServerSimple(int port)
throws Exception {
serverConfig.setServerListenPort(port);
NettyRemotingServer nettyRemotingServer = new
NettyRemotingServer(workingThreads, serverConfig);
+ AtomicBoolean serverStatus = new AtomicBoolean();
new Thread(() -> {
- SessionHolder.init(null);
-
nettyRemotingServer.setHandler(DefaultCoordinator.getInstance(nettyRemotingServer));
- // set registry
- XID.setIpAddress(NetUtil.getLocalIp());
- XID.setPort(port);
- // init snowflake for transactionId, branchId
- UUIDGenerator.init(1L);
- nettyRemotingServer.init();
+ try {
+ SessionHolder.init(null);
+
nettyRemotingServer.setHandler(DefaultCoordinator.getInstance(nettyRemotingServer));
+ XID.setIpAddress(NetUtil.getLocalIp());
+ XID.setPort(port);
+ UUIDGenerator.init(1L);
+ nettyRemotingServer.init();
+ serverStatus.set(true);
+ } catch (Throwable t) {
+ serverStatus.set(false);
+ LOGGER.error("The seata-server failed to start", t);
+ }
})
.start();
- Thread.sleep(3000); // Simple wait
+ long start = System.nanoTime();
+ long maxWaitNanoTime = 10_000_000_000L;
+ while (System.nanoTime() - start < maxWaitNanoTime) {
+ Thread.sleep(100);
+ if (serverStatus.get()) {
+ break;
+ }
+ }
+ if (!serverStatus.get()) {
+ throw new RuntimeException("Waiting for a while, but the
seata-server did not start successfully.");
+ }
return new ServerInstance(nettyRemotingServer, port);
}
/**
* Configure client to use the specified port
*/
protected void configureClient(int port) {
- ConfigurationTestHelper.putConfig("service.default.grouplist",
"127.0.0.1:" + port);
-
ConfigurationTestHelper.putConfig(ConfigurationKeys.SERVER_SERVICE_PORT_CAMEL,
String.valueOf(port));
+ originalGroupList = System.getProperty("service.default.grouplist");
+ originalServerPort =
System.getProperty(ConfigurationKeys.SERVER_SERVICE_PORT_CAMEL);
+ originalShutdownWait =
System.getProperty(ConfigurationKeys.SHUTDOWN_WAIT);
Review Comment:
configureClient() should mark that it has modified global client-related
system properties so cleanupClientConfig() can be a no-op when the client was
never configured (avoids clearing unrelated properties in tests that only use
helper methods like getDynamicPort()).
##########
test-suite/test-new-version/src/test/java/org/apache/seata/core/rpc/netty/BaseNettyClientTest.java:
##########
@@ -119,36 +123,66 @@ protected ServerInstance startServerSimple(int port)
throws Exception {
serverConfig.setServerListenPort(port);
NettyRemotingServer nettyRemotingServer = new
NettyRemotingServer(workingThreads, serverConfig);
+ AtomicBoolean serverStatus = new AtomicBoolean();
new Thread(() -> {
- SessionHolder.init(null);
-
nettyRemotingServer.setHandler(DefaultCoordinator.getInstance(nettyRemotingServer));
- // set registry
- XID.setIpAddress(NetUtil.getLocalIp());
- XID.setPort(port);
- // init snowflake for transactionId, branchId
- UUIDGenerator.init(1L);
- nettyRemotingServer.init();
+ try {
+ SessionHolder.init(null);
+
nettyRemotingServer.setHandler(DefaultCoordinator.getInstance(nettyRemotingServer));
+ XID.setIpAddress(NetUtil.getLocalIp());
+ XID.setPort(port);
+ UUIDGenerator.init(1L);
+ nettyRemotingServer.init();
+ serverStatus.set(true);
+ } catch (Throwable t) {
+ serverStatus.set(false);
+ LOGGER.error("The seata-server failed to start", t);
+ }
})
.start();
- Thread.sleep(3000); // Simple wait
+ long start = System.nanoTime();
+ long maxWaitNanoTime = 10_000_000_000L;
+ while (System.nanoTime() - start < maxWaitNanoTime) {
+ Thread.sleep(100);
+ if (serverStatus.get()) {
+ break;
+ }
+ }
+ if (!serverStatus.get()) {
+ throw new RuntimeException("Waiting for a while, but the
seata-server did not start successfully.");
+ }
return new ServerInstance(nettyRemotingServer, port);
}
/**
* Configure client to use the specified port
*/
protected void configureClient(int port) {
- ConfigurationTestHelper.putConfig("service.default.grouplist",
"127.0.0.1:" + port);
-
ConfigurationTestHelper.putConfig(ConfigurationKeys.SERVER_SERVICE_PORT_CAMEL,
String.valueOf(port));
+ originalGroupList = System.getProperty("service.default.grouplist");
+ originalServerPort =
System.getProperty(ConfigurationKeys.SERVER_SERVICE_PORT_CAMEL);
+ originalShutdownWait =
System.getProperty(ConfigurationKeys.SHUTDOWN_WAIT);
+ System.setProperty("service.default.grouplist", "127.0.0.1:" + port);
+ System.setProperty(ConfigurationKeys.SERVER_SERVICE_PORT_CAMEL,
String.valueOf(port));
+ System.setProperty(ConfigurationKeys.SHUTDOWN_WAIT, "0");
+ ConfigurationCache.clear();
}
/**
* Clean up client configuration
*/
protected void cleanupClientConfig() {
- ConfigurationTestHelper.removeConfig("service.default.grouplist");
-
ConfigurationTestHelper.removeConfig(ConfigurationKeys.SERVER_SERVICE_PORT_CAMEL);
+ restoreProperty("service.default.grouplist", originalGroupList);
+ restoreProperty(ConfigurationKeys.SERVER_SERVICE_PORT_CAMEL,
originalServerPort);
+ restoreProperty(ConfigurationKeys.SHUTDOWN_WAIT, originalShutdownWait);
+ ConfigurationCache.clear();
+ }
Review Comment:
cleanupClientConfig() should avoid restoring/clearing properties when
configureClient() was never called in the test, otherwise `@AfterEach` will
clear system properties for tests that only exercise helper methods. Reset the
flag after cleanup so repeated calls remain safe.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
