This PR moves sync_blocks and sync_mempool out from test_framework/util.py to test_framework/test_framework.py so they can take contextual information of test framework into account.
test_framework.py**kwargs which is not usedtimeout_factor when respecting timeout in function implementations../test/functional/test_runner.pyfixes #18930
Seems like the style of those files are auto-formatted by autopep8. I’ll leave the reviewers to comment about the format.
One question about the implementation. Does it make sense to move assert (all([len(x.getpeerinfo()) for x in rpc_connections])) up to immediately after the while loop starts? It will assert sooner and save some time and operations.
wallet_avoidreuse.py failed in travis but it works fine on my machine. Is there a way to replay the travis failed tests?
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, 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.
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.
Could someone explain what does conflicts mean here? Is there a logical order of PRs mentioned + mine to review and merge?
Re the format - autopep8 is making the diff much larger. I think it’s easier, for now, to keep the diff as minimal as possible so reviewers can focus on the changes relevant to your patch. At the very least, you can break the formatting changes into their own commit and follow the lead of this reformat.
I’d also suggest squashing your second commit since the first commit doesn’t pass the linter without it.
574+ Wait until everybody has the same tip.
575
576- def sync_all(self, nodes=None, **kwargs):
577- self.sync_blocks(nodes, **kwargs)
578- self.sync_mempools(nodes, **kwargs)
579+ sync_blocks needs to be called with an rpc_connections set that has least
blockhash parameter and enforce that hash instead?
Oo first PR? Concept ACK :) I’m guessing “contextual information of test framework” is the timeout_factor?
+1 to Jonas, just appeasing test/lint/lint-python.sh for now would be better.
Does it make sense to move
assert (all([len(x.getpeerinfo()) for x in rpc_connections]))up to immediately after the while loop starts? It will assert sooner and save some time and operations.
I had this thought too. But I think it would fail if the nodes happened to all be synced up (despite not being connected right now) before the first iteration of the while loop. I think it’s possible…?
Oo first PR? Concept ACK :) I’m guessing “contextual information of test framework” is the
timeout_factor?
Yeah for now timeout_factor is easiest to be used. There are other things potentially can be used. I’m open to suggestions.
rpc_timeout - I think rpc_timeout is usually sometime different than timeout people want to configure for syncing operations. Unless there is a factor that can be applied to rpc_timeout to estimate syncing timeout. Open to suggestions.nodes - We can by default sync all nodes and take in a parameter in sync_blocks and sync_mempool in case subset of nodes need to be synced. This way we get rid of nodes input of those two functions.+1 to Jonas, just appeasing test/lint/lint-python.sh for now would be better.
lint-python.sh gives me other linting errors which is not related to the file I touched. I guess I’ll just make those five files I touched to be pep8 in a separate commit. If it’s not necessary, it’s easy to take it out.
0test/functional/p2p_compactblocks.py:688:43: E741 ambiguous variable name 'l'
1test/functional/p2p_compactblocks.py:694:13: E741 ambiguous variable name 'l'
2test/functional/p2p_compactblocks.py:697:17: E741 ambiguous variable name 'l'
3test/functional/p2p_dos_header_tree.py:37:38: E741 ambiguous variable name 'l'
4test/functional/p2p_dos_header_tree.py:42:31: E741 ambiguous variable name 'l'
5test/functional/p2p_dos_header_tree.py:43:55: E741 ambiguous variable name 'l'
6test/functional/test_framework/messages.py:70:22: E741 ambiguous variable name 'l'
7test/functional/test_framework/messages.py:142:16: E741 ambiguous variable name 'l'
8test/functional/test_framework/messages.py:161:24: E741 ambiguous variable name 'l'
9test/functional/test_framework/messages.py:177:23: E741 ambiguous variable name 'l'
10test/functional/wallet_labels.py:149:13: E741 ambiguous variable name 'l'
11test/functional/wallet_labels.py:155:13: E741 ambiguous variable name 'l'
12test/lint/lint-python.sh: line 105: mypy: command not found
Does it make sense to move
assert (all([len(x.getpeerinfo()) for x in rpc_connections]))up to immediately after the while loop starts? It will assert sooner and save some time and operations.I had this thought too. But I think it would fail if the nodes happened to all be synced up (despite not being connected right now) before the first iteration of the while loop. I think it’s possible…?
I guess it’s theoretically possible.
219
220 os.environ['PATH'] = os.pathsep.join([
221 os.path.join(config['environment']['BUILDDIR'], 'src'),
222- os.path.join(config['environment']['BUILDDIR'], 'src', 'qt'), os.environ['PATH']
223+ os.path.join(config['environment']['BUILDDIR'],
224+ 'src', 'qt'), os.environ['PATH']
545- def sync_mempools(self, nodes=None, **kwargs):
546- sync_mempools(nodes or self.nodes, **kwargs)
547-
548- def sync_all(self, nodes=None, **kwargs):
549- self.sync_blocks(nodes, **kwargs)
550- self.sync_mempools(nodes, **kwargs)
**kwargs which are not used.
563+ return
564+ # Check that each peer has at least one connection
565+ assert (all([len(x.getpeerinfo()) for x in rpc_connections]))
566+ time.sleep(wait)
567+ raise AssertionError("Block sync timed out after {}s:{}".format(
568+ int(timeout * self.options.timeout_factor),
timeout = int(timeout * self.options.timeout_factor) at the beginning of the function
585+ return
586+ # Check that each peer has at least one connection
587+ assert (all([len(x.getpeerinfo()) for x in rpc_connections]))
588+ time.sleep(wait)
589+ raise AssertionError("Mempool sync timed out after {}s:{}".format(
590+ int(timeout * self.options.timeout_factor),
Concept ACK, but please change the commit title to
0test: move sync_blocks and sync_mempool functions to test_framework.py
ACK cc84460c164bcb2a874d4f08b3a2624e5ee9ff0a , reviewed with –color-moved=dimmed-zebra –color-moved-ws=ignore-all-space 💫
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3ACK cc84460c164bcb2a874d4f08b3a2624e5ee9ff0a , reviewed with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space 💫
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUh0+gv+MXi8uR6BQOeLwvt+luY9+MDS6qKEVkd1x6tUrhNL4R/1/HzIbCPA0n0O
8FH/bMuzMk19Fn4qkH4vGEJHPQTeT+CNEf9omuWRSrNU8pqicaBpAN0cpWcHbJSxD
9A8TC5uecJ1vWLmYruW9mnSTyYFTL7maKas7yKv4JsmsLIRhMPhWnJ2VU7MrCq9Uv
10UyR5VTYjiqmw3Gz24RJ9QTtJWTDJjlkNXs01ZrO1ijVX5Gak9QLx47wz60lh8wtT
11XtRQKOxsBGbJfrFexpU3IdThhNTuLTx1zcULgE2S6IOBF1Lph/CrSuZLv69H3bhW
12S45h7iliCQR5njyC93TB1GB56BDFGcvbFqkjP0jgB0sVdh+pRP4lcgTNRGPD8O+t
13p4TbuTKMo6P1JbqB1TAzZCmWUjVLmTjReSvGlMKegA3FHTm2XyXQEuOT7KHNk2Bm
142BHyvABW/hlIB/79YlUQ59pTC8CIvjEl0JQxY5VXOARyESD9PAteaJ5aFplnHtYl
15eIUZJOgR
16=adom
17-----END PGP SIGNATURE-----
Timestamp of file with hash bea4e035a3dba2f2997391d4f35010649fa6ca21167797054925a5e15c237f47 -