New cases coverage:
- GetBlockHash()
- ToString()
- BuildSkip()
- GetAncestor()
Details
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
./src/test/test_bitcoin --log_level=all run_test=blockindex_tests
Suggested reviewers:
- jasonbcox
- deadalnix
Completes T355
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| src/test/blockindex_tests.cpp | ||
|---|---|---|
| 103 ↗ | (On Diff #5441) | Remove spaces around uint8_t . C++ style constructor rather than C style cast. |
| 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 :) |
| src/test/blockindex_tests.cpp | ||
|---|---|---|
| 214 ↗ | (On Diff #5446) | 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 ↗ | (On Diff #5446) | 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 ↗ | (On Diff #5446) | 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 |
| src/test/blockindex_tests.cpp | ||
|---|---|---|
| 214 ↗ | (On Diff #5446) | I'm ok seeing how much of a maintenance hazard this ends up being. If it turns out bad, we can reduce the test. |