muxers: Add test harness for StreamMuxer implementations#2952
muxers: Add test harness for StreamMuxer implementations#2952thomaseizinger merged 17 commits intomasterfrom
StreamMuxer implementations#2952Conversation
This is now covered by the compliance test suite.
This ensures we cover the same things as the original mplex tests.
mxinden
left a comment
mxinden
left a comment
There was a problem hiding this comment.
Very much in favor of this change. Thanks @thomaseizinger for the excellent design.
| /// Verifies that the dialer of a substream can receive a message. | ||
| pub async fn dialer_can_receive<A, B, S, E>(alice: A, bob: B) | ||
| where | ||
| A: StreamMuxer<Substream = S, Error = E> + Unpin, | ||
| B: StreamMuxer<Substream = S, Error = E> + Unpin, | ||
| S: AsyncRead + AsyncWrite + Send + Unpin + 'static, | ||
| E: fmt::Debug, | ||
| { | ||
| run_commutative( | ||
| alice, | ||
| bob, | ||
| |mut stream| async move { | ||
| let mut buf = Vec::new(); | ||
| stream.read_to_end(&mut buf).await.unwrap(); | ||
|
|
||
| assert_eq!(buf, b"PING"); | ||
| }, | ||
| |mut stream| async move { | ||
| stream.write_all(b"PING").await.unwrap(); | ||
| stream.close().await.unwrap(); | ||
| }, | ||
| ) | ||
| .await; | ||
| } |
There was a problem hiding this comment.
This is the official winner in the competition of most-concise-test-case within the rust-libp2p repository!
There was a problem hiding this comment.
When it comes to tests, every character of noise counts. You don't want to read details that are not important :)
I'd like to get rid of the trait bounds put I don't think it is possible.
| } | ||
|
|
||
| #[async_std::test] | ||
| #[ignore] // Hangs forever, is this a harness bug? It passes if we try to write to the stream. |
There was a problem hiding this comment.
May it be the case that the Rust yamux implementation only announces the stream to the remote once it writes the first byte onto it?
There was a problem hiding this comment.
Could be yeah.
Because of multistream-select, we will always write something but it is still a bit odd.
|
I managed to adapt the mplex implementation so I got the reset test case to pass. Going to work on yamux next. |
thomaseizinger
force-pushed
the
508-muxer-tests
branch
from
5a9f29e to
c3147f8
Compare
We should probably extract that into a separate PR. It might break something and that will make bisecting easier. |
thomaseizinger
force-pushed
the
508-muxer-tests
branch
from
c3147f8 to
c7ae3e7
Compare
|
Given that both our muxers, We can then fix the muxer implementations in a separate PR! |
thomaseizinger
force-pushed
the
508-muxer-tests
branch
from
c7ae3e7 to
ef5464a
Compare
|
Removed the mplex changes as per comment above and chat with @mxinden out-of-band. This is ready for another review :) |
mxinden
left a comment
mxinden
left a comment
There was a problem hiding this comment.
Again, wonderful work! Thanks Thomas!
|
Merging because semver checks are a false-positive error. |
3 participants
Description
This adds a basic test harness for stream muxers. It covers everything that was covered by the old mplex tests but additionally runs them also for yamux.
yamux doesn't pass one of them. I don't understand yet whether it is a harness bug or an actual bug in yamux. In any case, this is a strict improvement over what we currently have so I would like to not block this PR on that. The harness will be used to make progress on the WebRTC implementation. See melekes#9 (comment).
As part of writing this harness, I realised that
StreamMuxerExt::next_{inbound,outbound}were a bad idea because they don't uphold theStreamMuxercontract of callingStreamMuxer::pollso I went ahead and deprecated them.cc @melekes
Links to any relevant issues
Open Questions
Open tasks
StreamMuxerExtfunctions into separate PR: core/muxer: DeprecateStreamMuxerExt::next_{inbound,outbound}#3002write_to_remote_closedtestAdd test for resetting stream on dropChange checklist