Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add GossipParam tests and fix a few small issues #232

Merged
merged 7 commits into from
Mar 7, 2022
Merged

Add GossipParam tests and fix a few small issues #232

merged 7 commits into from
Mar 7, 2022

Conversation

jtraglia
Copy link
Contributor

@jtraglia jtraglia commented Mar 3, 2022

  • For gossipFactor, there was a range mismatch.
  • For graylistThreshold, the message said "gossipThreshold" instead.
  • The others checked >= or <= but the message said > or <.

@jtraglia
Fix some gossip param error messages
d7dd621 
* For gossipFactor, there was a range mismatch.
* For graylistThreshold, the message said "gossipThreshold" instead.
* The others checked >= or <= but the message said > or <.
@jtraglia
Copy link
Contributor Author

jtraglia commented Mar 3, 2022

Before merging this, please look at #233. It is possible that the error messages were correct.

jtraglia and others added 6 commits March 4, 2022 12:08
@jtraglia
Revert graylistThreshold error msg & add tests
c81563f 
Right now I've only added gossip score tests. I will add other tests next;
those should be easier.
@jtraglia
Merge branch 'libp2p:develop' into fix-error-message
f12ef1a
@jtraglia
Add some GossipParams tests
16c28a6 
Also added check to make sure these values are >= 0.

Also modified graylistThreshold error message to better match other messages.
@jtraglia
Add some more GossipParams tests
784e999
@jtraglia
Add GossipPeerScoreParams tests
4eb086f
@jtraglia
Add GossipTopicScoreParams tests
1be39e2
@jtraglia jtraglia changed the title Fix some gossip param error messages Add GossipParam tests and fix a few small issues Mar 4, 2022
@jtraglia
Copy link
Contributor Author

jtraglia commented Mar 4, 2022

Sorry this branch evolved into something else. Changing the name to reflect that.

@jtraglia jtraglia mentioned this pull request Mar 4, 2022
Closed
@jtraglia
Copy link
Contributor Author

jtraglia commented Mar 4, 2022

To be exhaustive, should I add negative checks to other values that should never be negative? For example all of the durations. It feels like it could be too much.

ajsutton
ajsutton approved these changes Mar 7, 2022
View reviewed changes
Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I could see it being reasonable for graylistThreshold, publishThreshold and gossipThreashold to be equal but given the spec says less than that works even if it isn't always entirely consistent about it.

@ajsutton ajsutton merged commit c022027 into libp2p:develop Mar 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajsutton ajsutton ajsutton approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jtraglia @ajsutton