tests: Handle some failures when using -race (#739)

See commits for details.

Close: #703

UDENG-5710

internal/errno/errno_c.go

@@ -13,15 +13,10 @@ static int get_errno(void) {
   return errno;
 }
 
-static void set_errno(int e) {
-  errno = e;
-}
-
 */
 import "C"
 
 import (
-	"errors"
 	"sync"
 )
 
@@ -46,15 +41,18 @@ const (
 // All these functions are expected to be called while this mutex is locked.
 var mu sync.Mutex
 
+var getErrno = func() int { return int(C.get_errno()) }
+var unsetErrno = func() { C.unset_errno() }
+
 // Lock the usage of errno.
 func Lock() {
 	mu.Lock()
-	C.unset_errno()
+	unsetErrno()
 }
 
 // Unlock unlocks the errno package for being re-used.
 func Unlock() {
-	C.unset_errno()
+	unsetErrno()
 	mu.Unlock()
 }
 
@@ -64,21 +62,9 @@ func Get() error {
 		mu.Unlock()
 		panic("Using errno without locking!")
 	}
-	if errno := C.get_errno(); errno != 0 {
+
+	if errno := getErrno(); errno != 0 {
 		return Error(errno)
 	}
 	return nil
 }
-
-func set(err error) {
-	if mu.TryLock() {
-		mu.Unlock()
-		panic("Using errno without locking!")
-	}
-
-	var e Error
-	if err != nil && !errors.As(err, &e) {
-		panic("Not a valid errno value")
-	}
-	C.set_errno(C.int(e))
-}

internal/errno/errno_c_test.go

@@ -1,40 +1,41 @@
-package errno
+package errno_test
 
 import (
 	"errors"
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"github.com/ubuntu/authd/internal/errno"
 )
 
 func TestNoError(t *testing.T) {
 	t.Parallel()
 
-	Lock()
-	defer Unlock()
+	errno.Lock()
+	t.Cleanup(errno.Unlock)
 
-	require.NoError(t, Get())
+	require.NoError(t, errno.Get())
 }
 
 func TestGetWithoutLocking(t *testing.T) {
 	// This test can't be parallel, since other tests may locking meanwhile.
 
-	require.PanicsWithValue(t, "Using errno without locking!", func() { _ = Get() })
+	require.PanicsWithValue(t, "Using errno without locking!", func() { _ = errno.Get() })
 }
 
 func TestSetWithoutLocking(t *testing.T) {
 	// This test can't be parallel, since other tests may locking meanwhile.
 
-	require.PanicsWithValue(t, "Using errno without locking!", func() { set(nil) })
+	require.PanicsWithValue(t, "Using errno without locking!", func() { errno.Set(nil) })
 }
 
 func TestSetInvalidError(t *testing.T) {
 	t.Parallel()
 
-	Lock()
-	defer Unlock()
+	errno.Lock()
+	t.Cleanup(errno.Unlock)
 
-	require.PanicsWithValue(t, "Not a valid errno value", func() { set(errors.New("invalid")) })
+	require.PanicsWithValue(t, "Not a valid errno value", func() { errno.Set(errors.New("invalid")) })
 }
 
 func TestErrorValues(t *testing.T) {
@@ -44,21 +45,21 @@ func TestErrorValues(t *testing.T) {
 		err error
 	}{
 		"No error":                  {},
-		"No such file or directory": {err: ErrNoEnt},
-		"No such process":           {err: ErrSrch},
-		"Bad file descriptor":       {err: ErrBadf},
-		"Operation not permitted":   {err: ErrPerm},
+		"No such file or directory": {err: errno.ErrNoEnt},
+		"No such process":           {err: errno.ErrSrch},
+		"Bad file descriptor":       {err: errno.ErrBadf},
+		"Operation not permitted":   {err: errno.ErrPerm},
 	}
 	for name, tc := range testCases {
 		t.Run(name, func(t *testing.T) {
 			t.Parallel()
 
-			Lock()
-			defer Unlock()
+			errno.Lock()
+			t.Cleanup(errno.Unlock)
 
-			set(tc.err)
+			errno.Set(tc.err)
 			t.Logf("Checking for error %v", tc.err)
-			require.ErrorIs(t, Get(), tc.err, "Errorno is not matching")
+			require.ErrorIs(t, errno.Get(), tc.err, "Errno is not matching")
 		})
 	}
 }

internal/errno/export_test.go

@@ -0,0 +1,26 @@
+package errno
+
+import "errors"
+
+// Our code is not racing (as these tests prove), but errno is, so we're writing on a
+// variable here to check that the code we have is still race-proof.
+var testErrno int
+
+func init() {
+	getErrno = func() int { return testErrno }
+	unsetErrno = func() { testErrno = 0 }
+}
+
+// Set sets the errno to the err value. It's only used for testing.
+func Set(err error) {
+	if mu.TryLock() {
+		mu.Unlock()
+		panic("Using errno without locking!")
+	}
+
+	var errno Error
+	if err != nil && !errors.As(err, &errno) {
+		panic("Not a valid errno value")
+	}
+	testErrno = int(errno)
+}

pam/integration-tests/vhs-helpers_test.go

@@ -1,6 +1,7 @@
 package main_test
 
 import (
+	"errors"
 	"fmt"
 	"maps"
 	"math"
@@ -245,8 +246,18 @@ func (td tapeData) RunVhs(t *testing.T, testType vhsTestType, outDir string, cli
 		cmd.Env = append(cmd.Env, fmt.Sprintf("%s=%s", pam_test.RunnerEnvLogFile, e))
 	}
 
+	var raceLog string
+	if testutils.IsRace() {
+		raceLog = filepath.Join(t.TempDir(), "gorace.log")
+		cmd.Env = append(cmd.Env, fmt.Sprintf("GORACE=log_path=%s", raceLog))
+		saveArtifactsForDebugOnCleanup(t, []string{raceLog})
+	}
+
 	cmd.Args = append(cmd.Args, td.PrepareTape(t, testType, outDir))
 	out, err := cmd.CombinedOutput()
+	if err != nil {
+		checkDataRace(t, raceLog)
+	}
 
 	isSSHError := func(processOut []byte) bool {
 		const sshConnectionResetByPeer = "Connection reset by peer"
@@ -302,28 +313,46 @@ func (td tapeData) Output() string {
 	return txt
 }
 
-func (td tapeData) ExpectedOutput(t *testing.T, outputDir string) string {
+func checkDataRace(t *testing.T, raceLog string) {
 	t.Helper()
 
-	outPath := filepath.Join(outputDir, td.Output())
-	out, err := os.ReadFile(outPath)
-	require.NoError(t, err, "Could not read output file of tape %q (%s)", td.Name, outPath)
-	got := string(out)
+	if !testutils.IsRace() || raceLog == "" {
+		return
+	}
+
+	content, err := os.ReadFile(raceLog)
+	if err == nil || errors.Is(err, os.ErrNotExist) {
+		return
+	}
+	require.NoError(t, err, "TearDown: Error reading race log %q", raceLog)
+
+	out := string(content)
+	if strings.TrimSpace(out) == "" {
+		return
+	}
 
-	if testutils.IsRace() && strings.Contains(got, "WARNING: DATA RACE") &&
-		strings.Contains(got, "bubbles/cursor.(*Model).BlinkCmd.func1") {
+	if strings.Contains(out, "bubbles/cursor.(*Model).BlinkCmd.func1") {
 		// FIXME: This is a well known race of bubble tea:
 		// https://github.com/charmbracelet/bubbletea/issues/909
 		// We can't do much here, as the workaround will likely affect the
 		// GUI behavior, but we ignore this since it's definitely not our bug.
-		t.Skip("This is a very well known bubble tea bug (#909), ignoring it")
-		if testutils.IsVerbose() {
-			t.Logf("Ignored bubbletea race:\n%s", got)
-		} else {
-			fmt.Fprintf(os.Stderr, "Ignored bubbletea race:\n%s", got)
-		}
+
+		// TODO: In case other races are detected, we should still fail here.
+		t.Skipf("This is a very well known bubble tea bug (#909), ignoring it:\n%s", out)
+		return
 	}
 
+	t.Fatalf("Got a GO Race on vhs child:\n%s", out)
+}
+
+func (td tapeData) ExpectedOutput(t *testing.T, outputDir string) string {
+	t.Helper()
+
+	outPath := filepath.Join(outputDir, td.Output())
+	out, err := os.ReadFile(outPath)
+	require.NoError(t, err, "Could not read output file of tape %q (%s)", td.Name, outPath)
+	got := string(out)
+
 	// We need to format the output a little bit, since the txt file can have some noise at the beginning.
 	command := "> " + td.Command
 	maxCommandLen := 0

pam/tools/pam-runner/pam-runner.go

@@ -62,6 +62,9 @@ func main() {
 	if coverDir := os.Getenv("GOCOVERDIR"); coverDir != "" {
 		defaultArgs = append(defaultArgs, "--exec-env", fmt.Sprintf("GOCOVERDIR=%s", coverDir))
 	}
+	if goRace := os.Getenv("GORACE"); goRace != "" {
+		defaultArgs = append(defaultArgs, "--exec-env", fmt.Sprintf("GORACE=%s", goRace))
+	}
 	if asanOptions := os.Getenv("ASAN_OPTIONS"); asanOptions != "" {
 		defaultArgs = append(defaultArgs, "--exec-env", fmt.Sprintf("ASAN_OPTIONS=%s", asanOptions))
 	}