test: chain reorg for coinstatsindex

mabu44 commented at 6:04 pm on February 16, 2025: none

Improved coverage of src/index/coinstatsindex.cpp with a new unit test that simulate a reorg of the chain (one block deep).

DrahtBot commented at 6:04 pm on February 16, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31885.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	jonatack

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Tests on Feb 16, 2025

DrahtBot commented at 6:56 pm on February 16, 2025: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/37302278090

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

DrahtBot added the label CI failed on Feb 16, 2025

Unit test for chain reorg in coinstatsindex 9679761a88

mabu44 force-pushed on Feb 16, 2025

DrahtBot removed the label CI failed on Feb 16, 2025

fjahr commented at 9:43 pm on February 16, 2025: contributor

Hm, we have a reorg test for coinstatsindex in the functional tests: https://github.com/bitcoin/bitcoin/blob/master/test/functional/feature_coinstatsindex.py#L258 Can you give additional context why you think this improves coverage or other motivation to add a unit test for this?

davidgumberg commented at 2:06 am on February 18, 2025: contributor

@fjahr It seems to me that there are still parts of CoinStatsIndex that could be tested that might be appropriate for a unit test, for example, in ReverseBlock() that the hash of the block being reversed matches the hash of the latest block and some that could be either in a unit test or a functional test e.g. checking that unspendable totals are reversed, but it seems like the interface for BaseIndex‘es in general kind of enforces integration or functional style testing, because they basically only expose Init(), StartBackgroundSync(), Stop() and reading, which seems well-designed, but difficult to unit test.

This is a more general question, and maybe this PR is not the venue, but if what I’m saying makes sense, because of this limitation should testing of the indexes generally be done in functional tests and we should avoid trying to directly test private interfaces or should some mechanism like e.g. making members protected and creating a test class that inherits from baseindex or coinstatsindex be used to make testing the private interface possible for unit testing?

mabu44 commented at 4:22 pm on February 22, 2025: none

@fjahr I don’t think my new proposed test actually has any advantages over the existing functional one, so I’m converting the PR to draft. I’ll evaluate whether to approach the coverage increase differently.

mabu44 marked this as a draft on Feb 22, 2025

jonatack commented at 4:48 pm on February 22, 2025: member

Didn’t compare coverage or look further, but this unit test runs near instantly on my laptop:

0$ time ./build/src/test/test_bitcoin --run_test=coinstatsindex_tests/coinstatsindex_reorg
1./build/src/test/test_bitcoin   0.14s user 0.03s system 62% cpu 0.265 total

or the whole file

0$ time ./build/src/test/test_bitcoin --run_test=coinstatsindex_tests
1Running 3 test cases...
2
3*** No errors detected
4./build/src/test/test_bitcoin --run_test=coinstatsindex_tests  0.18s user 0.08s system 43% cpu 0.587 total

whereas the running functional test file is a bit slower.

Thanks to their run time, I run the unit tests much more frequently in my local environment than any of the others.

Test size in LOC here isn’t too different from the python code.

If I were working on this code, in most cases I think I would rather be repeatedly running coinstatsindex_tests than feature_coinstatsindex.py.

Concept ACK

fjahr commented at 3:37 pm on March 5, 2025: contributor

We are usually using a combination of functional and unit tests and for user facing features like the index which directly changes how gettxoutsetinfo works and what results it returns, we definitely need functional tests to check that end to end. Where we draw the line and if it would be appropriate to have some duplication between unit and functional tests is a bigger question and I think there should probably an issue to discuss this with a reference to this PR as an example. I think people prefer unit tests for everything that can be done in unit tests due to the speed. If there are parts of indices that are not tested in either definitely add them as unit tests if they are appropriate, which it sounds like it is in you case @davidgumberg . But I guess I would question if there really isn’t a better way to a unit test without exposing more of the interface. Couldn’t you drive the test index test through manipulating the test chain and its validation interface in a unit test? I haven’t taken a look at this in a long time, just an idea of an approach I would look into.

Thanks to their run time, I run the unit tests much more frequently in my local environment than any of the others.

Test size in LOC here isn’t too different from the python code.

If I were working on this code, in most cases I think I would rather be repeatedly running coinstatsindex_tests than feature_coinstatsindex.py.

IMO we need the functional tests for the index for end-to-end testing, so removing the functional test isn’t an option. Which means adding this test here as is would create duplication between unit and functional tests. This comes at the expense of the general runtime of the full test suite though. For example, this would slow down the CI which runs the full test suite in most cases. So while it’s more comfortable for local development, overall I think the downside is greater than the upside.

When it comes to moving things between unit and functional tests, like making the functional tests a bit lighter while adding the same case as a unit test, I am definitely open to that. Would be interesting to look for example of that in the past and how rationale has been laid out there. I would definitely like to see a rationale that lays out how coverage is maintained at the same level after moving test cases from functional to unit tests. But if you can do that I think you can get wide conceptual support for such changes easily because everyone loves a speed up (on all levels) without compromising coverage.

test: chain reorg for coinstatsindex #31885

Code Coverage & Benchmarks

Reviews