Expand tests #67
Expand tests #67
Conversation
| "go-reuseport requires go >= 1.11" | ||
|
|
||
| ; | ||
| package reuseport |
There was a problem hiding this comment.
Added the package keyword because gx-go would complain and make deps would fail.
There was a problem hiding this comment.
This seems great. @Stebalien will know if this is appropriate.
There was a problem hiding this comment.
So, I put that where it is so "go-reuseport requires go >= 1.11" would show up in the compiler's error message. However, I can confirm that this breaks gx so that's probably good enough.
There was a problem hiding this comment.
If the error message isn't useful anymore, there's probably no point having this file anymore, unless gx is fixed or exposes a build tag of its own.
There was a problem hiding this comment.
Probably. Note: there's nothing gx can do about this. Unfortunately, it has to ignore build tags (it can't know what build tags you're going to use later).
| ) | ||
|
|
||
| func testDialFromListeningPort(t *testing.T, network, host string) { |
There was a problem hiding this comment.
Fully removed the previous tests because they were outdated and some of their logic had been moved to interface.go.
There was a problem hiding this comment.
I'm not sure how the new tests are better. If you want to add "udp" to the networks tested, I suggest you make a test wrapping testDialFromListeningPort(t, "udp", "localhost")
There was a problem hiding this comment.
I think the new tests are better because they exercise the code via the public API. Namely I directly call Listen(network, address string), ListenPacket(network, address string), Dial(network, laddr, raddr string) and Available(). Instead the current tests instantiate their own ListenConfig and call methods on that instead of via the wrappers offered in interface.go. For this reason a large part of the code does not get exercised.
Instead of re-writing the tests, I'd be happy to rework the current testDialFromListeningPort method to make the right calls.
| ) | ||
|
|
||
| func testDialFromListeningPort(t *testing.T, network, host string) { |
There was a problem hiding this comment.
I'm not sure how the new tests are better. If you want to add "udp" to the networks tested, I suggest you make a test wrapping testDialFromListeningPort(t, "udp", "localhost")
| pc, err := ListenPacket("udp", ":0") | ||
| require.NoError(t, err) | ||
| pc.Close() | ||
| func TestErrorWhenDialUnresolvable(t *testing.T) { |
There was a problem hiding this comment.
This isn't a very useful test. You might test that the function returns net.UnknownNetworkError, but that's not documented.
There was a problem hiding this comment.
I think it is a useful test because it ensures that Dial performs the correct error handling and doesn't swallow errors from ResolveAddr. I'd be happy to document the error handling of Dial. Or did you want this test removed?
There was a problem hiding this comment.
It's useful to test all error paths. I've written if err == nil instead of if err != nil several times.
| "go-reuseport requires go >= 1.11" | ||
|
|
||
| ; | ||
| package reuseport |
There was a problem hiding this comment.
This seems great. @Stebalien will know if this is appropriate.
| network string | ||
| } | ||
|
|
||
| func TestResolveAddr(t *testing.T) { |
There was a problem hiding this comment.
Why do you think it's not useful? I added the test because ResolveAddr is an exported method of this package and the test verifies the method will resolve addresses for these protocols. While the test may seem silly given that we can see the implementation, its purpose is to verify behaviour offered by ResolveAddr from a user's perspective. So if someone were to refactor ResolveAddr the test would help them ensure that no functionality was lost.
|
@anacrolix thanks for taking the time to review my changes! The context of this PR is this issue libp2p/go-libp2p#83 |
|
I've reworked this into #68. |
No description provided.