Move GC tests in PG out of parallel running

This is necessary because GC takes an exclusive lock now

internal/datastore/postgres/postgres_shared_test.go

@@ -81,12 +81,35 @@ func testPostgresDatastore(t *testing.T, pc []postgresConfig) {
 		if config.pgbouncer {
 			pgbouncerStr = "pgbouncer-"
 		}
+
+		t.Run(fmt.Sprintf("%spostgres-%s-%s-%s-gc", pgbouncerStr, config.pgVersion, config.targetMigration, config.migrationPhase), func(t *testing.T) {
+			b := testdatastore.RunPostgresForTesting(t, "", config.targetMigration, config.pgVersion, config.pgbouncer)
+			ctx := context.Background()
+
+			// NOTE: gc tests take exclusive locks, so they are run under non-parallel.
+			test.OnlyGCTests(t, test.DatastoreTesterFunc(func(revisionQuantization, gcInterval, gcWindow time.Duration, watchBufferLength uint16) (datastore.Datastore, error) {
+				ds := b.NewDatastore(t, func(engine, uri string) datastore.Datastore {
+					ds, err := newPostgresDatastore(ctx, uri, primaryInstanceID,
+						RevisionQuantization(revisionQuantization),
+						GCWindow(gcWindow),
+						GCInterval(gcInterval),
+						WatchBufferLength(watchBufferLength),
+						DebugAnalyzeBeforeStatistics(),
+						MigrationPhase(config.migrationPhase),
+					)
+					require.NoError(t, err)
+					return ds
+				})
+				return ds, nil
+			}), false)
+		})
+
 		t.Run(fmt.Sprintf("%spostgres-%s-%s-%s", pgbouncerStr, config.pgVersion, config.targetMigration, config.migrationPhase), func(t *testing.T) {
 			t.Parallel()
 			b := testdatastore.RunPostgresForTesting(t, "", config.targetMigration, config.pgVersion, config.pgbouncer)
 			ctx := context.Background()
 
-			test.All(t, test.DatastoreTesterFunc(func(revisionQuantization, gcInterval, gcWindow time.Duration, watchBufferLength uint16) (datastore.Datastore, error) {
+			test.AllWithExceptions(t, test.DatastoreTesterFunc(func(revisionQuantization, gcInterval, gcWindow time.Duration, watchBufferLength uint16) (datastore.Datastore, error) {
 				ds := b.NewDatastore(t, func(engine, uri string) datastore.Datastore {
 					ds, err := newPostgresDatastore(ctx, uri, primaryInstanceID,
 						RevisionQuantization(revisionQuantization),
@@ -100,7 +123,7 @@ func testPostgresDatastore(t *testing.T, pc []postgresConfig) {
 					return ds
 				})
 				return ds, nil
-			}), false)
+			}), test.WithCategories(test.GCCategory), false)
 
 			t.Run("TransactionTimestamps", createDatastoreTest(
 				b,
@@ -261,6 +284,7 @@ func testPostgresDatastoreWithoutCommitTimestamps(t *testing.T, pc []postgresCon
 			b := testdatastore.RunPostgresForTestingWithCommitTimestamps(t, "", "head", false, pgVersion, enablePgbouncer)
 
 			// NOTE: watch API requires the commit timestamps, so we skip those tests here.
+			// NOTE: gc tests take exclusive locks, so they are run under non-parallel.
 			test.AllWithExceptions(t, test.DatastoreTesterFunc(func(revisionQuantization, gcInterval, gcWindow time.Duration, watchBufferLength uint16) (datastore.Datastore, error) {
 				ds := b.NewDatastore(t, func(engine, uri string) datastore.Datastore {
 					ds, err := newPostgresDatastore(ctx, uri, primaryInstanceID,
@@ -274,7 +298,26 @@ func testPostgresDatastoreWithoutCommitTimestamps(t *testing.T, pc []postgresCon
 					return ds
 				})
 				return ds, nil
-			}), test.WithCategories(test.WatchCategory), false)
+			}), test.WithCategories(test.WatchCategory, test.GCCategory), false)
+		})
+
+		t.Run(fmt.Sprintf("postgres-%s-gc", pgVersion), func(t *testing.T) {
+			ctx := context.Background()
+			b := testdatastore.RunPostgresForTestingWithCommitTimestamps(t, "", "head", false, pgVersion, enablePgbouncer)
+			test.OnlyGCTests(t, test.DatastoreTesterFunc(func(revisionQuantization, gcInterval, gcWindow time.Duration, watchBufferLength uint16) (datastore.Datastore, error) {
+				ds := b.NewDatastore(t, func(engine, uri string) datastore.Datastore {
+					ds, err := newPostgresDatastore(ctx, uri, primaryInstanceID,
+						RevisionQuantization(revisionQuantization),
+						GCWindow(gcWindow),
+						GCInterval(gcInterval),
+						WatchBufferLength(watchBufferLength),
+						DebugAnalyzeBeforeStatistics(),
+					)
+					require.NoError(t, err)
+					return ds
+				})
+				return ds, nil
+			}), false)
 		})
 	}
 }

pkg/datastore/test/datastore.go

@@ -152,8 +152,7 @@ func AllWithExceptions(t *testing.T, tester DatastoreTester, except Categories,
 	t.Run("TestCheckRevisions", runner(tester, CheckRevisionsTest))
 
 	if !except.GC() {
-		t.Run("TestRevisionGC", runner(tester, RevisionGCTest))
-		t.Run("TestInvalidReads", runner(tester, InvalidReadsTest))
+		OnlyGCTests(t, tester, concurrent)
 	}
 
 	t.Run("TestBulkUpload", runner(tester, BulkUploadTest))
@@ -201,6 +200,16 @@ func AllWithExceptions(t *testing.T, tester DatastoreTester, except Categories,
 	t.Run("TestRelationshipCounterOverExpired", runner(tester, RelationshipCounterOverExpiredTest))
 }
 
+func OnlyGCTests(t *testing.T, tester DatastoreTester, concurrent bool) {
+	runner := serial
+	if concurrent {
+		runner = parallel
+	}
+
+	t.Run("TestRevisionGC", runner(tester, RevisionGCTest))
+	t.Run("TestInvalidReads", runner(tester, InvalidReadsTest))
+}
+
 // All runs all generic datastore tests on a DatastoreTester.
 func All(t *testing.T, tester DatastoreTester, concurrent bool) {
 	AllWithExceptions(t, tester, noException, concurrent)

pkg/datastore/test/revisions.go

@@ -131,7 +131,7 @@ func RevisionGCTest(t *testing.T, tester DatastoreTester) {
 	gcable, ok := ds.(common.GarbageCollector)
 	if ok {
 		gcable.ResetGCCompleted()
-		require.Eventually(func() bool { return gcable.HasGCRun() }, 5*time.Second, 50*time.Millisecond, "GC was never run as expected")
+		require.Eventually(func() bool { return gcable.HasGCRun() }, 10*time.Second, 100*time.Millisecond, "GC was never run as expected")
 	}
 
 	// FIXME currently the various datastores behave differently when a revision was requested and GC Window elapses.