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.

Sign up for GitHub

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

Jump to bottom

fix: fix flaky unstable network tests #5013

Closed
wants to merge 11 commits into from
Closed

fix: fix flaky unstable network tests #5013

wants to merge 11 commits into from

Conversation

Erigara
Copy link
Contributor

@Erigara Erigara commented Aug 27, 2024

Context

We have unstable_network tests which are constantly failing in the CI.

Solution

Solutions consist of multiple parts:

  • @SamHSmith fix to use only genesis peer as client introduced in fix: Fix bug in test harness for unstable network #4980
  • Remove fixed time polling mechanism in favor of event based solution without fixed timeout
    • Idea is that external timeout is anyway exist which would cut off test execution if it takes too long
  • Increase transactions ttl to prevent the case when transaction is expired before end up in the committed block

@Erigara Erigara added the Tests label Aug 27, 2024
@Erigara Erigara self-assigned this Aug 27, 2024
@Erigara Erigara changed the title Fix flaky unstable network tests fix: fix flaky unstable network tests Aug 27, 2024
@Erigara
Copy link
Contributor Author

Erigara commented Aug 27, 2024

Ok, i can see that unstable_network tests passed, but the rest of extra_functional tests are still not working with failure to wait for genesis.

Probably i should apply new async genesis await mechanism for them as well.

@Erigara Erigara mentioned this pull request Aug 29, 2024
Merged
@Erigara Erigara force-pushed the fix_flaky_unstable_network_tests branch 2 times, most recently from 4192f5b to 85207ce Compare August 29, 2024 11:40
@mversic mversic force-pushed the fix_flaky_unstable_network_tests branch from 85207ce to fe216de Compare September 4, 2024 07:16
@Erigara
refactor: add async mechanism to wait for genesis
2cf9702 
Signed-off-by: Shanin Roman <[email protected]>
@Erigara
fix: use blocking transaction submission instead of polling for unsta…
9fd70b3 
…ble tests

Signed-off-by: Shanin Roman <[email protected]>
@Erigara
refactor: rename wait_for_genesis_committed_events to wait_for_genesi…
da83d11 
…s_committed_async

Signed-off-by: Shanin Roman <[email protected]>
@Erigara
refactor: use async genesis wait for extra functional tests
3bde764 
Signed-off-by: Shanin Roman <[email protected]>
@Erigara
fix(register_new_peer): send tx through new peer client
0eb14e2 
Signed-off-by: Shanin Roman <[email protected]>
@Erigara
fix(unstable_network): set max ttl for all transactions
8c8ad28 
Signed-off-by: Shanin Roman <[email protected]>
@Erigara
fix(unstable_netwrok): increase wait for tx result
d23688e 
Signed-off-by: Shanin Roman <[email protected]>
@Erigara
fix(connected_peers): submit transaction by reconnected peer
44392fb 
Signed-off-by: Shanin Roman <[email protected]>
@Erigara
refactor: include only online peers when waiting for genesis
e69af9c 
Signed-off-by: Shanin Roman <[email protected]>
@Erigara
refactor(offline_peers,connected_peers): wait for block instead of sleep
bc8b9bb 
Signed-off-by: Shanin Roman <[email protected]>
@Erigara
refactor(intergation): use new genesis wait async
67c62b6 
Signed-off-by: Shanin Roman <[email protected]>
@Erigara Erigara force-pushed the fix_flaky_unstable_network_tests branch from fe216de to 67c62b6 Compare September 5, 2024 12:14
@Erigara Erigara marked this pull request as ready for review September 5, 2024 12:39
nxsaken
nxsaken reviewed Sep 5, 2024
View reviewed changes
client/tests/integration/extra_functional/connected_peers.rs
Comment on lines +31 to +32
iroha.transaction_ttl = Some(Duration::from_millis(u64::MAX));
iroha.transaction_status_timeout = Duration::from_millis(u64::MAX);
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider compressing into a method, this repeats often

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure it's wroth extracting into test_network because this function is kinda specific for this particular tests...

DCNick3
DCNick3 approved these changes Sep 11, 2024
View reviewed changes
client/tests/integration/extra_functional/unstable_network.rs
@@ -96,23 +97,22 @@ fn unstable_network(
quantity,
AssetId::new(asset_definition_id.clone(), account_id.clone()),
);
iroha.submit(mint_asset).expect("Failed to create asset.");
iroha
.submit_blocking(mint_asset)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we replace all uses of submit with submit_blocking and get rid of polling? Or does it matter less for non-extra-functional tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should by default opt to submit_blocking and use submit in places where it's really required.

@Erigara
Copy link
Contributor Author

Erigara commented Sep 20, 2024

Closing for now in favor of #5087.

@Erigara Erigara closed this Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nxsaken nxsaken nxsaken left review comments

@DCNick3 DCNick3 DCNick3 approved these changes

@s8sato s8sato Awaiting requested review from s8sato s8sato is a code owner

@mversic mversic Awaiting requested review from mversic mversic is a code owner

@SamHSmith SamHSmith Awaiting requested review from SamHSmith SamHSmith is a code owner

@outoftardis outoftardis Awaiting requested review from outoftardis outoftardis is a code owner

@0x009922 0x009922 Awaiting requested review from 0x009922 0x009922 is a code owner

@dima74 dima74 Awaiting requested review from dima74 dima74 is a code owner

Assignees

@dima74 dima74

@DCNick3 DCNick3

@Erigara Erigara

Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Erigara @DCNick3 @nxsaken @dima74