-
Notifications
You must be signed in to change notification settings - Fork 1
add kademlia tests #41
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
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
nfnt
left a 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.
This looks good overall, but let's document why we need sleep in the testing code so that we can remove it in the future.
Signed-off-by: Orlando Hohmeier <[email protected]>
ede9f6c to
b37be08
Compare
nfnt
left a 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.
LGTM!
| @@ -0,0 +1,674 @@ | |||
| use std::{collections::HashMap, time::Duration}; | |||
|
|
|||
| use futures::StreamExt; | |||
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.
Nit, but a complicated one because of re-exports in multiple packages: IMO, we don't need the futures package at all, because we can find all of it either in futures_util or in tokio. futures re-exports these types from futures_util, the only difference is that it includes an async runtime similar to tokio but which we don't use. I.e. by using futures_util::stream::StreamExt instead and consistently, we can remove a dependency on futures.
tokio also provides its own version of StreamExt, but that ones different from the one in futures_util, missing a lot features.
Right now our code is using all of these in different places and IMO, we should at least remove futures in favor of futures_util, maybe not here but in a separate commit.
Add Kademlia (integration) tests, improving the test coverage from 17.95 % (Lines), 35.46 % (Functions) to 82.37 % (Lines), 51.77 % (Functions). Some of the error cases aren't well covered yet.
In case you've never check coverage run the test with the
instrument-coverageflag:And then generate a report using grcov as follows:
This will give you a nice html report in
./htmlthat you can open in your browser to understand the coverage.This is the first PR in a series to improve test coverage pulled from #38