Add test for ArgsManager::GetChainName #15988

ryanofsky · 2019-05-09T11:30:12Z

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

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.

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.

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

maflcko

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

src/test/util_tests.cpp

DrahtBot · 2019-05-09T14:39:10Z

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

Updated ede1812 -> 0a82350 (pr/testset2.1 -> pr/testset2.2, compare) with Marco's suggestions (thanks!)

maflcko · 2019-05-13T16:24:07Z

utACK 0a82350

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

Show signature and timestamp

Signature:

Timestamp of file with hash 81dc0a6c7efde6beda8e20d7654843fbdeddec4c54e180c02d9c193b1a3225ef -

ryanofsky · 2019-05-13T16:34:05Z

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

maflcko · 2019-05-13T16:40:02Z

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

ryanofsky · 2019-05-13T17:37:18Z

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.

bitcoin/src/util/system.cpp

Lines 159 to 164 in e79bbb7

    
           static bool InterpretBool(const std::string& strValue) 
        
           { 
        
               if (strValue.empty()) 
        
                   return true; 
        
               return (atoi(strValue) != 0); 
        
           }

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.

maflcko · 2019-05-13T19:12:38Z

re-utACK f6bb11f

Show signature and timestamp

Signature:

Timestamp of file with hash 9e787e94a78c234ae577714f7e54430e71958b3215367ff36ed61ff3a94b49ad -

f6bb11f Add test for ArgsManager::GetChainName (Russell Yanofsky) 4b33115 Add unit test NextString, ForEachNoDup functions (Russell Yanofsky) 05bfee3 util_SettingsMerge test cleanup (Russell Yanofsky) Pull request description: There was some test coverage previously, but it was limited and didn't test conflicting and negated arguments. ACKs for commit f6bb11: MarcoFalke: re-utACK f6bb11f Tree-SHA512: d03596614dc48584c7a9440117b107c6abb23fd4c7fa15fb4015351ec3de08b2656bc956ce05310663675672343d7a6aff35421657f29172080c7005045680b0

Summary: * 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. * 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. * Add test for ArgsManager::GetChainName There was some test coverage previously, but it was limited and didn't test conflicting and negated arguments. This is a backport of Core [[bitcoin/bitcoin#15988 | PR15988]] Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D5860

ryanofsky added 3 commits May 7, 2019 11:51

util_SettingsMerge test cleanup

05bfee3

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.

Add test for ArgsManager::GetChainName

f6bb11f

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

fanquake added the Tests label May 9, 2019

maflcko approved these changes May 9, 2019

View reviewed changes

src/test/util_tests.cpp Outdated Show resolved Hide resolved

src/test/util_tests.cpp Outdated Show resolved Hide resolved

ryanofsky force-pushed the pr/testset2 branch from ede1812 to 0a82350 Compare May 9, 2019 15:11

ryanofsky commented May 9, 2019

View reviewed changes

ryanofsky force-pushed the pr/testset2 branch from 0a82350 to f6bb11f Compare May 13, 2019 16:32

maflcko merged commit f6bb11f into bitcoin:master May 14, 2019

maflcko mentioned this pull request May 30, 2019

Feedback for GitHub CEO #15847

Closed

ryanofsky mentioned this pull request Oct 23, 2019

Merge settings one place instead of five places #15934

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add test for ArgsManager::GetChainName #15988

Add test for ArgsManager::GetChainName #15988

Uh oh!

ryanofsky commented May 9, 2019

Uh oh!

maflcko left a comment

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented May 9, 2019

Uh oh!

ryanofsky left a comment

Uh oh!

maflcko commented May 13, 2019

Uh oh!

ryanofsky commented May 13, 2019

Uh oh!

maflcko commented May 13, 2019

Uh oh!

ryanofsky commented May 13, 2019

Uh oh!

maflcko commented May 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Add test for ArgsManager::GetChainName #15988

Add test for ArgsManager::GetChainName #15988

Uh oh!

Conversation

ryanofsky commented May 9, 2019

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented May 9, 2019

Conflicts

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented May 13, 2019

Uh oh!

ryanofsky commented May 13, 2019

Uh oh!

maflcko commented May 13, 2019

Uh oh!

ryanofsky commented May 13, 2019

Uh oh!

maflcko commented May 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants