Extra test for rust-libp2p ping protocol #3242
Conversation
github-actions
bot
added
Component: Testing
|
Just adding a comment that we're waiting on the epoll and netlink PRs to be merged before we look at this one. I think one thing we'll want to change is to make the build conditional on the "--extra" flag. This PR adds a number of larger dependencies like tokio which increase shadow's build time, so since these tests only run in the "extra" configuration, we'd want them to only build in the "extra" configuration as well. We briefly discussed some possibilities for how to do this (cargo feature flags, cargo examples, etc) and we hadn't decided what method would be best. But we can consider this more in-depth once the epoll and netlink PRs are closer to being merged. |
|
@stevenengler Since the epoll PR and netlink PR have been merged, do you have a plan for this PR? Side question. Do you think this should be an extra test? Is there any chance this can be a non-extra one? |
Since this is a libp2p compatibility test, I'm leaning towards this being an extra test like the other compatibility tests such as tor, tgen, golang, the examples, etc. And then leaving the default tests to be just system call tests and other small tests. We do run all extra tests in the CI though, so any changes that break your libp2p tests would be caught before merging any future PRs. This PR adds quite a few dependencies to the tests (from 207 to 341 compilation units) and increases the build time of the tests from 65 seconds to 99 seconds.[1] Shadow's build time is already painfully long, so I think it would be best to avoid increasing it by a lot in the default case unless there's a strong argument for it. For users that want to run only the libp2p tests, they should be able to use [1] If #3304 is applied (both to shadow and this PR), the compilation units increase from 150 to 285 and the build time increases from 42 seconds to 71 seconds.
I opened a discussion post about building the extra tests at #3305. |
I merged #3309 which should allow you to make the test in this PR an extra test by:
If you have any questions, let me know. |
| @@ -229,3 +235,4 @@ signal-hook = "0.3.17" | |||
| once_cell = "1.19.0" | |||
| vasi-sync = { path = "../lib/vasi-sync" } | |||
| static_assertions = "1.1.0" | |||
| tokio = { version = "1.34.0", features = ["full"] } | |||
There was a problem hiding this comment.
Is the "full" feature needed, or is there a smaller set of features that could be used instead?
| text="Listening on \"/ip4/1.1.1.2/tcp/9001\"" | ||
| cat ./hosts/peer2/*.stdout | grep "$text" | ||
|
|
||
| expected_ping_count=20 |
There was a problem hiding this comment.
Why is the expected count 20? Can you add a comment?
| async fn main() -> Result<(), Box<dyn Error>> { | ||
| // Usage: ./test_libp2p_ping [listen-port] [peer-multiaddr] | ||
|
|
||
| let mut swarm = libp2p::SwarmBuilder::with_new_identity() |
There was a problem hiding this comment.
For those of us who aren't familiar with libp2p, can you add a few more comments about what the program is doing, and what things like noise::Config::new and yamux::Config::default are used for?
| expected_ping_count=20 | ||
| regex="Event { peer: PeerId(.*), connection: ConnectionId(.*), result: Ok(.*) }" | ||
|
|
||
| actual_ping_count=$(cat ./hosts/peer1/*.stdout | grep "$regex" | wc -l) |
There was a problem hiding this comment.
From the shell lint check:
actual_ping_count=$(cat ./hosts/peer1/*.stdout | grep "$regex" | wc -l)
^-----------^ SC2126 (style): Consider using grep -c instead of grep|wc -l.
(And for the other grep | wc below.)
| loop { | ||
| match swarm.select_next_some().await { | ||
| SwarmEvent::NewListenAddr { address, .. } => println!("Listening on {address:?}"), | ||
| SwarmEvent::Behaviour(event) => println!("{event:?}"), |
There was a problem hiding this comment.
Since the verify.sh script searches for this println and depends on the formatting of it, can you add a match case for SwarmEvent::Behaviour(libp2p::ping::Event(event)) that formats the output in some explicit way. This way if the debug formatting changes for libp2p::ping::Event, it won't cause the regexes to fail.
As discussed in #3201, this PR is the first PR to add extra tests for rust-libp2p. It includes only the simple libp2p ping protocol. More advanced libp2p protocols (such as gossipsub) will come later.
Note that this PR depends on netlink sockets, as implemented in #3198, and linux-specific edge-triggered epoll, as discussed in #2673 and implemented in #3243 (previously in #2675).
So both of them have to be merged first before this test can pass.
Thank you.