Speed up integration tests and fix amopAsyncSubTest silent timeout - #963
Open
morebtcg wants to merge 3 commits into
Open
Speed up integration tests and fix amopAsyncSubTest silent timeout#963morebtcg wants to merge 3 commits into
morebtcg wants to merge 3 commits into
Conversation
amopAsyncSubTest: subscribe once and wait for the subscription to propagate before broadcasting, then assert on a CountDownLatch. The old test re-subscribed the same topic in a loop and swallowed the resulting TimeoutException, so it always passed while wasting ~10s per run; the first broadcasts were also dropped silently because they were sent before the subscription reached the node. PrecompiledTest: reduce the CRUD stress loops from 100 to 20 iterations (300 -> 60 transactions), poll async receipts every 100ms with a 60s deadline, and assert the received receipt count instead of hanging forever on failure. AssembleTransactionProcessorTest / AssembleTransactionWithRemoteSignProcessorTest: replace the fixed 1s end-of-test sleeps with deterministic CompletableFuture.get() waits. AmopTest: shorten the subscription-propagation waits from 2s to 1s. Measured locally against a single-node air chain: total integrationTest time drops from 217s to 152s (-30%), all 57 tests pass.
FakeTransactionCallback counted every onResponse as a sealed tx, so error responses (e.g. client-side timeouts under load) made the receipt-count wait pass while the on-chain tx count assertion failed with no diagnostics. Count only successful receipts and log the error code/message otherwise.
The old test fired asyncInsert + asyncUpdate + asyncRemove for the same key simultaneously, so update/remove routinely landed before the insert and failed with -51507/-51508; the callback also treated every response as success, hiding those failures completely. Chain the three calls per key through the callbacks (the next call is submitted to the test thread pool, since resolving the table address is a blocking call that must not run on the sdk callback thread), count receipts by receipt status (a successful CRUD retCode carries the affected row count, e.g. 1, not 0), and keep the 60s deadline so a lost callback fails the test instead of hanging.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
The FISCO-BCOS CI step
Integration test - Airruns this repo'sintegrationTestsuite three times (non-sm / sm / baseline), ~146s each. Two kinds of waste were found by analyzing the CI logs and reproducing locally:amopAsyncSubTestalways wastes ~10s and can never fail. It broadcasts 5 messages immediately after start, but the subscriber's topic subscription is pushed to the node asynchronously — early broadcasts are silently dropped (verified in the node log: broadcast arrived 6ms before the subscription took effect). The subscriber loop then re-subscribes the same topic 5 times, and the last iteration(s) block onfuture.get(10s), throwing aTimeoutExceptionthat is caught and only printed. The test passes no matter what, burns 10s per run, and prints a misleading exception into CI logs.test51SyncCRUDService/test52AsyncCRUDServiceeach fire 300 transactions; several tests use fixed sleeps where a deterministic wait works.Control experiment: when the subscription is given time to propagate before broadcasting, 5/5 broadcasts are delivered — the AMOP delivery path (node + cpp-sdk + JNI) is fine; the problem is purely in the test.
Changes
AmopTest.amopAsyncSubTest: subscribe once, wait 3s for the subscription to propagate, then broadcast; collect results in aCountDownLatchand fail the test if not all 5 broadcasts arrive. No more swallowedTimeoutException.AmopTest(other cases): shorten the subscription-propagation sleeps from 2s to 1s.PrecompiledTest.test51SyncCRUDService/test52AsyncCRUDService: 100 → 20 iterations (300 → 60 txs); poll async receipts every 100ms with a 60s deadline and assert the count — the old code would hang forever if a callback was lost.AssembleTransactionProcessorTest.test11HelloWorldAsync/AssembleTransactionWithRemoteSignProcessorTest.test2HelloWorldAsync: replace fixed 1s end-of-test sleeps withCompletableFuture.get()+ status assertions.Measured impact
Local run against a single-node air chain (FISCO-BCOS 3.18.0):
test52AsyncCRUDServicetest51SyncCRUDServiceamopAsyncSubTestAll 57 tests pass with zero failures. Since the suite runs 3 times per FISCO-BCOS CI job, this should save roughly 1.5-2 minutes per
Integration test - Airstep and remove the spuriousTimeoutExceptionfrom the logs.