chore(webtransport): Add interop-tests support with wasm #4063

zvolin · 2023-06-12T12:30:40Z

Description

Add interop tests for webtransport backend. The test is built as a wasm package and executed in headless chrome instance using webdriver.

Related #4015.

Notes & open questions

~~I've tested this both locally and from within the docker image versus the go-libp2p ping implementation.~~

~~This still needs to be picked up in https://github.com/libp2p/test-plans/blob/master/multidim-interop/versions.ts~~

~~I'm not sure how should I modify the ping-version.json file.~~

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 (this itself is a test)
A changelog entry has been made in the appropriate crates (changelog added in @oblique PR's)

Uses `web-sys::WebTransport` and provides a `Transport` implementation.

thomaseizinger · 2023-06-12T15:16:11Z

I've retargeted this to a copy of the original PR that now lives on a branch in this repository so we can stack the PRs on top of each other.

thomaseizinger

Thank you for this! Very high quality!

I made a few comments. Overall, I think it will be better to stick to one binary and have a conditionally-compiled run_test function.

interop-tests/src/bin/wasm_ping.rs

interop-tests/src/compat.rs

interop-tests/Dockerfile.chromium

interop-tests/src/bin/wasm_ping.rs

interop-tests/Cargo.toml

thomaseizinger · 2023-06-12T15:20:58Z

This still needs to be picked up in libp2p/test-plans@master/multidim-interop/versions.ts

It will be with the next release, nothing to be done here for now.

I'm not sure how should I modify the ping-version.json file.

We will have to make a copy of that and modify our CI to also build the 2nd docker image and run the tests for that. We can simply append more steps to this job here:

rust-libp2p/.github/workflows/interop-test.yml

Lines 19 to 27 in 2910c98

    
           - name: Build image 
        
             run: docker buildx build --load -t rust-libp2p-head . -f interop-tests/Dockerfile 
        
           - uses: libp2p/test-plans/.github/actions/run-interop-ping-test@master 
        
             with: 
        
               test-filter: rust-libp2p-head 
        
               extra-versions: ${{ github.workspace }}/interop-tests/ping-version.json 
        
               s3-cache-bucket: libp2p-by-tf-aws-bootstrap 
        
               s3-access-key-id: ${{ vars.TEST_PLANS_BUILD_CACHE_KEY_ID }} 
        
               s3-secret-access-key: ${{ secrets.TEST_PLANS_BUILD_CACHE_KEY }}

I'd suggest sticking to the same "native" vs "wasm" naming.

thomaseizinger · 2023-06-12T15:27:09Z

Overall, my suggestion would be that we iterate on the tests in here and once we are happy, we merge them into #4065 and then merge the implementation plus test together into master as a single PR.

thomaseizinger · 2023-06-12T15:32:54Z

I am not quite sure why CI isn't running but I think it should run as soon as you push a new commit.

mergify · 2023-06-13T07:53:03Z

This pull request has merge conflicts. Could you please resolve them @zvolin? 🙏

thomaseizinger · 2023-06-13T07:54:18Z

@zvolin Would you mind opening a PR against https://github.com/oblique/rust-libp2p/tree/webtransport-wasm-ext instead? Then we can merge this PR into that branch once we have a passing interop-test.

@oblique Can you please enable GitHub actions on your fork?

thomaseizinger · 2023-06-13T07:56:58Z

Also, I'd prefer if you didn't force-push commits. It doesn't play well with GitHub's review system and we squash-merge anyway in here, thanks :)

interop-tests/src/bin/ping.rs

zvolin · 2023-06-13T10:52:13Z

@zvolin Would you mind opening a PR against https://github.com/oblique/rust-libp2p/tree/webtransport-wasm-ext instead? Then we can merge this PR into that branch once we have a passing interop-test.

As agreed I've opened the PR vs @oblique branch, however I'd like to first resolve all the things there to not cut off the discussion 👍

thomaseizinger · 2023-06-13T11:46:21Z

@zvolin Would you mind opening a PR against https://github.com/oblique/rust-libp2p/tree/webtransport-wasm-ext instead? Then we can merge this PR into that branch once we have a passing interop-test.

As agreed I've opened the PR vs @oblique branch, however I'd like to first resolve all the things there to not cut off the discussion 👍

Sounds good to me, I should have time for another review pass today!

oblique · 2023-06-14T08:54:07Z

@thomaseizinger Do you use self hosted runners for this repo? In my fork we are getting failure on webrtc interop tests and I was wondering if a self hosted runner is the solution to it. Can you run the CI for this PR once?

This is the error:

  Error: Command failed: docker compose -f /tmp/compose-runner/native-rust-libp2p-head-x-rust-v0-51-0--webrtc-direct-/compose.yaml up --exit-code-from=dialer --renew-anon-volumes
   redis Pulling 
   redis Error 
  Error response from daemon: received unexpected HTTP status: 500 Internal Server Error
  
      at ChildProcess.exithandler (node:child_process:419:12)
      at ChildProcess.emit (node:events:513:28)
      at ChildProcess.emit (node:domain:489:12)
      at maybeClose (node:internal/child_process:1091:16)
      at Process.ChildProcess._handle.onexit (node:internal/child_process:302:5) {
    code: 18,
    killed: false,
    signal: null,
    cmd: 'docker compose -f /tmp/compose-runner/native-rust-libp2p-head-x-rust-v0-51-0--webrtc-direct-/compose.yaml up --exit-code-from=dialer --renew-anon-volumes',
    stdout: '',
    stderr: ' redis Pulling \n' +
      ' redis Error \n' +
      'Error response from daemon: received unexpected HTTP status: 500 Internal Server Error\n'
  },

zvolin · 2023-06-14T09:29:26Z

This is the error:

I debugged this case locally and it seems that we have to start listener even if the node is a dialing one. Hidden it behind the cfg so that's not the case for wasm, but I think the latter cfg must stay in this case to not depend on id variable. Will play around this tho to see if we can do any cleaner

thomaseizinger · 2023-06-14T10:03:23Z

@thomaseizinger Do you use self hosted runners for this repo?

We do but they are simply more powerful machines on AWS without any specific configuration.

Can you run the CI for this PR once?

CI doesn't currently run because there are merge conflicts and we use pull_request as the trigger for all workflows.

This is the error:

I debugged this case locally and it seems that we have to start listener even if the node is a dialing one. Hidden it behind the cfg so that's not the case for wasm, but I think the latter cfg must stay in this case to not depend on id variable. Will play around this tho to see if we can do any cleaner

Does my suggestion here help?

Also note that the error is for the WebRTC transport, not the newly added WebTransport so it should be unrelated.

interop-tests/src/lib.rs

interop-tests/src/bin/wasm_ping.rs

interop-tests/src/lib.rs

thomaseizinger · 2023-06-14T10:05:41Z

interop-tests/src/lib.rs

+            tokio::spawn(async move {
+                loop {
+                    swarm.next().await;
+                }
+            });
+            tokio::time::sleep(test_timeout).await;


Additionally, we'd have to move the Swarm::listen_on into the match arm where is_dialer is false.

zvolin · 2023-06-14T10:26:45Z

@thomaseizinger So with 0eeb124 the native interop tests are working.
We are now experiencing no space left on device when this job proceeds to the chromium tests. @oblique is suggesting to either split it to 2 separate jobs or cleanup docker images after the native one finishes. I'm not personally fan of the latter and you suggested to keep those as a single job, so not sure how to tackle this

zvolin · 2023-06-14T10:29:09Z

we could also have a superjob that depends on the 2 jobs if you want to have a single required job

Co-authored-by: Yiannis Marangos yiannis@eiger.co

thomaseizinger · 2023-06-14T10:55:08Z

@thomaseizinger So with 0eeb124 the native interop tests are working. We are now experiencing no space left on device when this job proceeds to the chromium tests. @oblique is suggesting to either split it to 2 separate jobs or cleanup docker images after the native one finishes. I'm not personally fan of the latter and you suggested to keep those as a single job, so not sure how to tackle this

Yeah agreed, happy to go back to two jobs. Probably better anyway because otherwise it is going to be a super long CI job.

zvolin · 2023-06-14T13:41:22Z

seems stuck 🤔

thomaseizinger · 2023-06-14T15:38:02Z

seems stuck thinking

They are not, see https://github.com/libp2p/rust-libp2p/actions/runs/5267580644. But using a matrix changes the name of the job so we will have to adjust that before merging it. GitHub is listing it because it is a required check.

zvolin · 2023-06-14T15:52:13Z

Thanks for the review 👍
I'm closing this in favor of oblique#1 now

thomaseizinger · 2023-06-14T16:48:56Z

I've got two more follow-up commits so I am gonna quickly re-open to push those :)

oblique added 3 commits June 9, 2023 10:53

feat(noise): add WebTransport certhashes extension

649bd1e

chore(wasm-ext): Replace parity-send-wrapper with send-wrapper

b46517d

feat(transport): add WebTransport for web-sys environments

1cc6656

Uses `web-sys::WebTransport` and provides a `Transport` implementation.

This comment was marked as resolved.

Sign in to view

zvolin changed the title ~~chore(webtransport-websys): Add interop-tests support for webtransport in wasm~~ chore(webtransport): Add interop-tests support with wasm Jun 12, 2023

zvolin force-pushed the webtransport-interop-tests branch from 47baa5f to 8c26133 Compare June 12, 2023 12:37

thomaseizinger changed the base branch from master to webtransport-wasm-ext June 12, 2023 15:15

thomaseizinger reviewed Jun 12, 2023

View reviewed changes

zvolin force-pushed the webtransport-interop-tests branch from 8c26133 to 5e1149a Compare June 12, 2023 16:19

chore(webtransport): Add interop-tests support with wasm

05bf0d3

zvolin force-pushed the webtransport-interop-tests branch from 5e1149a to 05bf0d3 Compare June 12, 2023 16:19

thomaseizinger changed the base branch from webtransport-wasm-ext to master June 13, 2023 07:52

thomaseizinger reviewed Jun 13, 2023

View reviewed changes

interop-tests/src/bin/ping.rs Outdated Show resolved Hide resolved

zvolin added 4 commits June 13, 2023 11:40

Separate wasm and native test functions

dc8761f

Unify config reading between binaries

61588db

Wrap index.html in Html response struct

4968801

use Context instead of map_err

814e2a8

thomaseizinger reviewed Jun 13, 2023

View reviewed changes

interop-tests/src/bin/ping.rs Outdated Show resolved Hide resolved

zvolin added 2 commits June 13, 2023 13:11

Embed wasm package instead of reading it runtime

91e9ee2

Rename ping to native_ping

e06b7ee

Unify test implementations

3b98bd4

thomaseizinger and others added 3 commits June 13, 2023 21:15

Make wasm and non-wasm test have the same API

0850c13

Use JsError in run_test_wasm function

9d67199

Pass test results through request instead of DOM

00c1420

Start listener even if the node is a dialer

0eeb124

zvolin force-pushed the webtransport-interop-tests branch from a31c792 to 0eeb124 Compare June 14, 2023 09:27

thomaseizinger reviewed Jun 14, 2023

View reviewed changes

zvolin added 2 commits June 14, 2023 12:49

Properly cleanup chromium process

baa789c

Split interop test jobs to avoid 'no space left on device'

f085285

Co-authored-by: Yiannis Marangos yiannis@eiger.co

zvolin and others added 7 commits June 14, 2023 13:15

Remove TestOutcome in favour of Result

aebbab4

Update the readme

09c8cdf

Add step to superjob in CI to make it run

ddd0b03

Fix chromium ping version json syntax

eb90a12

Remove json autoformatting

e916d69

Deduplicate CI tests using matrix

a8f9837

Merge branch 'master' into webtransport-interop-tests

3c833dd

zvolin closed this Jun 14, 2023

thomaseizinger added 2 commits June 14, 2023 18:45

Remove some more cfgs in the run_test fn

af16956

Shrink dependency tree

d194188

thomaseizinger reopened this Jun 14, 2023

thomaseizinger closed this Jun 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(webtransport): Add interop-tests support with wasm #4063

chore(webtransport): Add interop-tests support with wasm #4063

zvolin commented Jun 12, 2023 •

edited by thomaseizinger

Loading

This comment was marked as resolved.

thomaseizinger commented Jun 12, 2023

thomaseizinger left a comment

thomaseizinger commented Jun 12, 2023 •

edited

Loading

thomaseizinger commented Jun 12, 2023 •

edited

Loading

thomaseizinger commented Jun 12, 2023

mergify bot commented Jun 13, 2023

thomaseizinger commented Jun 13, 2023

thomaseizinger commented Jun 13, 2023

zvolin commented Jun 13, 2023

thomaseizinger commented Jun 13, 2023

oblique commented Jun 14, 2023

zvolin commented Jun 14, 2023

thomaseizinger commented Jun 14, 2023

thomaseizinger Jun 14, 2023

zvolin commented Jun 14, 2023

zvolin commented Jun 14, 2023

thomaseizinger commented Jun 14, 2023

zvolin commented Jun 14, 2023

thomaseizinger commented Jun 14, 2023

zvolin commented Jun 14, 2023

thomaseizinger commented Jun 14, 2023 •

edited

Loading

chore(webtransport): Add interop-tests support with wasm #4063

chore(webtransport): Add interop-tests support with wasm #4063

Conversation

zvolin commented Jun 12, 2023 • edited by thomaseizinger Loading

Description

Notes & open questions

Change checklist

This comment was marked as resolved.

thomaseizinger commented Jun 12, 2023

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger commented Jun 12, 2023 • edited Loading

thomaseizinger commented Jun 12, 2023 • edited Loading

thomaseizinger commented Jun 12, 2023

mergify bot commented Jun 13, 2023

thomaseizinger commented Jun 13, 2023

thomaseizinger commented Jun 13, 2023

zvolin commented Jun 13, 2023

thomaseizinger commented Jun 13, 2023

oblique commented Jun 14, 2023

zvolin commented Jun 14, 2023

thomaseizinger commented Jun 14, 2023

thomaseizinger Jun 14, 2023

Choose a reason for hiding this comment

zvolin commented Jun 14, 2023

zvolin commented Jun 14, 2023

thomaseizinger commented Jun 14, 2023

zvolin commented Jun 14, 2023

thomaseizinger commented Jun 14, 2023

zvolin commented Jun 14, 2023

thomaseizinger commented Jun 14, 2023 • edited Loading

zvolin commented Jun 12, 2023 •

edited by thomaseizinger

Loading

thomaseizinger commented Jun 12, 2023 •

edited

Loading

thomaseizinger commented Jun 12, 2023 •

edited

Loading

thomaseizinger commented Jun 14, 2023 •

edited

Loading