use golangci-lint v2.1.6

[bgentry]

Fix missing t.Parallel() calls and disable funcorder for now (it wants to reorder tons of code).

[brandur]

I'm sure you saw the data race failures, but I think we were cheating a bit on these subtests by sharing the same transaction between all these test cases. Use of a tx across goroutines is obviously not safe and that's what was failing these all so hard.

Here's a patch for a fix if you want to use it:

diff --git a/rivertest/rivertest_test.go b/rivertest/rivertest_test.go
index a2182d6..aa19097 100644
--- a/rivertest/rivertest_test.go
+++ b/rivertest/rivertest_test.go
@@ -209,21 +209,20 @@ func TestRequireInsertedTx(t *testing.T) {
 	t.Run("InsertOpts", func(t *testing.T) {
 		t.Parallel()
 
-		riverClient, bundle := setup(t)
-
-		// Verify custom insertion options.
-		insertRes, err := riverClient.InsertTx(ctx, bundle.tx, Job2Args{Int: 123}, &river.InsertOpts{
-			MaxAttempts: 78,
-			Priority:    2,
-			Queue:       "another_queue",
-			ScheduledAt: testTime,
-			Tags:        []string{"tag1"},
-		})
-		require.NoError(t, err)
-		job := insertRes.Job
-
 		emptyOpts := func() *RequireInsertedOpts { return &RequireInsertedOpts{} }
 
+		insertJob := func(riverClient *river.Client[pgx.Tx], bundle *testBundle) *rivertype.JobRow {
+			insertRes, err := riverClient.InsertTx(ctx, bundle.tx, Job2Args{Int: 123}, &river.InsertOpts{
+				MaxAttempts: 78,
+				Priority:    2,
+				Queue:       "another_queue",
+				ScheduledAt: testTime,
+				Tags:        []string{"tag1"},
+			})
+			require.NoError(t, err)
+			return insertRes.Job
+		}
+
 		sameOpts := func() *RequireInsertedOpts {
 			return &RequireInsertedOpts{
 				MaxAttempts: 78,
@@ -238,6 +237,10 @@ func TestRequireInsertedTx(t *testing.T) {
 		t.Run("MaxAttempts", func(t *testing.T) {
 			t.Parallel()
 
+			riverClient, bundle := setup(t)
+
+			_ = insertJob(riverClient, bundle)
+
 			mockT := testutil.NewMockT(t)
 			opts := sameOpts()
 			opts.MaxAttempts = 77
@@ -251,6 +254,10 @@ func TestRequireInsertedTx(t *testing.T) {
 		t.Run("Priority", func(t *testing.T) {
 			t.Parallel()
 
+			riverClient, bundle := setup(t)
+
+			_ = insertJob(riverClient, bundle)
+
 			mockT := testutil.NewMockT(t)
 			opts := sameOpts()
 			opts.Priority = 3
@@ -264,6 +271,10 @@ func TestRequireInsertedTx(t *testing.T) {
 		t.Run("Queue", func(t *testing.T) {
 			t.Parallel()
 
+			riverClient, bundle := setup(t)
+
+			_ = insertJob(riverClient, bundle)
+
 			mockT := testutil.NewMockT(t)
 			opts := sameOpts()
 			opts.Queue = "wrong_queue"
@@ -277,6 +288,10 @@ func TestRequireInsertedTx(t *testing.T) {
 		t.Run("ScheduledAt", func(t *testing.T) {
 			t.Parallel()
 
+			riverClient, bundle := setup(t)
+
+			_ = insertJob(riverClient, bundle)
+
 			mockT := testutil.NewMockT(t)
 			opts := sameOpts()
 			opts.ScheduledAt = testTime.Add(3*time.Minute + 23*time.Second + 123*time.Microsecond)
@@ -290,6 +305,10 @@ func TestRequireInsertedTx(t *testing.T) {
 		t.Run("State", func(t *testing.T) {
 			t.Parallel()
 
+			riverClient, bundle := setup(t)
+
+			_ = insertJob(riverClient, bundle)
+
 			mockT := testutil.NewMockT(t)
 			opts := sameOpts()
 			opts.State = rivertype.JobStateCancelled
@@ -303,6 +322,10 @@ func TestRequireInsertedTx(t *testing.T) {
 		t.Run("Tags", func(t *testing.T) {
 			t.Parallel()
 
+			riverClient, bundle := setup(t)
+
+			_ = insertJob(riverClient, bundle)
+
 			mockT := testutil.NewMockT(t)
 			opts := sameOpts()
 			opts.Tags = []string{"tag2"}
@@ -316,6 +339,10 @@ func TestRequireInsertedTx(t *testing.T) {
 		t.Run("MultiplePropertiesSucceed", func(t *testing.T) {
 			t.Parallel()
 
+			riverClient, bundle := setup(t)
+
+			job := insertJob(riverClient, bundle)
+
 			mockT := testutil.NewMockT(t)
 			opts := emptyOpts()
 			opts.MaxAttempts = job.MaxAttempts
@@ -327,6 +354,10 @@ func TestRequireInsertedTx(t *testing.T) {
 		t.Run("MultiplePropertiesFails", func(t *testing.T) {
 			t.Parallel()
 
+			riverClient, bundle := setup(t)
+
+			_ = insertJob(riverClient, bundle)
+
 			mockT := testutil.NewMockT(t)
 			opts := sameOpts()
 			opts.MaxAttempts = 77
@@ -341,6 +372,10 @@ func TestRequireInsertedTx(t *testing.T) {
 		t.Run("AllSameSucceeds", func(t *testing.T) {
 			t.Parallel()
 
+			riverClient, bundle := setup(t)
+
+			_ = insertJob(riverClient, bundle)
+
 			mockT := testutil.NewMockT(t)
 			opts := sameOpts()
 			requireInsertedTx[*riverpgxv5.Driver](ctx, mockT, bundle.tx, emptySchema, &Job2Args{}, opts)
@@ -506,21 +541,20 @@ func TestRequireNotInsertedTx(t *testing.T) {
 	t.Run("InsertOpts", func(t *testing.T) {
 		t.Parallel()
 
-		riverClient, bundle := setup(t)
-
-		// Verify custom insertion options.
-		insertRes, err := riverClient.InsertTx(ctx, bundle.tx, Job2Args{Int: 123}, &river.InsertOpts{
-			MaxAttempts: 78,
-			Priority:    2,
-			Queue:       "another_queue",
-			ScheduledAt: testTime,
-			Tags:        []string{"tag1"},
-		})
-		require.NoError(t, err)
-		job := insertRes.Job
-
 		emptyOpts := func() *RequireInsertedOpts { return &RequireInsertedOpts{} }
 
+		insertJob := func(riverClient *river.Client[pgx.Tx], bundle *testBundle) *rivertype.JobRow {
+			insertRes, err := riverClient.InsertTx(ctx, bundle.tx, Job2Args{Int: 123}, &river.InsertOpts{
+				MaxAttempts: 78,
+				Priority:    2,
+				Queue:       "another_queue",
+				ScheduledAt: testTime,
+				Tags:        []string{"tag1"},
+			})
+			require.NoError(t, err)
+			return insertRes.Job
+		}
+
 		sameOpts := func() *RequireInsertedOpts {
 			return &RequireInsertedOpts{
 				MaxAttempts: 78,
@@ -535,6 +569,10 @@ func TestRequireNotInsertedTx(t *testing.T) {
 		t.Run("MaxAttempts", func(t *testing.T) {
 			t.Parallel()
 
+			riverClient, bundle := setup(t)
+
+			job := insertJob(riverClient, bundle)
+
 			mockT := testutil.NewMockT(t)
 			opts := emptyOpts()
 			opts.MaxAttempts = job.MaxAttempts
@@ -548,6 +586,10 @@ func TestRequireNotInsertedTx(t *testing.T) {
 		t.Run("Priority", func(t *testing.T) {
 			t.Parallel()
 
+			riverClient, bundle := setup(t)
+
+			job := insertJob(riverClient, bundle)
+
 			mockT := testutil.NewMockT(t)
 			opts := emptyOpts()
 			opts.Priority = job.Priority
@@ -561,6 +603,10 @@ func TestRequireNotInsertedTx(t *testing.T) {
 		t.Run("Queue", func(t *testing.T) {
 			t.Parallel()
 
+			riverClient, bundle := setup(t)
+
+			job := insertJob(riverClient, bundle)
+
 			mockT := testutil.NewMockT(t)
 			opts := emptyOpts()
 			opts.Queue = job.Queue
@@ -574,6 +620,10 @@ func TestRequireNotInsertedTx(t *testing.T) {
 		t.Run("ScheduledAt", func(t *testing.T) {
 			t.Parallel()
 
+			riverClient, bundle := setup(t)
+
+			job := insertJob(riverClient, bundle)
+
 			mockT := testutil.NewMockT(t)
 			opts := emptyOpts()
 			opts.ScheduledAt = job.ScheduledAt
@@ -587,6 +637,10 @@ func TestRequireNotInsertedTx(t *testing.T) {
 		t.Run("State", func(t *testing.T) {
 			t.Parallel()
 
+			riverClient, bundle := setup(t)
+
+			job := insertJob(riverClient, bundle)
+
 			mockT := testutil.NewMockT(t)
 			opts := emptyOpts()
 			opts.State = job.State
@@ -600,6 +654,10 @@ func TestRequireNotInsertedTx(t *testing.T) {
 		t.Run("Tags", func(t *testing.T) {
 			t.Parallel()
 
+			riverClient, bundle := setup(t)
+
+			job := insertJob(riverClient, bundle)
+
 			mockT := testutil.NewMockT(t)
 			opts := emptyOpts()
 			opts.Tags = job.Tags
@@ -613,6 +671,10 @@ func TestRequireNotInsertedTx(t *testing.T) {
 		t.Run("MultiplePropertiesSucceed", func(t *testing.T) {
 			t.Parallel()
 
+			riverClient, bundle := setup(t)
+
+			job := insertJob(riverClient, bundle)
+
 			mockT := testutil.NewMockT(t)
 			opts := emptyOpts()
 			opts.MaxAttempts = job.MaxAttempts // one property matches job, but the other does not
@@ -624,6 +686,10 @@ func TestRequireNotInsertedTx(t *testing.T) {
 		t.Run("MultiplePropertiesFail", func(t *testing.T) {
 			t.Parallel()
 
+			riverClient, bundle := setup(t)
+
+			job := insertJob(riverClient, bundle)
+
 			mockT := testutil.NewMockT(t)
 			opts := emptyOpts()
 			opts.MaxAttempts = job.MaxAttempts
@@ -638,6 +704,10 @@ func TestRequireNotInsertedTx(t *testing.T) {
 		t.Run("AllSameFails", func(t *testing.T) {
 			t.Parallel()
 
+			riverClient, bundle := setup(t)
+
+			job := insertJob(riverClient, bundle)
+
 			mockT := testutil.NewMockT(t)
 			opts := sameOpts()
 			requireNotInsertedTx[*riverpgxv5.Driver](ctx, mockT, bundle.tx, emptySchema, &Job2Args{}, opts)
@@ -650,6 +720,10 @@ func TestRequireNotInsertedTx(t *testing.T) {
 		t.Run("FailsWithTooManyInserts", func(t *testing.T) {
 			t.Parallel()
 
+			riverClient, bundle := setup(t)
+
+			_ = insertJob(riverClient, bundle)
+
 			_, err := riverClient.InsertTx(ctx, bundle.tx, Job2Args{Int: 123}, &river.InsertOpts{
 				Priority: 3,
 			})

[bgentry]

Thanks, didn't even run the tests after making the changes or this would have been obvious 😆

Had to lint disable a couple cases which were intentionally reusing a schema variable between parent and subtest as part of what was actually being tested.

[brandur]

I'm glad that the linter is checking for t.Parallel() on those sub-sub-test cases now. I noticed before that some were missing and have been fixing them on a case by case basis as I find them, but there's no way we'll get 'em all without a tool to help like this.