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

Adding a webrtc test #49

Closed

Conversation

John-LittleBearLabs
Copy link

No description provided.

@John-LittleBearLabs
Copy link
Author

Doesn't look like I can add reviewers, likely related to the target repo's rules.
@GlenDC @ckousik @ddimaria

I don't think I should switch to "ready" until I can stop pointing at Chinmay's repository (i.e. after webrtc lands in go-libp2p). Does that sound right to you folks?

webrtc/_compositions/go2go.toml Outdated Show resolved Hide resolved
webrtc/_compositions/go2go.toml Outdated Show resolved Hide resolved
webrtc/_compositions/go2go.toml Outdated Show resolved Hide resolved
webrtc/go/.gitignore Outdated Show resolved Hide resolved
webrtc/_compositions/go2go.toml Outdated Show resolved Hide resolved
webrtc/go/.idea/.gitignore Outdated Show resolved Hide resolved
webrtc/go/go.orig.mod Outdated Show resolved Hide resolved
webrtc/go/main.go Outdated Show resolved Hide resolved

[runners]
"local:docker" = { enabled = true }
"local:exec" = { enabled = true }
Copy link
Contributor

Choose a reason for hiding this comment

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

why is local:exec and cluster:k8s enabled?

Choose a reason for hiding this comment

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

Didn't think about it too much, but is there any particular reason this test couldn't be run in those circumstances?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would think that local:exec needs a different builder, the k8s one I do not know


[[testcases]]
name = "ping"
instances = { min = 1, max = 10000, default = 1 }
Copy link
Contributor

Choose a reason for hiding this comment

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

why a max of 10000? How do you ever expect such numbers to be reached?

Choose a reason for hiding this comment

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

The thing I changed here was to lower the default.

I would guess if someone asked for 10000 they're actually expecting the system to break and want to see what happens when it does. Though I don't really know.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm ok. Seems odd, but plausible.

@mxinden
Copy link
Member

mxinden commented Oct 4, 2022

Wonderful to see this happening. Thanks @John-LittleBearLabs for working your way through this stack. Thanks @GlenDC for the review.

Today we only test the TCP transport via the ping tests. Long term we will test:

  • TCP
  • QUIC
  • WebRTC
  • WebTranport
  • ...

With the above in mind, I think it makes sense to think of this pull request not as a one-off, but as a pull request that introduces an abstraction to https://github.com/libp2p/test-plans that enables us to test a variety of transports.

I haven't put enough thoughts into the ideal abstraction, thus I am happy for input. What I have today:

  • Extend the ping test with an additional transport parameter (we have max_latency_ms and iterations today). For now transports could be one of tcp and webrtc.
  • Depending on the transport parameter value, listen on either tcp or webrtc.
  • Extend the composition files to run with both tcp and webrtc. I would appreciate input from @laurentsenta here on how to best do this. Templating? Separate composition files?

@John-LittleBearLabs
Copy link
Author

@mxinden per your comment, ping/_compositions/go-webrtc.toml sets the transport flag to "webrtc" (the default is "tcp").

A couple complications:

  • go.toml - the versions of go that are compared under go-cross-versions.toml - none of these versions have the webrtc transport in them. So I added recent.go-toml with basically just the one version that does (and I think that file should probably be renamed).
  • The "version" that does have webrtc is in Chinmay's fork, which is why I consistently refer to that "version" as "ckousik". And the modfile & composition file still have some wonkiness to deal with that oddity.

@mxinden
Copy link
Member

mxinden commented Oct 11, 2022

Thanks @John-LittleBearLabs for looking into this!

A couple complications:

* go.toml - the versions of go that are compared under go-cross-versions.toml - none of these versions have the webrtc transport in them. So I added recent.go-toml with basically just the one version that does (and I think that file should probably be renamed).

* The "version" that _does_ have webrtc is in Chinmay's fork, which is why I consistently refer to that "version" as "ckousik". And the modfile & composition file still have some wonkiness to deal with that oddity.

This sparked the following proposal #53. We would define the supported transports per version. If I am not mistaken, this should resolve the issues above. Does that sound reasonable? //CC @laurentsenta

@John-LittleBearLabs
Copy link
Author

@mxinden

Does that sound reasonable?

Yes I do see how that approach does solve this. I'll think about this for a bit, and then I think I'll have a question or two but should probably put it in that other PR.

@p-shahi
Copy link
Member

p-shahi commented Nov 8, 2022

@John-LittleBearLabs Should this be closed in favor of #67 ?

@John-LittleBearLabs
Copy link
Author

@p-shahi

@John-LittleBearLabs Should this be closed in favor of #67 ?

This one has the advantage that the last time I tried to run it it worked.

That said, neither of these should get merged in while they still depend on Chinmay's private repo.
Once libp2p/go-libp2p#1655 is resolved & merged & released, I'd have to update one or both of these. Might as well only update the one that's more in line with the future vision.

So yeah, let's close this.

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.

4 participants