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

fix(ci): update fake_peer_set test to avoid spurious failures #5758

Merged
merged 2 commits into from
Dec 1, 2022

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Dec 1, 2022

Motivation

This test may still be unreliable due to timing issues if the transactions are being rejected because there is a second request in the mempool_requests_for_transactions for the same data that might also receive Nil.

Related to #5384.

Solution

  • Move mempool.enable() below peer_set.expect_request().await to wait for the second chain tip change

Review

Anyone can review.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
  • How do you know it works? Does it have tests?

@github-actions github-actions bot added the C-bug Category: This is a bug label Dec 1, 2022
@arya2 arya2 added I-integration-fail Continuous integration fails, including build and test failures C-testing Category: These are tests labels Dec 1, 2022
@arya2 arya2 self-assigned this Dec 1, 2022
@arya2 arya2 marked this pull request as ready for review December 1, 2022 03:32
@arya2 arya2 requested a review from a team as a code owner December 1, 2022 03:32
@arya2 arya2 requested review from teor2345 and removed request for a team December 1, 2022 03:32
@teor2345
Copy link
Contributor

teor2345 commented Dec 1, 2022

@arya2 can you please add a priority to this PR? Thanks!

@codecov
Copy link

codecov bot commented Dec 1, 2022

Codecov Report

Merging #5758 (fb0d312) into main (f573365) will decrease coverage by 0.10%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5758      +/-   ##
==========================================
- Coverage   78.69%   78.59%   -0.11%     
==========================================
  Files         306      306              
  Lines       38687    38687              
==========================================
- Hits        30445    30406      -39     
- Misses       8242     8281      +39     

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Thanks, let's see if it works!

mergify bot added a commit that referenced this pull request Dec 1, 2022
@mergify mergify bot merged commit 8f90318 into main Dec 1, 2022
@mergify mergify bot deleted the fix-nil-fake-peer-test-2 branch December 1, 2022 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug C-testing Category: These are tests I-integration-fail Continuous integration fails, including build and test failures
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants