Page MenuHomePhabricator

Add test cases for CBlockIndex class
ClosedPublic

Authored by Fabien on Oct 18 2018, 17:14.

Details

Reviewers
jasonbcox
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rSTAGINGcf7e38a4620a: Add test cases for CBlockIndex class
rABCcf7e38a4620a: Add test cases for CBlockIndex class
Summary

New cases coverage:

  • GetBlockHash()
  • ToString()
  • BuildSkip()
  • GetAncestor()
Test Plan

./src/test/test_bitcoin --log_level=all run_test=blockindex_tests

Suggested reviewers:

  • jasonbcox
  • deadalnix

Completes T355

Diff Detail

Repository
rABC Bitcoin ABC
Branch
blockindex_tests
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 3619
Build 5313: Bitcoin ABC Buildbot (legacy)
Build 5312: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.Oct 18 2018, 17:14
deadalnix added inline comments.
src/test/blockindex_tests.cpp
103 ↗(On Diff #5441)

Remove spaces around uint8_t . C++ style constructor rather than C style cast.

jasonbcox requested changes to this revision.Oct 19 2018, 06:07
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/test/blockindex_tests.cpp
90 ↗(On Diff #5441)

Though not strictly necessary, it's generally recommended to break up your changes into separate diffs. As an example, this diff can be broken into 4 different diffs so each test can be closely reviewed. Oftentimes, one test looks fine, while another needs work. There's no reason to delay landing the good test for the sake of the bad one.

94 ↗(On Diff #5441)

const

103 ↗(On Diff #5441)

s/spaces/parentheses I think he means. like so: uint8_t(...) rather than (uint8_t)(...)

105 ↗(On Diff #5441)

const

319 ↗(On Diff #5441)

Very important set of tests. Thank you :)

This revision now requires changes to proceed.Oct 19 2018, 06:07
Fabien marked an inline comment as done.

Fix as per review

schancel added inline comments.
src/test/blockindex_tests.cpp
214

I would really prefer we did not test these types of things. If this isn't part of an API, we shouldn't test it. Is there a reason to ensure this never changes?

src/test/blockindex_tests.cpp
214

I think that makes sense because these sorts of tests increase overhead of certain types of changes. That said, I think one simple test that sets a bunch of CBlockIndex members and just checks the expected ToString() output is correct is valuable to prevent silly mistakes. I think testing each field individually is overkill.

src/test/blockindex_tests.cpp
214

I agree this might not be the most useful test. However the ToString() method can be called as a consequence of some external calls which trigger a ReadBlockFromDisk(). I think it makes sense to test it, not only to ensure that the result never changes, but at least to ensure that there is no special case that is not handled and could cause the node to crash.

90 ↗(On Diff #5441)

OK, I'll make sure to keep the next diffs more compact

jasonbcox added inline comments.
src/test/blockindex_tests.cpp
214

I'm ok seeing how much of a maintenance hazard this ends up being. If it turns out bad, we can reduce the test.

This revision is now accepted and ready to land.Oct 23 2018, 19:07
This revision was automatically updated to reflect the committed changes.