test: refactor: introduce `replace_in_config` helper #26956

pull theStack wants to merge 1 commits into bitcoin:master from theStack:202301-test-introduce_replace_in_config_helper changing 4 files +19 −18

theStack commented at 4:48 PM on January 23, 2023: contributor

Currently two functional tests (p2p_permissions.py and wallet_crosschain.py) include quite similar code for substituting strings in a TestNode's bitcoind configuration file, so refactoring that out to a dedicated helper method seems to make sense (probably other tests could need that too in the future).
DrahtBot commented at 4:48 PM on January 23, 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 kouloumos

Stale ACK brunoerg

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
DrahtBot added the label Tests on Jan 23, 2023
kouloumos commented at 5:02 PM on January 23, 2023: contributor

Looks reasonable. Maybe you could also use the helper here: https://github.com/bitcoin/bitcoin/blob/5271c77f8356dc6de95fd6d08a415df3568eb7a1/test/functional/test_framework/test_framework.py#L536-L540
theStack force-pushed on Jan 23, 2023
theStack force-pushed on Jan 23, 2023
theStack commented at 6:33 PM on January 23, 2023: contributor

Looks reasonable. Maybe you could also use the helper here:

https://github.com/bitcoin/bitcoin/blob/5271c77f8356dc6de95fd6d08a415df3568eb7a1/test/functional/test_framework/test_framework.py#L536-L540

Good catch, thanks! Done.
in test/functional/test_framework/util.py:295 in d14740f126 outdated
```
 291 | @@ -292,6 +292,22 @@ def sha256sum_file(filename):
 292 |      return h.digest()
 293 |  
 294 |  
 295 | +def replace_in_config(node, replacements):
```
kouloumos commented at 7:13 PM on January 23, 2023:

nit: Didn't notice at first, but this is under "# Utility functions", I think it should be under "# Node functions"

maflcko commented at 1:44 PM on January 24, 2023:

We should probably remove those comments, given that they are wrong anyway. (modinv is under "Block functions", which is wrong)

maflcko commented at 4:30 PM on January 27, 2023:

nit: Shouldn't node functions be put under test_node.py, anyway?

theStack commented at 10:51 PM on January 28, 2023:

nit: Shouldn't node functions be put under test_node.py, anyway?

Good idea, done.
kouloumos commented at 7:15 PM on January 23, 2023: contributor

ACK d14740f126c3df6784675ec427a994f5f6224071

Type	Reviewers
ACK	kouloumos
Stale ACK	brunoerg

in test/functional/test_framework/util.py:304 in d14740f126 outdated

 299 | +        [("old", "new"), ("foo", "bar"), ...]
 300 | +    """
 301 | +    with open(node.bitcoinconf, 'r', encoding='utf8') as conf:
 302 | +        conf_data = conf.read()
 303 | +    for replacement in replacements:
 304 | +        assert len(replacement) == 2

brunoerg commented at 12:59 PM on January 24, 2023:

nit:

        assert_equal(len(replacement), 2)

theStack commented at 10:51 PM on January 28, 2023:

Done.

brunoerg approved
brunoerg commented at 6:19 PM on January 26, 2023: contributor

crACK d14740f126c3df6784675ec427a994f5f6224071
theStack force-pushed on Jan 28, 2023
test: refactor: introduce `replace_in_config` helper b530d9605d
theStack force-pushed on Jan 28, 2023
theStack commented at 10:54 PM on January 28, 2023: contributor

Force-pushed with a version that introduces the helper now as TestNode method (as suggested by Marco) where it's a better fit than in the util module. Sorry for invalidating ACKs, re-review should be quite trivial.
kouloumos commented at 7:01 AM on January 31, 2023: contributor

ACK b530d9605db863fd8d0e45e73ff2eb9462d1ad4c It's indeed a better fit as a TestNode method.
maflcko merged this on Jan 31, 2023
maflcko closed this on Jan 31, 2023
theStack deleted the branch on Jan 31, 2023
sidhujag referenced this in commit 40f9023d9e on Jan 31, 2023
bitcoin locked this on Jan 31, 2024

Contributors

Labels

test: refactor: introduce `replace_in_config` helper #26956

Reviews