test: added test for RPCConvertNamedValues in rpc

brakmic commented at 4:55 PM on November 21, 2019: contributor

This PR adds a test for function RPCConvertNamedValues from rpc/client.cpp. As there's already a test for RPCConvertValues, I thought, it would be appropriate to add this one too.

The test comprises of two checks:

proper conversions
throwing of exceptions when bad data is given, for example arguments without the madatory =. According to function description, the = must be present even if there is no data.

I am using this website to check for missing test coverages: https://marcofalke.github.io/btc_cov/test_bitcoin.coverage/src/rpc/client.cpp.gcov.html

I read about it here: #16450, which is also the reason why I started searching for missing test coverages.

My test results:

Entering test module "Bitcoin Core Test Suite"
test/rpc_tests.cpp:43: Entering test suite "rpc_tests"
test/rpc_tests.cpp:342: Entering test case "rpc_convert_named_values_generatetoaddress"
test/rpc_tests.cpp:351: info: check 'no exceptions thrown by result = RPCConvertNamedValues("generatetoaddress", valid_named_values)' has passed
test/rpc_tests.cpp:352: info: check result[0].get_int() == 10 has passed
test/rpc_tests.cpp:353: info: check result[1].get_str() == "2N7xz1EnWubTbQmWFcVpCvT2RwKeDuzWr5g" has passed
test/rpc_tests.cpp:354: info: check result[2].get_int() == 50 has passed
test/rpc_tests.cpp:362: info: check 'exception "std::runtime_error" raised as expected' has passed
test/rpc_tests.cpp:342: Leaving test case "rpc_convert_named_values_generatetoaddress"; testing time: 78684us
test/rpc_tests.cpp:43: Leaving test suite "rpc_tests"; testing time: 78740us
Leaving test module "Bitcoin Core Test Suite"; testing time: 78943us

fanquake added the label Tests on Nov 21, 2019

in src/test/rpc_tests.cpp:356 in d72de2e061 outdated

 351 | +    BOOST_CHECK_NO_THROW(result = RPCConvertNamedValues("generatetoaddress", valid_named_values));
 352 | +    BOOST_CHECK_EQUAL(result[0].get_int(), 10);
 353 | +    BOOST_CHECK_EQUAL(result[1].get_str(), "2N7xz1EnWubTbQmWFcVpCvT2RwKeDuzWr5g");
 354 | +    BOOST_CHECK_EQUAL(result[2].get_int(), 50);
 355 | +
 356 | +    std::vector<std::string> invalid_named_values = {

paymog commented at 3:41 PM on November 23, 2019:

what do you think about separating out the invalid value test logic into an entirely new test? I personally believe tests should be as small and focused as possible - would love to know what you think.

brakmic commented at 3:58 PM on November 23, 2019:

I followed the other tests from rpc_tests.cpp when writing this one. Not sure if this function should be tested via two separate tests. Nothing agains more tests, of course, so feel free to provide alternatives.

paymog commented at 8:09 AM on November 28, 2019:

Hmm, I'm new to the codebase, I guess we should stick to the style that already exists.

in src/test/rpc_tests.cpp:359 in d72de2e061 outdated

 354 | +    BOOST_CHECK_EQUAL(result[2].get_int(), 50);
 355 | +
 356 | +    std::vector<std::string> invalid_named_values = {
 357 | +        "nblocks=10",
 358 | +        "address",
 359 | +        "maxtries"

paymog commented at 3:43 PM on November 23, 2019:

do we need this param for RPCConvertNamedValues to throw? It seems that std::runtime_error will be thrown when address is parsed.

brakmic commented at 4:00 PM on November 23, 2019:

👍

theStack commented at 3:56 PM on March 20, 2020:

I'd agree to paymog here, the "maxtries" parameter is not needed for the exception to be thrown.

in src/test/rpc_tests.cpp:338 in d72de2e061 outdated

 338 | @@ -339,6 +339,29 @@ BOOST_AUTO_TEST_CASE(rpc_convert_values_generatetoaddress)
 339 |      BOOST_CHECK_EQUAL(result[2].get_int(), 9);
 340 |  }
 341 |  
 342 | +BOOST_AUTO_TEST_CASE(rpc_convert_named_values_generatetoaddress)

paymog commented at 3:45 PM on November 23, 2019:

do you want to be a hero and also test the logic even if it is empty from https://github.com/bitcoin/bitcoin/blob/cab94cc07489f704c4b95171b23be0e8025df794/src/rpc/client.cpp#L247

brakmic commented at 4:00 PM on November 23, 2019:

I must admit that I don't understand your question. Sorry.

paymog commented at 8:07 AM on November 28, 2019:

Sorry for the confusion. What do you think about adding a test like:

    UniValue result;
    std::vector<std::string> valid_named_values = {
        "nblocks=10",
        "address=", // note that no address is provided by the = exists, an error should not be thrown
    };

    BOOST_CHECK_NO_THROW(result = RPCConvertNamedValues("generatetoaddress", valid_named_values));

brakmic commented at 8:17 AM on November 28, 2019:

Looks good. Many thanks. Will try it out!

paymog approved

paymog commented at 8:11 AM on November 28, 2019: none

I'm new to the codebase but LGTM. ACK.

in src/test/rpc_tests.cpp:345 in d72de2e061 outdated

 338 | @@ -339,6 +339,29 @@ BOOST_AUTO_TEST_CASE(rpc_convert_values_generatetoaddress)
 339 |      BOOST_CHECK_EQUAL(result[2].get_int(), 9);
 340 |  }
 341 |  
 342 | +BOOST_AUTO_TEST_CASE(rpc_convert_named_values_generatetoaddress)
 343 | +{
 344 | +    UniValue result;
 345 | +    std::vector<std::string> valid_named_values = {

theStack commented at 3:55 PM on March 20, 2020:

nit: could declare this as const (or as alternative, not even create an explicit variable but simply directly pass an initializer list, like in the test case rpc_convert_values_generatetoaddress above).

brakmic commented at 4:22 PM on March 20, 2020:

Many thanks! I've updated the code.

in src/test/rpc_tests.cpp:362 in d72de2e061 outdated

 357 | +        "nblocks=10",
 358 | +        "address",
 359 | +        "maxtries"
 360 | +    };
 361 | +
 362 | +    BOOST_CHECK_THROW(RPCConvertNamedValues("generatetoaddress", invalid_named_values), std::runtime_error);

theStack commented at 3:58 PM on March 20, 2020:

There should be a short explanation comment somewhere (either here or directly on the invalid named parameter) on why this is invalid and throws an exception.

theStack commented at 3:58 PM on March 20, 2020: member

Concept ACK, I left a few code review comments.

test: added test for RPCConvertNamedValues in rpc_tests ed9a8e818d

brakmic closed this on May 10, 2020

MarcoFalke locked this on Feb 15, 2022

test: added test for RPCConvertNamedValues in rpc_tests #17551