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

[Merged by Bors] - Test the pruning of excess peers using randomly generated input #3248

Closed
wants to merge 12 commits into from
Closed

[Merged by Bors] - Test the pruning of excess peers using randomly generated input #3248

wants to merge 12 commits into from

Conversation

ackintosh
Copy link
Member

@ackintosh ackintosh commented Jun 8, 2022 •
edited
Loading

Issue Addressed

#3092

Proposed Changes

Added property-based tests for the pruning implementation. A randomly generated input for the test contains connection direction, subnets, and scores.

Additional Info

I left some comments on this PR, what I have tried, and a question.

@ackintosh ackintosh marked this pull request as ready for review June 8, 2022 07:04
@divagant-martian divagant-martian self-requested a review June 10, 2022 15:21
@divagant-martian divagant-martian changed the title Test the pruning using randomly generated input Test the pruning of excess peers using randomly generated input Jun 12, 2022
@divagant-martian
Copy link
Collaborator

divagant-martian commented Jun 13, 2022 •
edited
Loading

@michaelsproul could you give this a review? It seems to me we don't need to do this much work to produce the rand values. I'm thinking we could derive/implement quickcheck::Arbitrary on the BitField types and expose this under a flag as a step that would reduce most of the work done here, or maybe just depend on quickcheck generating random boolean vectors for us.

After you've reviewed this I can continue on the logic level review

@divagant-martian divagant-martian requested review from michaelsproul and removed request for divagant-martian June 13, 2022 18:37
@divagant-martian
Copy link
Collaborator

@ackintosh I think we can go with quickcheck 0.9.2 since it's what we are using in the rest of lighthouse, as to keep only one dep version

@ackintosh
Copy link
Member Author

@divagant-martian @AgeManning Thank you for your review!

@michaelsproul Could you have a look at this, please? 🙏

divagant-martian
divagant-martian reviewed Jun 21, 2022
View reviewed changes
Copy link
Collaborator

@divagant-martian divagant-martian left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This looks like a nice improvement to our tests! I added minor suggestions to the code, otherwise it's a 👍 from me. You've simplified the quickcheck use so I think Michael's review is no longer necessary. For the failing test (cargo audit) could you merge unstable?

Thanks!

beacon_node/lighthouse_network/src/peer_manager/mod.rs Outdated Show resolved Hide resolved
beacon_node/lighthouse_network/src/peer_manager/mod.rs Outdated Show resolved Hide resolved
beacon_node/lighthouse_network/src/peer_manager/mod.rs Outdated Show resolved Hide resolved
@divagant-martian divagant-martian removed the request for review from michaelsproul June 21, 2022 19:50
pawanjay176
pawanjay176 approved these changes Jun 24, 2022
View reviewed changes
Copy link
Member

@pawanjay176 pawanjay176 left a comment

Choose a reason for hiding this comment

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

LGTM.

@divagant-martian
Copy link
Collaborator

bors r+

bors bot pushed a commit that referenced this pull request Jun 24, 2022
@ackintosh @divagant-martian
Test the pruning of excess peers using randomly generated input (#3248)
37ce078 
## Issue Addressed

#3092


## Proposed Changes

Added property-based tests for the pruning implementation. A randomly generated input for the test contains connection direction, subnets, and scores.


## Additional Info

I left some comments on this PR, what I have tried, and [a question](#3248 (comment)).

Co-authored-by: Diva M <[email protected]>
bors bot pushed a commit that referenced this pull request Jun 24, 2022
@ackintosh @divagant-martian
Test the pruning of excess peers using randomly generated input (#3248)
5c1f4bf 
## Issue Addressed

#3092


## Proposed Changes

Added property-based tests for the pruning implementation. A randomly generated input for the test contains connection direction, subnets, and scores.


## Additional Info

I left some comments on this PR, what I have tried, and [a question](#3248 (comment)).

Co-authored-by: Diva M <[email protected]>
@bors
Copy link

bors bot commented Jun 24, 2022

Build failed:

@sigp sigp deleted a comment from bors bot Jun 24, 2022
@divagant-martian
Copy link
Collaborator

bors retry

bors bot pushed a commit that referenced this pull request Jun 25, 2022
@ackintosh @divagant-martian
Test the pruning of excess peers using randomly generated input (#3248)
082ed35 
## Issue Addressed

#3092


## Proposed Changes

Added property-based tests for the pruning implementation. A randomly generated input for the test contains connection direction, subnets, and scores.


## Additional Info

I left some comments on this PR, what I have tried, and [a question](#3248 (comment)).

Co-authored-by: Diva M <[email protected]>
@bors
Copy link

bors bot commented Jun 26, 2022

Pull request successfully merged into unstable.

Build succeeded:

@bors bors bot changed the title Test the pruning of excess peers using randomly generated input [Merged by Bors] - Test the pruning of excess peers using randomly generated input Jun 26, 2022
@bors bors bot closed this Jun 26, 2022
@ackintosh ackintosh deleted the quickcheck branch June 26, 2022 11:53
@divagant-martian divagant-martian mentioned this pull request Oct 20, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@divagant-martian divagant-martian divagant-martian left review comments

@AgeManning AgeManning AgeManning left review comments

@pawanjay176 pawanjay176 pawanjay176 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.
4 participants
@ackintosh @divagant-martian @AgeManning @pawanjay176