Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: Change feature_config_args.py not to rely on strange regtest=0 behavior #17556

Merged
merged 1 commit into from
Jan 21, 2021

Conversation

ryanofsky
Copy link
Contributor

@ryanofsky ryanofsky commented Nov 21, 2019

Update test to simply generate a normal mainnet configuration file instead of using a crazy setup where a regtest=1 config file using an includeconf in the [regtest] section includes another config file that specifies regtest=0, retroactively switching the network to mainnet.

This setup was fragile and only worked because the triggered InitError happened early enough that none of the ignored [regtest] options mattered (only affecting log output).

This change was originally made as part of #17493

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 22, 2019

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

Conflicts

No conflicts as of last run.

@laanwj
Copy link
Member

laanwj commented Nov 22, 2019

Concept ACK

@ryanofsky ryanofsky changed the title Change feature_config_args.py not to rely on strange regtest=0 behavior test: Change feature_config_args.py not to rely on strange regtest=0 behavior Nov 24, 2019
@instagibbs
Copy link
Member

utACK 228fb7e

test/functional/test_framework/util.py Outdated Show resolved Hide resolved
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Feb 10, 2020
Suggestions from MarcoFalke <falke.marco@gmail.com>
bitcoin#17556 (review)

- Fix comment mentioning chain name instead of subdirectory name
- Switch from -regtest/-testnet options to -chain option
@ryanofsky
Copy link
Contributor Author

Updated 228fb7e -> b339f83 (pr/wdpy.1 -> pr/wdpy.2, compare) using "" chain subdirectory instead of "main" and implementing cleanup suggestions

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Feb 11, 2020
Suggestions from MarcoFalke <falke.marco@gmail.com>
bitcoin#17556 (review)

- Fix comment mentioning chain name instead of subdirectory name
- Switch from -regtest/-testnet options to -chain option
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Feb 11, 2020
Suggestions from MarcoFalke <falke.marco@gmail.com>
bitcoin#17556 (review)

- Fix comment mentioning chain name instead of subdirectory name
- Switch from -regtest/-testnet options to -chain option
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Feb 11, 2020
Suggestions from MarcoFalke <falke.marco@gmail.com>
bitcoin#17556 (review)

- Fix comment mentioning chain name instead of subdirectory name
- Switch from -regtest/-testnet options to -chain option
@ryanofsky
Copy link
Contributor Author

ryanofsky commented Feb 11, 2020

Rebased b339f83 -> affbdfa (pr/wdpy.2 -> pr/wdpy.3, compare) fixing silent merge conflict with #16115. Thanks Marco for looking into the test failure
Updated affbdfa -> a3c0a70 (pr/wdpy.3 -> pr/wdpy.4, compare) just editing commit message
Rebased a3c0a70 -> 8fa28f3 (pr/wdpy.4 -> pr/wdpy.5, compare) to get past cirrus freebsd failures https://cirrus-ci.com/task/6666211096264704
Updated 8fa28f3 -> f147b00 (pr/wdpy.5 -> pr/wdpy.6, compare) dropping second commit to avoid feature_backwards_compatibility.py failures https://travis-ci.org/github/bitcoin/bitcoin/jobs/723528480#L5088

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Aug 11, 2020
Suggestions from MarcoFalke <falke.marco@gmail.com>
bitcoin#17556 (review)

- Fix comment mentioning chain name instead of subdirectory name
- Switch from -regtest/-testnet options to -chain option
…behavior

Update test to simply generate a normal mainnet configuration file instead of
using a crazy setup where a regtest=1 config file using an includeconf in the
[regtest] section includes another config file that specifies regtest=0,
retroactively switching the network to mainnet.

This setup was fragile and only worked because the triggered InitError happened
early enough that none of the ignored [regtest] options mattered (only
affecting log output).
@ryanofsky ryanofsky closed this Sep 2, 2020
@ryanofsky ryanofsky reopened this Sep 2, 2020
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Sep 2, 2020
Suggestions from MarcoFalke <falke.marco@gmail.com>
bitcoin#17556 (review)

- Fix comment mentioning chain name instead of subdirectory name
- Switch from -regtest/-testnet options to -chain option
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Sep 2, 2020
Suggestions from MarcoFalke <falke.marco@gmail.com>
bitcoin#17556 (review)

- Fix comment mentioning chain name instead of subdirectory name
- Switch from -regtest/-testnet options to -chain option
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

ACK

let me know whether you want to address the nit or not

return datadir


def write_config(config_path, n, chain, extra_config=""):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def write_config(config_path, n, chain, extra_config=""):
def write_config(config_path, *, n, chain, extra_config=""):

nit: would be good to force named args to avoid mixing up n and chain. E.g. setting chain=0 will work, but is a confusing way to say mainnet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

re: #17556 (comment)

nit: would be good to force named args to avoid mixing up n and chain. E.g. setting chain=0 will work, but is a confusing way to say mainnet.

Thanks, added and updated callers.

Copy link
Contributor Author

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

Updated f147b00 -> ff44cae (pr/wdpy.6 -> pr/wdpy.7, compare) just requiring write_config keyword arguments

return datadir


def write_config(config_path, n, chain, extra_config=""):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

re: #17556 (comment)

nit: would be good to force named args to avoid mixing up n and chain. E.g. setting chain=0 will work, but is a confusing way to say mainnet.

Thanks, added and updated callers.

@maflcko
Copy link
Member

maflcko commented Jan 21, 2021

ACK ff44cae

@maflcko maflcko merged commit 11cbd4b into bitcoin:master Jan 21, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 21, 2021
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants