[Tests] Adding unit tests for GetDifficulty in blockchain.cpp.

[merehap]

blockchain.cpp has low unit test coverage. This commit is intended
to start improving its code coverage to reasonable levels. One or more
follow up commits will complete the task that this commit is starting
(though the usefulness of this commit is not dependent upon later
commits).

Note that these tests were not written based upon a specification of how
GetDifficulty should work, but rather how it actually does work. As
a result, if there are any bugs in the current GetDifficulty
implementation, these unit tests serve to lock them in rather than
expose them.

-- Why has blockchain.cpp been modified if this is a unit testing change?

Since the existing GetDifficulty function relies on a global variable,
chainActive, it was not suitable for unit testing purposes. Both the
existing GetDifficulty function and the unit tests now call through to
a new, more modular version of GetDifficulty that can work on any chain,
not just chainActive.

-- Why does blockchain_tests.cpp directly include blockchain.cpp instead
of blockchain.h?

While the new GetDifficulty function's signature is arguably better than
the old one's, it still isn't great, and doesn't seem to warrant inclusion
as part of the blockchain.h API, especially since only test code is
directly using it. If a better way of exposing the new GetDifficulty
function to unit tests exists, please mention it and the commit will be
updated accordingly.

-- Why is the test fixture named blockchain_difficulty_tests rather than
blockchain_tests?

The Bitcoin Core policy for naming unit test files is to match the the
file under test ("blockchain" becomes "blockchain_tests"). While this
commit complies with that, blockchain.cpp is a massive file, such that
having all of the unit tests in one file will tend towards disorder.
Since there will be a lot more tests added to this file, the intention
is to divide up different types of tests into different test fixtures
within the same file.

[sipa]

blockchain.cpp has low unit test coverage.

Is that statement only about the unit tests as such, or does it include coverage from the RPC functional tests? As rpc/blockchain.cpp is an externally visible RPC interface implementation, I would expect most of its testing to happen through functional tests. Of course, I'm not opposed to improving unit tests too, where possible.

[merehap]

@sipa I've only checked the results of "make cov". I don't know anything about the RPC functional test coverage.

[merehap]

I'm unable to reproduce locally the errors that Travis is seeing with "make -j3 check VERBOSE=1".

It appears that either CChain.SetTip() or GetDifficulty() has undefined behavior in certain cases, such that my tests succeed on certain build targets but fail on others. For my new test get_difficulty_for_null_block_index, GetDifficulty() is returning 0.0 for Travis jobs X.4 and X.6, but returning the correct value of 0.00402 for all of the other Travis Jobs.

I'll keep looking.

[maflcko]

That pointer will most likely point to something unwanted after return in the line below.

[merehap]

Yep, you're right! That was the cause of the non-deterministic test failures. Please forgive my C++ inexperience. Fixed.

[maflcko]

We have two targets for coverage builds: test_bitcoin (just the unit tests) and total (including functional tests). See for example here: https://marcofalke.github.io/btc_cov/

[merehap]

@MarcoFalke It looks like the test coverage with functional tests included is almost 70%, but is still missing some functions entirely. For the time being I'll only be working on unit test coverage though.

[maflcko]

@merehap That is fine. Unit test run way quicker and are probably run more often (compared to the extended functional tests).

[merehap]

@MarcoFalke @sipa As far as I know I've done everything needed for this PR. Is there anything else I need to do to move this along? Or just wait until someone else has time to review and test this?

Thanks! And apologies if I'm being overly impatient.

[laanwj]

utACK 3e1ee31, thanks for adding tests.

[laanwj]

Regarding future direction, I think it's ok to add unit tests for utility functions used by RPC functions, such as this GetDifficulty. It is not directly testable externally.

But for really testing RPC functions, it is better to do so in the functional test framework than in unit tests as this directly tests API usability.