-
Notifications
You must be signed in to change notification settings - Fork 713
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
Add WS ProxyPath option #974
Conversation
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.
Thank you for the contribution. I would make some changes to the test.
ws_test.go
Outdated
@@ -1108,3 +1111,36 @@ func TestWSNoDeadlockOnAuthFailure(t *testing.T) { | |||
|
|||
tm.Stop() | |||
} | |||
|
|||
func TestWSProxyPath(t *testing.T) { | |||
const proxyPath = "/proxy1" |
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.
Instead of commenting line by line, I feel it is easier to show you how I would like the test to look like. The main difference with yours is that we are going to check with a prefix "/" and without, and use a go channel to check that the proxy was invoked. (note that you don't need to run the NATS Server since the connection is actually not happening)
const proxyPath = "proxy1"
// Listen to a random port
l, err := net.Listen("tcp", ":0")
if err != nil {
t.Fatalf("Error in listen: %v", err)
}
defer l.Close()
proxyPort := l.Addr().(*net.TCPAddr).Port
ch := make(chan struct{}, 1)
proxySrv := &http.Server{
Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.URL.Path == "/"+proxyPath {
ch <- struct{}{}
}
}),
}
defer proxySrv.Shutdown(context.Background())
go proxySrv.Serve(l)
for _, test := range []struct {
name string
path string
}{
{"without slash", proxyPath},
{"with slash", "/" + proxyPath},
} {
t.Run(test.name, func(t *testing.T) {
url := fmt.Sprintf("ws://127.0.0.1:%d", proxyPort)
nc, err := Connect(url, ProxyPath(test.path))
if err == nil {
nc.Close()
t.Fatal("Did not expect to connect")
}
select {
case <-ch:
// OK:
case <-time.After(time.Second):
t.Fatal("Proxy was not 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.
Thanks for your great feedback.
Done.
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.
LGTM. Thank you for your contribution!
This is a PR to fix the issue described here: #859