chore: improve cucumber tests to wait for broadcast #3461
Conversation
SWvheerden
force-pushed
the
sw_fix_transactions
branch
6 times, most recently
from
b996837
to
ac6e1c4
Compare
| return tx_result === pool; | ||
| }, | ||
| true, | ||
| 1200 * 1000 |
There was a problem hiding this comment.
NIT: This change - 20 * 60 * 1000 to 1200 * 1000 - makes it less obvious that the waitFor timeout for just less than the function timeout.
| await waitFor( | ||
| async () => { | ||
| let node_a_result = (await nodeA.get_node_state()) === "LISTENING"; | ||
| let node_b_result = (await nodeB.get_node_state()) === "LISTENING"; | ||
| return node_a_result && node_b_result; | ||
| }, | ||
| true, | ||
| 30 * 1000 | ||
| ); |
There was a problem hiding this comment.
I am not sure that this will always be a guarantee that the nodes are connected and synced, as was the intention of the previous variable wait time after restart. Ideally, we should compare the best block hash of each node.
There was a problem hiding this comment.
The idea behind this is not to test if they are synced. It is to check if the nodes have checked their state and the other node's state and are happy with the state.
For example, if the nodes have the same difficulty, they will not have the same block hash, but they will both be in listening mode. If you want to check if they are on the same tip, there is a step for that.
There was a problem hiding this comment.
Edit: I still think it will be more beneficial to verify that nodes are then either synced to the same best block or have the same difficulty, which will make the tests more resilient. It is the same idea as verifying that transactions are broadcast before finishing the send transaction step.
Maybe something like a successful 'dial-peer' both ways then? - Just an idea for a future PR.
SWvheerden
force-pushed
the
sw_fix_transactions
branch
from
ac6e1c4
to
6556ee3
Compare
| //lets now wait for this transaction to be at least broadcast before we continue. | ||
| let waitfor_result = await waitFor( | ||
| async () => { | ||
| let result = true; | ||
| for (let i = 0; i < number; i++) { | ||
| result = | ||
| result && sourceClient.isTransactionAtLeastBroadcast(tx_ids[i]); | ||
| } | ||
| return result; | ||
| }, | ||
| true, | ||
| 60 * 1000, | ||
| 5 * 1000, | ||
| 5 | ||
| ); |
There was a problem hiding this comment.
This is a nice improvement overall where we do not want to send transactions as fast as possible.
…ct-chain-metadata * development: fix: fix recovery test reporting message (tari-project#3479) chore: improve cucumber tests to wait for broadcast (tari-project#3461) test: use TCP node for daily sync test (tari-project#3464)
* development: (31 commits) feat!: revalidate all outputs (tari-project#3471) fix: check SAF message inflight and check stored_at is in past (tari-project#3444) feat!: apps should not depend on other app configs (tari-project#3469) fix: fix recovery test reporting message (tari-project#3479) chore: improve cucumber tests to wait for broadcast (tari-project#3461) test: use TCP node for daily sync test (tari-project#3464) fix: remove unbounded vec allocations from base node grpc/p2p messaging (tari-project#3467) fix: upgrade rustyline dependencies (tari-project#3476) fix(dht): discard encrypted message with no destination (tari-project#3472) fix: remove consensus breaking change in transaction input (tari-project#3474) feat: tx weight takes tariscript and output features into account [igor] (tari-project#3411) fix: validate dht header before dedup cache (tari-project#3468) fix: sha256sum isn't available on all *nix platforms (tari-project#3466) fix: typo in console wallet (tari-project#3465) fix: ensure that accumulated orphan chain data is committed before header validation (tari-project#3462) fix: remove is_synced check for transaction validation (tari-project#3459) feat: improve logging for tari_mining_node (tari-project#3449) fix: remove unnecessary wallet dependency (tari-project#3438) test: simplify cucumber tests (tari-project#3457) ci: create script to update DNS records from hashes.txt (tari-project#3458) ...
Description
Changed connect node to wait for both nodes to listening
Changed send transactions to wait for broadcast state.
Removed some flaky and long-running tags.
Motivation and Context
This will test to run faster as more stable. Current SHA3 miner mines too fast for the wallets to finish sending. This PR market wait for the transaction to be broadcast before continuing.
This makes the connect node to await listing state of both nodes to avoid using waits.
How Has This Been Tested?
Manual test of tests