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

merehap commented at 1:32 AM on November 22, 2017: none

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 commented at 1:39 AM on November 22, 2017: member

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 commented at 1:43 AM on November 22, 2017: none

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

fanquake added the label Tests on Nov 22, 2017

merehap force-pushed on Nov 22, 2017

merehap commented at 3:25 AM on November 22, 2017: none

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.

merehap force-pushed on Nov 22, 2017

in src/test/blockchain_tests.cpp:29 in 9992424a12 outdated

  24 | +
  25 | +CChain CreateChainWithNbits(uint32_t nbits)
  26 | +{
  27 | +    CBlockIndex block_index = CreateBlockIndexWithNbits(nbits);
  28 | +    CChain chain;
  29 | +    chain.SetTip(&block_index);

MarcoFalke commented at 4:20 PM on November 22, 2017:

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

merehap commented at 12:09 AM on November 23, 2017:

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

MarcoFalke commented at 4:22 PM on November 22, 2017: member

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/

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

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.

3e1ee31043

merehap force-pushed on Nov 22, 2017

merehap commented at 12:11 AM on November 23, 2017: none

@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.

MarcoFalke commented at 12:21 AM on November 23, 2017: member

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

merehap commented at 9:24 PM on November 29, 2017: none

@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 commented at 10:15 AM on December 23, 2017: member

utACK 3e1ee31, thanks for adding tests.

laanwj commented at 10:18 AM on December 23, 2017: member

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.

laanwj merged this on Dec 23, 2017

laanwj closed this on Dec 23, 2017

laanwj referenced this in commit 20166f8a44 on Dec 23, 2017

laanwj referenced this in commit a589f536b5 on Jun 5, 2018

PastaPastaPasta referenced this in commit c164806451 on Jan 17, 2020

PastaPastaPasta referenced this in commit 8778b38dde on Jan 22, 2020

PastaPastaPasta referenced this in commit a821a6538d on Jan 22, 2020

PastaPastaPasta referenced this in commit adb07a1e29 on Jan 29, 2020

PastaPastaPasta referenced this in commit 71500f6d14 on Jan 29, 2020

PastaPastaPasta referenced this in commit 5581aa6d9d on Jan 29, 2020

PastaPastaPasta referenced this in commit e288330e03 on Jan 31, 2020

PastaPastaPasta referenced this in commit 6cffc0839c on Jun 17, 2020

PastaPastaPasta referenced this in commit 43e187ea3f on Jul 2, 2020

ckti referenced this in commit 349a712451 on Mar 28, 2021

DrahtBot locked this on Sep 8, 2021