test: Improve change detector performance (sourcenetwork#1433)

## Relevant issue(s) Resolves sourcenetwork#1186 Partially resolves sourcenetwork#1342 ## Description Improves the change detector performance. Decreases runtime from 17 minutes to 5 minutes on my local machine. It also cleans up the paths used by the change detector (part of sourcenetwork#1342). It does this by changing how the setup-stage is handled, instead of calling `go test` once per test, it now only calls it once per test package - setting up a bunch of test db instances for the entire package.
shahzadlone · May 1, 2023 · ca36af2 · ca36af2
1 parent 47b07d8
commit ca36af2
Show file tree

Hide file tree

Showing 4 changed files with 74 additions and 93 deletions.
diff --git a/docs/data_format_changes/i1186-change-detctor-setup.md b/docs/data_format_changes/i1186-change-detctor-setup.md
@@ -0,0 +1,5 @@
+# Modify change detector setup flow
+
+This is not a breaking (production code) change.
+
+This does however change the way the change detector sets up the target branch in a way that is incompatible with the current develop, this file is required to allow the CI to pass (including comparing develop vs master).
diff --git a/tests/integration/change_detector.go b/tests/integration/change_detector.go
@@ -14,16 +14,18 @@ import (
 	"context"
 	"fmt"
 	"io/fs"
+	"math/rand"
 	"os"
 	"os/exec"
 	"path"
 	"runtime"
 	"strings"
 	"testing"
-
-	"github.com/sourcenetwork/defradb/client"
+	"time"
 )
 
+var skip bool
+
 func IsDetectingDbChanges() bool {
 	return DetectDbChanges
 }
@@ -33,6 +35,10 @@ func DetectDbChangesPreTestChecks(
 	t *testing.T,
 	collectionNames []string,
 ) bool {
+	if skip {
+		t.SkipNow()
+	}
+
 	if previousTestCaseTestName == t.Name() {
 		// The database format changer currently only supports running the first test
 		//  case, if a second case is detected we return early
@@ -64,10 +70,21 @@ func detectDbChangesInit(repository string, targetBranch string) {
 		return
 	}
 
-	tempDir := os.TempDir()
+	defraTempDir := path.Join(os.TempDir(), "defradb")
+	changeDetectorTempDir := path.Join(defraTempDir, "tests", "changeDetector")
 
 	latestTargetCommitHash := getLatestCommit(repository, targetBranch)
-	detectDbChangesCodeDir = path.Join(tempDir, "defra", latestTargetCommitHash, "code")
+	detectDbChangesCodeDir = path.Join(changeDetectorTempDir, "code", latestTargetCommitHash)
+	rand.Seed(time.Now().Unix())
+	randNumber := rand.Int()
+	dbsDir := path.Join(changeDetectorTempDir, "dbs", fmt.Sprint(randNumber))
+
+	testPackagePath, isIntegrationTest := getTestPackagePath()
+	if !isIntegrationTest {
+		skip = true
+		return
+	}
+	rootDatabaseDir = path.Join(dbsDir, strings.ReplaceAll(testPackagePath, "/", "_"))
 
 	_, err := os.Stat(detectDbChangesCodeDir)
 	// Warning - there is a race condition here, where if running multiple packages in
@@ -110,22 +127,12 @@ func detectDbChangesInit(repository string, targetBranch string) {
 	}
 
 	areDatabaseFormatChangesDocumented = checkIfDatabaseFormatChangesAreDocumented()
-}
-
-func SetupDatabaseUsingTargetBranch(
-	ctx context.Context,
-	t *testing.T,
-	collectionNames []string,
-) client.DB {
-	currentTestPackage, err := os.Getwd()
-	if err != nil {
-		panic(err)
+	if areDatabaseFormatChangesDocumented {
+		// Dont bother doing anything if the changes are documented
+		return
 	}
 
-	targetTestPackage := detectDbChangesCodeDir + "/tests/integration/" + strings.Split(
-		currentTestPackage,
-		"/tests/integration/",
-	)[1]
+	targetTestPackage := detectDbChangesCodeDir + "/tests/integration/" + testPackagePath
 
 	// If we are checking for database changes, and we are not seting up the database,
 	// then we must be in the main test process, and need to create a new process
@@ -135,55 +142,41 @@ func SetupDatabaseUsingTargetBranch(
 		"go",
 		"test",
 		"./...",
-		"--run",
-		fmt.Sprintf("^%s$", t.Name()),
 		"-v",
 	)
 
-	path := t.TempDir()
-
 	goTestCmd.Dir = targetTestPackage
 	goTestCmd.Env = os.Environ()
 	goTestCmd.Env = append(
 		goTestCmd.Env,
 		setupOnlyEnvName+"=true",
-		fileBadgerPathEnvName+"="+path,
+		rootDBFilePathEnvName+"="+rootDatabaseDir,
 	)
 	out, err := goTestCmd.Output()
-
 	if err != nil {
-		// If file is not found - this must be a new test and
-		// doesn't exist in the target branch, so we pass it
-		// because the child process tries to run the test, but
-		// if it doesnt find it, the parent test should pass (not panic).
-		if strings.Contains(err.Error(), ": no such file or directory") {
-			t.SkipNow()
-		} else {
-			// Only log the output if there is an error different from above,
-			// logging child test runs confuses the go test runner making it
-			// think there are no tests in the parent run (it will still
-			// run everything though)!
-			log.ErrorE(ctx, string(out), err)
-			panic(err)
-		}
+		log.ErrorE(context.TODO(), string(out), err)
+		panic(err)
 	}
+}
 
-	refreshedDb, err := newBadgerFileDB(ctx, t, path)
+// getTestPackagePath returns the path to the package currently under test, relative
+// to `./tests/integration/`. Will return an empty string and false if the tests
+// are not within that directory.
+func getTestPackagePath() (string, bool) {
+	currentTestPackage, err := os.Getwd()
 	if err != nil {
 		panic(err)
 	}
 
-	_, err = refreshedDb.GetCollectionByName(ctx, collectionNames[0])
-	if err != nil {
-		if err.Error() == "datastore: key not found" {
-			// If collection is not found - this must be a new test and
-			// doesn't exist in the target branch, so we pass it
-			t.SkipNow()
-		} else {
-			panic(err)
-		}
+	splitPath := strings.Split(
+		currentTestPackage,
+		"/tests/integration/",
+	)
+
+	if len(splitPath) != 2 {
+		return "", false
 	}
-	return refreshedDb
+	return splitPath[1], true
 }
 
 func checkIfDatabaseFormatChangesAreDocumented() bool {

diff --git a/tests/integration/explain/utils.go b/tests/integration/explain/utils.go
@@ -154,38 +154,22 @@ func ExecuteExplainRequestTestCase(
 		log.Info(ctx, explainTest.Description, logging.NewKV("Database", dbi.name))
 
 		if testUtils.DetectDbChanges {
-			if testUtils.SetupOnly {
-				setupDatabase(
-					ctx,
-					t,
-					dbi,
-					schema,
-					collectionNames,
-					explainTest.Description,
-					explainTest.ExpectedError,
-					explainTest.Docs,
-					immutable.None[map[int]map[int][]string](),
-				)
-				dbi.db.Close(ctx)
-				return
-			}
-
-			dbi.db.Close(ctx)
-			db = testUtils.SetupDatabaseUsingTargetBranch(ctx, t, collectionNames)
-		} else {
-			setupDatabase(
-				ctx,
-				t,
-				dbi,
-				schema,
-				collectionNames,
-				explainTest.Description,
-				explainTest.ExpectedError,
-				explainTest.Docs,
-				immutable.None[map[int]map[int][]string](),
-			)
+			t.SkipNow()
+			return
 		}
 
+		setupDatabase(
+			ctx,
+			t,
+			dbi,
+			schema,
+			collectionNames,
+			explainTest.Description,
+			explainTest.ExpectedError,
+			explainTest.Docs,
+			immutable.None[map[int]map[int][]string](),
+		)
+
 		result := db.ExecRequest(ctx, explainTest.Request)
 		if assertExplainRequestResults(
 			ctx,

diff --git a/tests/integration/utils2.go b/tests/integration/utils2.go
@@ -14,6 +14,7 @@ import (
 	"context"
 	"fmt"
 	"os"
+	"path"
 	"reflect"
 	"strings"
 	"syscall"
@@ -39,6 +40,7 @@ const (
 	memoryBadgerEnvName        = "DEFRA_BADGER_MEMORY"
 	fileBadgerEnvName          = "DEFRA_BADGER_FILE"
 	fileBadgerPathEnvName      = "DEFRA_BADGER_FILE_PATH"
+	rootDBFilePathEnvName      = "DEFRA_TEST_ROOT"
 	inMemoryEnvName            = "DEFRA_IN_MEMORY"
 	setupOnlyEnvName           = "DEFRA_SETUP_ONLY"
 	detectDbChangesEnvName     = "DEFRA_DETECT_DATABASE_CHANGES"
@@ -78,6 +80,7 @@ var (
 const subscriptionTimeout = 1 * time.Second
 
 var databaseDir string
+var rootDatabaseDir string
 
 /*
 If this is set to true the integration test suite will instead of its normal profile do
@@ -111,6 +114,7 @@ func init() {
 	badgerFileValue, _ := os.LookupEnv(fileBadgerEnvName)
 	badgerInMemoryValue, _ := os.LookupEnv(memoryBadgerEnvName)
 	databaseDir, _ = os.LookupEnv(fileBadgerPathEnvName)
+	rootDatabaseDir, _ = os.LookupEnv(rootDBFilePathEnvName)
 	detectDbChangesValue, _ := os.LookupEnv(detectDbChangesEnvName)
 	inMemoryStoreValue, _ := os.LookupEnv(inMemoryEnvName)
 	repositoryValue, repositorySpecified := os.LookupEnv(repositoryEnvName)
@@ -195,14 +199,16 @@ func NewInMemoryDB(ctx context.Context) (client.DB, error) {
 }
 
 func NewBadgerFileDB(ctx context.Context, t testing.TB) (client.DB, error) {
-	var path string
-	if databaseDir == "" {
-		path = t.TempDir()
+	var dbPath string
+	if databaseDir != "" {
+		dbPath = databaseDir
+	} else if rootDatabaseDir != "" {
+		dbPath = path.Join(rootDatabaseDir, t.Name())
 	} else {
-		path = databaseDir
+		dbPath = t.TempDir()
 	}
 
-	return newBadgerFileDB(ctx, t, path)
+	return newBadgerFileDB(ctx, t, dbPath)
 }
 
 func newBadgerFileDB(ctx context.Context, t testing.TB, path string) (client.DB, error) {
@@ -571,15 +577,8 @@ func getStartingNodes(
 
 	// If nodes have not been explicitly configured via actions, setup a default one.
 	if !hasExplicitNode {
-		var db client.DB
-		if DetectDbChanges && !SetupOnly {
-			// Setup the database using the target branch, and then refresh the current instance
-			db = SetupDatabaseUsingTargetBranch(ctx, t, collectionNames)
-		} else {
-			var err error
-			db, err = GetDatabase(ctx, t, dbt)
-			require.Nil(t, err)
-		}
+		db, err := GetDatabase(ctx, t, dbt)
+		require.Nil(t, err)
 
 		return []*node.Node{
 			{