Add webrtc transport multidim interop by jxs · Pull Request #100 · libp2p/test-plans

jxs · 2023-01-12T20:20:04Z

No description provided.

mxinden · 2023-01-13T11:12:28Z

🚀 let me know once this is ready for a review.

jxs · 2023-01-16T00:08:20Z

@mxinden thanks, ready! @MarcoPolo the tests are failing because of the "js-v0.41.0 x js-v0.41.0 (ws, noise, yamux)" which this PR doesn't affect afaict do you know what might it be?

MarcoPolo · 2023-01-16T01:41:45Z

Can you rebase please. The ping succeeded, but the test failed. I think it’s because it failed when closing the Redis client (which is fine).

mxinden

Minor comments. Good to merge from my end.

mxinden · 2023-01-16T12:46:18Z

multidim-interop/rust/src/bin/testplan_0500.rs

+    let timeout = Duration::from_secs(5);
+
+    match secure_channel_param {
+        "noise" => builder


As an aside, I suggest using enums here long-term instead of passing around strings. clap should make this reasonably easy. Neither important nor urgent.

yup, awesome idea! Updated ptal Max

mxinden · 2023-01-16T12:51:24Z

multidim-interop/src/generator.ts

+        await db.all(`SELECT DISTINCT a.id as id1, b.id as id2, a.transport
+                     FROM transports a, transports b
+                     WHERE a.transport == b.transport
+                     -- Only webtransport transports


Suggested change

-- Only webtransport transports

-- Only webrtc transports

Thanks! 🙏

multidim-interop/versions.ts

jxs · 2023-01-16T15:46:02Z

CC @thomaseizinger you also probably want to take a look, as this builds on your work on the previous interop tests

to allow us add the WebRTC transport test (which is only available with the tokio runtime) Clean up the test and update Cargo.toml to 0.2.0 to avoid conflicting with the older tests, and allow us to import lib.rs on the rust-libp2p repo and have the master tests there.

with KeepAlive Behaviour

webrtc instead of webtransport.

multidim-interop/rust/src/lib.rs

jxs · 2023-01-16T22:03:10Z

multidim-interop/src/generator.ts

+            listenerID: test.id2,
+            transport: test.transport,
+            muxer: "quic",
+            security: "tls",


@MarcoPolo what do you think of this change? It doesn't break Go quic tests, and allows Rust to have one less Variant in the enum. Plus I'd say it makes sense semantically.

I don’t think it makes sense. In every other context tls means add a tls encryption layer on top of the transport.

I know quic uses tls underneath and that isn’t a hardcoded decision (some different encryption layer could be possible in the future). But for now I think we should keep tls meaning add a tls security layer to the transport.

yeah you are right Marco, I reverted it int this PR so we can merge and address the rust-libp2p interop. But I also agree with @thomaseizinger on #100 (comment) if you agree we can submit a PR not using muxer nor transport for quic and webrtc cases

thomaseizinger

Some thoughts, otherwise LGTM.

thomaseizinger · 2023-01-17T04:52:11Z

multidim-interop/rust/src/bin/testplan_0500.rs

+    let transport_param: testplan::Transport = env::var("transport")
+        .context("transport environment variable is not set")?
+        .parse()
+        .context("unsupported transport")?;
+
+    let secure_channel_param: testplan::SecProtocol = env::var("security")
+        .context("security environment variable is not set")?
+        .parse()
+        .context("unsupported secure channel")?;
+
+    let muxer_param: Muxer = env::var("muxer")
+        .context("muxer environment variable is not set")?
+        .parse()
+        .context("unsupported muxer")?;


Until we move the test to our repository (where we deduplicate the test using Git revisions), I don't think it makes much sense to be typed here because we have to repeat it in every test.

it's now all on the testplans lib, which we can depend on the rust-libp2p repo and use from there wdyt?
update: wrote this before reading libp2p/rust-libp2p#3331 (comment) let's continue there.

thomaseizinger · 2023-01-17T04:54:20Z

multidim-interop/src/generator.ts

-        muxer: "quic",
-        security: "quic",


This is a bit odd, I think leaving these blank would make the most sense. It requires the tests to parse the variables in steps but I don't see why that is necessarily a problem.

Yeah agree Thomas, commented on #100 (comment)

I think this makes sense to me. We can make this change in a future PR so we do it across all impls

thomaseizinger · 2023-01-17T04:55:34Z

multidim-interop/rust/src/bin/testplan_0500.rs

-use testplan::{run_ping_redis, PingSwarm};
+use testplan::{run_ping, Muxer, PingSwarm, SecProtocol, Transport};

 fn build_builder<T, C>(


This function doesn't have much value because the type constraints are so insane. You are likely better off by just inlining things and accepting the duplication between the different invocations.

I still feel it's more lines of code and repetition, but feel free to submit a PR addressing that after Thomas

thomaseizinger · 2023-01-17T04:57:55Z

multidim-interop/rust/src/lib.rs

+    Quic,
+    Webrtc,


These aren't strictly muxers. I think we should leave the muxer variable empty when we don't explicitly set one. IMO, that will be cleaner implementation.

yeah makes sense, thanks Thomas addressed :)

- undo generator changes, let's do that later on - don't parse muxer and sec protocol where we don't need it

mxinden

Changes here look good to me. Ready to merge from my end.

@MarcoPolo any objections?

MarcoPolo · 2023-01-17T18:10:10Z

multidim-interop/src/generator.ts

+            security: "quic",
+        })))
+        .concat(webrtcQueryResults
+        .map((test): ComposeSpecification => buildSpec(containerImages, {


Can we indent this line? The map applies to webrtcQueryResults not the concat(...) expression like above.

yup makes sense Marco, updated :)

mxinden · 2023-01-18T10:54:57Z

@jxs two approvals, all checks are green. Please merge once this is ready from your end.

- Added in libp2p#2622 - Interoperability tests added in libp2p/test-plans#100

- Added in #2622 - Interoperability tests added in libp2p/test-plans#100

* update interop rust test lib to use tokio to allow us add the WebRTC transport test (which is only available with the tokio runtime) Clean up the test and update Cargo.toml to 0.2.0 to avoid conflicting with the older tests, and allow us to import lib.rs on the rust-libp2p repo and have the master tests there. * simplify build_builder function * replace deprecated ping keepalive with KeepAlive Behaviour * add WebRTC support for rust tests * add webrtc support on interop generator. * fix typo on generators.ts webrtc instead of webtransport. * update rust test plans to use enums instead. * update rust tests enum to match the ones by generator.ts * fix webrtc testcases to have proper security and muxer. * review: address suggestions: - undo generator changes, let's do that later on - don't parse muxer and sec protocol where we don't need it * ident generator.ts.

jxs marked this pull request as draft January 12, 2023 20:20

jxs force-pushed the add-webrtc-transport-multidim-interop branch from 63efd23 to eb1ab42 Compare January 12, 2023 20:21

p-shahi mentioned this pull request Jan 13, 2023

[tracking issue] Interop test-plans for all existing/developing libp2p transports #61

Open

28 tasks

jxs force-pushed the add-webrtc-transport-multidim-interop branch 4 times, most recently from e26ca25 to ffdaea7 Compare January 15, 2023 23:53

jxs marked this pull request as ready for review January 16, 2023 00:06

jxs force-pushed the add-webrtc-transport-multidim-interop branch 4 times, most recently from c0a5ace to 03721b7 Compare January 16, 2023 11:18

mxinden approved these changes Jan 16, 2023

View reviewed changes

jxs force-pushed the add-webrtc-transport-multidim-interop branch from 6ffa98e to f999445 Compare January 16, 2023 16:08

jxs added 6 commits January 16, 2023 16:19

simplify build_builder function

246a452

replace deprecated ping keepalive

cdd8fad

with KeepAlive Behaviour

add WebRTC support for rust tests

811a4d0

add webrtc support on interop generator.

4a3e9f5

fix typo on generators.ts

4012c07

webrtc instead of webtransport.

jxs force-pushed the add-webrtc-transport-multidim-interop branch 3 times, most recently from fe8e932 to b251928 Compare January 16, 2023 18:03

update rust test plans to use enums instead.

396c327

jxs force-pushed the add-webrtc-transport-multidim-interop branch from b251928 to 396c327 Compare January 16, 2023 18:18

mxinden reviewed Jan 16, 2023

View reviewed changes

multidim-interop/rust/src/lib.rs Show resolved Hide resolved

jxs added 2 commits January 16, 2023 21:42

update rust tests enum to match the ones by generator.ts

9a13371

fix webrtc testcases to have proper security and muxer.

fa3f360

jxs commented Jan 16, 2023

View reviewed changes

thomaseizinger reviewed Jan 17, 2023

View reviewed changes

review: address suggestions:

e18a135

- undo generator changes, let's do that later on - don't parse muxer and sec protocol where we don't need it

mxinden approved these changes Jan 17, 2023

View reviewed changes

MarcoPolo mentioned this pull request Jan 17, 2023

multidim-interop: Drop muxer/securechannel env params when using quic/webtransport transport #106

Open

MarcoPolo approved these changes Jan 17, 2023

View reviewed changes

ident generator.ts.

ca2fa7b

jxs merged commit b0f3a2d into libp2p:master Jan 18, 2023

mxinden added a commit to mxinden/rust-libp2p that referenced this pull request Jan 20, 2023

docs(roadmap): Mark WebRTC browser-to-server as done

d09c427

- Added in libp2p#2622 - Interoperability tests added in libp2p/test-plans#100

mxinden mentioned this pull request Jan 20, 2023

docs(roadmap): Mark WebRTC browser-to-server as done libp2p/rust-libp2p#3357

Merged

mergify bot pushed a commit to libp2p/rust-libp2p that referenced this pull request Jan 24, 2023

docs(roadmap): Mark WebRTC browser-to-server as done (#3357)

d736132

- Added in #2622 - Interoperability tests added in libp2p/test-plans#100

Conversation

jxs commented Jan 12, 2023

Uh oh!

mxinden commented Jan 13, 2023

Uh oh!

jxs commented Jan 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MarcoPolo commented Jan 16, 2023

Uh oh!

mxinden left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jxs commented Jan 16, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jxs Jan 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jxs Jan 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mxinden left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mxinden commented Jan 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jxs commented Jan 16, 2023 •

edited

Loading

jxs Jan 17, 2023 •

edited

Loading

jxs Jan 17, 2023 •

edited

Loading