Add settings merge test to prevent regresssions #15869
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Concept ACK. Going to go line-by-line in the next few days. |
|
Thanks for doing this. |
|
Concept ACK Very nice! Regression testing is our best friend! |
maflcko
approved these changes
Apr 29, 2019
There was a problem hiding this comment.
utACK 151f3e9
Show signature and timestamp
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
utACK 151f3e9cf1bbcf30a4fc7749682e66b4a73ddfc2
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjGCwv/W8QAcQ1+K+tGRBF2LDzuHibOOtOOgsVh2E5yX953lVKmEZzhthhVLDW+
S3JlOYfc6l1q0gD1cIcX5IZuTk+pA+cMZIg2Co2xMHCoIk+UIrGe1UdxOwPX3AXZ
R3xmcErQHTZwD7oRQ4vZpt7BhA4ZPUxIh00CJ12z49KFm/Eh0C7zodalENuwwIvN
7Rwd/kHXqA36w9JAF6JlxKmrLpftf+adIcEYjPyUldDnm+xF4M/VYxjR9XVjNkG4
8bziYMA/kB8/g/30qHUzxfH5gXb1cptpbbA/CFXFLLaxYGx9lgui+sz8CNF/AzRQ
mgOF6vJJhrzm45Bz9Oo/uitgh/UGK3jDzImADcIWX9lE7WRQp0HAVH1AlwNm8gtY
ceMTlwj1gtAz2Y9Hz3kf8uUmHpkU+fqE4NJBO1J9n8wb0poW4DL/mnAo5N0UMdf6
ZY5iE+Y/uNLdYOzyO/XjmmKk6G5PP1/xNOz4xNthe26c34QkznpLHiCbRNRO4rs6
w7ihnATE
=SUNq
-----END PGP SIGNATURE-----
Timestamp of file with hash c7ac440a2577d6ddd333508953cc427cd050c82938897165390cde198e35cbbc -
| template <typename Fn> | ||
| void ForEachActionList(Fn&& fn) | ||
| { | ||
| ActionList actions = {SET}; |
There was a problem hiding this comment.
I'd prefer if this didn't set the first element to SET and the others to 0. Could do the following and add a comment that 0==SET? Also, on first sight this might look a bit like the list only has one element.
Suggested change
| ActionList actions = {SET}; | |
| ActionList actions = {}; |
promag
reviewed
Apr 29, 2019
| FILE* out_file = nullptr; | ||
| if (const char* out_path = getenv("SETTINGS_MERGE_TEST_OUT")) { | ||
| out_file = fsbridge::fopen(out_path, "w"); | ||
| if (!out_file) throw std::system_error(errno, std::generic_category(), "fopen failed"); |
There was a problem hiding this comment.
BOOST_REQUIRE instead of throwing?
| BOOST_FIXTURE_TEST_CASE(util_SettingsMerge, SettingsMergeTestingSetup) | ||
| { | ||
| CHash256 out_sha; | ||
| FILE* out_file = nullptr; |
There was a problem hiding this comment.
nit, FILE* out_file{nullptr};
| //! debugging to make test results easier to understand. | ||
| static constexpr int MAX_ACTIONS = 3; | ||
|
|
||
| enum Action { SET = 0, NEGATE, SECTION_SET, SECTION_NEGATE, END }; |
There was a problem hiding this comment.
Why = 0 since it's already the default?
|
Going to merge this, I think our style-nits can be fixed up later or not at all |
maflcko
pushed a commit
that referenced
this pull request
Apr 30, 2019
151f3e9 Add settings merge test to prevent regresssions (Russell Yanofsky) Pull request description: Test-only change. Motivation: I'm trying to clean up settings code and add support for read/write settings without changing existing behavior, but current tests are very scattershot and don't actually cover a lot of current behavior. ACKs for commit 151f3e: jonasschnelli: utACK 151f3e9. MarcoFalke: utACK 151f3e9 Tree-SHA512: f9062f078da02855cdbdcae37d0cea5684e82adbe5c701a8eb042ee4a57d899f0ffb6a9db3bcf58b639dff22b2b2d8a75f9a7917402df58904036753d65a1e3e
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
May 3, 2019
This change makes it easier to add a new persistent settings source in followup PR bitcoin#15935 without introducing new bugs and inconsistencies. This PR does not change any behavior and has good test coverage through the util_SettingsMerge test added in bitcoin#15869. Right now settings from different sources are partially merged when settings are parsed, and partially merged when settings are retrieved. The logic implemented merging during retrieval is repeated 5 places: - ArgsManagerHelper::GetArg - ArgsManagerHelper::GetNetBoolArg - ArgsManager::GetArgs - ArgsManager::IsArgNegated - ArgsManager::GetUnsuitableSectionOnlyArgs which makes it cumbersome introduce a new settings source. Inconsistencies between the 5 implementations are also responsible for some confusing and counterintuitive behaviors (documented in this PR but not changed for now): - Ignored command line arguments (see ArgsManager::ParseParameters) - Inconsistently ignored network-specific arguments (see GetSetting) - Reappearing negated config arguments (see GetListSetting) After this change, merging of settings happens in one place: a new settings.h/settings.cpp file in code that has more comments and is less duplicative. It separates merging from parsing, using a data structure that holds complete representation of parsed config files and command line options, instead of the current more abstract data structure that has negated values represented by placeholder map entries.
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
May 7, 2019
This change makes it easier to add a new persistent settings source in followup PR bitcoin#15935 without introducing new bugs and inconsistencies. This PR does not change any behavior and has good test coverage through the util_SettingsMerge test added in bitcoin#15869. Right now settings from different sources are partially merged when settings are parsed, and partially merged when settings are retrieved. The logic implemented merging during retrieval is repeated 5 places: - ArgsManagerHelper::GetArg - ArgsManagerHelper::GetNetBoolArg - ArgsManager::GetArgs - ArgsManager::IsArgNegated - ArgsManager::GetUnsuitableSectionOnlyArgs which makes it cumbersome introduce a new settings source. Inconsistencies between the 5 implementations are also responsible for some confusing and counterintuitive behaviors (documented in this PR but not changed for now): - Ignored command line arguments (see ArgsManager::ParseParameters) - Inconsistently ignored network-specific arguments (see GetSetting) - Reappearing negated config arguments (see GetListSetting) After this change, merging of settings happens in one place: a new settings.h/settings.cpp file in code that has more comments and is less duplicative. It separates merging from parsing, using a data structure that holds complete representation of parsed config files and command line options, instead of the current more abstract data structure that has negated values represented by placeholder map entries.
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
May 8, 2019
This change makes it easier to add a new persistent settings source in followup PR bitcoin#15935 without introducing new bugs and inconsistencies. This PR does not change any behavior and has good test coverage through the util_SettingsMerge test added in bitcoin#15869. Right now settings from different sources are partially merged when settings are parsed, and partially merged when settings are retrieved. The logic implemented merging during retrieval is repeated 5 places: - ArgsManagerHelper::GetArg - ArgsManagerHelper::GetNetBoolArg - ArgsManager::GetArgs - ArgsManager::IsArgNegated - ArgsManager::GetUnsuitableSectionOnlyArgs which makes it cumbersome introduce a new settings source. Inconsistencies between the 5 implementations are also responsible for some confusing and counterintuitive behaviors (documented in this PR but not changed for now): - Ignored command line arguments (see ArgsManager::ParseParameters) - Inconsistently ignored network-specific arguments (see GetSetting) - Reappearing negated config arguments (see GetListSetting) After this change, merging of settings happens in one place: a new settings.h/settings.cpp file in code that has more comments and is less duplicative. It separates merging from parsing, using a data structure that holds complete representation of parsed config files and command line options, instead of the current more abstract data structure that has negated values represented by placeholder map entries.
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
May 9, 2019
Followup to bitcoin#15869. Treat "-wallet" as the network-specific argument in test instead of "-server", to make test output clearer and be more consistent with bitcoind. Update embedded hash to match changed output from this.
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
May 15, 2019
Followup to bitcoin#15869. Treat "-wallet" as the network-specific argument in test instead of "-server", to make test output clearer and be more consistent with bitcoind. Update embedded hash to match changed output from this.
HashUnlimited
pushed a commit
to HashUnlimited/chaincoin
that referenced
this pull request
Aug 30, 2019
Followup to bitcoin#15869. Treat "-wallet" as the network-specific argument in test instead of "-server", to make test output clearer and be more consistent with bitcoind. Update embedded hash to match changed output from this.
laanwj
added a commit
that referenced
this pull request
Nov 8, 2019
083c954 Add settings_tests (Russell Yanofsky) 7f40528 Deduplicate settings merge code (Russell Yanofsky) 9dcb952 Add util::Settings struct and helper functions. (Russell Yanofsky) e2e37cf Remove includeconf nested scope (Russell Yanofsky) 5a84aa8 Rename includeconf variables for clarity (Russell Yanofsky) dc8e1e7 Clarify emptyIncludeConf logic (Russell Yanofsky) Pull request description: This is a refactoring-only change that makes it easier to add a new settings source. This PR doesn't change behavior. The [`util_ArgsMerge`](https://github.com/bitcoin/bitcoin/blob/deb2327b435925c6a39ca654a79283b8eb6aeb86/src/test/util_tests.cpp#L626-L822) and [`util_ChainMerge`](https://github.com/bitcoin/bitcoin/blob/deb2327b435925c6a39ca654a79283b8eb6aeb86/src/test/util_tests.cpp#L843-L924) tests added in #15869 and #15988 were written specifically to confirm that ArgsManager settings are parsed, merged, and returned the same way before and after this change. This change: - Makes it easier to add new settings sources that can get merged with existing sources (see 70675c3 from #15935). - Separates parsing of settings from merging of settings, and deduplicates merging code so it doesn't happen five different places ([GetArg](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L221-L244), [GetNetBoolArg](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L255-L261), [GetArgs](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L460-L467), [IsArgNegated](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L482-L491), [GetUnsuitableSectionOnlyArgs](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L343-L352)) in inconsistent ways. - Documents and tests current strange merging behaviors, so they be cleaned up in the future if resulting code simplifications and UX improvements warrant loss of backwards compatibility. The newly documented behaviors are: command line [ignored arguments](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/system.cpp#L323-L326) and [more ignored arguments](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L67-L72), and config file [reverse precedence](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L61-L65), [inconsistently applied top-level settings](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L55-L59), and [zombie values](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L101-L108). The original motivation for this change was to make it easy to add a new persistent setting source without introducing more bugs and inconsistencies. Two commits building on top of this to add a persistent `-wallet` setting are pretty straightforward and show how the new code can be extended: * 70675c3 from #15935 – _Add \<datadir>/settings.json persistent settings storage_ * 04c80c4 from #15937 – _Add loadwallet and createwallet RPC load_on_startup options_ ACKs for top commit: ariard: ACK 083c954 jnewbery: ACK 083c954 jamesob: ACK 083c954 Tree-SHA512: 5d106746a44d64d3963c4ef3f4a2fa668a4bedcc9018d3ea12c86beae2fda48a0b036241665837f68685712366f70f2e1faba84d193fa1f456013503097b7659
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Nov 9, 2019
083c954 Add settings_tests (Russell Yanofsky) 7f40528 Deduplicate settings merge code (Russell Yanofsky) 9dcb952 Add util::Settings struct and helper functions. (Russell Yanofsky) e2e37cf Remove includeconf nested scope (Russell Yanofsky) 5a84aa8 Rename includeconf variables for clarity (Russell Yanofsky) dc8e1e7 Clarify emptyIncludeConf logic (Russell Yanofsky) Pull request description: This is a refactoring-only change that makes it easier to add a new settings source. This PR doesn't change behavior. The [`util_ArgsMerge`](https://github.com/bitcoin/bitcoin/blob/deb2327b435925c6a39ca654a79283b8eb6aeb86/src/test/util_tests.cpp#L626-L822) and [`util_ChainMerge`](https://github.com/bitcoin/bitcoin/blob/deb2327b435925c6a39ca654a79283b8eb6aeb86/src/test/util_tests.cpp#L843-L924) tests added in bitcoin#15869 and bitcoin#15988 were written specifically to confirm that ArgsManager settings are parsed, merged, and returned the same way before and after this change. This change: - Makes it easier to add new settings sources that can get merged with existing sources (see 70675c3 from bitcoin#15935). - Separates parsing of settings from merging of settings, and deduplicates merging code so it doesn't happen five different places ([GetArg](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L221-L244), [GetNetBoolArg](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L255-L261), [GetArgs](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L460-L467), [IsArgNegated](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L482-L491), [GetUnsuitableSectionOnlyArgs](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L343-L352)) in inconsistent ways. - Documents and tests current strange merging behaviors, so they be cleaned up in the future if resulting code simplifications and UX improvements warrant loss of backwards compatibility. The newly documented behaviors are: command line [ignored arguments](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/system.cpp#L323-L326) and [more ignored arguments](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L67-L72), and config file [reverse precedence](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L61-L65), [inconsistently applied top-level settings](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L55-L59), and [zombie values](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L101-L108). The original motivation for this change was to make it easy to add a new persistent setting source without introducing more bugs and inconsistencies. Two commits building on top of this to add a persistent `-wallet` setting are pretty straightforward and show how the new code can be extended: * 70675c3 from bitcoin#15935 – _Add \<datadir>/settings.json persistent settings storage_ * 04c80c4 from bitcoin#15937 – _Add loadwallet and createwallet RPC load_on_startup options_ ACKs for top commit: ariard: ACK 083c954 jnewbery: ACK 083c954 jamesob: ACK 083c954 Tree-SHA512: 5d106746a44d64d3963c4ef3f4a2fa668a4bedcc9018d3ea12c86beae2fda48a0b036241665837f68685712366f70f2e1faba84d193fa1f456013503097b7659
deadalnix
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Apr 27, 2020
Summary: Thsi is a backport of Core [[bitcoin/bitcoin#15869 | PR15869]] Test Plan: make check Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D5846
sidhujag
pushed a commit
to syscoin-core/syscoin
that referenced
this pull request
Nov 10, 2020
083c954 Add settings_tests (Russell Yanofsky) 7f40528 Deduplicate settings merge code (Russell Yanofsky) 9dcb952 Add util::Settings struct and helper functions. (Russell Yanofsky) e2e37cf Remove includeconf nested scope (Russell Yanofsky) 5a84aa8 Rename includeconf variables for clarity (Russell Yanofsky) dc8e1e7 Clarify emptyIncludeConf logic (Russell Yanofsky) Pull request description: This is a refactoring-only change that makes it easier to add a new settings source. This PR doesn't change behavior. The [`util_ArgsMerge`](https://github.com/bitcoin/bitcoin/blob/deb2327b435925c6a39ca654a79283b8eb6aeb86/src/test/util_tests.cpp#L626-L822) and [`util_ChainMerge`](https://github.com/bitcoin/bitcoin/blob/deb2327b435925c6a39ca654a79283b8eb6aeb86/src/test/util_tests.cpp#L843-L924) tests added in bitcoin#15869 and bitcoin#15988 were written specifically to confirm that ArgsManager settings are parsed, merged, and returned the same way before and after this change. This change: - Makes it easier to add new settings sources that can get merged with existing sources (see 70675c3 from bitcoin#15935). - Separates parsing of settings from merging of settings, and deduplicates merging code so it doesn't happen five different places ([GetArg](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L221-L244), [GetNetBoolArg](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L255-L261), [GetArgs](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L460-L467), [IsArgNegated](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L482-L491), [GetUnsuitableSectionOnlyArgs](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L343-L352)) in inconsistent ways. - Documents and tests current strange merging behaviors, so they be cleaned up in the future if resulting code simplifications and UX improvements warrant loss of backwards compatibility. The newly documented behaviors are: command line [ignored arguments](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/system.cpp#L323-L326) and [more ignored arguments](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L67-L72), and config file [reverse precedence](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L61-L65), [inconsistently applied top-level settings](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L55-L59), and [zombie values](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L101-L108). The original motivation for this change was to make it easy to add a new persistent setting source without introducing more bugs and inconsistencies. Two commits building on top of this to add a persistent `-wallet` setting are pretty straightforward and show how the new code can be extended: * 70675c3 from bitcoin#15935 – _Add \<datadir>/settings.json persistent settings storage_ * 04c80c4 from bitcoin#15937 – _Add loadwallet and createwallet RPC load_on_startup options_ ACKs for top commit: ariard: ACK 083c954 jnewbery: ACK 083c954 jamesob: ACK 083c954 Tree-SHA512: 5d106746a44d64d3963c4ef3f4a2fa668a4bedcc9018d3ea12c86beae2fda48a0b036241665837f68685712366f70f2e1faba84d193fa1f456013503097b7659
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Sep 11, 2021
151f3e9 Add settings merge test to prevent regresssions (Russell Yanofsky) Pull request description: Test-only change. Motivation: I'm trying to clean up settings code and add support for read/write settings without changing existing behavior, but current tests are very scattershot and don't actually cover a lot of current behavior. ACKs for commit 151f3e: jonasschnelli: utACK 151f3e9. MarcoFalke: utACK 151f3e9 Tree-SHA512: f9062f078da02855cdbdcae37d0cea5684e82adbe5c701a8eb042ee4a57d899f0ffb6a9db3bcf58b639dff22b2b2d8a75f9a7917402df58904036753d65a1e3e
Munkybooty
pushed a commit
to Munkybooty/dash
that referenced
this pull request
Oct 12, 2021
151f3e9 Add settings merge test to prevent regresssions (Russell Yanofsky) Pull request description: Test-only change. Motivation: I'm trying to clean up settings code and add support for read/write settings without changing existing behavior, but current tests are very scattershot and don't actually cover a lot of current behavior. ACKs for commit 151f3e: jonasschnelli: utACK 151f3e9. MarcoFalke: utACK 151f3e9 Tree-SHA512: f9062f078da02855cdbdcae37d0cea5684e82adbe5c701a8eb042ee4a57d899f0ffb6a9db3bcf58b639dff22b2b2d8a75f9a7917402df58904036753d65a1e3e
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Test-only change. Motivation: I'm trying to clean up settings code and add support for read/write settings without changing existing behavior, but current tests are very scattershot and don't actually cover a lot of current behavior.