Skip to content

test: refactor: introduce replace_in_config helper#26956

Merged
maflcko merged 1 commit intobitcoin:masterfrom
theStack:202301-test-introduce_replace_in_config_helper
Jan 31, 2023
Merged

test: refactor: introduce replace_in_config helper#26956
maflcko merged 1 commit intobitcoin:masterfrom
theStack:202301-test-introduce_replace_in_config_helper

Conversation

@theStack
Copy link
Contributor

@theStack theStack commented Jan 23, 2023 •
edited
Loading

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
Copy link
Contributor

DrahtBot commented Jan 23, 2023 •
edited
Loading

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 DrahtBot added the Tests label Jan 23, 2023
kouloumos
kouloumos reviewed Jan 23, 2023
View reviewed changes
Copy link
Contributor

@kouloumos kouloumos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

bitcoin/test/functional/test_framework/test_framework.py

Lines 536 to 540 in 5271c77

conf_file = test_node_i.bitcoinconf
with open(conf_file, 'r', encoding='utf8') as conf:
conf_data = conf.read()
with open(conf_file, 'w', encoding='utf8') as conf:
conf.write(conf_data.replace('[regtest]', ''))

@theStack theStack force-pushed the 202301-test-introduce_replace_in_config_helper branch 2 times, most recently from 7eb1a9d to d14740f Compare January 23, 2023 18:14
@theStack
Copy link
Contributor Author

theStack commented Jan 23, 2023

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

bitcoin/test/functional/test_framework/test_framework.py

Lines 536 to 540 in 5271c77

conf_file = test_node_i.bitcoinconf
with open(conf_file, 'r', encoding='utf8') as conf:
conf_data = conf.read()
with open(conf_file, 'w', encoding='utf8') as conf:
conf.write(conf_data.replace('[regtest]', ''))

Good catch, thanks! Done.

kouloumos
kouloumos reviewed Jan 23, 2023
View reviewed changes
Copy link
Contributor

@kouloumos kouloumos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK d14740f126c3df6784675ec427a994f5f6224071

brunoerg
brunoerg reviewed Jan 24, 2023
View reviewed changes
brunoerg
brunoerg approved these changes Jan 26, 2023
View reviewed changes
Copy link
Contributor

@brunoerg brunoerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

crACK d14740f126c3df6784675ec427a994f5f6224071

@theStack theStack force-pushed the 202301-test-introduce_replace_in_config_helper branch from d14740f to 6da0096 Compare January 28, 2023 22:48
@theStack theStack force-pushed the 202301-test-introduce_replace_in_config_helper branch from 6da0096 to b530d96 Compare January 28, 2023 22:50
@theStack
Copy link
Contributor Author

theStack commented Jan 28, 2023 •
edited
Loading

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
Copy link
Contributor

kouloumos commented Jan 31, 2023

ACK b530d96
It's indeed a better fit as a TestNode method.

@maflcko maflcko merged commit 357d750 into bitcoin:master Jan 31, 2023
@theStack theStack deleted the 202301-test-introduce_replace_in_config_helper branch January 31, 2023 12:51
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 31, 2023
Merge bitcoin#26956: test: refactor: introduce replace_in_config helper
40f9023
@bitcoin bitcoin locked and limited conversation to collaborators Jan 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@maflcko maflcko maflcko left review comments

@kouloumos kouloumos kouloumos left review comments

@brunoerg brunoerg brunoerg approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@theStack @DrahtBot @kouloumos @maflcko @brunoerg