-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test(gossipsub): Topic Membership Tests #1201
base: master
Are you sure you want to change the base?
Conversation
how about |
We follow conventional commits for the PR title. In this case, it will start with |
I am adding other tests in the same file related to Topic Membership Tests for Join topic, leave topic, and mesh updated on peer subscribing and leaving topic. Earlier suggestion looks fine but else will modify as per your suggestion |
Thanks. Will follow same in upcoming PRs |
added test wrt subscribe and unsubscribe added tests/pubsub/testgossipinternal2 file linters feat: rendezvous refactor (#1183) Hello! This PR aim to refactor rendezvous code so that it is easier to impl. Waku rdv strategy. The hardcoded min and max TTL were out of range with what we needed and specifying which peers to interact with is also needed since Waku deals with peers on multiple separate shards. I tried to keep the changes to a minimum, specifically I did not change the name of any public procs which result in less than descriptive names in some cases. I also wanted to return results instead of raising exceptions but didn't. Would it be acceptable to do so? Please advise on best practices, thank you. --------- Co-authored-by: Ludovic Chenut <ludovic@status.im> refactor and suite name refactor chore(ci): Enable S3 caching for interop (#1193) - Adds our S3 bucket for caching docker images as Protocol Labs shut down their shared one. - Remove the free disk space workaround that prevented the jobs from failing for using too much space for the images. --------- Co-authored-by: diegomrsantos <diego@status.im> PR review comment changes
9230ecd
to
eb2f6bf
Compare
I'm not sure if gossip membership makes sense. |
…eal instead of simulations
Note: #1205 (review) 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 believe you are making some assumptions in how information flows which are not correct. 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! :) I have updated these tests as per your suggestions to use peer and common handler and subscribe and unsubscribe with multiple peers to topic Test updated:
Methods created:
Common handler: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed review comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All done
await gossipSub.switch.stop() | ||
|
||
# Test for verifying peers joining a topic using `JOIN(topic)` | ||
asyncTest "handle JOIN topic and mesh is updated": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the difference between this case and the handle SUBSCRIBE to the topic
one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first test is primarily about verifying mesh behavior after joining.
The second test is broader and checks both mesh and gossipsub structures to ensure proper subscription handling.
In short, the second test provides a more thorough validation by checking both the mesh and gossipsub structures, while the first test is specifically focused on the mesh when peers join a topic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you might be able to condense the two cases into one.
Are they two different cases in the test plans?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were 6 TC on documentation, and I followed the same.
Let me know if this is not required now will remove it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed all the review comments as per request.
await gossipSub.switch.stop() | ||
|
||
# Test for verifying peers joining a topic using `JOIN(topic)` | ||
asyncTest "handle JOIN topic and mesh is updated": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first test is primarily about verifying mesh behavior after joining.
The second test is broader and checks both mesh and gossipsub structures to ensure proper subscription handling.
In short, the second test provides a more thorough validation by checking both the mesh and gossipsub structures, while the first test is specifically focused on the mesh when peers join a topic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be a good addition to this PR to also follow the Given-When-Then formula used in the rest of the codebase. It documents the case by high-level blocks, making it easier to understand for external eyes.
You can check other PRs and or test files for examples.
Also, here's an example using one of your implemented cases:
asyncTest "handle SUBSCRIBE to the topic":
# Given 5 gossipsub nodes
let
numberOfNodes = 5
topic = "test-topic"
nodes = generateNodes(numberOfNodes, gossip = true)
await allFuturesThrowing(nodes.mapIt(it.switch.start()))
# When all of them are connected and subscribed to the same topic
await subscribeNodes(nodes)
for node in nodes:
node.subscribe(topic, voidTopicHandler)
await sleepAsync(2 * DURATION_TIMEOUT)
# Then their related attributes should reflect that
for node in nodes:
let currentGossip = GossipSub(node)
check currentGossip.topics.contains(topic)
check currentGossip.gossipsub[topic].len() == numberOfNodes - 1
check currentGossip.mesh[topic].len() == numberOfNodes - 1
await allFuturesThrowing(nodes.mapIt(allFutures(it.switch.stop())))
Done. Added documentation as per Given When Then |
|
||
await allFuturesThrowing(nodes.mapIt(it.switch.start())) | ||
|
||
# When nodes subscribe and then unsubscribe to multiple topics |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd remove the second part here. Leave only: When nodes subscribe to multiple topics
, as that's what you're doing here, and later you're handling the unsubscribe
part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
for topic in topicNames: | ||
if gossipSub.topics.len < gossipSub.topicsHigh: | ||
gossipSub.subscribe( | ||
topic, | ||
proc(topic: string, data: seq[byte]): Future[void] {.async.} = | ||
discard | ||
, | ||
) | ||
else: | ||
# Then the subscription count should not exceed the limit | ||
check gossipSub.topics.len == gossipSub.topicsHigh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you don't need to handle the if gossipSub.topics.len < gossipSub.topicsHigh:
. That's precisely the situation that we want to test, and that subscribe
should be handling:
for topic in topicNames: | |
if gossipSub.topics.len < gossipSub.topicsHigh: | |
gossipSub.subscribe( | |
topic, | |
proc(topic: string, data: seq[byte]): Future[void] {.async.} = | |
discard | |
, | |
) | |
else: | |
# Then the subscription count should not exceed the limit | |
check gossipSub.topics.len == gossipSub.topicsHigh | |
for topic in topicNames: | |
gossipSub.subscribe( | |
topic, | |
proc(topic: string, data: seq[byte]): Future[void] {.async.} = | |
discard | |
, | |
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
# Then the subscription count should not exceed the limit | ||
check gossipSub.topics.len == gossipSub.topicsHigh | ||
|
||
check gossipSub.topics.len == gossipSub.topicsHigh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would check the gossipsub.topics.len
against gossipSubParams
, which is the limit you defined here. That would make the check accurate.
Also, maybe it'd be interesting to connect the nodes and check for mesh
and gossipsub
population.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
await allFuturesThrowing(nodes.mapIt(it.switch.start())) | ||
|
||
# When nodes join and subscribe to the topic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't join and subscribe the same thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
||
await sleepAsync(2 * DURATION_TIMEOUT) | ||
|
||
# When they all join the topic, their attributes should reflect this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first half of this should be a bit up, because the connection and subscription is done in the lines above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
check currentGossip.mesh.hasKey(topic) | ||
check currentGossip.topics.contains(topic) | ||
|
||
# When peers subscribe to each other's topics |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The subscription is done above, on the subscribe
. This is just a synchronous check to turn the test deterministic: Make sure all nodes are connected between themselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done all as per request
|
||
await allFuturesThrowing(nodes.mapIt(it.switch.start())) | ||
|
||
# When nodes subscribe and then unsubscribe to multiple topics |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
for topic in topicNames: | ||
if gossipSub.topics.len < gossipSub.topicsHigh: | ||
gossipSub.subscribe( | ||
topic, | ||
proc(topic: string, data: seq[byte]): Future[void] {.async.} = | ||
discard | ||
, | ||
) | ||
else: | ||
# Then the subscription count should not exceed the limit | ||
check gossipSub.topics.len == gossipSub.topicsHigh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
# Then the subscription count should not exceed the limit | ||
check gossipSub.topics.len == gossipSub.topicsHigh | ||
|
||
check gossipSub.topics.len == gossipSub.topicsHigh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
await allFuturesThrowing(nodes.mapIt(it.switch.start())) | ||
|
||
# When nodes join and subscribe to the topic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
||
await sleepAsync(2 * DURATION_TIMEOUT) | ||
|
||
# When they all join the topic, their attributes should reflect this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
check currentGossip.mesh.hasKey(topic) | ||
check currentGossip.topics.contains(topic) | ||
|
||
# When peers subscribe to each other's topics |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
check currentGossip.gossipsub.hasKey(topic) | ||
check currentGossip.topics.contains(topic) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, do these checks as exhaustive as possible :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all 3 validations is already there. Please help with any validation you think is missed out
check currentGossip.gossipsub.hasKey(topic) | ||
check currentGossip.mesh.hasKey(topic) | ||
check currentGossip.topics.contains(topic) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here (re: exhaustiveness).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all three validations are already there. Please help with any validation you think is missed out
Test Plan: https://www.notion.so/Gossipsub-651e02d4d7894bb2ac1e4edb55f3192d
Topic Membership Tests:
Test added: (TC6.6: Verify the handling of subscription changes when the underlying pubsub framework sends SUBSCRIBE and UNSUBSCRIBE events.)
SUBSCRIBE
event and check proper handling in the mesh and gossipsub structuresPart 2:
TC6.1: Verify that peers can correctly join a topic using JOIN(topic).
TC6.2: Ensure that peers can correctly leave a topic using LEAVE(topic).
TC6.5: Multiple peers join and leave topic simultaneously