test: Implicitly sync after generate* to preempt races and intermittent test failures#20362
test: Implicitly sync after generate* to preempt races and intermittent test failures#20362maflcko wants to merge 4 commits intobitcoin:masterfrom
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Concept ACK: thanks for doing this - intermittent test failures are the worst! Bike shed nit: |
maflcko
force-pushed
the
2011-noSync
branch
2 times, most recently
from
faf0319 to
fae7684
Compare
promag
left a comment
promag
left a comment
There was a problem hiding this comment.
Concept ACK.
How about for now:
try_dispatch_with_sync
try
dispatch
except
sync_all
dispatch
log with print_stack
There was a problem hiding this comment.
faf4ef6b4bd8955ff5a9b714bc17b179a80a8b75
An alternative to "overload" these generate* is to add a generic _rpc_dispatch_and_sync (__getattr__ -> _rpc_dispatch_and_sync -> _rpc_dispatch).
It would be cool to keep the last generate* extract_stack that when a related error occurs we could print it.
There was a problem hiding this comment.
That would make calling code tremendously verbose, no?
There was a problem hiding this comment.
Maybe I am misunderstanding. Do you have a working diff that I can steal?
|
Concept ACK! very nice, thank you |
|
Concept ACK, rooting out a cause of intermittent issues (due to test flakiness) is great. |
maflcko
force-pushed
the
2011-noSync
branch
2 times, most recently
from
fa7ef91 to
faf6073
Compare
|
Concept ACK |
|
What do you think about adding a |
|
That would require changing all calls to |
I think it's worth it, to avoid adding an implicit dependency from TestNode onto TestFramework. |
-BEGIN VERIFY SCRIPT- perl -0777 -pi -e 's/(generate[^\n]*\)[^\n]*)(\n|\s)+self.sync_.*\(\)\n/\1\n/g' $(git grep -l generate ./test) -END VERIFY SCRIPT-
|
🐙 This pull request conflicts with the target branch and needs rebase. Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft". |
10 participants
The most frequent failure in functional tests are intermittent races. Fixing such bugs is cumbersome because it involves:
self.sync_all()where it was forgottenAlso, writing a linter to catch those is not possible, nor is review effective in finding these bugs prior to merge.
Fix all future intermittent races caused by a missing sync_block call by calling
sync_allimplicitly after eachgenerate*, unless opted out. This ensures that the code is race-free (with regards to blocks) when the tests pass once, instead of our current approach where the code can never be guaranteed to be race-free.