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

Add settings merge test to prevent regresssions #15869

Merged
merged 1 commit into from Apr 30, 2019

Conversation

@ryanofsky
Copy link
Contributor

ryanofsky commented Apr 22, 2019

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.

@DrahtBot DrahtBot added the Tests label Apr 22, 2019
@jamesob

This comment has been minimized.

Copy link
Member

jamesob commented Apr 27, 2019

Concept ACK. Going to go line-by-line in the next few days.

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Apr 29, 2019

Thanks for doing this.
utACK 151f3e9.
I haven't verified the sha256 check hash.

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Apr 29, 2019

Concept ACK

Very nice! Regression testing is our best friend!

Copy link
Member

MarcoFalke left a 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};

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Apr 29, 2019

Member

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 = {};
Copy link
Member

promag left a comment

Concept ACK.

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");

This comment has been minimized.

Copy link
@promag

promag Apr 29, 2019

Member

BOOST_REQUIRE instead of throwing?

BOOST_FIXTURE_TEST_CASE(util_SettingsMerge, SettingsMergeTestingSetup)
{
CHash256 out_sha;
FILE* out_file = nullptr;

This comment has been minimized.

Copy link
@promag

promag Apr 29, 2019

Member

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 };

This comment has been minimized.

Copy link
@promag

promag Apr 29, 2019

Member

Why = 0 since it's already the default?

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Apr 30, 2019

Going to merge this, I think our style-nits can be fixed up later or not at all

@MarcoFalke MarcoFalke merged commit 151f3e9 into bitcoin:master Apr 30, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
MarcoFalke added 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 added 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.
Kiku-Reise added a commit to Kiku-Reise/bitcoin that referenced this pull request May 16, 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.
@LitecoinZ LitecoinZ referenced this pull request May 31, 2019
44 of 244 tasks complete
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 added 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.