test: Add test for rpcwhitelistdefault #17805
Conversation
d522a1a to
5450f6f
Compare
5450f6f to
669d046
Compare
|
Concept ACK |
669d046 to
d76a516
Compare
| from test_framework.util import ( | ||
| assert_equal, | ||
| get_datadir_path, | ||
| str_to_b64str |
There was a problem hiding this comment.
nit: comma at EOL so it doesn't need to be touched if adding a method below it
There was a problem hiding this comment.
The comma is at the end of every line or am I getting it wrong?
There was a problem hiding this comment.
Have a look at example_test.py
| f.write("rpcwhitelist=__cookie__:getblockcount,getwalletinfo,importprivkey,getblockchaininfo,submitblock,addnode,getpeerinfo,getbestblockhash,getrawmempool,syncwithvalidationinterfacequeue,stop\n") | ||
|
|
||
| def run_test(self): | ||
| self.log.info("Testing rpcwhitelistdefault=0 with no specififed permissions") |
There was a problem hiding this comment.
"specified" -- typo here and lines 91 and 94 below
|
|
||
| def run_test(self): | ||
| self.log.info("Testing rpcwhitelistdefault=0 with no specififed permissions") | ||
| assert_equal(200, rpccall(self.nodes[0], "user1", "12345", "getbestblockhash")) |
There was a problem hiding this comment.
perhaps hoist 200 and 403 to constants like HTTP_OK and HTTP_FORBIDDEN
There was a problem hiding this comment.
This is a featrue for another PR. Other tests use the plain codes as well
|
|
||
| class RPCWhitelistDefaultTest(BitcoinTestFramework): | ||
| def set_test_params(self): | ||
| self.num_nodes = 4 |
There was a problem hiding this comment.
perhaps add self.setup_clean_chain = False for explicitness
There was a problem hiding this comment.
It doesn't matter anyway as this isn't a chain related test
There was a problem hiding this comment.
It is false by default (see test_framework.py::L96), but most tests set it explicitly even when false. It matters a bit for test speed as it controls whether or not to use the cached data directories for the test setup.
They are two different options. The test would be to big to test both. |
d76a516 to
a478f6d
Compare
The tests can be organised into different methods within the same test class and file. Many functional test files are much larger than this one would become. Have a look at the size at p2p_segwit.py, feature_block.py, etc. It may be a bit faster in test run time to group similar tests into the same testfile setup where possible. And otherwise, rpccall could be extracted to a common helper method (if one does not already exist). I could be wrong but this is what I'd try to do. It's up to you. |
|
Even if the featrue is very similar, they test both two different things. |
There was a problem hiding this comment.
I think the new file is fine.
However have you considered to use just one node? Like
- change config
- restart
- test calls/access
- repeat
I prefer this way because:
- new test cases don't imply messing with more nodes;
- testing the calls/access is immediately after the config.
Yes but after all I believe this would make the code much messier because I would need to re-implement some functions. This is probably the most cleanest way on how to do it |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
|
Closing as "Up for grabs". If these tests are going to be added, I think adding them to the existing |
A test for
rpcwhitelistdefault.