Merge #96865

craig[bot] · tbg · craig[bot] · commit 9c41c4874399 · 2023-02-13T12:06:36.000Z
96865: kvserver: deflake TestLearnerReplicateQueueRace r=pavelkalinnikov a=tbg It's not a good idea to `t.Fatal` on a goroutine. I verified that the test fails cleanly even when the callback doesn't actually do the removal. Fixes #95130. Epic: none Release note: None Co-authored-by: Tobias Grieger <tobias.b.grieger@gmail.com>
diff --git a/pkg/kv/kvserver/replica_learner_test.go b/pkg/kv/kvserver/replica_learner_test.go
@@ -46,6 +46,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/tracing/tracingpb"
 	"github.com/cockroachdb/cockroach/pkg/util/uuid"
 	"github.com/cockroachdb/errors"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"golang.org/x/sync/errgroup"
 )
@@ -1269,9 +1270,10 @@ func TestLearnerReplicateQueueRace(t *testing.T) {
 		// leaving the 2 voters.
 		startKey := tc.ScratchRange(t)
 		desc, err := tc.RemoveVoters(startKey, tc.Target(2))
-		require.NoError(t, err)
-		require.Len(t, desc.Replicas().VoterDescriptors(), 2)
-		require.Len(t, desc.Replicas().LearnerDescriptors(), 0)
+		// NB: don't fatal on this goroutine, as we can't recover cleanly.
+		assert.NoError(t, err)
+		assert.Len(t, desc.Replicas().VoterDescriptors(), 2)
+		assert.Len(t, desc.Replicas().LearnerDescriptors(), 0)
 		return false
 	}
 	tc = testcluster.StartTestCluster(t, 3, base.TestClusterArgs{
@@ -1300,10 +1302,8 @@ func TestLearnerReplicateQueueRace(t *testing.T) {
 			if err != nil {
 				return err
 			}
-			if !strings.Contains(processErr.Error(), `descriptor changed`) {
-				// NB: errors.Wrapf(nil, ...) returns nil.
-				// nolint:errwrap
-				return errors.Errorf(`expected "descriptor changed" error got: %+v`, processErr)
+			if processErr == nil || !strings.Contains(processErr.Error(), `descriptor changed`) {
+				return errors.Wrap(processErr, `expected "descriptor changed" error got: %+v`)
 			}
 			formattedTrace := trace.String()
 			expectedMessages := []string{
