100% test coverage for validation.go #379
Conversation
…ing test coverage for validateFileContracts
…teV2FileContractsValidateRevisionClosure
|
Fixing the failing Lint tests now. |
|
@Alrighttt once this is ready make sure to assign PJ, Nate and me as reviewers. |
|
This is already a beast to review, so I will continue with providing 100% test coverage in subsequent pull requests. |
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
n8mgr
left a comment
n8mgr
left a comment
There was a problem hiding this comment.
There are two duplicate test cases for "storage proof conflicts" you didn't add them, but might as well remove them.
Also this file is pretty large. I wouldn't move any existing tests, but if you added any new ones see if they might make sense in a new file i,.e. validation_txn_test.go. We should move existing tests in a follow-up so the diff isn't polluted.
peterjan
left a comment
peterjan
left a comment
There was a problem hiding this comment.
Left some minor comments, LGTM otherwise. I must admit though I didn't read through every line here 😳
There was a problem hiding this comment.
This case doesn't make sense. Since we're updating this file anyway we might as well take this along.
There was a problem hiding this comment.
What code is this referencing?
|
I addressed all review comments. |
consensus/validation_test.go
Outdated
| signTxn(cs, &corruptBlock.V2.Transactions[0]) | ||
| if len(corruptBlock.MinerPayouts) > 0 { | ||
| corruptBlock.V2.Commitment = cs.Commitment(corruptBlock.MinerPayouts[0].Address, corruptBlock.Transactions, corruptBlock.V2Transactions()) | ||
| } | ||
| test.corrupt(&corruptBlock) | ||
| findBlockNonce(cs, &corruptBlock) | ||
|
|
||
| if err := ValidateBlock(cs, corruptBlock, db.supplementTipBlock(corruptBlock)); err == nil { |
There was a problem hiding this comment.
All of these now return commitment hash mismatch:
validation_test.go:1638: double spend of non-parent siacoin output: commitment hash mismatch
validation_test.go:1638: double spend of non-parent siafund output: commitment hash mismatch
validation_test.go:1638: revision after proof height: commitment hash mismatch
validation_test.go:1638: storage proof expiration at wrong proof height: commitment hash mismatch
validation_test.go:1638: file contract expiration submitted before expiration height: commitment hash mismatch
validation_test.go:1638: file contract renewal with invalid final revision: commitment hash mismatch
validation_test.go:1638: file contract renewal with invalid initial revision: commitment hash mismatch
validation_test.go:1638: invalid commitment: commitment hash mismatch
Altering the block and not updating the commitment makes all the cases fail with commitment hash mismatch instead of the intended error. Not really the point of the test and somehow doesn't appear to have reduced test coverage.
There was a problem hiding this comment.
Sorry, silly mistake. Thank you for catching that. I updated the test to actually check the errors similar to other tests.
I additionally added another function to modify the block after the commitment has been generated. It's not the cleanest, but it's certainly better than checking against the test name. I don't mind making a standalone test for this single case if this isn't sufficient.
… case; Make each case run as a distinct test and verify that the error message matches
This pull request adds test coverage for any remaining cases within validation.go.
Some tests are non-exhaustive and only add test cases that were previously missing. These are noted with developer comments.