8000 fix: make TestMempoolProgressInHigherRound less flaky by faddat · Pull Request #1953 · cometbft/cometbft · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: make TestMempoolProgressInHigherRound less flaky #1953

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

Closed
wants to merge 15 commits into from

Conversation

faddat
Copy link
Contributor
@faddat faddat commented Jan 2, 2024

This PR should make TestMempoolProgressInHigherRound less flaky and a teensy big more rigorous.

The critical change is the order of the steps in the test itself, which does seem to make the test more reliable.

If the order should not have changed then we should probably just close this PR and try to find another way to make the test more reliable.

closes: #1932

found in: #1954


PR checklist

  • Tests written/updated
  • Changelog entry added in .changelog (we use unclog to manage our changelog)
  • Updated relevant documentation (docs/ or spec/) and code comments

@faddat faddat requested review from a team as code owners January 2, 2024 20:43
Copy link
Contributor
@melekes melekes left a comment

Choose a reason for hiding this comment

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

Thanks @faddat ❤️ Unsure how changing/adding comments fixes the test's flackiness though. Could you please explain?

@faddat
Copy link
Contributor Author
faddat commented Jan 3, 2024

wrangling.... will mark this as draft.

@melekes melekes marked this pull request as draft January 5, 2024 04:15
@faddat
Copy link
Contributor Author
faddat commented Jan 5, 2024

That's right draft is a good way for this one.

@faddat faddat marked this pull request as ready for review January 5, 2024 09:37
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these changes somehow related with the mempool test?

ensureNewRound(newRoundCh, height, round) // first round at first height
ensureNewEventOnChannel(newBlockCh) // first block gets committed
// Ensure new round at first height and block commit
ensureNewRound(newRoundCh, height, round)
Copy link
Contributor

Choose a reason for hiding this comment

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

The problem here is that the new round event takes more than 200ms?

Copy link
Contributor Author
@faddat faddat Jan 9, 2024

Choose a reason for hiding this comment

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

These are both good questions that I had missed. I will review this tomorrow and thank you for your review.

Also, yes I think so.

@faddat
Copy link
Contributor Author
faddat commented Jan 11, 2024

@cason I think we should defer further review here until #2021

is merged.

@faddat
Copy link
Contributor Author
faddat commented Jan 19, 2024

@cason please see #1923

I just added some commentary about another time-related test issue and I'm not sure how to handle these safely in all cases.

@faddat faddat changed the title make TestMempoolProgressInHigherRound less flaky fix: make TestMempoolProgressInHigherRound less flaky Jan 23, 2024
@faddat faddat marked this pull request as draft January 31, 2024 17:39
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale For use by stalebot label Feb 11, 2024
@github-actions github-actions bot closed this Feb 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale For use by stalebot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TestMempoolProgressInHigherRound is flaky
3 participants
0