-
Notifications
You must be signed in to change notification settings - Fork 1.7k
test(framework): fix test quality issues #6655
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.
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
base: develop
Are you sure you want to change the base?
Changes from all commits
5d4db4d
ea477ce
4321e3d
d92db81
61aadd4
f3dfd3c
8b721fc
85dddb6
ed551ee
50998e6
036abb9
c4bb315
e9cdd01
b173311
21b16ea
669b1f2
ed42e92
f5a42f8
bb8894a
3881b87
5d5e96f
e11fce0
7bd3373
cb7e038
61a0e3c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -57,8 +57,6 @@ | |
| @Scope("prototype") | ||
| public class PeerConnection { | ||
|
|
||
| private static List<InetSocketAddress> relayNodes = Args.getInstance().getFastForwardNodes(); | ||
|
|
||
| @Getter | ||
| private PeerStatistics peerStatistics = new PeerStatistics(); | ||
|
|
||
|
|
@@ -163,10 +161,16 @@ public class PeerConnection { | |
| private volatile boolean needSyncFromUs = true; | ||
| @Getter | ||
| private P2pRateLimiter p2pRateLimiter = new P2pRateLimiter(); | ||
| @Getter | ||
| private List<InetSocketAddress> relayNodes; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [NIT] Three small follow-ups on the Moving
Suggestion: add a one-line comment above the null-check explaining the Args-init ordering + test-injection contract; make the field |
||
|
|
||
| public void setChannel(Channel channel) { | ||
| this.channel = channel; | ||
| if (relayNodes.stream().anyMatch(n -> n.getAddress().equals(channel.getInetAddress()))) { | ||
| if (this.relayNodes == null) { | ||
| this.relayNodes = Args.getInstance().getFastForwardNodes(); | ||
| } | ||
| if (relayNodes != null | ||
| && relayNodes.stream().anyMatch(n -> n.getAddress().equals(channel.getInetAddress()))) { | ||
| this.isRelayPeer = true; | ||
| } | ||
| this.nodeStatistics = TronStatsManager.getNodeStatistics(channel.getInetAddress()); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,9 @@ | |
|
|
||
| import static org.tron.core.config.Parameter.ChainConstant.BLOCK_PRODUCED_INTERVAL; | ||
|
|
||
| import java.util.concurrent.ExecutorService; | ||
| import java.util.concurrent.LinkedBlockingDeque; | ||
| import java.util.concurrent.TimeUnit; | ||
| import java.util.concurrent.atomic.AtomicLong; | ||
| import lombok.extern.slf4j.Slf4j; | ||
| import org.springframework.beans.factory.support.DefaultListableBeanFactory; | ||
|
|
@@ -11,6 +13,7 @@ | |
| import org.tron.common.application.ApplicationFactory; | ||
| import org.tron.common.application.TronApplicationContext; | ||
| import org.tron.common.client.DatabaseGrpcClient; | ||
| import org.tron.common.es.ExecutorServiceManager; | ||
| import org.tron.common.parameter.CommonParameter; | ||
| import org.tron.common.prometheus.Metrics; | ||
| import org.tron.core.ChainBaseManager; | ||
|
|
@@ -39,6 +42,9 @@ public class SolidityNode { | |
|
|
||
| private volatile boolean flag = true; | ||
|
|
||
| private ExecutorService getBlockEs; | ||
| private ExecutorService processBlockEs; | ||
|
|
||
| public SolidityNode(Manager dbManager) { | ||
| this.dbManager = dbManager; | ||
| this.chainBaseManager = dbManager.getChainBaseManager(); | ||
|
|
@@ -72,13 +78,25 @@ public static void start() { | |
| appT.startup(); | ||
| SolidityNode node = new SolidityNode(appT.getDbManager()); | ||
| node.run(); | ||
| appT.blockUntilShutdown(); | ||
| awaitShutdown(appT, node); | ||
| } | ||
|
|
||
| static void awaitShutdown(Application appT, SolidityNode node) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [NIT] Signal test-only visibility
Suggestion: add |
||
| try { | ||
| appT.blockUntilShutdown(); | ||
| } finally { | ||
| // SolidityNode is created manually rather than managed by Spring/Application, | ||
| // so its executors must be shut down explicitly on exit. | ||
| node.shutdown(); | ||
| } | ||
| } | ||
|
|
||
| private void run() { | ||
| try { | ||
| new Thread(this::getBlock).start(); | ||
| new Thread(this::processBlock).start(); | ||
| getBlockEs = ExecutorServiceManager.newSingleThreadExecutor("solid-get-block"); | ||
| processBlockEs = ExecutorServiceManager.newSingleThreadExecutor("solid-process-block"); | ||
| getBlockEs.execute(this::getBlock); | ||
| processBlockEs.execute(this::processBlock); | ||
| logger.info("Success to start solid node, ID: {}, remoteBlockNum: {}.", ID.get(), | ||
| remoteBlockNum); | ||
| } catch (Exception e) { | ||
|
|
@@ -88,6 +106,12 @@ private void run() { | |
| } | ||
| } | ||
|
|
||
| public void shutdown() { | ||
| flag = false; | ||
| ExecutorServiceManager.shutdownAndAwaitTermination(getBlockEs, "solid-get-block"); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Re: @halibobo1205 #3077488830 — concrete findings on the shutdown-time verification you asked for: [SHOULD] shutdown() worst-case latency can reach 240s Both The current SolidityNodeTest uses a mocked node so CI is not affected, but any integration test or operational restart that runs the real worker will see this latency. Suggestion: run the two |
||
| ExecutorServiceManager.shutdownAndAwaitTermination(processBlockEs, "solid-process-block"); | ||
| } | ||
|
|
||
| private void getBlock() { | ||
| long blockNum = ID.incrementAndGet(); | ||
| while (flag) { | ||
|
|
@@ -137,7 +161,7 @@ private void loopProcessBlock(Block block) { | |
| } | ||
|
|
||
| private Block getBlockByNum(long blockNum) { | ||
| while (true) { | ||
| while (flag) { | ||
| try { | ||
| long time = System.currentTimeMillis(); | ||
| Block block = databaseGrpcClient.getBlock(blockNum); | ||
|
|
@@ -155,10 +179,11 @@ private Block getBlockByNum(long blockNum) { | |
| sleep(exceptionSleepTime); | ||
| } | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| private long getLastSolidityBlockNum() { | ||
| while (true) { | ||
| while (flag) { | ||
| try { | ||
| long time = System.currentTimeMillis(); | ||
| long blockNum = databaseGrpcClient.getDynamicProperties().getLastSolidityBlockNum(); | ||
|
|
@@ -171,6 +196,7 @@ private long getLastSolidityBlockNum() { | |
| sleep(exceptionSleepTime); | ||
| } | ||
| } | ||
| return 0; | ||
| } | ||
|
|
||
| public void sleep(long time) { | ||
|
|
@@ -193,4 +219,4 @@ private void resolveCompatibilityIssueIfUsingFullNodeDatabase() { | |
| chainBaseManager.getDynamicPropertiesStore().saveLatestSolidifiedBlockNum(headBlockNum); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| package org.tron.common; | ||
|
|
||
| import io.grpc.ManagedChannel; | ||
| import java.util.concurrent.TimeUnit; | ||
| import org.tron.common.application.ApplicationFactory; | ||
| import org.tron.common.application.TronApplicationContext; | ||
| import org.tron.core.config.DefaultConfig; | ||
|
|
||
| /** | ||
| * Shared class-level fixture for tests that manually manage a TronApplicationContext. | ||
| */ | ||
| public class ClassLevelAppContextFixture { | ||
|
|
||
| private TronApplicationContext context; | ||
|
|
||
| public TronApplicationContext createContext() { | ||
| context = new TronApplicationContext(DefaultConfig.class); | ||
| return context; | ||
| } | ||
|
|
||
| public TronApplicationContext createAndStart() { | ||
| createContext(); | ||
| startApp(); | ||
| return context; | ||
| } | ||
|
|
||
| public void startApp() { | ||
| ApplicationFactory.create(context).startup(); | ||
| } | ||
|
|
||
| public TronApplicationContext getContext() { | ||
| return context; | ||
| } | ||
|
|
||
| public void close() { | ||
| if (context != null) { | ||
| context.close(); | ||
| context = null; | ||
| } | ||
| } | ||
|
|
||
| public static void shutdownChannel(ManagedChannel channel) { | ||
| if (channel == null) { | ||
| return; | ||
| } | ||
| try { | ||
| channel.shutdown(); | ||
| if (!channel.awaitTermination(5, TimeUnit.SECONDS)) { | ||
| channel.shutdownNow(); | ||
| } | ||
| } catch (InterruptedException e) { | ||
| channel.shutdownNow(); | ||
| Thread.currentThread().interrupt(); | ||
| } | ||
| } | ||
|
|
||
| public static void shutdownChannels(ManagedChannel... channels) { | ||
| for (ManagedChannel channel : channels) { | ||
| shutdownChannel(channel); | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,7 +21,7 @@ public class BackupServerTest { | |
| public TemporaryFolder temporaryFolder = new TemporaryFolder(); | ||
|
|
||
| @Rule | ||
| public Timeout globalTimeout = Timeout.seconds(60); | ||
| public Timeout globalTimeout = Timeout.seconds(90); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [QUESTION] Why does this test need 90s? The Suggestion: add a short comment explaining the new upper bound, or tighten the shutdown path so 60s still fits. |
||
| private BackupServer backupServer; | ||
|
|
||
| @Before | ||
|
|
@@ -43,7 +43,7 @@ public void tearDown() { | |
| Args.clearParam(); | ||
| } | ||
|
|
||
| @Test(timeout = 60_000) | ||
| @Test | ||
| public void test() throws InterruptedException { | ||
| backupServer.initServer(); | ||
| // wait for the server to start | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,11 @@ | ||
| package org.tron.common.logsfilter; | ||
|
|
||
| import java.util.concurrent.ExecutorService; | ||
| import java.util.concurrent.TimeUnit; | ||
| import org.junit.After; | ||
| import org.junit.Assert; | ||
| import org.junit.Test; | ||
| import org.tron.common.es.ExecutorServiceManager; | ||
| import org.tron.common.logsfilter.nativequeue.NativeMessageQueue; | ||
| import org.zeromq.SocketType; | ||
| import org.zeromq.ZContext; | ||
|
|
@@ -13,6 +17,21 @@ public class NativeMessageQueueTest { | |
| public String dataToSend = "################"; | ||
| public String topic = "testTopic"; | ||
|
|
||
| private ExecutorService subscriberExecutor; | ||
|
|
||
| @After | ||
| public void tearDown() { | ||
| if (subscriberExecutor != null) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [SHOULD] Use the project's standard shutdown helper Every other test in this PR routes executor shutdown through Suggestion: replace the manual shutdown block with |
||
| subscriberExecutor.shutdownNow(); | ||
| try { | ||
| subscriberExecutor.awaitTermination(2, TimeUnit.SECONDS); | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| } | ||
| subscriberExecutor = null; | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void invalidBindPort() { | ||
| boolean bRet = NativeMessageQueue.getInstance().start(-1111, 0); | ||
|
|
@@ -39,22 +58,23 @@ public void publishTrigger() { | |
| try { | ||
| Thread.sleep(1000); | ||
| } catch (InterruptedException e) { | ||
| e.printStackTrace(); | ||
| Thread.currentThread().interrupt(); | ||
| } | ||
|
|
||
| NativeMessageQueue.getInstance().publishTrigger(dataToSend, topic); | ||
|
|
||
| try { | ||
| Thread.sleep(1000); | ||
| } catch (InterruptedException e) { | ||
| e.printStackTrace(); | ||
| Thread.currentThread().interrupt(); | ||
| } | ||
|
|
||
| NativeMessageQueue.getInstance().stop(); | ||
| } | ||
|
|
||
| public void startSubscribeThread() { | ||
| Thread thread = new Thread(() -> { | ||
| subscriberExecutor = ExecutorServiceManager.newSingleThreadExecutor("zmq-subscriber"); | ||
| subscriberExecutor.execute(() -> { | ||
| try (ZContext context = new ZContext()) { | ||
| ZMQ.Socket subscriber = context.createSocket(SocketType.SUB); | ||
|
|
||
|
|
@@ -70,6 +90,5 @@ public void startSubscribeThread() { | |
| // ZMQ.Socket will be automatically closed when ZContext is closed | ||
| } | ||
| }); | ||
| thread.start(); | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[SHOULD] The close()/start() race is only partially closed, and the remaining control flow is confusing.
The new
if (shutdown) break;at line 81 only covers a narrow window — the case whereclose()has already setshutdown=truebut has not yet readchannel. The other window is still open: ifclose()runs whileb.bind(port).sync()is still in progress,close()sees the previous iteration's (already-closed)channel(or null) and itschannel.close().await(10s)is a no-op on the channel that is about to be bound. The newly-bound channel is then closed indirectly bygroup.shutdownGracefully().sync()in start()'sfinally, meaning there are two distinct close paths with two different timeout semantics (10s vs. group's default quiet period).Ordering in
close():backupManager.stop()runs beforechannel.close(). Any in-flight UDP datagram already picked up by the event loop will still be dispatched through the pipeline into a stoppedBackupManager. The usual order is to stop ingress (channel) first, then business state (backupManager).channel.close().await(10, SECONDS)is effectively redundant —ExecutorServiceManager.shutdownAndAwaitTermination(executor, name)already waits forstart()to return, andstart()only returns aftergroup.shutdownGracefully().sync()has closed the channel. Its only real job is to wake up thechannel.closeFuture().sync()in start(); a fire-and-forgetchannel.close()would do that.The
while (!shutdown)+logger.warn("Restart backup server ...")branch is practically dead code for a UDPNioDatagramChannel:closeFuture()only completes onclose()(→shutdown=true→ break) or on an exception (→ outer catch → finally). There is no tested path that loops back and rebinds, so the three shutdown checks + restart loop read as defensive code that will never run.Suggestion: drop the restart loop and extra
if (shutdown) break;guards, callchannel.close()(fire-and-forget) inclose(), rely onExecutorServiceManager.shutdownAndAwaitTermination(executor, name)as the single sync barrier, and stopbackupManageronly after the executor has terminated.