-
Notifications
You must be signed in to change notification settings - Fork 46
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
Test improvements; add server.CloseConnection method #39
Conversation
* Handle errors from client.Send. * Use the higher-level server.ListenAndServe, which makes a connection internally. * Handle the non-problematic case where we get an expected error because we forcibly closed the connection.
See 52238bd for a summary of the test improvements. |
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.
Pretty much LGTM, just a couple of similfications.
@@ -568,6 +572,18 @@ func (s *Server) Serve(c net.PacketConn) error { | |||
} | |||
} | |||
|
|||
// CloseConnection forcibly closes a server's connection. | |||
// | |||
// This causes a "use of closed network connection" error the next time the |
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 have a feeling I found a way around this before but I can't remember it now 🤔
I think it involved using the read timeout value, but I wouldn't worry about working it out for the sake of the tests.
Also, btw, disclaimer, I have absolutely no control when it comes to this repository. |
…et` error osc/osc_test.go:157:54: conversion from int to string yields a string of one rune I found some context here: golang/go#32479
Thanks for your feedback and for helping to move things along more smoothly! I've changed a few things based on your feedback and fixed the CI failure, which appears to be from an interesting recent addition to |
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. Hopefully @hypebeast will be available to take a look sometime soon 🙂
A small part of #37. On @glynternet 's suggestion, I'm turning that big PR into a series of smaller PRs that are easier to review and merge.