Skip to content
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

chore: improve cucumber tests to wait for broadcast #3461

Merged

Conversation

SWvheerden
Copy link
Collaborator

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

stringhandler
stringhandler previously approved these changes Oct 15, 2021
return tx_result === pool;
},
true,
1200 * 1000
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: This change - 20 * 60 * 1000 to 1200 * 1000 - makes it less obvious that the waitFor timeout for just less than the function timeout.

Comment on lines +254 to +262
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
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@hansieodendaal hansieodendaal Oct 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +1867 to +1881
//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
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice improvement overall where we do not want to send transactions as fast as possible.

Copy link
Contributor

@philipr-za philipr-za left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aviator-app aviator-app bot merged commit 4bc7c2a into tari-project:development Oct 21, 2021
sdbondi added a commit to sdbondi/tari that referenced this pull request Oct 21, 2021
…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)
sdbondi added a commit to sdbondi/tari that referenced this pull request Oct 25, 2021
* 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)
  ...
@SWvheerden SWvheerden deleted the sw_fix_transactions branch October 27, 2021 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants