test(gossipsub): block5 protobuf test cases #1204
Conversation
|
Could you please write a more descriptive description, like the one on #1201 ? |
tests/pubsub/testgossipinternal.nim
Outdated
| let topic = "dummytopic" | ||
|
|
||
| let msgID = @[0'u8, 1, 2, 3] | ||
| let msg = ControlIHave(topicID: topic, messageIDs: @[msgID]) |
There was a problem hiding this comment.
what are these tests for? they're effectively testing that nim object initialization works, and nothing beyond that since they don't exercise any actual logic such as encoding to and from protobuf. As such, they add maintenance and code volume without providing any value.
There was a problem hiding this comment.
if I add the checks of objects parameters to an already existing test case to reduce code volume would it be acceptable ?
the reason of these checks is to verify that message objects are defined with respect to protobuf schema
There was a problem hiding this comment.
to test schema conformance, you need to create encoded byte streams and decode these messages into the nim types that we use. You can create such messages using https://www.tutorialspoint.com/protobuf/protobuf_command_line_usage.htm as long as it's possible to regenerate the tests in a reasonable way - ie the aim of such a test is to ensure that encoding and decoding of the nim types results in the expected protobuf bytes (and that the test remain maintainable, ie if someone comes across the code in 5 months and wants to add a test, this is reasonably simple)
There was a problem hiding this comment.
One way to structure such tests can be seen in https://github.com/status-im/nim-eth/blob/master/tests/common/test_common.nim - this tests RLP (a different encoding) but premise is the same - there, you can see how example content is loaded and cross-verified against a "decoded" version of the same message.
There was a problem hiding this comment.
can we create the instances, encode, decode, and check the decoded instance is the same as the original instance?
There was a problem hiding this comment.
that's called a roundtrip test, which broadly are less valuable / useful - it tests that the encoder is compatible with the decoder, with the risk that they both share the same bug and therefore remain incompatible with the schema.
When testing conformance to specifications, it's better to work with canonical examples based on the spec that have been generated independently using a reference implementation.
There was a problem hiding this comment.
I have a question then what is the proper max time for the encoding/decoding as I couldn't find it in the specs
|
We follow conventional commits for the PR title. In this case, it will start with |
AYAHASSAN287
changed the title
AYAHASSAN287
changed the title
tests/pubsub/testgossipinternal.nim
Outdated
| @@ -662,6 +662,61 @@ suite "GossipSub internal": | |||
| await allFuturesThrowing(conns.mapIt(it.close())) | |||
| await gossipSub.switch.stop() | |||
|
|
|||
| # test cases for block 5 gossibsub test plan | |||
| #check correctly parsed ihave/iwant messages | |||
There was a problem hiding this comment.
I think this comment need updating:
- small missallignment
- Graft and Prune is also checked in the test
| let id: seq[byte] = @[1] | ||
| let originMessage = RPCMsg( | ||
| control: some( | ||
| ControlMessage( |
There was a problem hiding this comment.
since you used all message types in the encoding, does it make sense to use all of them in the decoding as well?
| ihave: @[ControlIHave(topicID: "foobar", messageIDs: @[id])], | ||
| iwant: @[ControlIWant(messageIDs: @[id])], | ||
| graft: @[ControlGraft(topicID: "foobar")], | ||
| prune: @[ControlPrune(topicID: "foobar", backoff: backofftime)], |
There was a problem hiding this comment.
I know that TC5.9 was not in the original scope of this task but maybe we should add it here as well if it makes sense
There was a problem hiding this comment.
idontwant added in the latest commit
tests/pubsub/testgossipinternal.nim
Outdated
| 111, 98, 97, 114, 16, 10, 42, 5, 10, 3, 49, 50, 51, | ||
| ] #encoded using protoc cmd tool | ||
|
|
||
| let encodeTimeout = Moment.now() + 1.milliseconds |
There was a problem hiding this comment.
tests for functionality that is deterministic should never have timing-based components - this is testing whether the clock on the computer is working and the speed of the cpu, which is irrelevant for the given functionality
I added idontwant to the schema used to generate the expected . |
Note the 2 tests failed and issue #1208 is opened accordingly
Note: PR ready for review
test cases written according to test plan here
Test "Check RPCMsg encoding"
Test "Check RPCMsg decoding"