state: add more tests for block validation#3674
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #3674 +/- ##
===========================================
+ Coverage 63.95% 64.12% +0.17%
===========================================
Files 241 242 +1
Lines 19996 19963 -33
===========================================
+ Hits 12788 12801 +13
+ Misses 6162 6122 -40
+ Partials 1046 1040 -6
|
…stripped-down version of it
The required tests already appear to be implemented (implicitly) through the TestValidateBlockHeader test.
Having the tests littered with helper functions makes them less easily readable imho, so I've pulled them out into a separate file. This also makes it easier to see what helper functions are available during testing, so we minimize the chance of duplication when writing new tests.
|
@ebuchman, could you please clarify: /*
TODO(#2589):
- test unmarshalling BlockParts that are too big into a Block that
(note this logic happens in the consensus, not in the validation here).
- test making blocks from the types.MaxXXX functions works/fails as expected
*/
func TestValidateBlockSize(t *testing.T) {
}The first test request's sentence is incomplete, and looks like a note that this test should actually happen in the With regard to the second requested test, it appears as though this is better tested by testing the |
This refactors all of the state package's tests into a state_test package, so as to keep any usage of the state package's internal methods explicit. Any internal methods/constants used by tests are now explicitly exported in state/export_test.go
|
So I've marked this PR as R4R since I think it increases test coverage a little, but now it also refactors the This is a stop-gap measure until such time that we can refactor the tests to remove the dependence on these private methods altogether. If there are additional tests we should add here, let's please put up a specific issue describing exactly what we want to test in detail? |
state/export_test.go
Outdated
| const ValSetCheckpointInterval = valSetCheckpointInterval | ||
|
|
||
| // UpdateState is an alias for updateState exported from execution.go, | ||
| // exclusively and explicitly for testing. TODO: Remove. |
There was a problem hiding this comment.
Is the TODO only related to this function? or all functions in this package?
There was a problem hiding this comment.
All functions in this file need to eventually be refactored away. In our tests, we shouldn't be depending on private methods inside packages.
We could have one big TODO for the whole file instead, but when using certain IDEs the IDE pops up with the description of the method. That'll hopefully discourage people from using them.
There was a problem hiding this comment.
Should we add TODO to every function then?
There was a problem hiding this comment.
There's already a TODO for each function. Are you asking if we should remove the TODOs and rather just have one in the file somewhere? We can certainly do that, and include a note about why dependence on the functions/constants needs to be removed.
state/helpers_test.go
Outdated
| AppHash: nil, | ||
| }) | ||
|
|
||
| // save validators to db for 2 heights |
There was a problem hiding this comment.
I'm not sure actually, I didn't write the original code. @ebuchman - do you perhaps know the reasoning behind this?
There was a problem hiding this comment.
Looking at this a little deeper, I suspect this comment is just an artifact of old code and should probably be removed. It doesn't make sense in context.
There was a problem hiding this comment.
I think it's because SaveState persists the validator sets for heights 1 and 2 when the LastBlockHeight is 0
| (note this logic happens in the consensus, not in the validation here). | ||
| - test making blocks from the types.MaxXXX functions works/fails as expected | ||
| */ | ||
| func TestValidateBlockSize(t *testing.T) { |
There was a problem hiding this comment.
Is this being tracked somewhere else?
* Expose priv validators for use in testing * Generalize block header validation test past height 1 * Remove ineffectual assignment * Remove redundant SaveState call * Reorder comment for clarity * Use the block executor ApplyBlock function instead of implementing a stripped-down version of it * Remove commented-out code * Remove unnecessary test The required tests already appear to be implemented (implicitly) through the TestValidateBlockHeader test. * Allow for catching of specific error types during TestValidateBlockCommit * Make return error testable * Clean up and add TestValidateBlockCommit code * Fix formatting * Extract function to create a new mock test app * Update comment for clarity * Fix comment * Add skeleton code for evidence-related test * Allow for addressing priv val by address * Generalize test beyond a single validator * Generalize TestValidateBlockEvidence past first height * Reorder code to clearly separate tests and utility code * Use a common constant for stop height for testing in state/validation_test.go * Refactor errors to resemble existing conventions * Fix formatting * Extract common helper functions Having the tests littered with helper functions makes them less easily readable imho, so I've pulled them out into a separate file. This also makes it easier to see what helper functions are available during testing, so we minimize the chance of duplication when writing new tests. * Remove unused parameter * Remove unused parameters * Add field keys * Remove unused height constant * Fix typo * Fix incorrect return error * Add field keys * Use separate package for tests This refactors all of the state package's tests into a state_test package, so as to keep any usage of the state package's internal methods explicit. Any internal methods/constants used by tests are now explicitly exported in state/export_test.go * Refactor: extract helper function to make, validate, execute and commit a block * Rename state function to makeState * Remove redundant constant for number of validators * Refactor mock evidence registration into TestMain * Remove extraneous nVals variable * Replace function-level TODOs with file-level TODO and explanation * Remove extraneous comment * Fix linting issues brought up by GolangCI (pulled in from latest merge from develop)
Attempts to address #2589