From d376de5901daf63ce52b226fedc3d1c8647976e4 Mon Sep 17 00:00:00 2001 From: Nathan VanBenschoten Date: Thu, 10 Aug 2023 19:27:56 -0400 Subject: [PATCH] kv: introduce isolation.RunEachLevel test utility This commit introduces a new `isolation.RunEachLevel` test utility, which runs a subtest for each isolation level. This is similar to the `testutils.RunTrueAndFalse` test utility, but for isolation levels. It avoids some boiler plate code and an indentation for tests that can use the helper. The commit also uses the new utility in a few places. While doing so, it fixes a few tests that were incorrectly handling subtests by failing to plumb the `t` parameter through to the subtest. Epic: None Release note: None --- .../kvcoord/dist_sender_server_test.go | 8 +- pkg/kv/kvclient/kvcoord/txn_test.go | 76 +++++++++---------- .../kvserver/concurrency/isolation/levels.go | 13 ++++ 3 files changed, 50 insertions(+), 47 deletions(-) diff --git a/pkg/kv/kvclient/kvcoord/dist_sender_server_test.go b/pkg/kv/kvclient/kvcoord/dist_sender_server_test.go index e475f4bf7a5e..965611c3359c 100644 --- a/pkg/kv/kvclient/kvcoord/dist_sender_server_test.go +++ b/pkg/kv/kvclient/kvcoord/dist_sender_server_test.go @@ -3938,11 +3938,9 @@ func TestTxnCoordSenderRetries(t *testing.T) { } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - for _, isoLevel := range isolation.Levels() { - t.Run(isoLevel.String(), func(t *testing.T) { - run(t, tc, isoLevel) - }) - } + isolation.RunEachLevel(t, func(t *testing.T, isoLevel isolation.Level) { + run(t, tc, isoLevel) + }) }) } } diff --git a/pkg/kv/kvclient/kvcoord/txn_test.go b/pkg/kv/kvclient/kvcoord/txn_test.go index 7f47c63101f8..4bda5c83993b 100644 --- a/pkg/kv/kvclient/kvcoord/txn_test.go +++ b/pkg/kv/kvclient/kvcoord/txn_test.go @@ -145,7 +145,7 @@ func TestTxnLostIncrement(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - run := func(isoLevel isolation.Level, commitInBatch bool) { + run := func(t *testing.T, isoLevel isolation.Level, commitInBatch bool) { s := createTestDB(t) defer s.Stop() ctx := context.Background() @@ -208,13 +208,11 @@ func TestTxnLostIncrement(t *testing.T) { require.NoError(t, err) } - for _, isoLevel := range isolation.Levels() { - t.Run(isoLevel.String(), func(t *testing.T) { - testutils.RunTrueAndFalse(t, "commitInBatch", func(t *testing.T, commitInBatch bool) { - run(isoLevel, commitInBatch) - }) + isolation.RunEachLevel(t, func(t *testing.T, isoLevel isolation.Level) { + testutils.RunTrueAndFalse(t, "commitInBatch", func(t *testing.T, commitInBatch bool) { + run(t, isoLevel, commitInBatch) }) - } + }) } // TestTxnLostUpdate verifies that transactions are not susceptible to the @@ -233,7 +231,7 @@ func TestTxnLostUpdate(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - run := func(isoLevel isolation.Level, commitInBatch bool) { + run := func(t *testing.T, isoLevel isolation.Level, commitInBatch bool) { s := createTestDB(t) defer s.Stop() ctx := context.Background() @@ -308,13 +306,11 @@ func TestTxnLostUpdate(t *testing.T) { require.Equal(t, []byte(expVal), gr.ValueBytes()) } - for _, isoLevel := range isolation.Levels() { - t.Run(isoLevel.String(), func(t *testing.T) { - testutils.RunTrueAndFalse(t, "commitInBatch", func(t *testing.T, commitInBatch bool) { - run(isoLevel, commitInBatch) - }) + isolation.RunEachLevel(t, func(t *testing.T, isoLevel isolation.Level) { + testutils.RunTrueAndFalse(t, "commitInBatch", func(t *testing.T, commitInBatch bool) { + run(t, isoLevel, commitInBatch) }) - } + }) } // TestTxnWeakIsolationLevelsTolerateWriteSkew verifies that transactions run @@ -325,7 +321,7 @@ func TestTxnWeakIsolationLevelsTolerateWriteSkew(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - run := func(isoLevel isolation.Level) { + run := func(t *testing.T, isoLevel isolation.Level) { s := createTestDB(t) defer s.Stop() ctx := context.Background() @@ -373,9 +369,7 @@ func TestTxnWeakIsolationLevelsTolerateWriteSkew(t *testing.T) { } } - for _, isoLevel := range isolation.Levels() { - t.Run(isoLevel.String(), func(t *testing.T) { run(isoLevel) }) - } + isolation.RunEachLevel(t, run) } // TestTxnReadCommittedPerStatementReadSnapshot verifies that transactions run @@ -387,7 +381,7 @@ func TestTxnReadCommittedPerStatementReadSnapshot(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - run := func(isoLevel isolation.Level, mode kv.SteppingMode, step bool, expObserveExternalWrites bool) { + run := func(t *testing.T, isoLevel isolation.Level, mode kv.SteppingMode, step, expObserveExternalWrites bool) { s := createTestDB(t) defer s.Stop() ctx := context.Background() @@ -441,29 +435,27 @@ func TestTxnReadCommittedPerStatementReadSnapshot(t *testing.T) { require.Equal(t, expVals, readVals) } - for _, isoLevel := range isolation.Levels() { - t.Run(isoLevel.String(), func(t *testing.T) { - testutils.RunTrueAndFalse(t, "steppingMode", func(t *testing.T, modeBool bool) { - mode := kv.SteppingMode(modeBool) - if mode == kv.SteppingEnabled { - // If stepping is enabled, run a variant of the test where the - // transaction is stepped between reads and a variant of the test - // where it is not. - testutils.RunTrueAndFalse(t, "step", func(t *testing.T, step bool) { - // Expect a new read snapshot on each kv operation if the - // transaction is read committed and is manually stepped. - expObserveExternalWrites := isoLevel == isolation.ReadCommitted && step - run(isoLevel, mode, step, expObserveExternalWrites) - }) - } else { + isolation.RunEachLevel(t, func(t *testing.T, isoLevel isolation.Level) { + testutils.RunTrueAndFalse(t, "steppingMode", func(t *testing.T, modeBool bool) { + mode := kv.SteppingMode(modeBool) + if mode == kv.SteppingEnabled { + // If stepping is enabled, run a variant of the test where the + // transaction is stepped between reads and a variant of the test + // where it is not. + testutils.RunTrueAndFalse(t, "step", func(t *testing.T, step bool) { // Expect a new read snapshot on each kv operation if the - // transaction is read committed. - expObserveExternalWrites := isoLevel == isolation.ReadCommitted - run(isoLevel, mode, false, expObserveExternalWrites) - } - }) + // transaction is read committed and is manually stepped. + expObserveExternalWrites := isoLevel == isolation.ReadCommitted && step + run(t, isoLevel, mode, step, expObserveExternalWrites) + }) + } else { + // Expect a new read snapshot on each kv operation if the + // transaction is read committed. + expObserveExternalWrites := isoLevel == isolation.ReadCommitted + run(t, isoLevel, mode, false, expObserveExternalWrites) + } }) - } + }) } // TestTxnWriteReadConflict verifies that write-read conflicts are non-blocking @@ -476,7 +468,7 @@ func TestTxnWriteReadConflict(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - run := func(writeIsoLevel, readIsoLevel isolation.Level) { + run := func(t *testing.T, writeIsoLevel, readIsoLevel isolation.Level) { s := createTestDB(t) defer s.Stop() ctx := context.Background() @@ -516,7 +508,7 @@ func TestTxnWriteReadConflict(t *testing.T) { for _, writeIsoLevel := range isolation.Levels() { for _, readIsoLevel := range isolation.Levels() { name := fmt.Sprintf("writeIso=%s,readIso=%s", writeIsoLevel, readIsoLevel) - t.Run(name, func(t *testing.T) { run(writeIsoLevel, readIsoLevel) }) + t.Run(name, func(t *testing.T) { run(t, writeIsoLevel, readIsoLevel) }) } } } diff --git a/pkg/kv/kvserver/concurrency/isolation/levels.go b/pkg/kv/kvserver/concurrency/isolation/levels.go index b570a93fbea3..8c43c2828af2 100644 --- a/pkg/kv/kvserver/concurrency/isolation/levels.go +++ b/pkg/kv/kvserver/concurrency/isolation/levels.go @@ -69,3 +69,16 @@ func (Level) SafeValue() {} // Levels returns a list of all isolation levels, ordered from strongest to // weakest. func Levels() []Level { return []Level{Serializable, Snapshot, ReadCommitted} } + +// RunEachLevel calls f in a subtest for each isolation level. +func RunEachLevel[T testingTB[T]](t T, f func(T, Level)) { + for _, l := range Levels() { + t.Run(l.String(), func(t T) { f(t, l) }) + } +} + +// testingTB is an interface that matches *testing.T and *testing.B, without +// incurring the package dependency. +type testingTB[T any] interface { + Run(name string, f func(t T)) bool +}