Fix log and grpc flakey/incomplete tests (#45)

This PR has a bunch of fixes to the tests. The env, log, and grpc packages were touched but most of the changes were in log and grpc.

The log package had a bunch of env vars leaking between tests and influencing tests within the test, it was a pain to keep straight so I've fixed all of that by clearing envs and running withing `t.Run` so that `defer ` works.

The grpc tests had an issue with the port used for the server tests staying around long enough between `make test` and `make coverage` where the latter would fail.

The env package got the same env clearing work just to do things right.

Author: mmlb

env/env_test.go

@@ -8,11 +8,13 @@ import (
 	"os"
 	"strconv"
 	"time"
+
+	"github.com/packethost/pkg/internal/testenv"
 )
 
 func ExampleBool() {
+	defer testenv.Clear().Restore()
 	name := "some_bool_environment_variable_that_is_not_set"
-	os.Unsetenv(name)
 
 	fmt.Println(Bool(name))
 	fmt.Println(Bool(name, true))
@@ -62,8 +64,9 @@ func ExampleBool() {
 }
 
 func ExampleGet() {
+	defer testenv.Clear().Restore()
+
 	name := "some_environment_variable_that_is_not_set"
-	os.Unsetenv(name)
 	fmt.Println(Get(name))
 	fmt.Println(Get(name, "this is the default"))
 	fmt.Println(Get(name, "this is the default", "this one is ignored"))
@@ -81,9 +84,9 @@ func ExampleGet() {
 }
 
 func ExampleInt() {
-	name := "some_int_environment_variable_that_is_not_set"
-	os.Unsetenv(name)
+	defer testenv.Clear().Restore()
 
+	name := "some_int_environment_variable_that_is_not_set"
 	fmt.Println(Int(name))
 	fmt.Println(Int(name, 42))
 	fmt.Println(Int(name, 42, 21))
@@ -102,8 +105,9 @@ func ExampleInt() {
 }
 
 func ExampleURL() {
+	defer testenv.Clear().Restore()
+
 	name := "some_url_environment_variable_that_is_not_set"
-	os.Unsetenv(name)
 	fmt.Println(URL(name))
 	fmt.Println(URL(name, "https://packet.com"))
 	fmt.Println(URL(name, "https://packet.com", "https://www.equinix.com"))
@@ -123,8 +127,9 @@ func ExampleURL() {
 }
 
 func ExampleDuration() {
+	defer testenv.Clear().Restore()
+
 	name := "some_duration_environment_variable_that_is_not_set"
-	os.Unsetenv(name)
 	fmt.Println(Duration(name))
 	fmt.Println(Duration(name, 15*time.Minute))
 	fmt.Println(Duration(name, 15*time.Minute, 2*time.Hour))

grpc/grpc.go

@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"net"
 	"strconv"
+	"sync"
 
 	grpc_middleware "github.com/grpc-ecosystem/go-grpc-middleware"
 	grpc_prometheus "github.com/grpc-ecosystem/go-grpc-prometheus"
@@ -22,13 +23,17 @@ import (
 // Server is used to hold configured info and ultimately the grpc server
 type Server struct {
 	err       error
-	port      int
 	server    *grpc.Server
 	cert      tls.Certificate
 	options   []grpc.ServerOption
 	streamers []grpc.StreamServerInterceptor
 	unariers  []grpc.UnaryServerInterceptor
 	registry  []func(*grpc.Server)
+
+	mu       sync.RWMutex
+	port     int
+	once     sync.Once
+	listener net.Listener
 }
 
 // The ServiceRegister type is used as a callback once the underlying grpc server is setup to register the main service.
@@ -99,6 +104,8 @@ func NewServer(l log.Logger, reg ServiceRegister, options ...Option) (*Server, e
 
 // Port returns the port the server is listening on
 func (s *Server) Port() int {
+	s.mu.RLock()
+	defer s.mu.RUnlock()
 	return s.port
 }
 
@@ -107,15 +114,49 @@ func (s *Server) Server() *grpc.Server {
 	return s.server
 }
 
+func (s *Server) listen() error {
+	var err error
+	s.once.Do(func() {
+		s.mu.RLock()
+		port := s.port
+		s.mu.RUnlock()
+
+		s.listener, err = net.Listen("tcp", fmt.Sprintf(":%d", port)) //nolint:ineffassign // we do in fact want to assign to err in the outer scope
+		if err != nil {
+			err = errors.Wrap(err, "listen")
+			return
+		}
+
+		var p string
+		_, p, err = net.SplitHostPort(s.listener.Addr().String()) //nolint:ineffassign // we do in fact want to assign to err in the outer scope
+		if err != nil {
+			err = errors.Wrap(err, "extract server port")
+			return
+		}
+
+		port, err = strconv.Atoi(p)
+		if err != nil {
+			err = errors.Wrap(err, "parse server port")
+			return
+		}
+
+		s.mu.Lock()
+		s.port = port
+		s.mu.Unlock()
+	})
+
+	return err
+}
+
 // Serve starts the grpc server
 func (s *Server) Serve() error {
-	lis, err := net.Listen("tcp", fmt.Sprintf(":%d", s.port))
+	err := s.listen()
 	if err != nil {
-		return errors.Wrap(err, "listen")
+		return err
 	}
-	defer lis.Close()
 
-	return errors.Wrap(s.server.Serve(lis), "serve")
+	defer s.listener.Close()
+	return errors.Wrap(s.server.Serve(s.listener), "serve")
 }
 
 // ServerOption will add the opt param to the underlying grpc.NewServer() call.

grpc/grpc_test.go

@@ -20,8 +20,10 @@ import (
 	"testing"
 	"time"
 
+	"github.com/packethost/pkg/internal/testenv"
 	"github.com/packethost/pkg/log"
 	"github.com/stretchr/testify/require"
+	assert "github.com/stretchr/testify/require"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/credentials"
 	pb "google.golang.org/grpc/examples/helloworld/helloworld"
@@ -64,7 +66,7 @@ func TestDefaultsAndOrdering(t *testing.T) {
 	s, err := NewServer(l, srv, pre1, Register(reg1), pre2, Register(reg2))
 	assert.NoError(err)
 	assert.NotNil(s)
-	assert.Equal(s.port, 8080)
+	assert.Equal(s.Port(), 8080)
 
 	// ensure order
 	assert.True(pre1I > 0)
@@ -75,13 +77,15 @@ func TestDefaultsAndOrdering(t *testing.T) {
 }
 
 func TestPort(t *testing.T) {
+	defer testenv.Clear().Restore()
+
 	l := log.Test(t, svc)
 	assert := require.New(t)
 
 	s, err := NewServer(l, defSrv)
 	assert.NoError(err)
 	assert.NotNil(s)
-	assert.Equal(s.port, 8080)
+	assert.Equal(s.Port(), 8080)
 	assert.Equal(s.Port(), 8080)
 
 	os.Setenv("GRPC_PORT", "4242")
@@ -90,13 +94,13 @@ func TestPort(t *testing.T) {
 	s, err = NewServer(l, defSrv)
 	assert.NoError(err)
 	assert.NotNil(s)
-	assert.Equal(s.port, 4242)
+	assert.Equal(s.Port(), 4242)
 	assert.Equal(s.Port(), 4242)
 
 	s, err = NewServer(l, defSrv, Port(2424))
 	assert.NoError(err)
 	assert.NotNil(s)
-	assert.Equal(s.port, 2424)
+	assert.Equal(s.Port(), 2424)
 	assert.Equal(s.Port(), 2424)
 
 	os.Setenv("GRPC_PORT", "0")
@@ -191,6 +195,8 @@ func serve(t *testing.T, s *Server, test func()) {
 	wg.Add(1)
 
 	go func() {
+		s.port = 0
+		s.listen()
 		wg.Done()
 		if err := s.Serve(); err != nil {
 			panic(err)
@@ -232,6 +238,8 @@ func connectGRPC(t *testing.T, port int, cert string) error {
 }
 
 func TestX509(t *testing.T) {
+	defer testenv.Clear().Restore()
+
 	t.Run("insecure", func(t *testing.T) {
 		l := log.Test(t, svc)
 		assert := require.New(t)
@@ -240,7 +248,7 @@ func TestX509(t *testing.T) {
 		assert.NoError(err)
 		assert.NotNil(s)
 		serve(t, s, func() {
-			assert.NoError(connectGRPC(t, s.port, ""))
+			assert.NoError(connectGRPC(t, s.Port(), ""))
 		})
 	})
 
@@ -257,8 +265,8 @@ func TestX509(t *testing.T) {
 		assert.NoError(err)
 		assert.NotNil(s)
 		serve(t, s, func() {
-			assert.Error(connectGRPC(t, s.port, ""))
-			assert.NoError(connectGRPC(t, s.port, certE))
+			assert.Error(connectGRPC(t, s.Port(), ""))
+			assert.NoError(connectGRPC(t, s.Port(), certE))
 		})
 	})
 
@@ -271,9 +279,9 @@ func TestX509(t *testing.T) {
 		assert.NoError(err)
 		assert.NotNil(s)
 		serve(t, s, func() {
-			assert.Error(connectGRPC(t, s.port, ""))
-			assert.Error(connectGRPC(t, s.port, certE))
-			assert.NoError(connectGRPC(t, s.port, certKP))
+			assert.Error(connectGRPC(t, s.Port(), ""))
+			assert.Error(connectGRPC(t, s.Port(), certE))
+			assert.NoError(connectGRPC(t, s.Port(), certKP))
 		})
 	})
 
@@ -287,10 +295,10 @@ func TestX509(t *testing.T) {
 		assert.NoError(err)
 		assert.NotNil(s)
 		serve(t, s, func() {
-			assert.Error(connectGRPC(t, s.port, ""))
-			assert.Error(connectGRPC(t, s.port, certE))
-			assert.Error(connectGRPC(t, s.port, certKP))
-			assert.NoError(connectGRPC(t, s.port, certLKP))
+			assert.Error(connectGRPC(t, s.Port(), ""))
+			assert.Error(connectGRPC(t, s.Port(), certE))
+			assert.Error(connectGRPC(t, s.Port(), certKP))
+			assert.NoError(connectGRPC(t, s.Port(), certLKP))
 		})
 	})
 
@@ -303,3 +311,52 @@ func TestX509(t *testing.T) {
 		assert.Nil(s)
 	})
 }
+
+func TestListen(t *testing.T) {
+	t.Run("invalid port fails", func(t *testing.T) {
+		s := Server{port: -1}
+		assert.Error(t, s.listen())
+		assert.Nil(t, s.listener)
+	})
+	t.Run("port 0 updates .port", func(t *testing.T) {
+		s := Server{}
+		assert.NoError(t, s.listen())
+		assert.NotNil(t, s.listener)
+		defer s.listener.Close()
+
+		assert.NotZero(t, s.Port())
+	})
+	t.Run("can run 2 servers by using port 0", func(t *testing.T) {
+		s1 := Server{}
+		assert.NoError(t, s1.listen())
+		assert.NotNil(t, s1.listener)
+		defer s1.listener.Close()
+		assert.NotZero(t, s1.port)
+
+		s2 := Server{}
+		assert.NoError(t, s2.listen())
+		assert.NotNil(t, s2.listener)
+		defer s2.listener.Close()
+		assert.NotZero(t, s2.port)
+
+		assert.NotEqual(t, s1.port, s2.port)
+	})
+	t.Run("Serve calls s.listen if necessary", func(t *testing.T) {
+		l := log.Test(t, t.Name())
+
+		s, err := NewServer(l, defSrv)
+		assert.NoError(t, err)
+		assert.NotNil(t, s)
+
+		go func() {
+			if err := s.Serve(); err != nil {
+				panic(err)
+			}
+		}()
+
+		for port := s.Port(); port == 0; port = s.Port() {
+			time.Sleep(100 * time.Microsecond)
+		}
+		assert.NoError(t, connectGRPC(t, s.Port(), ""))
+	})
+}

internal/testenv/clearer.go

@@ -0,0 +1,42 @@
+// Package testenv provides functionality for clearing and restoring back the environment.
+// This is mainly intended to be used in tests.
+package testenv
+
+import (
+	"os"
+	"strings"
+)
+
+// Restorer is the interface that wraps the Restore method.
+//
+// Restore will fully restore the environment variables to what it was
+// before cleared with Clear.
+type Restorer interface {
+	Restore()
+}
+
+type restorer func()
+
+func (r restorer) Restore() {
+	r()
+
+}
+
+// Clear will clear the environment and return a Restorer, whose
+// Restore function should be called to return the environment
+// back to what it was before Clear was called.
+//
+// A nice way to call this would be `defer Clear().Restore()`
+func Clear() Restorer {
+	old := os.Environ()
+	fn := func() {
+		os.Clearenv()
+		for _, e := range old {
+			kv := strings.SplitN(e, "=", 2)
+			os.Setenv(kv[0], kv[1])
+		}
+	}
+	os.Clearenv()
+
+	return restorer(fn)
+}