Add test for ArgsManager::GetChainName

ryanofsky commented at 11:30 am on May 9, 2019: member

There was some test coverage previously, but it was limited and didn’t test conflicting and negated arguments.

util_SettingsMerge test cleanup

Followup to #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.

05bfee3451

Add unit test NextString, ForEachNoDup functions

Remove testcase generating code from util_SettingsMerge so it can be reused in
new tests.

The hash value expected in util_SettingsMerge changes as a result of this, but
only because the testcases are generated in a different order, not because any
cases are added or removed. It is possible to verify this with:

    SETTINGS_MERGE_TEST_OUT=new.txt test/test_bitcoin --run_test=util_tests/util_SettingsMerge
    git checkout HEAD~1
    make test/test_bitcoin
    SETTINGS_MERGE_TEST_OUT=old.txt test/test_bitcoin --run_test=util_tests/util_SettingsMerge
    diff -u <(sort old.txt) <(sort new.txt)

The new output is a little more readable, with simpler testcases sorted first.

4b331159df

Add test for ArgsManager::GetChainName

There was some test coverage previously, but it was limited and didn't test
conflicting and negated arguments.

f6bb11fd37

fanquake added the label Tests on May 9, 2019

in src/test/util_tests.cpp:814 in ede1812765 outdated

811+        TestArgsManager parser;
812+        LOCK(parser.cs_args);
813+        parser.AddArg("-regtest", "regtest", false, OptionsCategory::OPTIONS);
814+        parser.AddArg("-testnet", "testnet", false, OptionsCategory::OPTIONS);
815+
816+        auto arg = [](Action action) { return action == SET_TEST ? "-testnet" : action == NEGATE_TEST ? "-notestnet" : action == SET_REG ? "-regtest" : action == NEGATE_REG ? "-noregtest" : nullptr; };

MarcoFalke commented at 11:42 am on May 9, 2019:

style-nit: Could flow into multiple lines?

in src/test/util_tests.cpp:609 in ede1812765 outdated

606     //! Max number of actions to sequence together. Can decrease this when
607     //! debugging to make test results easier to understand.
608     static constexpr int MAX_ACTIONS = 3;
609 
610-    enum Action { SET = 0, NEGATE, SECTION_SET, SECTION_NEGATE, END };
611+    enum Action { END, SET, NEGATE, SECTION_SET, SECTION_NEGATE };

MarcoFalke commented at 11:50 am on May 9, 2019:

In commit 00093ed7aae76d98792cfe9e3706ec3e27be2a13

Shouldn’t this be named (or similar)

0    enum Action { NONE, SET, NEGATE, SECTION_SET, SECTION_NEGATE };

MarcoFalke approved

MarcoFalke commented at 11:54 am on May 9, 2019: member

ACK, just some style questions. Will scroll through the generated file later today.

DrahtBot commented at 2:39 pm on May 9, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15926 (tinyformat: Add format_no_throw by MarcoFalke)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

ryanofsky force-pushed on May 9, 2019

ryanofsky commented at 3:15 pm on May 9, 2019: member

Updated ede18127657652aa1b49b4370f53cb1bfb2e0886 -> 0a82350f92e087ba4dab276215679a1044c8bf2e (pr/testset2.1 -> pr/testset2.2, compare) with Marco’s suggestions (thanks!)

MarcoFalke commented at 4:24 pm on May 13, 2019: member

utACK 0a82350f92e087ba4dab276215679a1044c8bf2e

Checked that 4b331159dfa599eced9f9d4e5780173367b43c74 only re-sorts the txt file Checked that 0a82350f92e087ba4dab276215679a1044c8bf2e produces a nice file which looks plausibly correct (only checked the first 5 lines)

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3utACK 0a82350f92e087ba4dab276215679a1044c8bf2e
 4
 5Checked that 4b331159dfa599eced9f9d4e5780173367b43c74 only re-sorts the txt file
 6Checked that 0a82350f92e087ba4dab276215679a1044c8bf2e produces a nice file which looks plausibly correct (only checked the first 5 lines)
 7-----BEGIN PGP SIGNATURE-----
 8
 9iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
10pUgagAv7BrgDc1Dcsc7PdRJLiKhqhRZ0UAL5FbNROeLV232+YJHopWYdMJYhvPL7
11xvQkMgrLyDy/xvDR74+2CusA0pX+YaAICit9QSV4r+Vb6WwAuuXNB+g757JVvqJI
12ietTVrW2zK3sk8x7UWCPssJGUQruXnk3gQM2A7SMBgy8Xs4pPSWgv3CdfctGcyLL
13ncoW/wEgRRJgMWJml3NFbdTogD525IoCDpPlOAEAIUwlXTeNzKvC6m2erJ7UaNvt
14iof1t5gZOFiPR5s/Gd81BRjpjYuxnJ7xFRd0fUUcuVVmdxiosoEe3u0/xb9RCCRf
15PD9Ii+L1Ba8/TAYF8xZ/lxmH+8g+cN/SiJ2fcPM+dAi0wEbBXwYo3py5QYjUbjYO
160G0j/yn95HFZOcyrgKDSaCClA2wXDbWVFdxTMGlzfMza5OG2ZKcxmclLelskiqtY
17sUBJGksj4RGgnPZHEJ/NfiSzUGOE7U9rUn5BTvrMxJaoSFJelNeRtrYcrdzwt6M/
18J8XA0Ror
19=TUSH
20-----END PGP SIGNATURE-----

Timestamp of file with hash 81dc0a6c7efde6beda8e20d7654843fbdeddec4c54e180c02d9c193b1a3225ef -

ryanofsky force-pushed on May 13, 2019

ryanofsky commented at 4:34 pm on May 13, 2019: member

Updated 0a82350f92e087ba4dab276215679a1044c8bf2e -> f6bb11fd37f8a2c985786b688ea07699ba75780e (pr/testset2.2 -> pr/testset2.3, compare) adding test for disabled arguments (-regtest=0) in addition to negated arguments (-noregtest)

MarcoFalke commented at 4:40 pm on May 13, 2019: member

I think we should still test for flags on the command line (I’d doubt that anyone types -regtest=1 over -regtest)

ryanofsky commented at 5:37 pm on May 13, 2019: member

I think we should still test for flags on the command line (I’d doubt that anyone types -regtest=1 over -regtest)

I could add more variations to test this, but the reason I didn’t is because InterpretBool(), interprets both"1" and the empty string as true, and we already have tests for variations of InterpretBool parsing behavior elsewhere.

https://github.com/bitcoin/bitcoin/blob/e79bbb73e08e3f191e97d3b67a2fbb510c5545ff/src/util/system.cpp#L159-L164

I think the new test will be less fragile if it is only testing merging of overlapping and negated settings and not trying to test boolean parsing at the same time.

Another option would be to test -regtest instead of -regtest=1 not in addition to it. I didn’t do this because it would make translating enum values into argument and config strings messier, but I could do it if you’d really prefer it.

MarcoFalke commented at 7:12 pm on May 13, 2019: member

re-utACK f6bb11fd37f8a2c985786b688ea07699ba75780e

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-utACK f6bb11fd37f8a2c985786b688ea07699ba75780e
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUhZeAv/esRJrFMEBsJ5m0DMtJYJEC8wlFodQn4/2qpURnfd84q3IjefgrFAoZ4N
 8Iz1f5Q6mLN8HJNyHppbnsqgtO5DWFyH/EuXnKV0/G+qRTreltdGla8mxQ3YIveZ0
 9hmaa/ReAz3/aCfTQtRqtbXId8cg1+ReemaEIfadtZYOvvEAsgThGM6WQBV6VW9Rb
10f1P1jIhkoX4c1VaqC5HLC73ZQk8La2kC/yrG9pHRTX056hpWlf1Yn20dyxwlrcoT
1184/JnntV7PXFaxxw0gakrKwUKnckGJwmg0oBMr8XeH01vzE/VXBVizQUFXiRZ8uC
12twZ4mAGpOdXtOLqUgTPJgLRQxQnFbnw4RwBaCBD+FtLm6PMJOAzoXxM+356Yk19n
13tAoUyuTBLZHvzQ4VICbMzS/KoS6r+UM7ObZpcITgyteV846GuvD0kNXZ+ablOyJQ
14BPBYf8Jr/C6bZ2zQrulyTQgiiO2APS5b891Z1saYMauwTOAuK2zqImTarKxgz96/
152/7KVy4H
16=IS1+
17-----END PGP SIGNATURE-----

Timestamp of file with hash 9e787e94a78c234ae577714f7e54430e71958b3215367ff36ed61ff3a94b49ad -

MarcoFalke referenced this in commit 6f4ba6492a on May 14, 2019

MarcoFalke merged this on May 14, 2019

MarcoFalke closed this on May 14, 2019

sidhujag referenced this in commit aae3cb93db on May 18, 2019

laanwj referenced this in commit a7aec7ad97 on Nov 8, 2019

sidhujag referenced this in commit 16a52029e7 on Nov 9, 2019

deadalnix referenced this in commit b2500c3d81 on Apr 28, 2020

sidhujag referenced this in commit c1677b430c on Nov 10, 2020

DrahtBot locked this on Dec 16, 2021

Add test for ArgsManager::GetChainName #15988

Conflicts