Merge pull request #2165 from josephschorr/gc-test-fix

Change GC test to always call GC directly

internal/datastore/postgres/postgres_shared_test.go

@@ -798,9 +798,10 @@ func QuantizedRevisionTest(t *testing.T, b testdatastore.RunningEngineForTest) {
 			ds := b.NewDatastore(t, func(engine, uri string) datastore.Datastore {
 				var err error
 				conn, err = pgx.Connect(ctx, uri)
-				RegisterTypes(conn.TypeMap())
 				require.NoError(err)
 
+				RegisterTypes(conn.TypeMap())
+
 				ds, err := newPostgresDatastore(
 					ctx,
 					uri,

pkg/datastore/test/datastore.go

@@ -208,6 +208,7 @@ func OnlyGCTests(t *testing.T, tester DatastoreTester, concurrent bool) {
 
 	t.Run("TestRevisionGC", runner(tester, RevisionGCTest))
 	t.Run("TestInvalidReads", runner(tester, InvalidReadsTest))
+	t.Run("TestGCProcessRuns", runner(tester, GCProcessRunTest))
 }
 
 // All runs all generic datastore tests on a DatastoreTester.

pkg/datastore/test/revisions.go

@@ -92,12 +92,58 @@ func RevisionSerializationTest(t *testing.T, tester DatastoreTester) {
 	require.NoError(meta.Validate())
 }
 
+// GCProcessRunTest tests whether the custom GC process runs for the datastore.
+// For datastores that do not have custom GC processes, will no-op.
+func GCProcessRunTest(t *testing.T, tester DatastoreTester) {
+	require := require.New(t)
+	gcWindow := 300 * time.Millisecond
+	gcInterval := 500 * time.Millisecond
+
+	ds, err := tester.New(0, gcInterval, gcWindow, 1)
+	require.NoError(err)
+
+	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+	defer cancel()
+
+	testCaveat := createCoreCaveat(t)
+	_, err = ds.ReadWriteTx(ctx, func(ctx context.Context, rwt datastore.ReadWriteTransaction) error {
+		if err := rwt.WriteNamespaces(ctx, ns.Namespace("foo/createdtxgc")); err != nil {
+			return err
+		}
+		return rwt.WriteCaveats(ctx, []*core.CaveatDefinition{
+			testCaveat,
+		})
+	})
+	require.NoError(err)
+
+	_, err = ds.ReadWriteTx(ctx, func(ctx context.Context, rwt datastore.ReadWriteTransaction) error {
+		return rwt.WriteNamespaces(ctx, testNamespace)
+	})
+	require.NoError(err)
+
+	gcable, ok := ds.(common.GarbageCollector)
+	if !ok {
+		return
+	}
+
+	// Reset that GC was run.
+	gcable.ResetGCCompleted()
+
+	// Wait the GC interval + a bit more time.
+	time.Sleep(gcInterval + 100*time.Millisecond)
+
+	// Ensure GC was run.
+	require.True(gcable.HasGCRun(), "GC was never run as expected")
+}
+
 // RevisionGCTest makes sure revision GC takes place, revisions out-side of the GC window
 // are invalid, and revisions inside the GC window are valid.
 func RevisionGCTest(t *testing.T, tester DatastoreTester) {
 	require := require.New(t)
+	gcWindow := 300 * time.Millisecond
 
-	ds, err := tester.New(0, 10*time.Millisecond, 300*time.Millisecond, 1)
+	// NOTE: we disable the background GC process here and instead manually run it below.
+	ds, err := tester.New(0, veryLargeGCInterval, gcWindow, 1)
 	require.NoError(err)
 
 	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
@@ -125,13 +171,16 @@ func RevisionGCTest(t *testing.T, tester DatastoreTester) {
 	require.NoError(err)
 	require.NoError(ds.CheckRevision(ctx, head), "expected head revision to be valid in GC Window")
 
-	// Make sure GC kicks in after the window.
-	time.Sleep(300 * time.Millisecond)
+	// Sleep to ensure we're past the GC window.
+	time.Sleep(gcWindow)
 
 	gcable, ok := ds.(common.GarbageCollector)
 	if ok {
+		// Run garbage collection.
 		gcable.ResetGCCompleted()
-		require.Eventually(func() bool { return gcable.HasGCRun() }, 10*time.Second, 150*time.Millisecond, "GC was never run as expected")
+		err := common.RunGarbageCollection(gcable, gcWindow, 10*time.Second)
+		require.NoError(err)
+		require.True(gcable.HasGCRun(), "GC was never run as expected")
 	}
 
 	// FIXME currently the various datastores behave differently when a revision was requested and GC Window elapses.