test: RPC coverage check doesn't work?

maflcko commented at 8:14 am on May 8, 2023: member

Is there an existing issue for this?

I have searched the existing issues

Current behaviour

Currently CI reports that all RPCs are covered: https://cirrus-ci.com/task/6244871116161024?logs=ci#L4674

Expected behaviour

However, some RPCs are clearly not covered because they are never called. For example:

0$ git grep abortrescan  ./test/  | grep -v '. Please call `abortrescan` before ' | wc -l 
10

Steps to reproduce

?

Relevant log output

No response

How did you obtain Bitcoin Core

Compiled from source

What version of Bitcoin Core are you using?

current master

Operating system and version

any

Machine specifications

No response

maflcko added the label Tests on May 8, 2023

maflcko added the label good first issue on May 8, 2023

maflcko commented at 10:11 am on May 8, 2023: member

This may be a good first issue for anyone with python background and passion to dig into the test framework. A starting entry point would be the help text: ./test/functional/test_runner.py --help |grep coverage.

For guidance on contributing, please read CONTRIBUTING.md before opening your pull request.

kouloumos commented at 1:06 pm on May 8, 2023: contributor

The RPC coverage works by comparing the invoked RPCs with the full list of available RPC commands per bitcoin-cli help. What seems to be problem is that the rpc_interface.txt reference-list that we create as part of write_all_rpc_commands() doesn’t contain the wallet RPC commands.

Because this is per test run (rpc_interface.txt is created once per coverage directory), a possible issue could be that the first time we invoke help through write_all_rpc_commands() we do not support the wallet.

Digging a bit deeper it seems that this is indeed the case. The issue is with the cache-creation that happens as part of test runner’s initialization: https://github.com/bitcoin/bitcoin/blob/322ec63b01499c1ec52d3912ee382ebd59f2366b/test/functional/test_runner.py#L550-L553

At this point, create_cache.py initializes the testing framework in order to “manually” create the cached blockchain before running any tests. But part of cache-creation is starting the node which kickstarts the rpc coverage logic, but with a node that doesn’t have a wallet.

If we don’t pass the --coveragedir as part of the flags that we give to create_cache.py we can avoid this issue, but just a reordering of cache-creation<>coverage-initialization pushes the issue to the next coverage-initialization which in that case will be the first test of the test-runner. If the node in that first test doesn’t have a wallet, the issue remains.

maflcko removed the label good first issue on May 8, 2023

maflcko commented at 2:30 pm on August 17, 2023: member

Looks like there are also other bugs with this: https://cirrus-ci.com/task/6126543575973888?logs=ci#L5061

jonatack commented at 8:29 pm on August 17, 2023: contributor

Verified the current CI issue (red on all pulls) locally

0$ test/functional/test_runner.py --jobs=11 --timeout-factor=0 --coverage
1
2.../...
3
4Uncovered RPC commands:
5  - Internal

BrandonOdiwuor commented at 1:17 pm on February 2, 2024: contributor

I have reviewed the issue and it’s evident that the wallet RPCs are indeed absent in the generated rpc_interface.txt file. This observation aligns with the analysis provided by @kouloumos.

The root cause lies in the invocation of start_node(...) within create_cache.py, where the write_all_rpc_commands(...) function is called. This occurs during the creation of the cache on a node that lacks wallet functionality. Consequently, the wallet RPCs are not included in the generated rpc_interface.txt file (I have included my generated rpc_interface.txt file) rpc_interface.txt

BrandonOdiwuor commented at 1:21 pm on February 2, 2024: contributor

@kouloumos what do you mean by?

we can avoid this issue, but just a reordering of cache-creation<>coverage-initialization pushes the issue to the next coverage-initialization which in that case will be the first test of the test-runner. If the node in that first test doesn’t have a wallet, the issue remains.

Doesn’t the coverage-initialization happen inside cache-creation. Do you mean there should be a coverage-initialization (generation of the rpc_interface.txt) before create_cache.py is invoked?

maflcko commented at 1:28 pm on February 2, 2024: member

Why not simply enable the wallet in create_cache?

BrandonOdiwuor commented at 6:20 pm on February 5, 2024: contributor

I have created a draft PR #29387 enabling the wallet in the create_cache.py. The list of RPCs is more comprehensive - check the rpc_interface.txt generated

test: RPC coverage check doesn’t work? #27593