test: Replace threading with concurrent.futures #27287

pull maflcko wants to merge 1 commits into bitcoin:master from maflcko:2303-test-check-return-value-😝 changing 2 files +30 −32

maflcko commented at 8:59 AM on March 21, 2023: member

threading has no easy way to get the return value or exception once the target function stops. Not checking the return value or exception can make tests more fragile and failures harder to debug.

Fix this by checking the return value (or exception) by wrapping the function execution into a future and calling result() on it.

Can be reviewed with --ignore-all-space.

(There are still some uses of threading around, because some tests do expect an exception to be thrown and caught in the target function)
test: Replace threading with concurrent.futures fa0696e786
DrahtBot commented at 8:59 AM on March 21, 2023: contributor


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK ishaanam, stickies-v

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #22838 (descriptors: Be able to specify change and receiving in a single descriptor string by achow101)
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.
DrahtBot added the label Tests on Mar 21, 2023
fanquake requested review from ishaanam on Mar 21, 2023
ishaanam commented at 11:23 PM on March 21, 2023: contributor

ACK fa0696e7863af03efbffa026c2060ff2b5720fb1 Thanks for working on this, these changes should help figure out the occasional failures.
maflcko assigned fanquake on Mar 22, 2023

Type	Reviewers
ACK	ishaanam, stickies-v

in test/functional/wallet_importdescriptors.py:694 in fa0696e786

 690 | @@ -691,25 +691,24 @@ def run_test(self):
 691 |          descriptor["next_index"] = 0
 692 |  
 693 |          encrypted_wallet.walletpassphrase("passphrase", 99999)
 694 | -        t = threading.Thread(target=encrypted_wallet.importdescriptors, args=([descriptor],))
 695 | +        with concurrent.futures.ThreadPoolExecutor(max_workers=1) as thread:

stickies-v commented at 1:58 PM on March 23, 2023:

nit: I think thread is a misnomer, even with just 1 worker it's still a pool or executor.

stickies-v approved
stickies-v commented at 2:11 PM on March 23, 2023: contributor

ACK fa0696e7863af03efbffa026c2060ff2b5720fb1
fanquake merged this on Mar 23, 2023
fanquake closed this on Mar 23, 2023
maflcko deleted the branch on Mar 23, 2023
sidhujag referenced this in commit e08d5bf0bb on Mar 24, 2023
bitcoin locked this on Mar 22, 2024

Contributors

Labels

test: Replace threading with concurrent.futures #27287

Reviews

Conflicts