Add test for ArgsManager::GetChainName #15988

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)

    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)

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 81dc0a6c7efde6beda8e20d7654843fbdeddec4c54e180c02d9c193b1a3225ef -

</details>

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

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 9e787e94a78c234ae577714f7e54430e71958b3215367ff36ed61ff3a94b49ad -

</details>

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