-
Notifications
You must be signed in to change notification settings - Fork 43
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
Conversation
webrtc/go/manifest.toml
Outdated
|
||
[runners] | ||
"local:docker" = { enabled = true } | ||
"local:exec" = { enabled = true } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
webrtc/go/manifest.toml
Outdated
|
||
[[testcases]] | ||
name = "ping" | ||
instances = { min = 1, max = 10000, default = 1 } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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:
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:
|
@mxinden per your comment, A couple complications:
|
Thanks @John-LittleBearLabs for looking into this!
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 |
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. |
@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. So yeah, let's close this. |
No description provided.