From 50704a5014a7429705b8b6922cfe5fddfd10fb47 Mon Sep 17 00:00:00 2001 From: Ethan <39577870+ethanndickson@users.noreply.github.com> Date: Thu, 4 Sep 2025 14:28:29 +1000 Subject: [PATCH] ci: improve 'tfail in goroutine' ruleguard rule (#19682) This PR improves the ruleguard rule for detecting `t.Fail` calls in goroutines. It picks up additional violations, of which are fixed in this PR. See self-review for details. The motivation for fixing this comes from a flake I fixed in https://github.com/coder/coder/pull/19599, where tests would fail from a `require` in an `Eventually`. --- coderd/files_test.go | 3 ++- enterprise/tailnet/pgcoord_test.go | 4 ++-- scaletest/createworkspaces/run_test.go | 2 +- scripts/rules.go | 12 ++++-------- 4 files changed, 9 insertions(+), 12 deletions(-) diff --git a/coderd/files_test.go b/coderd/files_test.go index fb13cb30e4..b7f981d5e5 100644 --- a/coderd/files_test.go +++ b/coderd/files_test.go @@ -9,6 +9,7 @@ import ( "testing" "github.com/google/uuid" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/coder/coder/v2/archive" @@ -88,7 +89,7 @@ func TestPostFiles(t *testing.T) { data := make([]byte, 1024) _, err := client.Upload(ctx, codersdk.ContentTypeTar, bytes.NewReader(data)) end.Done() - require.NoError(t, err) + assert.NoError(t, err) }() } wg.Done() diff --git a/enterprise/tailnet/pgcoord_test.go b/enterprise/tailnet/pgcoord_test.go index 15153c2c40..7923ffdb81 100644 --- a/enterprise/tailnet/pgcoord_test.go +++ b/enterprise/tailnet/pgcoord_test.go @@ -409,8 +409,8 @@ func TestPGCoordinatorSingle_SendsHeartbeats(t *testing.T) { if len(heartbeats) < 2 { return false } - require.Greater(t, heartbeats[0].Sub(start), time.Duration(0)) - require.Greater(t, heartbeats[1].Sub(start), time.Duration(0)) + assert.Greater(t, heartbeats[0].Sub(start), time.Duration(0)) + assert.Greater(t, heartbeats[1].Sub(start), time.Duration(0)) return assert.Greater(t, heartbeats[1].Sub(heartbeats[0]), tailnet.HeartbeatPeriod*3/4) }, testutil.WaitMedium, testutil.IntervalMedium) } diff --git a/scaletest/createworkspaces/run_test.go b/scaletest/createworkspaces/run_test.go index edade6b79e..88992c4226 100644 --- a/scaletest/createworkspaces/run_test.go +++ b/scaletest/createworkspaces/run_test.go @@ -257,7 +257,7 @@ func Test_Runner(t *testing.T) { err := runner.Run(runnerCtx, "1", logs) logsStr := logs.String() t.Log("Runner logs:\n\n" + logsStr) - require.ErrorIs(t, err, context.Canceled) + assert.ErrorIs(t, err, context.Canceled) close(done) }() diff --git a/scripts/rules.go b/scripts/rules.go index dce029a102..7fd3c0ca44 100644 --- a/scripts/rules.go +++ b/scripts/rules.go @@ -182,32 +182,28 @@ func doNotCallTFailNowInsideGoroutine(m dsl.Matcher) { m.Match(` go func($*_){ $*_ - $require.$_($*_) + require.$_($*_) $*_ }($*_)`). - At(m["require"]). - Where(m["require"].Text == "require"). Report("Do not call functions that may call t.FailNow in a goroutine, as this can cause data races (see testing.go:834)") // require.Eventually runs the function in a goroutine. m.Match(` require.Eventually(t, func() bool { $*_ - $require.$_($*_) + require.$_($*_) $*_ }, $*_)`). - At(m["require"]). - Where(m["require"].Text == "require"). Report("Do not call functions that may call t.FailNow in a goroutine, as this can cause data races (see testing.go:834)") m.Match(` go func($*_){ $*_ - $t.$fail($*_) + t.$fail($*_) $*_ }($*_)`). At(m["fail"]). - Where(m["t"].Type.Implements("testing.TB") && m["fail"].Text.Matches("^(FailNow|Fatal|Fatalf)$")). + Where(m["fail"].Text.Matches("^(FailNow|Fatal|Fatalf)$")). Report("Do not call functions that may call t.FailNow in a goroutine, as this can cause data races (see testing.go:834)") }