swarm_test: support more transports for GenSwarm #3130
Conversation
sukunrt
force-pushed
the
sukun/swarm-test-1
branch
2 times, most recently
from
cc8e5fa to
29f2fa4
Compare
MarcoPolo
left a comment
MarcoPolo
left a comment
There was a problem hiding this comment.
one question, but otherwise looks good.
p2p/net/swarm/testing/testing.go
Outdated
| } | ||
| for _, a := range s.ListenAddresses() { | ||
| if _, err := a.ValueForProtocol(ma.P_QUIC_V1); err == nil { | ||
| quicListenAddr = a |
There was a problem hiding this comment.
It's not obvious to me why you want to share this address with all the other transports. Can you explain? Also what happens if ListenAddresses contains a different address for WebTransport?
There was a problem hiding this comment.
You're right. We should replicate how the basic host works which just listens on port 0.
There was a problem hiding this comment.
The intention was to be maximially similar to how people use this in production. That can be handled separately.
This adds support for `/webtransport` andn `/webrtc-direct` to GenSwarm. Ideally, we should rewrite this to have the same semantics, opt into transports not opt out, as `libp2p.New`. But I need webtransport and webrtc support to write address inference tests for #3075 Depending on how disruptive this is to users, we can decide on whether to merge or drop this.
sukunrt
force-pushed
the
sukun/swarm-test-1
branch
from
6e684b7 to
2c1efe2
Compare
guillaumemichel
left a comment
guillaumemichel
left a comment
There was a problem hiding this comment.
Depending on how disruptive this is to users, we can decide on whether to merge or drop this.
I don't see how it could break existing users to support more transports in tests. Worst case failing tests may flag bugs in webrtc and webtransport.
This adds support for
/webtransportand/webrtc-directto GenSwarm.Ideally, we should rewrite this to have the same semantics, opt into transports not opt out, as
libp2p.New. But I need webtransport and webrtc support to write address inference tests for #3075Depending on how disruptive this is to users, we can decide on whether to merge or drop this.