test(gossipsub): Part 2 Test cases covering JOIN and LEAVE Events #1205
Conversation
shashankshampi
requested review from
AlejandroCabeza,
romanzac,
fbarbu15 and
AYAHASSAN287
shashankshampi
force-pushed
the
block6Test2
branch
from
66774dc
to
2923a2d
Compare
Done |
shashankshampi
force-pushed
the
block6Test
branch
from
9230ecd
to
eb2f6bf
Compare
shashankshampi
changed the title
There was a problem hiding this comment.
There's one issue in some of the tests, regarding SUBSCRIBING and UNSUBSCRIBING, that I feel would benefit from some clarification, as it's more complex than it may seem at first.
There's a few key things to understand those operations. For that, let's define node as the local instance and peer as a remote instance:
When node subscribes to a topic it begins listening to the messages that arrive for that topic. In order to do this, node will add that topic to node.topics. That's a PubSub feature.
When a peer connects to node, node will keeps track of peer's subscribed topics in the GossipSub.gossipsub table. If no topics are shared, it will remain as a metadata connection.
When both peer and node are connected and subscribed to the same topic, there will be an attempt to GRAFT (PRUNE being the opposite operation) them, which means converting the connecting to full-message. Esentially, both GossipSub instances will add an entry for the connected peer into mesh. When GossipSub receives a message it will send it to all peers in that topic's mesh, if any.
With all this context:
-
I didn't realise this until now (so my bad 😅) but it'd be great to have tests for both situations:
- Node without connections
- Node connected with some peer/s
-
Some tests are making improper use of
conns.- In some tests it's declared but not used, or even initialised.
- It does make sense to have it and use it (when testing the "connected" cases), but in some of those cases you're not actually initialising the variable, or triggering the connection as you may have expected.
|
|
||
| check gossipSub.mesh[topic].len == 6 | ||
|
|
||
| # Simulate 3 peers leaving the topic by unsubscribing them individually |
There was a problem hiding this comment.
Instead of running the excl method I'd suggest unsubscribing using the higher level unsubscribe, so we use the opposite (and public) methods.
There was a problem hiding this comment.
used unsubscribe now.
Thanks done.
| var peersToUnsubscribe = gossipSub.mesh[topic].toSeq()[0 .. 2] | ||
| for peer in peersToUnsubscribe: | ||
| echo "Unsubscribing peer: ", peer.peerId | ||
| gossipSub.PubSub.unsubscribe(topic, dummyHandler) |
There was a problem hiding this comment.
A couple of importante notes:
I would recommend not using the subscribe system you're using here. The reason being it's misleading, makes you think stuff works differently than it actually does. That behaves more like a mock of sorts, than the actual expected behaviour.
I may have said that too late because I didn't wanna bother you too much, but I've realised it's quite misleading. I suggest you follow the approach I use in my tests. You can check them out here: https://github.com/vacp2p/nim-libp2p/pull/1184/files, for example in messages are not sent back to source or forwarding peer. You might need to check several test cases, as some of them are slightly more complex than others. Checking some of them might be key to get the idea.
I believe you are making some assumptions in how information flows which are not correct.
E.g.: You are calling unsubscribe multiple times on the same node on the same topic on the same unsubscribed-handler, which means nothing's going to be unsubscribed.
I suggest checking the provided docs and/or code and/or requesting a deeper explanation if that's not clear.
Following from 2. Be careful not to conform tests to the results, but the other way around. If results are not as expected you should double check your test does what it intends. If it does, then that's a signal there might be something wrong in the codebase. But absolutely never adapt the assertions just because.
EDIT: Please, apply this suggestions (if and whenever they fit) into the other PR as well). Thanks! :)
Test Plan: https://www.notion.so/Gossipsub-651e02d4d7894bb2ac1e4edb55f3192d
Test Scenario: Block 6
Topic Membership Tests:
Part 2:
JOIN(topic).LEAVE(topic).