Add test cases for CBlockIndex class
ClosedPublic
Actions

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

Details

Reviewers

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

Fabien created this revision.Oct 18 2018, 17:14

Owners added a reviewer: Restricted Owners Package.Oct 18 2018, 17:14

Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 18 2018, 17:14

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B3615: Diff 5441.Oct 18 2018, 17:25

deadalnix added a subscriber: deadalnix.Oct 18 2018, 22:40

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

Fix as per review

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Harbormaster completed remote builds in B3619: Diff 5446.Oct 19 2018, 13:33

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

schancel added a subscriber: schancel.Oct 19 2018, 21:32

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?

jasonbcox added inline comments.Oct 20 2018, 16:46

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.

Fabien added inline comments.Oct 20 2018, 17:14

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