-
Notifications
You must be signed in to change notification settings - Fork 984
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: introduce libp2p-swarm-test
#2888
Conversation
Returns an event or panics in case nothing happens for X seconds.
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.
I'm in great support of this ❤️
This forces us to get rid of the muxer choice which I am not yet sure about.
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.
Direction looks good to me. Thanks for pushing this forward.
I am guessing that defining |
I think all our non |
I still have to put more thoughts into this, though as an intuition I suggest that a test only tests one thing. E.g. the |
That is correct.
Cool, that makes this simpler :)
Yeah I am tending towards the same. We should have a good muxer test harness instead! |
NB implementations should not make any assumptions about the underlying transport implementations. Defaulting this to `MemoryTransport` allows us to simplify the API a great deal.
We remove the dynamic muxer choice because it is not the responsibility of the ping tests to check that the muxers behave identically.
Not sure where that came from!
That is unfortunate. In that case, I am in favor of providing an |
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.
Nice!
The timeout is implied by the testing lib and this is more consistent with `next_behaviour_event`.
/// | ||
/// Especially (2) is crucial for our usage of this function. | ||
/// If a [`Swarm`] is not polled, nothing within it makes progress. | ||
/// This can "starve" the other swarm which for example may wait for another message to be sent on a connection. |
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.
I spent a good two hours of debugging timeouts in the test and unwoken tasks until it dawned on me why things were not working. @elenaf9 I believe this might also be the reason for your timeouts in #3345.
This function may seem like a beast but after trying various solutions locally, I am convinced it makes sense to add this to the test-harness and use this instead of future::join
for testing Swarm
s.
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.
Just looking at the diff, this is already a great step forward.
A couple of comments. Nothing major. Good to merge from my end.
while res1.len() < NUM_EVENTS_SWARM_1 || res2.len() < NUM_EVENTS_SWARM_2 { | ||
match futures::future::select(swarm1.next_swarm_event(), swarm2.next_swarm_event()).await { | ||
Either::Left((o1, _)) => { | ||
if let Ok(o1) = o1.try_into_output() { | ||
res1.push(o1); | ||
} | ||
} | ||
Either::Right((o2, _)) => { | ||
if let Ok(o2) = o2.try_into_output() { | ||
res2.push(o2); | ||
} | ||
} | ||
} | ||
} |
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.
Long term it would be nice to detect missed wake-ups in a NetworkBehaviour
implementation with libp2p-swarm-test
. If I am not mistaken, that is not possible with the above while
select!
loop as long as the other swarm triggers a wake-up.
Lets defer to a future pull request.
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 is kind of a conflicting requirement. I added this because it is very easy to poll a Swarm
in a wrong way in a test, thus it is often the test that is buggy, not the NetworkBehaviour
implementation.
let peer_id = PeerId::from(identity.public()); | ||
|
||
let transport = MemoryTransport::default() | ||
.or_transport(libp2p_tcp::async_io::Transport::default()) |
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.
Why do we need the TCP transport? Is this needed for libp2p-autonat
?
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.
Yes, and dcutr
needs it too.
let event = loop { | ||
futures::select!( | ||
event = swarm1.select_next_some() => { | ||
if let SwarmEvent::Behaviour(request_response::Event::Message { | ||
peer, | ||
message: request_response::Message::Request { request, channel, .. } | ||
}) = event { | ||
assert_eq!(&request, &ping); | ||
assert_eq!(&peer, &peer2_id); | ||
|
||
drop(channel); | ||
continue; | ||
} | ||
}, | ||
event = swarm2.select_next_some() => { | ||
if let SwarmEvent::Behaviour(ev) = event { | ||
break ev; | ||
} | ||
}, | ||
) | ||
}; |
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.
Can this not be done via the new shiny drive
function?
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.
Probably, I've introduced that one only at the very end!
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.
I am not sure actually, this one is a bit different because the two peers operate autonomously. drive
is useful if you want to "step" through a test in a more linear fashion because it gives you access to the Swarm
s inbetween. Here, we can just create to tasks that operate independently.
Co-authored-by: Max Inden <[email protected]>
Circular dependencies are problematic in several ways: - They result in cognitive overhead for developers, in trying to figure out what depends on what. - They present `cargo` with limits in what order the crates can be compiled in. - They invalidate build caches unnecessarily thus forcing `cargo` to rebuild certain crates. - They cause problems with tooling such as `release-please`. To actually break the circular dependencies, this patch inlines the uses of `development_transport` in the examples and tests for all sub-crates. This is only meant to be a short-term fix until libp2p#3111 and libp2p#2888 are fixed. To ensure we don't accidentally reintroduce this dependency, we add a basic CI that queries `cargo metadata` using `jq`. Resolves libp2p#3053. Fixes libp2p#3223. Related: libp2p#2918 (comment) Related: googleapis/release-please#1662
This patch-set introduces `libp2p-swarm-test`. It provides utilities for quick and safe bootstrapping of tests for `NetworkBehaviour`s. The main design features are: - Everything has timeouts - APIs don't get in your way - Minimal boilerplate Closes libp2p#2884. Pull-Request: libp2p#2888.
Description
This patch-set introduces
libp2p-swarm-test
. It provides utilities for quick and safe bootstrapping of tests forNetworkBehaviour
s. The main design features are:Closes #2884.
Notes
I did not port
libp2p-kad
because the testing logic was too complicated. I also did not add tests for protocols that don't already have some. That is out of scope for this PR.Open Questions
Should this testing crate also pull together utilities likeArbitraryPeerId
?quickcheck-ext
.Can we make theSwarm::new_ephemeral
API any better? At the moment it only allows to configure the underlying transport but the authentication and muxer are hardcoded. I don't want to introduce any more parameters because it will get too verbose and there wouldn't be a benefit in having the function in the first place. Should we perhaps introduce a dedicatedSwarmBuilder
for our tests that start with some reasonable test defaults, like aMemoryTransport
?Trying to port the new harness tolibp2p-ping
, I noticed that we'd lose theMuxerChoice
as part of the prop test. If we go forward with theSwarmBuilder
extension idea, we could make a function there that cycles through various muxers and authentication algorithms to make sure things work across various stacks. I am not sure it is worth it though to be honest.Open tasks
Port to more cratesChange checklist
- [ ] I have made corresponding changes to the documentation- [ ] I have added tests that prove my fix is effective or that my feature works- [ ] A changelog entry has been made in the appropriate crates