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

Expand tests #67

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
# go-reuseport

[![travisbadge](https://travis-ci.org/libp2p/go-reuseport.svg)](https://travis-ci.org/libp2p/go-reuseport)
[![](https://img.shields.io/badge/made%20by-Protocol%20Labs-blue.svg?style=flat-square)](https://protocol.ai)
[![](https://img.shields.io/badge/project-libp2p-blue.svg?style=flat-square)](https://libp2p.io/)
[![](https://img.shields.io/badge/freenode-%23libp2p-blue.svg?style=flat-square)](https://webchat.freenode.net/?channels=%23libp2p)
[![codecov](https://codecov.io/gh/libp2p/go-reuseport/branch/master/graph/badge.svg)](https://codecov.io/gh/libp2p/go-reuseport)
[![Travis CI](https://travis-ci.org/libp2p/go-reuseport.svg?branch=master)](https://travis-ci.org/libp2p/go-reuseport)

**NOTE:** This package REQUIRES go >= 1.11.

Expand Down
27 changes: 27 additions & 0 deletions addr_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
package reuseport

import (
"testing"
)

type netAddr struct {
address string
network string
}

func TestResolveAddr(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't very useful.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

netAddrs := []netAddr{
netAddr{"127.0.0.1", "ip"}, netAddr{"127.0.0.1", "ip4"},
netAddr{"::1", "ip6"}, netAddr{"127.0.0.1:1234", "tcp"},
netAddr{"127.0.0.1:1234", "tcp4"}, netAddr{"[::1]:1234", "tcp6"},
netAddr{"127.0.0.1:1234", "udp"}, netAddr{"127.0.0.1:1234", "udp4"},
netAddr{"[::1]:1234", "udp6"}, netAddr{"127.0.0.1:1234", "unix"},
netAddr{"127.0.0.1:1234", "unixgram"}, netAddr{"127.0.0.1:1234", "unixpacket"},
}
for _, na := range netAddrs {
_, err := ResolveAddr(na.network, na.address)
if err != nil {
t.Errorf("Failed to resolve address %v", err)
}
}
}
2 changes: 1 addition & 1 deletion interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ func ListenPacket(network, address string) (net.PacketConn, error) {
}

// Dial dials the given network and address. see net.Dialer.Dial
// Returns a net.Conn created from a file discriptor for a socket
// Returns a net.Conn created from a file descriptor for a socket
// with SO_REUSEPORT and SO_REUSEADDR option set.
func Dial(network, laddr, raddr string) (net.Conn, error) {
nla, err := ResolveAddr(network, laddr)
Expand Down
5 changes: 2 additions & 3 deletions old_go_warning.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
// +build !go1.11

"go-reuseport requires go >= 1.11"

;
package reuseport
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the package keyword because gx-go would complain and make deps would fail.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems great. @Stebalien will know if this is appropriate.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


"go-reuseport requires go >= 1.11";
80 changes: 50 additions & 30 deletions reuse_test.go
Original file line number Diff line number Diff line change
@@ -1,45 +1,65 @@
package reuseport

import (
"context"
"net"
"strings"
"testing"

"github.com/stretchr/testify/require"
)

func testDialFromListeningPort(t *testing.T, network, host string) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fully removed the previous tests because they were outdated and some of their logic had been moved to interface.go.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

lc := net.ListenConfig{
Control: Control,
var message = "Error %v when attempting to listen to 127.0.0.1"

func TestReusePortFeatureAvailable(t *testing.T) {
if ok := Available(); !ok {
t.Error("SO_REUSEPORT is not available on this OS")
}
ctx := context.Background()
ll, err := lc.Listen(ctx, network, host+":0")
if err != nil && strings.Contains(err.Error(), "cannot assign requested address") {
t.Skip(err)
}

func TestListenOnSamePort(t *testing.T) {
l1, err := Listen("tcp", "127.0.0.1:1234")
if err != nil {
t.Errorf(message+":1234", err)
}
require.NoError(t, err)
rl, err := lc.Listen(ctx, network, host+":0")
require.NoError(t, err)
d := net.Dialer{
LocalAddr: ll.Addr(),
Control: Control,
l2, err := Listen("tcp", "127.0.0.1:1234")
if err != nil {
t.Errorf(message+":1234", err)
}
c, err := d.Dial(network, rl.Addr().String())
require.NoError(t, err)
c.Close()
}
l1.Close()
l2.Close()

func TestDialFromListeningPort(t *testing.T) {
testDialFromListeningPort(t, "tcp", "localhost")
lp1, err := ListenPacket("udp", "127.0.0.1:1235")
if err != nil {
t.Errorf(message+":1234", err)
}
lp2, err := ListenPacket("udp", "127.0.0.1:1235")
if err != nil {
t.Errorf(message+":1234", err)
}
lp1.Close()
lp2.Close()
}

func TestDialFromListeningPortTcp6(t *testing.T) {
testDialFromListeningPort(t, "tcp6", "[::1]")
func TestDialFromSamePort(t *testing.T) {
_, err := Listen("tcp", "127.0.0.1:1234")
if err != nil {
t.Errorf(message+":1234", err)
}
_, err = Listen("tcp", "127.0.0.1:1235")
if err != nil {
t.Errorf(message+":1235", err)
}
c, err := Dial("tcp", "127.0.0.1:1234", "127.0.0.1:1235")
if err != nil {
t.Errorf("Error %v when attempting to dial from 127.0.0.1:1234 to 127.0.0.1:1235", err)
}
c.Close()
}

func TestListenPacketWildcardAddress(t *testing.T) {
pc, err := ListenPacket("udp", ":0")
require.NoError(t, err)
pc.Close()
func TestErrorWhenDialUnresolvable(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't a very useful test. You might test that the function returns net.UnknownNetworkError, but that's not documented.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's useful to test all error paths. I've written if err == nil instead of if err != nil several times.

_, err := Dial("asd", "127.0.0.1:1234", "127.0.0.1:1234")
if err == nil {
t.Error("Expected error when trying to dial an unknown protocol")
}

_, err = Dial("tcp", "a.b.c.d:1234", "a.b.c.d:1235")
if err == nil {
t.Error("Expected error when trying to dial an unknown address")
}
}