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 test flakes #13163

Merged
merged 9 commits into from
Aug 29, 2022
Merged

fix test flakes #13163

merged 9 commits into from
Aug 29, 2022

Conversation

almogdepaz
Copy link
Contributor

@almogdepaz almogdepaz commented Aug 25, 2022

assert spend is in mempool before farming blocks

reduce farmed blocks in tests

reduce timeouts

make tests consistent

@lgtm-com
Copy link

lgtm-com bot commented Aug 25, 2022

This pull request introduces 1 alert when merging 6695527 into 9a0a2b1 - view on LGTM.com

new alerts:

  • 1 for Module is imported more than once

@almogdepaz almogdepaz changed the base branch from main to coverage August 26, 2022 17:44
Copy link
Contributor

@altendky altendky left a comment

Choose a reason for hiding this comment

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

The reduced timeouts concern me in terms of CI runners liking to just randomly stop running our stuff for awhile, at least on macOS in my previous experiences, but otherwise I'm good with this. I can give it a big rerun blast in another PR to see if my concerns are well founded or not. If not I'm happy to merge this into the coverage branch.

@altendky
Copy link
Contributor

Also... thanks!

@almogdepaz almogdepaz changed the title Debug test flakes fix test flakes Aug 28, 2022
@almogdepaz
Copy link
Contributor Author

almogdepaz commented Aug 28, 2022

The reduced timeouts concern me in terms of CI runners liking to just randomly stop running our stuff for awhile, at least on macOS in my previous experiences, but otherwise I'm good with this. I can give it a big rerun blast in another PR to see if my concerns are well founded or not. If not I'm happy to merge this into the coverage branch.

i dont mind changing back but i feel like this is just being too defensive so we can avoid CI failures where the real goal is to find bugs/slowdowns
i think the timeouts here are just a result of hiding the real issue/copy paste from other tests
there is no reason for 20 sec, can we run this like 100 times and see? i hate giving long timeouts it just hides stuff

@almogdepaz almogdepaz marked this pull request as ready for review August 28, 2022 09:28
@almogdepaz almogdepaz requested a review from a team as a code owner August 28, 2022 09:28
@altendky
Copy link
Contributor

Testing robustness in #13217. That is running only the wallet.rpc job and creating 60x duplicates. That hits 240 jobs in the linux and windows workflows where 255 (or 256) is the limit per matrix.

If that works out, ok. I both agree with disliking long timeouts and also have spent too much time dealing with macOS runners out of the blue doing nothing at all for 30 seconds at a time. Anyways, we'll see what the run above says.

@altendky altendky merged commit 7432e28 into coverage Aug 29, 2022
@altendky altendky deleted the debug_test_flakes branch August 29, 2022 19:34
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