Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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): Part 1 Test cases covering subscribe and unsubscribe Events #1201
base: master
Are you sure you want to change the base?
test(gossipsub): Part 1 Test cases covering subscribe and unsubscribe Events #1201
Changes from 6 commits
871efab
dc7f8d4
5790b6f
eced002
1c2e221
2923a2d
fda0d2b
eb2f6bf
9c0966e
27c2850
25df50d
f0c8c5b
46b7125
19d3ead
f42a763
e10e4d0
89473da
806592d
2d38e8a
d594c04
a12b56c
cb7ccae
ff6b274
567a456
cc8e976
e68685c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This test is still not checking changes to the mesh.
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.
Mesh test is written separately in PR Part 2 as per designed Test case
PR: https://github.com/vacp2p/nim-libp2p/pull/1205/files
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.
Im not sure this separation is necessary tbh.
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.
merged Part 2 PR in this PR only
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 existing text also checks
gossipSub.mesh[topic].len() > 0
. Is there a place defining what the tests should ensure?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.
Test case are designed here: 6. Topic Membership Tests (❌ To Do)
https://www.notion.so/Gossipsub-651e02d4d7894bb2ac1e4edb55f3192d
These are split tests which check individual logic rather than subscribe and unsubscribe together
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.
Only test title is there and based on that we are adding checks to ensure that functionality. We can have details check test if required
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.
If we move this line before
subscribeToTopics(gossipSub, @[topic])
it's already true, so we aren't testing anything here.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.
what was exactly 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.
Its after subscription and I am testing subscribed value
check gossipSub.gossipsub[topic].len() == 5
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.
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
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.
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
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.
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.
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.
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 areGossipSub
cases.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.
PubSub removed