Skip to content

fix(graphql-client): Fix an issue that can cause the test_total_transaction_blocks to fail randomly #77

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

Open
wants to merge 2 commits into
base: sdk-bindings
Choose a base branch
from

Conversation

DaughterOfMars
Copy link
Contributor

Description

This test can now fail randomly if the queries are not fast enough and return different checkpoints. This fixes the problem by changing the assert.

Comment on lines +2322 to +2324
assert!(
total_transaction_blocks_by_seq_num >= total_transaction_blocks,
"expected at least {total_transaction_blocks} transaction blocks, found {total_transaction_blocks_by_seq_num}"
Copy link
Contributor

@Alex6323 Alex6323 Aug 7, 2025

Choose a reason for hiding this comment

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

this is essentially a race condition, no? Wondering whether we should just change the assert to also just enforce > 0 🤔

Copy link
Contributor

@Alex6323 Alex6323 Aug 7, 2025

Choose a reason for hiding this comment

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

my reasoning is that the assert! should proof something about the internal logic, right? But here ... if you just change the order of calls in this test, the assert will sometimes fail and the test be flaky again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

don't change the order :)

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.

2 participants