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.

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

Add muxer compliance test suite and refactor tests to not depend on libp2p-swarm #27

Merged
merged 9 commits into from Nov 11, 2022

Conversation

thomaseizinger
Copy link

Description

The first part is likely to not be controversial.
Migrating away from libp2p-swarm avoids circular dependencies across our workspace. libp2p-quic is a transport and I'd argue that if the implementation only depends on libp2p-core, we should also test it outside of a Swarm context.

For example, this brings up the question on what to do with the wrong_peer_id test. Previously, this was testing functionality that is in libp2p-swarm.

Links to any relevant issues

Open Questions

Change checklist

  • I have performed a self-review of my own code
  • 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

Copy link

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thanks @thomaseizinger.

Great to see the muxer tests used here.

I suggest waiting for @elenaf9's approval.

transports/quic/tests/stream_compliance.rs Show resolved Hide resolved
transports/quic/tests/stream_compliance.rs Show resolved Hide resolved
@kpp
Copy link
Owner

kpp commented Nov 3, 2022

For example, this brings up the question on what to do with the wrong_peer_id test. Previously, this was testing functionality that is in libp2p-swarm.

There was an intention to test TLS handshakes: you can't setup a connection if you dial with one PeerID and you get another in response.

@thomaseizinger
Copy link
Author

For example, this brings up the question on what to do with the wrong_peer_id test. Previously, this was testing functionality that is in libp2p-swarm.

There was an intention to test TLS handshakes: you can't setup a connection if you dial with one PeerID and you get another in response.

Fair enough. But to actually test this on the TLS level, we need to pass in the expected peer ID as part of creating the config:

https://github.com/libp2p/rust-libp2p/blob/f9b4af3d9d5b12b20756e496349b0866baa862da/transports/tls/src/lib.rs#L44

But we currently pass None here:

let client_tls_config = Arc::new(libp2p_tls::make_client_config(keypair, None).unwrap());

The code will have to undergo some refactoring where we don't reuse the same config for all connections so we can pass the expected PeerId in here.

I'd suggest we do this as a follow-up. The current test only validates logic that is present in libp2p-swarm so we are not losing anything but not running this test atm.

Copy link

@elenaf9 elenaf9 left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. Thanks @thomaseizinger!

The concurrent_connections_and_streams_async_std test runs forever when I run it locally. Any idea why that could be the case @thomaseizinger?

transports/quic/tests/smoke.rs Show resolved Hide resolved
transports/quic/tests/smoke.rs Outdated Show resolved Hide resolved
transports/quic/tests/smoke.rs Show resolved Hide resolved
transports/quic/tests/stream_compliance.rs Show resolved Hide resolved
@thomaseizinger thomaseizinger requested review from elenaf9 and kpp and removed request for elenaf9 and kpp November 11, 2022 00:58
@thomaseizinger
Copy link
Author

I don't know why GitHub is stupid and doesn't let me re-request a review from both of you @elenaf9 @kpp. In any case, I'd appreciate another review :)

Copy link

@elenaf9 elenaf9 left a comment

Choose a reason for hiding this comment

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

Thanks @thomaseizinger!

@kpp kpp merged commit eb9bfb6 into kpp:libp2p-quic Nov 11, 2022
@thomaseizinger thomaseizinger deleted the rework-tests branch November 11, 2022 19:50
mergify bot pushed a commit to libp2p/rust-libp2p that referenced this pull request Nov 14, 2022
Various muxer implementations struggle to fulfill this test. In practice, it doesn't matter much because we always run `multistream-select` on top of a newly negotiated stream so we never end up actually reading from a stream that we have never written to.

Relevant discussion: kpp#27 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants