Drastically improve non autobahn test coverage

Also simplified and refactored the Conn tests.

More changes soon.

accept_test.go

@@ -6,6 +6,39 @@ import (
 	"testing"
 )
 
+func TestAccept(t *testing.T) {
+	t.Parallel()
+
+	t.Run("badClientHandshake", func(t *testing.T) {
+		t.Parallel()
+
+		w := httptest.NewRecorder()
+		r := httptest.NewRequest("GET", "/", nil)
+
+		_, err := Accept(w, r, AcceptOptions{})
+		if err == nil {
+			t.Fatalf("unexpected error value: %v", err)
+		}
+
+	})
+
+	t.Run("requireHttpHijacker", func(t *testing.T) {
+		t.Parallel()
+
+		w := httptest.NewRecorder()
+		r := httptest.NewRequest("GET", "/", nil)
+		r.Header.Set("Connection", "Upgrade")
+		r.Header.Set("Upgrade", "websocket")
+		r.Header.Set("Sec-WebSocket-Version", "13")
+		r.Header.Set("Sec-WebSocket-Key", "meow123")
+
+		_, err := Accept(w, r, AcceptOptions{})
+		if err == nil || !strings.Contains(err.Error(), "http.Hijacker") {
+			t.Fatalf("unexpected error value: %v", err)
+		}
+	})
+}
+
 func Test_verifyClientHandshake(t *testing.T) {
 	t.Parallel()
 

ci/test.sh

@@ -4,19 +4,34 @@ set -euo pipefail
 cd "$(dirname "${0}")"
 cd "$(git rev-parse --show-toplevel)"
 
-mkdir -p ci/out/websocket
-testFlags=(
+argv=(
+  go run gotest.tools/gotestsum
+  # https://circleci.com/docs/2.0/collect-test-data/
+  "--junitfile=ci/out/websocket/testReport.xml"
+  "--format=short-verbose"
+  --
   -race
   "-vet=off"
-  #  "-bench=."
+  "-bench=."
+)
+# Interactive usage probably does not want to enable benchmarks, race detection
+# turn off vet or use gotestsum by default.
+if [[ $# -gt 0 ]]; then
+  argv=(go test "$@")
+fi
+
+# We always want coverage.
+argv+=(
   "-coverprofile=ci/out/coverage.prof"
   "-coverpkg=./..."
 )
-# https://circleci.com/docs/2.0/collect-test-data/
-go run gotest.tools/gotestsum \
-  --junitfile ci/out/websocket/testReport.xml \
-  --format=short-verbose \
-  -- "${testFlags[@]}"
+
+mkdir -p ci/out/websocket
+"${argv[@]}"
+
+# Removes coverage of generated files.
+grep -v _string.go < ci/out/coverage.prof > ci/out/coverage2.prof
+mv ci/out/coverage2.prof ci/out/coverage.prof
 
 go tool cover -html=ci/out/coverage.prof -o=ci/out/coverage.html
 if [[ ${CI:-} ]]; then

dial_test.go

@@ -33,6 +33,10 @@ func TestBadDials(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "badTLS",
+			url:  "wss://totallyfake.nhooyr.io",
+		},
 	}
 
 	for _, tc := range testCases {
@@ -40,7 +44,10 @@ func TestBadDials(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			t.Parallel()
 
-			_, _, err := Dial(context.Background(), tc.url, tc.opts)
+			ctx, cancel := context.WithTimeout(context.Background(), time.Second*5)
+			defer cancel()
+
+			_, _, err := Dial(ctx, tc.url, tc.opts)
 			if err == nil {
 				t.Fatalf("expected non nil error: %+v", err)
 			}

export_test.go

 package websocket
 
-var Compute = handleSecWebSocketKey
+import (
+	"context"
+)
+
+type Addr = websocketAddr
+
+type Header = header
+
+func (c *Conn) WriteFrame(ctx context.Context, fin bool, opcode opcode, p []byte) (int, error) {
+	return c.writeFrame(ctx, fin, opcode, p)
+}

header_test.go

@@ -2,6 +2,7 @@ package websocket
 
 import (
 	"bytes"
+	"io"
 	"math/rand"
 	"strconv"
 	"testing"
@@ -21,6 +22,36 @@ func randBool() bool {
 func TestHeader(t *testing.T) {
 	t.Parallel()
 
+	t.Run("eof", func(t *testing.T) {
+		t.Parallel()
+
+		testCases := []struct {
+			name  string
+			bytes []byte
+		}{
+			{
+				"start",
+				[]byte{0xff},
+			},
+			{
+				"middle",
+				[]byte{0xff, 0xff, 0xff},
+			},
+		}
+		for _, tc := range testCases {
+			tc := tc
+			t.Run(tc.name, func(t *testing.T) {
+				t.Parallel()
+
+				b := bytes.NewBuffer(tc.bytes)
+				_, err := readHeader(nil, b)
+				if io.ErrUnexpectedEOF != err {
+					t.Fatalf("expected %v but got: %v", io.ErrUnexpectedEOF, err)
+				}
+			})
+		}
+	})
+
 	t.Run("writeNegativeLength", func(t *testing.T) {
 		t.Parallel()
 

netconn.go

@@ -101,8 +101,8 @@ func (c *netConn) Read(p []byte) (int, error) {
 			return 0, err
 		}
 		if typ != c.msgType {
-			c.c.Close(StatusUnsupportedData, fmt.Sprintf("can only accept %v messages", c.msgType))
-			return 0, xerrors.Errorf("unexpected frame type read for net conn adapter (expected %v): %v", c.msgType, typ)
+			c.c.Close(StatusUnsupportedData, fmt.Sprintf("unexpected frame type read (expected %v): %v", c.msgType, typ))
+			return 0, c.c.closeErr
 		}
 		c.reader = r
 	}

statuscode.go

@@ -35,7 +35,7 @@ const (
 	StatusTryAgainLater
 	StatusBadGateway
 	// statusTLSHandshake is unexported because we just return
-	// handshake error in dial. We do not return a conn
+	// the handshake error in dial. We do not return a conn
 	// so there is nothing to use this on. At least until WASM.
 	statusTLSHandshake
 )

statuscode_test.go

@@ -4,14 +4,13 @@ import (
 	"math"
 	"strings"
 	"testing"
+
+	"github.com/google/go-cmp/cmp"
 )
 
 func TestCloseError(t *testing.T) {
 	t.Parallel()
 
-	// Other parts of close error are tested by websocket_test.go right now
-	// with the autobahn tests.
-
 	testCases := []struct {
 		name    string
 		ce      CloseError
@@ -50,7 +49,108 @@ func TestCloseError(t *testing.T) {
 
 			_, err := tc.ce.bytes()
 			if (err == nil) != tc.success {
-				t.Fatalf("unexpected error value: %v", err)
+				t.Fatalf("unexpected error value: %+v", err)
+			}
+		})
+	}
+}
+
+func Test_parseClosePayload(t *testing.T) {
+	t.Parallel()
+
+	testCases := []struct {
+		name    string
+		p       []byte
+		success bool
+		ce      CloseError
+	}{
+		{
+			name:    "normal",
+			p:       append([]byte{0x3, 0xE8}, []byte("hello")...),
+			success: true,
+			ce: CloseError{
+				Code:   StatusNormalClosure,
+				Reason: "hello",
+			},
+		},
+		{
+			name:    "nothing",
+			success: true,
+			ce: CloseError{
+				Code: StatusNoStatusRcvd,
+			},
+		},
+		{
+			name:    "oneByte",
+			p:       []byte{0},
+			success: false,
+		},
+		{
+			name:    "badStatusCode",
+			p:       []byte{0x17, 0x70},
+			success: false,
+		},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			ce, err := parseClosePayload(tc.p)
+			if (err == nil) != tc.success {
+				t.Fatalf("unexpected expected error value: %+v", err)
+			}
+
+			if tc.success && tc.ce != ce {
+				t.Fatalf("unexpected close error: %v", cmp.Diff(tc.ce, ce))
+			}
+		})
+	}
+}
+
+func Test_validWireCloseCode(t *testing.T) {
+	t.Parallel()
+
+	testCases := []struct {
+		name  string
+		code  StatusCode
+		valid bool
+	}{
+		{
+			name:  "normal",
+			code:  StatusNormalClosure,
+			valid: true,
+		},
+		{
+			name:  "noStatus",
+			code:  StatusNoStatusRcvd,
+			valid: false,
+		},
+		{
+			name:  "3000",
+			code:  3000,
+			valid: true,
+		},
+		{
+			name:  "4999",
+			code:  4999,
+			valid: true,
+		},
+		{
+			name:  "unknown",
+			code:  5000,
+			valid: false,
+		},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			if valid := validWireCloseCode(tc.code); tc.valid != valid {
+				t.Fatalf("expected %v for %v but got %v", tc.valid, tc.code, valid)
 			}
 		})
 	}

websocket.go

@@ -7,8 +7,8 @@ import (
 	"fmt"
 	"io"
 	"io/ioutil"
+	"log"
 	"math/rand"
-	"os"
 	"runtime"
 	"strconv"
 	"sync"
@@ -210,9 +210,8 @@ func (c *Conn) readTillMsg(ctx context.Context) (header, error) {
 		}
 
 		if h.rsv1 || h.rsv2 || h.rsv3 {
-			err := xerrors.Errorf("received header with rsv bits set: %v:%v:%v", h.rsv1, h.rsv2, h.rsv3)
-			c.Close(StatusProtocolError, err.Error())
-			return header{}, err
+			c.Close(StatusProtocolError, fmt.Sprintf("received header with rsv bits set: %v:%v:%v", h.rsv1, h.rsv2, h.rsv3))
+			return header{}, c.closeErr
 		}
 
 		if h.opcode.controlOp() {
@@ -227,9 +226,8 @@ func (c *Conn) readTillMsg(ctx context.Context) (header, error) {
 		case opBinary, opText, opContinuation:
 			return h, nil
 		default:
-			err := xerrors.Errorf("received unknown opcode %v", h.opcode)
-			c.Close(StatusProtocolError, err.Error())
-			return header{}, err
+			c.Close(StatusProtocolError, fmt.Sprintf("received unknown opcode %v", h.opcode))
+			return header{}, c.closeErr
 		}
 	}
 }
@@ -273,15 +271,13 @@ func (c *Conn) readFrameHeader(ctx context.Context) (header, error) {
 
 func (c *Conn) handleControl(ctx context.Context, h header) error {
 	if h.payloadLength > maxControlFramePayload {
-		err := xerrors.Errorf("control frame too large at %v bytes", h.payloadLength)
-		c.Close(StatusProtocolError, err.Error())
-		return err
+		c.Close(StatusProtocolError, fmt.Sprintf("control frame too large at %v bytes", h.payloadLength))
+		return c.closeErr
 	}
 
 	if !h.fin {
-		err := xerrors.Errorf("received fragmented control frame")
-		c.Close(StatusProtocolError, err.Error())
-		return err
+		c.Close(StatusProtocolError, "received fragmented control frame")
+		return c.closeErr
 	}
 
 	ctx, cancel := context.WithTimeout(ctx, time.Second*5)
@@ -311,8 +307,9 @@ func (c *Conn) handleControl(ctx context.Context, h header) error {
 	case opClose:
 		ce, err := parseClosePayload(b)
 		if err != nil {
-			c.Close(StatusProtocolError, "received invalid close payload")
-			return xerrors.Errorf("received invalid close payload: %w", err)
+			err = xerrors.Errorf("received invalid close payload: %w", err)
+			c.Close(StatusProtocolError, err.Error())
+			return c.closeErr
 		}
 		// This ensures the closeErr of the Conn is always the received CloseError
 		// in case the echo close frame write fails.
@@ -376,9 +373,8 @@ func (c *Conn) reader(ctx context.Context) (MessageType, io.Reader, error) {
 
 	if c.activeReader != nil && !c.activeReader.eof() {
 		if h.opcode != opContinuation {
-			err := xerrors.Errorf("received new data message without finishing the previous message")
-			c.Close(StatusProtocolError, err.Error())
-			return 0, nil, err
+			c.Close(StatusProtocolError, "received new data message without finishing the previous message")
+			return 0, nil, c.closeErr
 		}
 
 		if !h.fin || h.payloadLength > 0 {
@@ -392,9 +388,8 @@ func (c *Conn) reader(ctx context.Context) (MessageType, io.Reader, error) {
 			return 0, nil, err
 		}
 	} else if h.opcode == opContinuation {
-		err := xerrors.Errorf("received continuation frame not after data or text frame")
-		c.Close(StatusProtocolError, err.Error())
-		return 0, nil, err
+		c.Close(StatusProtocolError, "received continuation frame not after data or text frame")
+		return 0, nil, c.closeErr
 	}
 
 	c.readerMsgCtx = ctx
@@ -460,9 +455,8 @@ func (r *messageReader) read(p []byte) (int, error) {
 	}
 
 	if r.c.readMsgLeft <= 0 {
-		err := xerrors.Errorf("read limited at %v bytes", r.c.msgReadLimit)
-		r.c.Close(StatusMessageTooBig, err.Error())
-		return 0, err
+		r.c.Close(StatusMessageTooBig, fmt.Sprintf("read limited at %v bytes", r.c.msgReadLimit))
+		return 0, r.c.closeErr
 	}
 
 	if int64(len(p)) > r.c.readMsgLeft {
@@ -476,9 +470,8 @@ func (r *messageReader) read(p []byte) (int, error) {
 		}
 
 		if h.opcode != opContinuation {
-			err := xerrors.Errorf("received new data message without finishing the previous message")
-			r.c.Close(StatusProtocolError, err.Error())
-			return 0, err
+			r.c.Close(StatusProtocolError, "received new data message without finishing the previous message")
+			return 0, r.c.closeErr
 		}
 
 		r.c.readerMsgHeader = h
@@ -828,7 +821,7 @@ func (c *Conn) writePong(p []byte) error {
 func (c *Conn) Close(code StatusCode, reason string) error {
 	err := c.exportedClose(code, reason)
 	if err != nil {
-		return xerrors.Errorf("failed to close connection: %w", err)
+		return xerrors.Errorf("failed to close websocket connection: %w", err)
 	}
 	return nil
 }
@@ -844,7 +837,7 @@ func (c *Conn) exportedClose(code StatusCode, reason string) error {
 	// Definitely worth seeing what popular browsers do later.
 	p, err := ce.bytes()
 	if err != nil {
-		fmt.Fprintf(os.Stderr, "websocket: failed to marshal close frame: %v\n", err)
+		log.Printf("websocket: failed to marshal close frame: %+v", err)
 		ce = CloseError{
 			Code: StatusInternalError,
 		}
@@ -853,12 +846,13 @@ func (c *Conn) exportedClose(code StatusCode, reason string) error {
 
 	// CloseErrors sent are made opaque to prevent applications from thinking
 	// they received a given status.
-	err = c.writeClose(p, xerrors.Errorf("sent close frame: %v", ce))
+	sentErr := xerrors.Errorf("sent close frame: %v", ce)
+	err = c.writeClose(p, sentErr)
 	if err != nil {
 		return err
 	}
 
-	if !xerrors.Is(c.closeErr, ce) {
+	if !xerrors.Is(c.closeErr, sentErr) {
 		return c.closeErr
 	}