test: Implicitly sync after generate* to preempt races and intermittent test failures #22567
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This is exactly identical functionality like #20362 (comment), but implemented differently. Here the sync happens in the test framework, which makes the diff larger. In the other pull the sync happens in the test node. |
|
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. |
This was referenced Jul 28, 2021
This was referenced Jul 28, 2021
This was referenced Jul 28, 2021
This was referenced Jul 28, 2021
|
Strong Concept ACK Intermittent test failures are the worst! |
There was a problem hiding this comment.
Strong concept ACK.
But I am getting certain failures when I am running all the tests together with test/function/test_runner.py -j 10.
I am not sure if that's something to do with this PR or it's some issue at my end.
- They are also happening in master.
- Failed tests are passing when I run them independently.
Failure summary :
wallet_watchonly.py --legacy-wallet | ✓ Passed | 22 s
wallet_watchonly.py --usecli --legacy-wallet | ✓ Passed | 25 s
feature_backwards_compatibility.py --descriptors | ○ Skipped | 0 s
feature_backwards_compatibility.py --legacy-wallet | ○ Skipped | 0 s
feature_taproot.py --previous_release | ○ Skipped | 0 s
mempool_compatibility.py | ○ Skipped | 0 s
wallet_upgradewallet.py --legacy-wallet | ○ Skipped | 0 s
feature_coinstatsindex.py | ✖ Failed | 68 s
feature_config_args.py | ✖ Failed | 28 s
rpc_invalidateblock.py | ✖ Failed | 63 s
wallet_backup.py --legacy-wallet | ✖ Failed | 317 s
wallet_dump.py --legacy-wallet | ✖ Failed | 168 s
One sample failure log:
202/215 - feature_config_args.py failed, Duration: 28 s
stdout:
2021-07-29T13:11:46.329000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_₿_🏃_20210729_182102/feature_config_args_9
2021-07-29T13:12:00.419000Z TestFramework (INFO): Test config args logging
2021-07-29T13:12:02.455000Z TestFramework (INFO): Test seed peers
2021-07-29T13:12:12.318000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/home/raj/github-repo/mybitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 128, in main
self.run_test()
File "/home/raj/github-repo/mybitcoin/bitcoin/test/functional/feature_config_args.py", line 221, in run_test
self.test_seed_peers()
File "/home/raj/github-repo/mybitcoin/bitcoin/test/functional/feature_config_args.py", line 186, in test_seed_peers
self.start_node(0, extra_args=['-dnsseed=0', '-fixedseeds=1'])
File "/usr/lib/python3.8/contextlib.py", line 120, in __exit__
next(self.gen)
File "/home/raj/github-repo/mybitcoin/bitcoin/test/functional/test_framework/test_node.py", line 408, in assert_debug_log
self._raise_assertion_error('Expected messages "{}" does not partially match log:\n\n{}\n\n'.format(str(expected_msgs), print_log))
File "/home/raj/github-repo/mybitcoin/bitcoin/test/functional/test_framework/test_node.py", line 166, in _raise_assertion_error
raise AssertionError(self._node_msg(msg))
AssertionError: [node 0] Expected messages "['Loaded 0 addresses from peers.dat', 'DNS seeding disabled', 'Adding fixed seeds as -dnsseed=0, -addnode is not provided and all -seednode(s) attempted\n']" does not partially match log:
|
Concept ACK Is it correct to say the tests will be more upfront in catching syncing issues because, other than being race-free, the syncing happens within the test framework? Versus #20362. Just trying to understand the tradeoffs here despite the bigger diff. Either way, I'm all in favor to merging this one ASAP to avoid an impending avalanche of conflicting PRs :) |
If the tests also fail on master, then it can't be due to this PR. You can file an issue and hope it makes someone fix the issue or fix the issue yourself.
As explained in the first comment (#22567 (comment)), the two are supposed to be functionally identical. |
|
Maybe name the Looks like two tests needed updated copyrights; scripted-diff check fails in the last commit 09fbdc3: $ test/lint/commit-script-check.sh f4e12fd..09fbdc3
diff --git a/test/functional/rpc_signmessagewithprivkey.py b/test/functional/rpc_signmessagewithprivkey.py
index 27aee44d25..80555eab75 100755
--- a/test/functional/rpc_signmessagewithprivkey.py
+++ b/test/functional/rpc_signmessagewithprivkey.py
@@ -1,5 +1,5 @@
#!/usr/bin/env python3
-# Copyright (c) 2016-2020 The Bitcoin Core developers
+# Copyright (c) 2016-2021 The Bitcoin Core developers
# Distributed under the MIT software license, see the accompanying
# file COPYING or http://www.opensource.org/licenses/mit-license.php.
"""Test RPC commands for signing messages with private key."""
diff --git a/test/functional/wallet_signmessagewithaddress.py b/test/functional/wallet_signmessagewithaddress.py
index bf6f95e3f1..74a8f2eef2 100755
--- a/test/functional/wallet_signmessagewithaddress.py
+++ b/test/functional/wallet_signmessagewithaddress.py
@@ -1,5 +1,5 @@
#!/usr/bin/env python3
-# Copyright (c) 2016-2019 The Bitcoin Core developers
+# Copyright (c) 2016-2021 The Bitcoin Core developers
# Distributed under the MIT software license, see the accompanying
# file COPYING or http://www.opensource.org/licenses/mit-license.php.
"""Test Wallet commands for signing and verifying messages."""
Failed |
maflcko
pushed a commit
that referenced
this pull request
Sep 9, 2021
fa0b916 scripted-diff: Use generate* from TestFramework (MarcoFalke) Pull request description: This is needed for #22567. By using the newly added `generate*` member functions of the test framework, it paves the way to make it easier to implicitly call `sync_all` after block generation to avoid intermittent issues. ACKs for top commit: jonatack: ACK fa0b916 Tree-SHA512: e74a324b60250a87c08847cdfd7b6ce3e1d89b891659fd168f6dd7dc0aa718d0edd28285374a613f462f34f4ef8e12c90ad44fb58721c91b2ea691406ad22c2a
maflcko
force-pushed
the
2107-testSync
branch
from
09fbdc3 to
f2f5bea
Compare
This was referenced Sep 10, 2021
This was referenced Sep 21, 2021
maflcko
pushed a commit
that referenced
this pull request
Oct 18, 2021
fac62e6 test: Delete generate* calls from TestNode (MarcoFalke) fac7f61 test: Use generate* node RPC, not wallet RPC (MarcoFalke) faac1cd test: Use generate* from TestFramework, not TestNode (MarcoFalke) Pull request description: Deleting the methods is needed for #22567 to pave the way to make it easier to implicitly call the `sync_all` member function. Without the methods being deleted, nothing prevents developers from adding calls to it. As history showed, developers *will* add calls to it. For example, see commit eb02dbb from today or the first commit in this pull request. ACKs for top commit: stratospher: Tested ACK fac62e6. brunoerg: tACK fac62e6 promag: Code review ACK fac62e6. Tree-SHA512: 6d4dea8f95ead954acfef2e6a5d98897ce0c2d02265c5b137bb149d0265543bd51d7e8403e1945b9af75df5524ca50064fe1d2a432b25c8abc71bbb28ed6ed53
added 4 commits
October 18, 2021 13:15
-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-
The previous diff touched most files in ./test/, so bump the headers to avoid having to touch them again for a bump later. -BEGIN VERIFY SCRIPT- ./contrib/devtools/copyright_header.py update ./test/ -END VERIFY SCRIPT-
maflcko
force-pushed
the
2107-testSync
branch
from
322d994 to
fad9d45
Compare
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Oct 18, 2021
fac62e6 test: Delete generate* calls from TestNode (MarcoFalke) fac7f61 test: Use generate* node RPC, not wallet RPC (MarcoFalke) faac1cd test: Use generate* from TestFramework, not TestNode (MarcoFalke) Pull request description: Deleting the methods is needed for bitcoin#22567 to pave the way to make it easier to implicitly call the `sync_all` member function. Without the methods being deleted, nothing prevents developers from adding calls to it. As history showed, developers *will* add calls to it. For example, see commit eb02dbb from today or the first commit in this pull request. ACKs for top commit: stratospher: Tested ACK fac62e6. brunoerg: tACK fac62e6 promag: Code review ACK fac62e6. Tree-SHA512: 6d4dea8f95ead954acfef2e6a5d98897ce0c2d02265c5b137bb149d0265543bd51d7e8403e1945b9af75df5524ca50064fe1d2a432b25c8abc71bbb28ed6ed53
This was referenced Oct 27, 2021
maflcko
pushed a commit
to bitcoin-core/gui
that referenced
this pull request
Nov 9, 2021
…nless opted out facc352 test: Implicitly sync after generate*, unless opted out (MarcoFalke) Pull request description: The most frequent failure in functional tests are intermittent races. Fixing such bugs is cumbersome because it involves: * Noticing the failure * Fetching and reading the log to determine the test case that failed * Adding a `self.sync_all()` where it was forgotten * Spamming out a pr and waiting for review, which is already sparse Also, 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_all` implicitly after each `generate*`, 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. There are some scripted-diff cleanups (see bitcoin/bitcoin#22567), but they will be submitted in a follow-up to reduce the conflicts in this pull. ACKs for top commit: lsilva01: tACK facc352 on Ubuntu 20.04 brunoerg: tACK facc352 on MacOS 11.6 Tree-SHA512: 046a40a066b4a3bd28a3077bd654fa8887442dd1f0ec6fd11671865809ef02376f126eb667a1320ebd67b6e372c78c00dbf8bd25d86ed86f1d9a25363103ed97
|
I've archived the discussion here and created a follow-up for the remaining (scripted-diff) changes: #23474
Sounds acceptable. Should I add a scripted-diff for that to the other pull? |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.