QBFT Tests - Instance & Message #328
Conversation
| "github.com/bloxapp/ssv-spec/qbft/spectest/tests" | ||
| "github.com/bloxapp/ssv-spec/types" | ||
| "github.com/bloxapp/ssv-spec/types/testingutils" | ||
| ) | ||
|
|
||
| // InvalidPrepareJustificationsUnmarshalling tests unmarshalling invalid prepare justifications (during message.validate()) | ||
| func InvalidPrepareJustificationsUnmarshalling() tests.SpecTest { |
There was a problem hiding this comment.
@alonmuroch
It seems this is mainly testing ssz logic that is the repsonsibility of a 3rd party lib.
We can have this basic test but just wondering what is your view of testing such parts of the code?
There was a problem hiding this comment.
Answering myself since Alon won't answer:
- for valid messages we need to make sure the encoding is consistent if the ssz lib changes
- invalid encodings in my eyes are optional to test. Since even if malformed object is created later validations should help here
qbft/spectest/tests/messages/prepare_justification_unmarshaling.go
Outdated
Show resolved
Hide resolved
qbft/spectest/tests/messages/round_change_justification_unmarshaling.go
Outdated
Show resolved
Hide resolved
| "github.com/bloxapp/ssv-spec/qbft/spectest/tests" | ||
| "github.com/bloxapp/ssv-spec/types" | ||
| "github.com/bloxapp/ssv-spec/types/testingutils" | ||
| ) | ||
|
|
||
| // InvalidPrepareJustificationsUnmarshalling tests unmarshalling invalid prepare justifications (during message.validate()) | ||
| func InvalidPrepareJustificationsUnmarshalling() tests.SpecTest { |
There was a problem hiding this comment.
Answering myself since Alon won't answer:
- for valid messages we need to make sure the encoding is consistent if the ssz lib changes
- invalid encodings in my eyes are optional to test. Since even if malformed object is created later validations should help here
|
|
||
| return &tests.MsgSpecTest{ |
There was a problem hiding this comment.
Maybe the issue here is that the test code is wrong... we should be doing BaseMsgValidation() and not msg.Validate() that is partial
There was a problem hiding this comment.
You mean the instance BaseMsgValidation or the controller one?
In the test structure we don't have any of these objects btw. Should I add it?
My only concern is that the other tests will probably need to change.
Another way would be to move this single test to ssv. What do you think?
There was a problem hiding this comment.
Maybe we open an issue and just remember to fix this when we do spec rethink
There was a problem hiding this comment.
So we just need @alonmuroch to explain us the test and we're done
Tests
Messages
message.Validate()doesn't testrootfield. Should this test be dropped?message.Validate()doesn't testrootfield. Should this test be dropped?Start Instance