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

x/net/nettest: extend TestConn with optional interface checks #31033

Open
mdlayher opened this issue Mar 25, 2019 · 3 comments

Comments

@mdlayher
Copy link
Member

commented Mar 25, 2019

EDIT: we'll proceed with Option 1.


For the same project as discussed in #30984, I'm using nettest.TestConn to test a custom net.Conn implementation.

Now that I've got the basics covered, I realize it'd be useful to be able to extend the tests to make sure that my CloseRead/CloseWrite methods that are not a mandatory part of net.Conn could also be tested on my type.

I see two options here:

  1. Allow TestConn to perform a couple of type assertion tests to see if methods such as CloseRead and CloseWrite are implemented on the net.Conn type, and then run additional tests if so.

This has the advantage that anyone who consumes this package and passes a net.Conn with these methods will have these tests run. net.TCPConn, net.UnixConn, and my custom vsock.Conn type (as an example) would run these added tests, but net.UDPConn would not.

Perhaps it'd also make a sense to have an optional test for SyscallConn as well, since it is widely implemented.

  1. Export timeoutWrapper and connTester in some form, to enable callers to create their own local tests in a nettest.TestConn-style.

This could be nice to keep the amount of code in this package smaller, but it'd also mean that certain tests such as the CloseRead/CloseWrite would need to be duplicated between different projects.

With this said, I think option 1 is preferable in order to reduce duplication in the community, but I can understand why one wouldn't want to add additional complexity to this package to test methods that are not a required part of net.Conn.

/cc @dsnet @mikioh @acln0

@rsc

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

Thoughts, @mikioh?

@mdlayher

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

It appears that @mikioh has taken approach 1 with the TestListener API (checking if net.Listener has a SetDeadline method) in https://go-review.googlesource.com/c/net/+/123056, which implies to me that this would be the preferred approach.

@rsc

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

SGTM. We trust that you @mdlayher and @mikioh will converge on something good.

@rsc rsc changed the title proposal: x/net/nettest: extensibility options for TestConn x/net/nettest: extensibility options for TestConn May 7, 2019

@rsc rsc modified the milestones: Proposal, Unreleased May 7, 2019

@mdlayher mdlayher changed the title x/net/nettest: extensibility options for TestConn x/net/nettest: extend TestConn with optional interface checks May 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.