This test has been growing too large, making it difficult to maintain. Especially when multiple pull requests change it.
ipc_util.pyipc_mining.py, keeping only an interface sanity check in interface_ipc.pyipc_mining.pyReview hint:
0git show --color-moved=dimmed-zebra --color-moved-ws=ignore-space-change
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | ryanofsky, sedited |
| Concept ACK | ismaelsadeeq |
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
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.
113+ ctx = capnp_modules['proxy'].Context()
114+ ctx.thread = thread
115+ return ctx, init
116+
117+
118+async def parse_and_deserialize_block(block_template, ctx):
In commit “test: move helpers to ipc_util.py” (766c008920c2d45516065e502a826b60d9e8f5f1)
Not important, but as long as these function signatures are being changed anyway (dropping self parameters) it would be nice to improve their current naming, which I think is misleading as have mentioned, and inconsistent with the names of the remote methods they call:
parse_and_deserialize_block -> get_blockparse_and_deserialize_coinbase_tx -> get_coinbase_raw_txparse_and_deserialize_coinbase -> get_coinbase_tx40+ yield obj
41+ finally:
42+ await obj.destroy(ctx)
43+
44+
45+async def create_block_template(mining, stack, ctx, opts):
In commit “test: move helpers to ipc_util.py” (766c008920c2d45516065e502a826b60d9e8f5f1)
May be good to move create_block_template and wait_next_template below near parse_and_deserialize_block so the mining functions are grouped together and the file is organized with generic async functions at the top, followed by capnproto functions, followed by mining functions.
289@@ -266,7 +290,15 @@ async def wait_for_block():
290 asyncio.run(capnp.run(async_routine()))
291
292 def run_test(self):
293+ self.miniwallet = MiniWallet(self.nodes[0])
In commit “test: split interface_ipc_mining.py” (9f80bd7c390409cc1713a0ccc58018bd801758a0)
Seems good to use a shared miniwallet variable like the shared opts variable, but also inconsistent to pass one as a parameter and attach other to self. It would seem better to pass them both as parameters so it’s clear which tests are using them.
Move helper functions and dataclass from interface_ipc.py to a new
test_framework/ipc_util.py module.
Rename some helpers for clarity:
- parse_and_deserialize_block -> get_block
- parse_and_deserialize_coinbase_tx -> get_coinbase_raw_tx
- parse_and_deserialize_coinbase -> get_coinbase_tx
Split the Mining interface tests from interface_ipc.py into a dedicated
interface_ipc_mining.py file. The original file now only tests basic IPC
functionality (echo) and simple Mining inspectors.
Plus a few minor improvements.
ACK 836ed1fb88ed66bd966b35eb4aec69fb195a0f77
Nit: The commit messages could be a bit more descriptive.