test(gossipsub): Part 1 Test cases covering subscribe and unsubscribe Events #1201

shashankshampi · 2024-09-26T06:02:10Z

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.)

Simulate the SUBSCRIBE event and check proper handling in the mesh and gossipsub structures
This test will simulate an UNSUBSCRIBE event and check if the topic is removed from the relevant data structures but remains in gossipsub
This test ensures that multiple topics can be subscribed to and unsubscribed from, with proper initialization of the topic structures.
This test ensures that the number of subscriptions does not exceed the limit set in the GossipSub parameters

Part 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

diegomrsantos · 2024-09-30T09:16:54Z

how about testgossiptopicmembership.nim?

diegomrsantos · 2024-09-30T09:34:51Z

tests/pubsub/testgossipmembership.nim

+  # Addition of Designed Test cases for 6. Topic Membership Tests: https://www.notion.so/Gossipsub-651e02d4d7894bb2ac1e4edb55f3192d
+
+  # Simulate the `SUBSCRIBE` event and check proper handling in the mesh and gossipsub structures
+  asyncTest "handle SUBSCRIBE event":


This test is very similar to

nim-libp2p/tests/pubsub/testgossipinternal.nim

Line 36 in 68306cf

asyncTest "subscribe/unsubscribeAll":

Agree on this but little difference is there

Subscription Handler:

Newly added test (handle SUBSCRIBE event):
Uses an anonymous function when subscribing to the topic, and it does not test for the dynamic dispatch mechanism used to handle topic messages.

Existing test (subscribe/unsubscribeAll):
It defines a handler function for handling messages sent to the subscribed topic. This makes the test more practical for simulating message reception and processing. It also uses dynamic dispatch for testing the subscription and unsubscription.

You can keep the newly added test if you want a lightweight, focused check for basic subscription functionality. If not required then will remove it as it was added keeping in mind for light sanity @AlejandroCabeza

Uses an anonymous function when subscribing to the topic, and it does not test for the dynamic dispatch mechanism used to handle topic messages.

This doesn't need to be tested in this test.

But we could replace the existing test with the first two you created. There's a lot of code repetition in testgossipinternal as pointed out here #1207.

Agree on it. I Will follow this as another task and can move all common lines of code to func to ensure testgossipinternal is clean with no redundant line of code in relevant file as well

Please remove asyncTest "subscribe/unsubscribeAll" in testgossipinternal.

Please remove asyncTest "subscribe/unsubscribeAll" in testgossipinternal.

Removed

tests/pubsub/testgossipmembership.nim

diegomrsantos · 2024-09-30T09:52:51Z

We follow conventional commits for the PR title. In this case, it will start with test(gossipsub):

shashankshampi · 2024-10-01T11:30:03Z

how about testgossiptopicmembership.nim?

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

shashankshampi · 2024-10-01T11:31:03Z

We follow conventional commits for the PR title. In this case, it will start with test(gossipsub):

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

diegomrsantos · 2024-10-02T11:53:13Z

how about testgossiptopicmembership.nim?

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

I'm not sure if gossip membership makes sense.

tests/pubsub/testgossipmembership.nim

diegomrsantos · 2024-10-03T09:47:59Z

tests/pubsub/testgossipmembership.nim

+    check gossipSub.topics.contains(topic)
+
+    # Check if the topic is added to gossipsub and the peers list is not empty
+    check gossipSub.gossipsub[topic].len() > 0


If we move this line before subscribeToTopics(gossipSub, @[topic]) it's already true, so we aren't testing anything here.

what was exactly done?

Its after subscription and I am testing subscribed value
check gossipSub.gossipsub[topic].len() == 5

codecov-commenter · 2024-10-03T09:53:44Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.55%. Comparing base (09fe199) to head (46b7125).
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1201      +/-   ##
==========================================
+ Coverage   84.54%   84.55%   +0.01%     
==========================================
  Files          93       93              
  Lines       16650    16681      +31     
==========================================
+ Hits        14076    14104      +28     
- Misses       2574     2577       +3

see 6 files with indirect coverage changes

fbarbu15

LGTM

AYAHASSAN287 · 2024-10-09T05:36:08Z

tests/pubsub/testgossipmembership.nim

+    check gossipSub.topics.contains(topic)
+
+    # Check if the topic is added to gossipsub and the peers list is not empty
+    check gossipSub.gossipsub[topic].len() > 0


since the numbers of peers is set to 5 so to ensure the length is set correctly we could change to
check gossipSub.gossipsub[topic].len() == 5

AYAHASSAN287 · 2024-10-09T06:27:55Z

tests/pubsub/testgossipmembership.nim

+      gossipSub.topicParams[topic] = TopicParams.init()
+      gossipSub.gossipsub[topic] = initHashSet[PubSubPeer]()
+
+      if gossipSub.topics.len < gossipSub.topicsHigh:


since we want to test the reaction of the logic if we subscribed to more than the limit then we need to try to add more subscribtions than the limit and check the reaction

let topicCount = 15 # Total number of topics to be tested
let gossipSubParams = 10 # Subscription limit for the topics
let topicNames = toSeq(mapIt(0..topicCount - 1, "topic" & $it))

checking subscription beyond the limit and asserting it.

tests/pubsub/testgossipmembership.nim

diegomrsantos · 2024-10-09T18:39:47Z

tests/pubsub/testgossipmembership.nim

+      gossipSub.PubSub.unsubscribeAll(topic)
+
+  # Simulate the `SUBSCRIBE` to the topic and check proper handling in the mesh and gossipsub structures
+  asyncTest "handle SUBSCRIBE to the topic":


This test is still not checking changes to the mesh.

Mesh test is written separately in PR Part 2 as per designed Test case

"handle JOIN topic and mesh is updated"

"handle LEAVE topic and mesh is updated"

PR: https://github.com/vacp2p/nim-libp2p/pull/1205/files

Im not sure this separation is necessary tbh.

merged Part 2 PR in this PR only

diegomrsantos

There are things to be addressed

shashankshampi

Thanks for review.
Have addressed all review as per requirements

tests/pubsub/testgossipmembership.nim

shashankshampi · 2024-10-10T05:02:45Z

tests/pubsub/testgossipmembership.nim

+    check gossipSub.topics.contains(topic)
+
+    # Check if the topic is added to gossipsub and the peers list is not empty
+    check gossipSub.gossipsub[topic].len() > 0


tests/pubsub/testgossipmembership.nim

shashankshampi · 2024-10-10T05:14:40Z

tests/pubsub/testgossipmembership.nim

+      gossipSub.topicParams[topic] = TopicParams.init()
+      gossipSub.gossipsub[topic] = initHashSet[PubSubPeer]()
+
+      if gossipSub.topics.len < gossipSub.topicsHigh:


let topicCount = 15 # Total number of topics to be tested
let gossipSubParams = 10 # Subscription limit for the topics
let topicNames = toSeq(mapIt(0..topicCount - 1, "topic" & $it))

checking subscription beyond the limit and asserting it.

tests/pubsub/testgossipmembership.nim

shashankshampi · 2024-10-10T05:20:23Z

tests/pubsub/testgossipmembership.nim

+    check gossipSub.topics.contains(topic)
+
+    # Check if the topic is added to gossipsub and the peers list is not empty
+    check gossipSub.gossipsub[topic].len() > 0


Its after subscription and I am testing subscribed value
check gossipSub.gossipsub[topic].len() == 5

shashankshampi · 2024-10-10T05:27:05Z

tests/pubsub/testgossipmembership.nim

+      gossipSub.PubSub.unsubscribeAll(topic)
+
+  # Simulate the `SUBSCRIBE` to the topic and check proper handling in the mesh and gossipsub structures
+  asyncTest "handle SUBSCRIBE to the topic":


Mesh test is written separately in PR Part 2 as per designed Test case

"handle JOIN topic and mesh is updated"

"handle LEAVE topic and mesh is updated"

PR: https://github.com/vacp2p/nim-libp2p/pull/1205/files

AlejandroCabeza · 2024-10-10T09:50:40Z

tests/pubsub/testgossipmembership.nim


+    for topic in topicNames:
      if gossipSub.topics.len < gossipSub.topicsHigh:


Now that I think about this, this doesn't make a lot of sense. The limit shouldn't be handled by the test, not manually by us. This is what the test should be asserting, actually: That regardless of the amount of subscriptions done, it shouldn't exceed the limit.

P.S.: Please, remove all the PubSub usages you added as these are GossipSub cases.

PubSub removed

shashankshampi added 3 commits September 26, 2024 11:28

added test wrt subscribe and unsubscribe

871efab

added tests/pubsub/testgossipinternal2 file

dc7f8d4

linters

5790b6f

shashankshampi marked this pull request as ready for review September 26, 2024 07:58

shashankshampi requested review from diegomrsantos, lchenut, AlejandroCabeza and fbarbu15 September 26, 2024 07:59

shashankshampi added 2 commits September 26, 2024 13:33

Merge branch 'master' into block6Test

eced002

refactor and suite name refactor

1c2e221

diegomrsantos reviewed Sep 30, 2024

View reviewed changes

tests/pubsub/testgossipmembership.nim Outdated Show resolved Hide resolved

diegomrsantos mentioned this pull request Sep 30, 2024

test(gossipsub): block5 protobuf test cases #1204

Open

shashankshampi requested review from romanzac and AYAHASSAN287 October 1, 2024 10:20

shashankshampi closed this Oct 1, 2024

shashankshampi reopened this Oct 1, 2024

shashankshampi added 3 commits October 1, 2024 17:18

test(gossipsub): added test for membership for join and leave topic

2923a2d

test(gossipsub): import optimization

fda0d2b

shashankshampi force-pushed the block6Test branch from 9230ecd to eb2f6bf Compare October 1, 2024 11:59

shashankshampi changed the title ~~Part 1: Test cases covering subscribe and unsubscribe Events~~ test(gossipsub): Part 1 Test cases covering subscribe and unsubscribe Events Oct 1, 2024

shashankshampi added 2 commits October 2, 2024 16:25

Merge branch 'master' into block6Test

9c0966e

Merge branch 'master' into block6Test2

27c2850

diegomrsantos reviewed Oct 2, 2024

View reviewed changes

tests/pubsub/testgossipmembership.nim Show resolved Hide resolved

diegomrsantos reviewed Oct 3, 2024

View reviewed changes

tests/pubsub/testgossipmembership.nim Outdated Show resolved Hide resolved

shashankshampi added 2 commits October 3, 2024 15:00

review comments to remove unwanted comments

f0c8c5b

removed internal subscribe and unsubscribe test

46b7125

diegomrsantos reviewed Oct 3, 2024

View reviewed changes

shashankshampi added 5 commits October 3, 2024 16:04

removed unwanted check

19d3ead

added assertion for handle SUBSCRIBE to the topic

f42a763

rebase with block6Test

e10e4d0

TC fix

89473da

PR update

806592d

fbarbu15 approved these changes Oct 7, 2024

View reviewed changes

fix in test logic for multiple peers join and leave topic simultaneously

2d38e8a

AYAHASSAN287 reviewed Oct 9, 2024

View reviewed changes

AlejandroCabeza reviewed Oct 9, 2024

View reviewed changes

diegomrsantos reviewed Oct 9, 2024

View reviewed changes

diegomrsantos requested changes Oct 9, 2024

View reviewed changes

addressed wornderful review comments

d594c04

shashankshampi commented Oct 10, 2024

View reviewed changes

shashankshampi added 2 commits October 10, 2024 11:03

Merge branch 'block6Test' into block6Test2

a12b56c

updated as per review comment

cb7ccae

AlejandroCabeza reviewed Oct 10, 2024

View reviewed changes

shashankshampi added 4 commits October 10, 2024 19:16

review comment fix

ff6b274

Merge branch 'block6Test' into block6Test2

567a456

removed PubSub in sunscribe

cc8e976

added part 2 PR in same

e68685c

shashankshampi requested review from AlejandroCabeza, diegomrsantos and AYAHASSAN287 October 16, 2024 09:58

fbarbu15 marked this pull request as draft October 17, 2024 08:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(gossipsub): Part 1 Test cases covering subscribe and unsubscribe Events #1201

test(gossipsub): Part 1 Test cases covering subscribe and unsubscribe Events #1201

shashankshampi commented Sep 26, 2024 •

edited

Loading

diegomrsantos commented Sep 30, 2024

diegomrsantos Sep 30, 2024

shashankshampi Oct 1, 2024

diegomrsantos Oct 2, 2024

diegomrsantos Oct 2, 2024 •

edited

Loading

shashankshampi Oct 3, 2024

diegomrsantos Oct 3, 2024 •

edited

Loading

shashankshampi Oct 3, 2024

diegomrsantos commented Sep 30, 2024

shashankshampi commented Oct 1, 2024

shashankshampi commented Oct 1, 2024

diegomrsantos commented Oct 2, 2024

diegomrsantos Oct 3, 2024

shashankshampi Oct 3, 2024

diegomrsantos Oct 9, 2024

shashankshampi Oct 10, 2024

codecov-commenter commented Oct 3, 2024

fbarbu15 left a comment

AYAHASSAN287 Oct 9, 2024

shashankshampi Oct 10, 2024

AYAHASSAN287 Oct 9, 2024

shashankshampi Oct 10, 2024

diegomrsantos Oct 9, 2024

shashankshampi Oct 10, 2024

diegomrsantos Oct 10, 2024

shashankshampi Oct 16, 2024

diegomrsantos left a comment

shashankshampi left a comment

shashankshampi Oct 10, 2024

shashankshampi Oct 10, 2024

shashankshampi Oct 10, 2024

shashankshampi Oct 10, 2024

AlejandroCabeza Oct 10, 2024

shashankshampi Oct 10, 2024


		for topic in topicNames:
		if gossipSub.topics.len < gossipSub.topicsHigh:

test(gossipsub): Part 1 Test cases covering subscribe and unsubscribe Events #1201

Are you sure you want to change the base?

test(gossipsub): Part 1 Test cases covering subscribe and unsubscribe Events #1201

Conversation

shashankshampi commented Sep 26, 2024 • edited Loading

diegomrsantos commented Sep 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegomrsantos Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegomrsantos Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegomrsantos commented Sep 30, 2024

shashankshampi commented Oct 1, 2024

shashankshampi commented Oct 1, 2024

diegomrsantos commented Oct 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 3, 2024

Codecov Report

fbarbu15 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegomrsantos left a comment

Choose a reason for hiding this comment

shashankshampi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shashankshampi commented Sep 26, 2024 •

edited

Loading

diegomrsantos Oct 2, 2024 •

edited

Loading

diegomrsantos Oct 3, 2024 •

edited

Loading