Copilot commented on code in PR #8112:
URL: https://github.com/apache/incubator-seata/pull/8112#discussion_r3304309693
##########
test-suite/test-new-version/src/test/java/org/apache/seata/core/rpc/netty/multiversion/AbstractMultiVersionCompatibilityTest.java:
##########
@@ -136,8 +134,9 @@ public static String toPrettyJson(Object obj) {
@BeforeEach
public void setUp() {
- originalTransportProtocol =
ConfigurationFactory.getInstance().getConfig(ConfigurationKeys.TRANSPORT_PROTOCOL);
-
ConfigurationTestHelper.putConfig(ConfigurationKeys.TRANSPORT_PROTOCOL,
Protocol.SEATA.value);
+ originalTransportProtocol =
System.getProperty(ConfigurationKeys.TRANSPORT_PROTOCOL);
+ System.setProperty(ConfigurationKeys.TRANSPORT_PROTOCOL,
Protocol.SEATA.value);
+ System.setProperty(ConfigurationKeys.SHUTDOWN_WAIT, "0");
bossGroup = new NioEventLoopGroup(1);
Review Comment:
SHUTDOWN_WAIT is set to "0" for this test but the previous value isn’t
preserved/restored. Since this runs in the same JVM as other tests within a
Surefire fork, this can leak into subsequent tests (and with ConfigurationCache
enabled, clearing the system property alone may not revert what the code
reads). Consider saving the original value and restoring it in tearDown, and
clearing the Seata ConfigurationCache after changes so subsequent reads see the
restored value.
##########
test-suite/test-new-version/src/test/java/org/apache/seata/core/rpc/netty/multiversion/AbstractMultiVersionCompatibilityTest.java:
##########
@@ -169,14 +168,15 @@ 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);
}
+ System.clearProperty(ConfigurationKeys.SHUTDOWN_WAIT);
}
Review Comment:
tearDown restores/clears system properties, but Seata’s ConfigurationCache
can retain the previously read values (it doesn’t automatically refresh on
System property changes). That can make later tests still observe
Protocol.SEATA / SHUTDOWN_WAIT=0 even after restoration. Clearing the config
cache (or reloading ConfigurationFactory) after restoring properties would
prevent cross-test contamination within the same fork.
##########
test-suite/test-new-version/src/test/java/org/apache/seata/core/rpc/netty/BaseNettyClientTest.java:
##########
@@ -128,27 +128,37 @@ protected ServerInstance startServerSimple(int port)
throws Exception {
// init snowflake for transactionId, branchId
UUIDGenerator.init(1L);
nettyRemotingServer.init();
+ serverStatus.set(true);
})
.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;
+ }
+ }
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));
+ 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");
}
/**
* Clean up client configuration
*/
protected void cleanupClientConfig() {
- ConfigurationTestHelper.removeConfig("service.default.grouplist");
-
ConfigurationTestHelper.removeConfig(ConfigurationKeys.SERVER_SERVICE_PORT_CAMEL);
+ System.clearProperty("service.default.grouplist");
+ System.clearProperty(ConfigurationKeys.SERVER_SERVICE_PORT_CAMEL);
+ System.clearProperty(ConfigurationKeys.SHUTDOWN_WAIT);
}
Review Comment:
configureClient/cleanupClientConfig switch from ConfigurationTestHelper to
System properties, but with Seata’s ConfigurationCache enabled, clearing a
System property doesn’t necessarily change what later code reads unless the
cache is cleared/reloaded. Also, cleanup unconditionally clears SHUTDOWN_WAIT,
which can interfere with other tests in the same fork. Consider
preserving/restoring original values for these properties and clearing
ConfigurationCache after modifications to avoid cross-test leakage.
##########
test-suite/test-new-version/src/test/java/org/apache/seata/core/rpc/netty/BaseNettyClientTest.java:
##########
@@ -128,27 +128,37 @@ protected ServerInstance startServerSimple(int port)
throws Exception {
// init snowflake for transactionId, branchId
UUIDGenerator.init(1L);
nettyRemotingServer.init();
+ serverStatus.set(true);
})
.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;
+ }
+ }
return new ServerInstance(nettyRemotingServer, port);
}
Review Comment:
startServerSimple now polls for up to 10s, but it never handles server init
failures: exceptions in the server thread will be lost and this method will
still return a ServerInstance even if the server never started. This can lead
to flaky failures later in the test. Consider wrapping init() in try/catch (set
a failure flag) and throwing if the server isn’t started after the timeout
(similar to startServer()).
--
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]
