Copilot commented on code in PR #8112:
URL: https://github.com/apache/incubator-seata/pull/8112#discussion_r3328622397
##########
test-suite/test-new-version/src/test/java/org/apache/seata/saga/engine/db/AbstractServerTest.java:
##########
@@ -38,10 +42,34 @@
*/
public abstract class AbstractServerTest {
+ private static final int SERVER_PORT = findAvailablePort();
+
+ static {
+ System.setProperty("config.type", "file");
+ System.setProperty("config.file.name", "file.conf");
+ System.setProperty("service.default.grouplist", "127.0.0.1:" +
SERVER_PORT);
+ try {
+ Method method =
ConfigurationFactory.class.getDeclaredMethod("reload");
+ method.setAccessible(true);
+ method.invoke(null);
+ } catch (Exception e) {
Review Comment:
Calling ConfigurationFactory.reload() via reflection is unnecessary (the
method is public) and can fail under stricter reflection/access rules. Prefer
invoking ConfigurationFactory.reload() directly and drop the reflective Method
usage.
##########
test-suite/test-new-version/src/test/java/org/apache/seata/saga/engine/db/AbstractServerTest.java:
##########
@@ -38,10 +42,34 @@
*/
public abstract class AbstractServerTest {
+ private static final int SERVER_PORT = findAvailablePort();
+
+ static {
+ System.setProperty("config.type", "file");
+ System.setProperty("config.file.name", "file.conf");
+ System.setProperty("service.default.grouplist", "127.0.0.1:" +
SERVER_PORT);
+ try {
+ Method method =
ConfigurationFactory.class.getDeclaredMethod("reload");
+ method.setAccessible(true);
+ method.invoke(null);
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ }
+
private static NettyRemotingServer nettyServer;
- private static final ThreadPoolExecutor workingThreads = new
ThreadPoolExecutor(
+ private static final ThreadPoolExecutor WORKING_THREADS = new
ThreadPoolExecutor(
100, 500, 500, TimeUnit.SECONDS, new LinkedBlockingQueue(20000),
new ThreadPoolExecutor.CallerRunsPolicy());
+ private static int findAvailablePort() {
+ try (ServerSocket socket = new ServerSocket(0)) {
+ socket.setReuseAddress(true);
+ return socket.getLocalPort();
+ } catch (IOException e) {
+ return 8091;
+ }
Review Comment:
Falling back to a fixed port (8091) when ephemeral port allocation fails can
introduce hard-to-debug port conflicts and flaky tests. Fail fast with a clear
exception instead of silently using a default port.
##########
test-suite/test-new-version/src/test/java/org/apache/seata/core/rpc/netty/mockserver/ProtocolTestConstants.java:
##########
@@ -16,12 +16,28 @@
*/
package org.apache.seata.core.rpc.netty.mockserver;
+import java.io.IOException;
+import java.net.ServerSocket;
+
/**
* Mock Constants
**/
public class ProtocolTestConstants {
public static final String APPLICATION_ID = "mock_tx_app_id";
public static final String SERVICE_GROUP = "mock_tx_group";
- public static final int MOCK_SERVER_PORT = 8099;
+ public static final int MOCK_SERVER_PORT = findAvailablePort();
public static final String MOCK_SERVER_ADDRESS = "0.0.0.0:" +
MOCK_SERVER_PORT;
+
+ static {
+ System.setProperty("service.mock.grouplist", "127.0.0.1:" +
MOCK_SERVER_PORT);
+ }
Review Comment:
Setting system properties in a constants holder class creates hidden global
side effects that are hard to reason about (and hard to clean up) when tests
run with fork reuse. Prefer moving this property setup into the relevant test
@BeforeAll/@BeforeEach and restoring it in @AfterAll/@AfterEach so tests remain
isolated.
##########
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();
}
Review Comment:
The PR description calls out ConfigurationTestHelper.putConfig() as a major
source of 60s stalls, but there are still several tests in
test-suite/test-new-version that call ConfigurationTestHelper.putConfig (e.g.
core/rpc/netty/mockserver/* and integration/rocketmq/SeataMQProducerSendTest).
To achieve the claimed CI-time reduction consistently, those remaining call
sites should be migrated to System.setProperty + ConfigurationCache.clear(), or
ConfigurationTestHelper itself should be updated to avoid waiting on listeners
that don’t fire.
##########
test-suite/test-new-version/src/test/java/org/apache/seata/core/rpc/netty/mockserver/ProtocolTestConstants.java:
##########
@@ -16,12 +16,28 @@
*/
package org.apache.seata.core.rpc.netty.mockserver;
+import java.io.IOException;
+import java.net.ServerSocket;
+
/**
* Mock Constants
**/
public class ProtocolTestConstants {
public static final String APPLICATION_ID = "mock_tx_app_id";
public static final String SERVICE_GROUP = "mock_tx_group";
- public static final int MOCK_SERVER_PORT = 8099;
+ public static final int MOCK_SERVER_PORT = findAvailablePort();
public static final String MOCK_SERVER_ADDRESS = "0.0.0.0:" +
MOCK_SERVER_PORT;
+
+ static {
+ System.setProperty("service.mock.grouplist", "127.0.0.1:" +
MOCK_SERVER_PORT);
+ }
+
+ private static int findAvailablePort() {
+ try (ServerSocket socket = new ServerSocket(0)) {
+ socket.setReuseAddress(true);
+ return socket.getLocalPort();
+ } catch (IOException e) {
+ return 8099;
+ }
Review Comment:
Falling back to a fixed port (8099) defeats the purpose of using an
ephemeral port and can cause port collisions/flaky tests when the port is
already in use. Prefer failing fast with an exception so the test run surfaces
the real root cause.
##########
test-suite/test-new-version/src/test/java/org/apache/seata/core/rpc/netty/mockserver/ProtocolTestConstants.java:
##########
@@ -16,12 +16,28 @@
*/
package org.apache.seata.core.rpc.netty.mockserver;
+import java.io.IOException;
+import java.net.ServerSocket;
+
/**
* Mock Constants
**/
public class ProtocolTestConstants {
public static final String APPLICATION_ID = "mock_tx_app_id";
public static final String SERVICE_GROUP = "mock_tx_group";
- public static final int MOCK_SERVER_PORT = 8099;
+ public static final int MOCK_SERVER_PORT = findAvailablePort();
public static final String MOCK_SERVER_ADDRESS = "0.0.0.0:" +
MOCK_SERVER_PORT;
+
+ static {
+ System.setProperty("service.mock.grouplist", "127.0.0.1:" +
MOCK_SERVER_PORT);
+ }
Review Comment:
This class sets a global system property but doesn’t clear the configuration
cache, so the new value may not be observed if ConfigurationCache has already
cached config in the current JVM/fork. Clearing the cache here makes the
dynamic port/group list deterministic when tests are run with fork reuse.
##########
test-suite/test-new-version/src/test/java/org/apache/seata/core/rpc/netty/multiversion/AbstractMultiVersionCompatibilityTest.java:
##########
@@ -169,14 +172,20 @@ public void tearDown() throws InterruptedException {
serverWorkingThreads.shutdown();
}
- bossGroup.shutdownGracefully().sync();
- workerGroup.shutdownGracefully().sync();
- clientGroup.shutdownGracefully().sync();
+ bossGroup.shutdownGracefully(0, 2, TimeUnit.SECONDS).sync();
+ workerGroup.shutdownGracefully(0, 2, TimeUnit.SECONDS).sync();
+ clientGroup.shutdownGracefully(0, 2, TimeUnit.SECONDS).sync();
if (StringUtils.isBlank(originalTransportProtocol)) {
-
ConfigurationTestHelper.removeConfig(ConfigurationKeys.TRANSPORT_PROTOCOL);
+ System.clearProperty(ConfigurationKeys.TRANSPORT_PROTOCOL);
} else {
-
ConfigurationTestHelper.putConfig(ConfigurationKeys.TRANSPORT_PROTOCOL,
originalTransportProtocol);
+ System.setProperty(ConfigurationKeys.TRANSPORT_PROTOCOL,
originalTransportProtocol);
}
Review Comment:
tearDown() treats an empty/blank originalTransportProtocol as “unset” and
clears the property, which can change behavior when a test (or build)
explicitly sets TRANSPORT_PROTOCOL to an empty string. Since you’re reading
from System.getProperty(), a null check is the correct way to detect “unset”.
##########
test-suite/test-new-version/src/test/java/org/apache/seata/saga/engine/db/AbstractServerTest.java:
##########
@@ -38,10 +42,34 @@
*/
public abstract class AbstractServerTest {
+ private static final int SERVER_PORT = findAvailablePort();
+
+ static {
+ System.setProperty("config.type", "file");
+ System.setProperty("config.file.name", "file.conf");
+ System.setProperty("service.default.grouplist", "127.0.0.1:" +
SERVER_PORT);
Review Comment:
This static initializer mutates global System properties
(config.type/config.file.name/service.default.grouplist) and never restores
them. With surefire reuseForks enabled, these values can leak into other tests
running later in the same JVM, making test order matter. Consider capturing the
original values and restoring them in stopSeataServer()/@AfterAll (or moving
property setup into the test lifecycle) to keep tests isolated.
--
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]
