Add gotestfmt to improve test output and fix flaky tests (#6528)

.github/workflows/fleetctl-preview.yml

@@ -68,6 +68,7 @@ jobs:
         SLACK_WEBHOOK_TYPE: INCOMING_WEBHOOK
 
     - name: Get fleet logs
+      if: always()
       run: |
         FLEET_LICENSE_KEY=foo docker compose -f ~/.fleet/preview/docker-compose.yml logs fleet01 fleet02 > fleet-logs.txt
         # Copying logs, otherwise the upload-artifact action uploads the logs in a hidden folder (.fleet)

.github/workflows/test-go.yaml

@@ -36,6 +36,9 @@ jobs:
       with:
         go-version: ${{ matrix.go-version }}
 
+    - name: Set up gotestfmt
+      run: go install github.com/haveyoudebuggedit/gotestfmt/v2/cmd/gotestfmt@v2.3.2
+
     - name: Checkout Code
       uses: actions/checkout@629c2de402a417ea7690ca6ce3f33229e27606a5 # v2
 
@@ -61,7 +64,12 @@ jobs:
 
     - name: Run Go Tests
       run: |
-        TEST_LOCK_FILE_PATH=$(pwd)/lock NETWORK_TEST=1 REDIS_TEST=1 MYSQL_TEST=1 RACE_ENABLED=$RACE_ENABLED GO_TEST_TIMEOUT=$GO_TEST_TIMEOUT make test-go
+        GO_TEST_EXTRA_FLAGS="-v -json -race=$RACE_ENABLED -timeout=$GO_TEST_TIMEOUT" \
+          TEST_LOCK_FILE_PATH=$(pwd)/lock \
+          NETWORK_TEST=1 \
+          REDIS_TEST=1 \
+          MYSQL_TEST=1 \
+          make test-go 2>&1 | tee /tmp/gotest.log | gotestfmt
 
     - name: Upload to Codecov
       uses: codecov/codecov-action@81cd2dc8148241f03f5839d295e000b8f761e378
@@ -88,3 +96,11 @@ jobs:
       env:
         SLACK_WEBHOOK_URL: ${{ secrets.SLACK_G_PLATFORM_WEBHOOK_URL }}
         SLACK_WEBHOOK_TYPE: INCOMING_WEBHOOK
+
+    - name: Upload test log
+      if: always()
+      uses: actions/upload-artifact@3cea5372237819ed00197afe530f5a7ea3e805c8 # v2
+      with:
+        name: test-log
+        path: /tmp/gotest.log
+        if-no-files-found: error

Makefile

@@ -10,16 +10,16 @@ REVSHORT = $(shell git rev-parse --short HEAD)
 USER = $(shell whoami)
 DOCKER_IMAGE_NAME = fleetdm/fleet
 
-ifdef RACE_ENABLED
-RACE_ENABLED_VAR := $(RACE_ENABLED)
+ifdef GO_TEST_EXTRA_FLAGS
+GO_TEST_EXTRA_FLAGS_VAR := $(GO_TEST_EXTRA_FLAGS)
 else
-RACE_ENABLED_VAR := false
+GO_TEST_EXTRA_FLAGS_VAR :=
 endif
 
-ifdef GO_TEST_TIMEOUT
-GO_TEST_TIMEOUT_VAR := $(GO_TEST_TIMEOUT)
+ifdef GO_BUILD_RACE_ENABLED
+GO_BUILD_RACE_ENABLED_VAR := true
 else
-GO_TEST_TIMEOUT_VAR := 10m
+GO_BUILD_RACE_ENABLED_VAR := false
 endif
 
 ifneq ($(OS), Windows_NT)
@@ -113,13 +113,18 @@ help:
 build: fleet fleetctl
 
 fleet: .prefix .pre-build .pre-fleet
-	CGO_ENABLED=1 go build -race=${RACE_ENABLED_VAR} -tags full,fts5,netgo -o build/${OUTPUT} -ldflags ${KIT_VERSION} ./cmd/fleet
+	CGO_ENABLED=1 go build -race=${GO_BUILD_RACE_ENABLED_VAR} -tags full,fts5,netgo -o build/${OUTPUT} -ldflags ${KIT_VERSION} ./cmd/fleet
 
-fleet-dev: RACE_ENABLED_VAR=true
+fleet-dev: GO_BUILD_RACE_ENABLED_VAR=true
 fleet-dev: fleet
 
 fleetctl: .prefix .pre-build .pre-fleetctl
-	CGO_ENABLED=0 go build -o build/fleetctl -ldflags ${KIT_VERSION} ./cmd/fleetctl
+	# Race requires cgo
+	$(eval CGO_ENABLED := $(shell [[ "${GO_BUILD_RACE_ENABLED_VAR}" = "true" ]] && echo 1 || echo 0))
+	CGO_ENABLED=${CGO_ENABLED} go build -race=${GO_BUILD_RACE_ENABLED_VAR} -o build/fleetctl -ldflags ${KIT_VERSION} ./cmd/fleetctl
+
+fleetctl-dev: GO_BUILD_RACE_ENABLED_VAR=true
+fleetctl-dev: fleetctl
 
 lint-js:
 	yarn lint
@@ -133,7 +138,7 @@ dump-test-schema:
 	go run ./tools/dbutils ./server/datastore/mysql/schema.sql
 
 test-go: dump-test-schema generate-mock
-	go test -tags full,fts5,netgo -timeout=${GO_TEST_TIMEOUT_VAR} -race=${RACE_ENABLED_VAR} -parallel 8 -coverprofile=coverage.txt -covermode=atomic ./cmd/... ./ee/... ./orbit/pkg/... ./orbit/cmd/orbit ./pkg/... ./server/... ./tools/...
+	go test -tags full,fts5,netgo ${GO_TEST_EXTRA_FLAGS_VAR} -parallel 8 -coverprofile=coverage.txt -covermode=atomic ./cmd/... ./ee/... ./orbit/pkg/... ./orbit/cmd/orbit ./pkg/... ./server/... ./tools/...
 
 analyze-go:
 	go test -tags full,fts5,netgo -race -cover ./...

cmd/fleetctl/preview_test.go

@@ -1,6 +1,7 @@
 package main
 
 import (
+	"bytes"
 	"os"
 	"path/filepath"
 	"regexp"
@@ -23,12 +24,21 @@ func TestPreview(t *testing.T) {
 		require.Equal(t, "", runAppForTest(t, []string{"preview", "--config", configPath, "stop"}))
 	})
 
-	output := runAppForTest(t, []string{"preview", "--config", configPath, "--tag", "main", "--disable-open-browser"})
+	var output *bytes.Buffer
+	nettest.RunWithNetRetry(t, func() error {
+		var err error
+		output, err = runAppNoChecks([]string{
+			"preview", "--config", configPath,
+			"--tag", "main",
+			"--disable-open-browser",
+		})
+		return err
+	})
 
 	queriesRe := regexp.MustCompile(`applied ([0-9]+) queries`)
 	policiesRe := regexp.MustCompile(`applied ([0-9]+) policies`)
-	require.True(t, queriesRe.MatchString(output))
-	require.True(t, policiesRe.MatchString(output))
+	require.True(t, queriesRe.MatchString(output.String()))
+	require.True(t, policiesRe.MatchString(output.String()))
 
 	// run some sanity checks on the preview environment
 

pkg/nettest/nettest.go

@@ -62,8 +62,11 @@ func Retryable(err error) bool {
 	}
 	// Using the exact same error check used in:
 	// https://github.com/golang/go/blob/a5d61be040ed20b5774bff1b6b578c6d393ab332/src/net/http/serve_test.go#L1417
+	//
+	// Also we use raw string matching because this method is used on raw error strings (returned by commands).
 	if errStr := err.Error(); (strings.Contains(errStr, "timeout") && strings.Contains(errStr, "TLS handshake")) ||
-		strings.Contains(errStr, "unexpected EOF") || strings.Contains(errStr, "connection reset by peer") || strings.Contains(errStr, "unexpected http response") {
+		strings.Contains(errStr, "unexpected EOF") || strings.Contains(errStr, "connection reset by peer") ||
+		strings.Contains(errStr, "unexpected http response") || strings.Contains(errStr, "context deadline exceeded") {
 		return true
 	}
 	return false