Skip to content

Commit

Permalink
refactor: Implement missing error checking for ArgsManager flags
Browse files Browse the repository at this point in the history 
Trigger startup errors if bitcoin is configured with bad setting values
according to flags. Also raise internal errors if settings are registered and
retrieved with inconsistent flags.

This change has no effect on behavior because ArgsManager flags were recently
added in bitcoin#16097 and aren't used anywhere yet.
  • Loading branch information
@ryanofsky
ryanofsky committed Dec 19, 2019
1 parent 6677be6 commit 5bb512a
Show file tree
Hide file tree
Showing 3 changed files with 307 additions and 68 deletions.
149 changes: 140 additions & 9 deletions src/test/util_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ class CheckValueTest : public TestChain100Setup

if (expect.error) {
BOOST_CHECK(!success);
BOOST_CHECK_NE(error.find(expect.error), std::string::npos);
BOOST_CHECK_EQUAL(error, expect.error);
} else {
BOOST_CHECK(success);
BOOST_CHECK_EQUAL(error, "");
Expand All @@ -253,16 +253,16 @@ class CheckValueTest : public TestChain100Setup
BOOST_CHECK_EQUAL(test.GetArg("-value", "zzzzz"), "zzzzz");
} else if (expect.string_value) {
BOOST_CHECK_EQUAL(test.GetArg("-value", "zzzzz"), expect.string_value);
} else {
BOOST_CHECK(!success);
} else if (success) {
BOOST_CHECK_THROW(test.GetArg("-value", "zzzzz"), std::logic_error);
}

if (expect.default_int) {
BOOST_CHECK_EQUAL(test.GetArg("-value", 99999), 99999);
} else if (expect.int_value) {
BOOST_CHECK_EQUAL(test.GetArg("-value", 99999), *expect.int_value);
} else {
BOOST_CHECK(!success);
} else if (success) {
BOOST_CHECK_THROW(test.GetArg("-value", 99999), std::logic_error);
}

if (expect.default_bool) {
Expand All @@ -271,15 +271,16 @@ class CheckValueTest : public TestChain100Setup
} else if (expect.bool_value) {
BOOST_CHECK_EQUAL(test.GetBoolArg("-value", false), *expect.bool_value);
BOOST_CHECK_EQUAL(test.GetBoolArg("-value", true), *expect.bool_value);
} else {
BOOST_CHECK(!success);
} else if (success) {
BOOST_CHECK_THROW(test.GetBoolArg("-value", false), std::logic_error);
BOOST_CHECK_THROW(test.GetBoolArg("-value", true), std::logic_error);
}

if (expect.list_value) {
auto l = test.GetArgs("-value");
BOOST_CHECK_EQUAL_COLLECTIONS(l.begin(), l.end(), expect.list_value->begin(), expect.list_value->end());
} else {
BOOST_CHECK(!success);
} else if (success) {
BOOST_CHECK_THROW(test.GetArgs("-value"), std::logic_error);
}
}
};
Expand All @@ -301,6 +302,136 @@ BOOST_FIXTURE_TEST_CASE(util_CheckValue, CheckValueTest)
CheckValue(M::ALLOW_ANY, "-value=1", Expect{"1"}.String("1").Int(1).Bool(true).List({"1"}));
CheckValue(M::ALLOW_ANY, "-value=2", Expect{"2"}.String("2").Int(2).Bool(true).List({"2"}));
CheckValue(M::ALLOW_ANY, "-value=abc", Expect{"abc"}.String("abc").Int(0).Bool(false).List({"abc"}));

CheckValue(M::ALLOW_BOOL, nullptr, Expect{{}}.DefaultBool());
CheckValue(M::ALLOW_BOOL, "-novalue", Expect{false}.Bool(false));
CheckValue(M::ALLOW_BOOL, "-novalue=", Expect{{}}.Error("Can not negate -value at the same time as setting value ''."));
CheckValue(M::ALLOW_BOOL, "-novalue=0", Expect{{}}.Error("Can not negate -value at the same time as setting value '0'."));
CheckValue(M::ALLOW_BOOL, "-novalue=1", Expect{false}.Bool(false));
CheckValue(M::ALLOW_BOOL, "-novalue=2", Expect{{}}.Error("Can not negate -value at the same time as setting value '2'."));
CheckValue(M::ALLOW_BOOL, "-novalue=abc", Expect{{}}.Error("Can not negate -value at the same time as setting value 'abc'."));
CheckValue(M::ALLOW_BOOL, "-value", Expect{true}.Bool(true));
CheckValue(M::ALLOW_BOOL, "-value=", Expect{""}.DefaultBool());
CheckValue(M::ALLOW_BOOL, "-value=0", Expect{false}.Bool(false));
CheckValue(M::ALLOW_BOOL, "-value=1", Expect{true}.Bool(true));
CheckValue(M::ALLOW_BOOL, "-value=2", Expect{{}}.Error("Can not set -value value to '2'. It must be set to 0 or 1."));
CheckValue(M::ALLOW_BOOL, "-value=abc", Expect{{}}.Error("Can not set -value value to 'abc'. It must be set to 0 or 1."));

CheckValue(M::ALLOW_INT, nullptr, Expect{{}}.DefaultInt().DefaultBool());
CheckValue(M::ALLOW_INT, "-novalue", Expect{false}.Int(0).Bool(false));
CheckValue(M::ALLOW_INT, "-novalue=", Expect{{}}.Error("Can not negate -value at the same time as setting value ''."));
CheckValue(M::ALLOW_INT, "-novalue=0", Expect{{}}.Error("Can not negate -value at the same time as setting value '0'."));
CheckValue(M::ALLOW_INT, "-novalue=1", Expect{false}.Int(0).Bool(false));
CheckValue(M::ALLOW_INT, "-novalue=2", Expect{{}}.Error("Can not negate -value at the same time as setting value '2'."));
CheckValue(M::ALLOW_INT, "-novalue=abc", Expect{{}}.Error("Can not negate -value at the same time as setting value 'abc'."));
CheckValue(M::ALLOW_INT, "-value", Expect{{}}.Error("Can not set -value with no value. It must be set to an integer."));
CheckValue(M::ALLOW_INT, "-value=", Expect{""}.DefaultInt().DefaultBool());
CheckValue(M::ALLOW_INT, "-value=0", Expect{0}.Int(0).Bool(false));
CheckValue(M::ALLOW_INT, "-value=1", Expect{1}.Int(1).Bool(true));
CheckValue(M::ALLOW_INT, "-value=2", Expect{2}.Int(2).Bool(true));
CheckValue(M::ALLOW_INT, "-value=abc", Expect{{}}.Error("Can not set -value value to 'abc'. It must be set to an integer."));

CheckValue(M::ALLOW_STRING, nullptr, Expect{{}}.DefaultString().DefaultBool());
CheckValue(M::ALLOW_STRING, "-novalue", Expect{false}.String("").Bool(false));
CheckValue(M::ALLOW_STRING, "-novalue=", Expect{{}}.Error("Can not negate -value at the same time as setting value ''."));
CheckValue(M::ALLOW_STRING, "-novalue=0", Expect{{}}.Error("Can not negate -value at the same time as setting value '0'."));
CheckValue(M::ALLOW_STRING, "-novalue=1", Expect{false}.String("").Bool(false));
CheckValue(M::ALLOW_STRING, "-novalue=2", Expect{{}}.Error("Can not negate -value at the same time as setting value '2'."));
CheckValue(M::ALLOW_STRING, "-novalue=abc", Expect{{}}.Error("Can not negate -value at the same time as setting value 'abc'."));
CheckValue(M::ALLOW_STRING, "-value", Expect{{}}.Error("Can not set -value with no value. It must be set to a string."));
CheckValue(M::ALLOW_STRING, "-value=", Expect{""}.DefaultString().DefaultBool());
CheckValue(M::ALLOW_STRING, "-value=0", Expect{"0"}.String("0").DefaultBool());
CheckValue(M::ALLOW_STRING, "-value=1", Expect{"1"}.String("1").DefaultBool());
CheckValue(M::ALLOW_STRING, "-value=2", Expect{"2"}.String("2").DefaultBool());
CheckValue(M::ALLOW_STRING, "-value=abc", Expect{"abc"}.String("abc").DefaultBool());

CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, nullptr, Expect{{}}.DefaultInt().DefaultBool());
CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-novalue", Expect{false}.Int(0).Bool(false));
CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-novalue=", Expect{{}}.Error("Can not negate -value at the same time as setting value ''."));
CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-novalue=0", Expect{{}}.Error("Can not negate -value at the same time as setting value '0'."));
CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-novalue=1", Expect{false}.Int(0).Bool(false));
CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-novalue=2", Expect{{}}.Error("Can not negate -value at the same time as setting value '2'."));
CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-novalue=abc", Expect{{}}.Error("Can not negate -value at the same time as setting value 'abc'."));
CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-value", Expect{true}.Int(1).Bool(true));
CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-value=", Expect{""}.DefaultInt().DefaultBool());
CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-value=0", Expect{0}.Int(0).Bool(false));
CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-value=1", Expect{1}.Int(1).Bool(true));
CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-value=2", Expect{2}.Int(2).Bool(true));
CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-value=abc", Expect{{}}.Error("Can not set -value value to 'abc'. It must be set to an integer."));

CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, nullptr, Expect{{}}.DefaultString().DefaultBool());
CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-novalue", Expect{false}.String("").Bool(false));
CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-novalue=", Expect{{}}.Error("Can not negate -value at the same time as setting value ''."));
CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-novalue=0", Expect{{}}.Error("Can not negate -value at the same time as setting value '0'."));
CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-novalue=1", Expect{false}.String("").Bool(false));
CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-novalue=2", Expect{{}}.Error("Can not negate -value at the same time as setting value '2'."));
CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-novalue=abc", Expect{{}}.Error("Can not negate -value at the same time as setting value 'abc'."));
CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-value", Expect{true}.DefaultString().Bool(true));
CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-value=", Expect{""}.DefaultString().DefaultBool());
CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-value=0", Expect{"0"}.String("0").DefaultBool());
CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-value=1", Expect{"1"}.String("1").DefaultBool());
CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-value=2", Expect{"2"}.String("2").DefaultBool());
CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-value=abc", Expect{"abc"}.String("abc").DefaultBool());

CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, nullptr, Expect{{}}.List({}));
CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-novalue", Expect{false}.List({}));
CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-novalue=", Expect{{}}.Error("Can not negate -value at the same time as setting value ''."));
CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-novalue=0", Expect{{}}.Error("Can not negate -value at the same time as setting value '0'."));
CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-novalue=1", Expect{false}.List({}));
CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-novalue=2", Expect{{}}.Error("Can not negate -value at the same time as setting value '2'."));
CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-novalue=abc", Expect{{}}.Error("Can not negate -value at the same time as setting value 'abc'."));
CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-value", Expect{{}}.Error("Can not set -value with no value. It must be set to a string."));
CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-value=", Expect{""}.List({""}));
CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-value=0", Expect{"0"}.List({"0"}));
CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-value=1", Expect{"1"}.List({"1"}));
CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-value=2", Expect{"2"}.List({"2"}));
CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-value=abc", Expect{"abc"}.List({"abc"}));
}

BOOST_FIXTURE_TEST_CASE(util_CheckBoolStringsNotSpecial, CheckValueTest)
{
using M = ArgsManager;

// Check that "true" and "false" strings are rejected for ALLOW_BOOL
// arguments. We might want to change this behavior in the future and
// interpret strings like "true" as true, and strings like "false" as false.
// But because it would be confusing to interpret "true" as true for
// ALLOW_BOOL arguments but false for ALLOW_ANY arguments (because
// atoi("true")==0), for now it is safer to just disallow strings like
// "true" and "false" for ALLOW_BOOL arguments as long as there are still
// other boolean arguments interpreted with ALLOW_ANY.
CheckValue(M::ALLOW_BOOL, "-value=true", Expect{{}}.Error("Can not set -value value to 'true'. It must be set to 0 or 1."));
CheckValue(M::ALLOW_BOOL, "-value=false", Expect{{}}.Error("Can not set -value value to 'false'. It must be set to 0 or 1."));

// Similarly, check "true" and "false" are not treated specially when
// ALLOW_BOOL is combined with ALLOW_INT and ALLOW_STRING. (The only
// difference ALLOW_BOOL makes for int and string arguments is that it
// enables "-foo" syntax with no equal sign assigning explicit int or string
// values. This is useful for arguments like "-upgradewallet" or "-listen"
// that primarily toggle features on and off, but also accept optional int
// or string values to influence behavior.)
CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-value=true", Expect{{}}.Error("Can not set -value value to 'true'. It must be set to an integer."));
CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-value=false", Expect{{}}.Error("Can not set -value value to 'false'. It must be set to an integer."));
CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-value=true", Expect{"true"}.String("true").DefaultBool());
CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-value=false", Expect{"false"}.String("false").DefaultBool());
}

BOOST_AUTO_TEST_CASE(util_CheckSingleValue)
{
TestArgsManager test;
test.SetupArgs({{"-single", ArgsManager::ALLOW_INT}});
std::istringstream stream("single=1\nsingle=2\n");
std::string error;
BOOST_CHECK(!test.ReadConfigStream(stream, "file.conf", error));
BOOST_CHECK_EQUAL(error, "Multiple values specified for -single in same section of config file.");
}

BOOST_AUTO_TEST_CASE(util_CheckBadFlagCombinations)
{
TestArgsManager test;
using M = ArgsManager;
BOOST_CHECK_THROW(test.AddArg("-arg1", "name", M::ALLOW_INT | M::ALLOW_ANY, OptionsCategory::OPTIONS), std::logic_error);
BOOST_CHECK_THROW(test.AddArg("-arg2", "name", M::ALLOW_INT | M::ALLOW_STRING, OptionsCategory::OPTIONS), std::logic_error);
}

BOOST_AUTO_TEST_CASE(util_ParseParameters)
Expand Down
Loading

0 comments on commit 5bb512a

Please sign in to comment.