Tests: Refine isolated test-folder cleanup and logging photoprism#5263

Author: lastzero

internal/config/test.go

@@ -19,13 +19,15 @@ import (
 	_ "github.com/jinzhu/gorm/dialects/sqlite"
 
 	"github.com/photoprism/photoprism/internal/config/customize"
+	"github.com/photoprism/photoprism/internal/event"
 	"github.com/photoprism/photoprism/internal/service/hub"
 	"github.com/photoprism/photoprism/internal/thumb"
 	"github.com/photoprism/photoprism/pkg/authn"
 	"github.com/photoprism/photoprism/pkg/capture"
 	"github.com/photoprism/photoprism/pkg/clean"
 	"github.com/photoprism/photoprism/pkg/dsn"
 	"github.com/photoprism/photoprism/pkg/fs"
+	"github.com/photoprism/photoprism/pkg/log/status"
 	"github.com/photoprism/photoprism/pkg/rnd"
 	"github.com/photoprism/photoprism/pkg/txt/report"
 )
@@ -337,8 +339,9 @@ func NewTestConfig(dbName string) *Config {
 
 	var tp string
 	var err error
+
 	if tp, err = os.MkdirTemp(storagePath, "test-photoprism-*"); err != nil {
-		log.Fatalf("config: %s", err.Error())
+		log.Panicf("config: %s", clean.Error(err))
 	}
 
 	tp = filepath.Join(tp, fs.TestdataDir)
@@ -352,22 +355,22 @@ func NewTestConfig(dbName string) *Config {
 
 	s := customize.NewSettings(c.DefaultTheme(), c.DefaultLocale(), c.DefaultTimezone().String())
 
-	if err := fs.MkdirAll(c.ConfigPath()); err != nil {
-		log.Panicf("config: %s", err.Error())
+	if err = fs.MkdirAll(c.ConfigPath()); err != nil {
+		log.Panicf("config: %s", clean.Error(err))
 	}
 
 	// Save settings next to the test config path, reusing any existing
 	// `.yaml`/`.yml` variant so the tests mirror production behavior.
-	if err := s.Save(fs.ConfigFilePath(c.ConfigPath(), "settings", fs.ExtYml)); err != nil {
-		log.Panicf("config: %s", err.Error())
+	if err = s.Save(fs.ConfigFilePath(c.ConfigPath(), "settings", fs.ExtYml)); err != nil {
+		log.Panicf("config: %s", clean.Error(err))
 	}
 
-	if err := c.Init(); err != nil {
-		log.Panicf("config: %s", err.Error())
+	if err = c.Init(); err != nil {
+		log.Panicf("config: %s", clean.Error(err))
 	}
 
-	if err := c.InitializeTestData(); err != nil {
-		log.Errorf("config: %s", err.Error())
+	if err = c.InitializeTestData(); err != nil {
+		log.Errorf("config: %s", clean.Error(err))
 	}
 
 	c.RegisterDb()
@@ -635,20 +638,28 @@ func (c *Config) AssertTestData(t *testing.T) {
 	}
 }
 
-// CleanupTestFolder uses RemoveAll to remove the storage path above testdata.
+// CleanupTestFolder removes the isolated storage directory created by NewTestConfig.
+//
+// It only deletes paths matching the isolated layout "test-photoprism-*/testdata" so a
+// misconfigured StoragePath can never remove a real storage directory. A failed removal
+// is logged as a warning rather than aborting, so a teardown hiccup does not turn a
+// passing test run into a hard exit.
 func (c *Config) CleanupTestFolder() {
 	if c.options == nil {
-		log.Warn("config: c.options is nil in CleanupTestFolder")
+		event.SystemWarn([]string{"config", "test", "c.options is nil in CleanupTestFolder"})
 		return
 	}
+
 	td := c.StoragePath()
-	if strings.HasSuffix(td, "/testdata") && strings.Contains(td, "test-photoprism") {
-		td = strings.TrimSuffix(td, "/testdata")
-		if err := os.RemoveAll(td); err != nil {
-			log.Fatalf("config: %s (cleantestfolder)", err.Error())
+	parent := filepath.Dir(td)
+
+	if filepath.Base(td) == fs.TestdataDir && strings.HasPrefix(filepath.Base(parent), "test-photoprism") {
+		if err := os.RemoveAll(parent); err != nil {
+			event.SystemWarn([]string{"config", "test", "cleanup %s", "%s"}, parent, clean.Error(err))
+			return
 		}
-		log.Debugf("config: cleaned up %s", td)
+		event.SystemDebug([]string{"config", "test", "cleanup %s", status.Succeeded}, parent)
 	} else {
-		log.Warnf("config: %s not cleaned up", td)
+		event.SystemWarn([]string{"config", "test", "cleanup %s", "failed"}, td)
 	}
 }

internal/config/test_test.go

@@ -2,13 +2,14 @@ package config
 
 import (
 	"bytes"
-	"os"
 	"testing"
 
 	"github.com/jinzhu/gorm"
+	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
 	"github.com/urfave/cli/v2"
 
+	"github.com/photoprism/photoprism/internal/event"
 	"github.com/photoprism/photoprism/pkg/fs"
 )
 
@@ -60,46 +61,46 @@ func TestNewTestErrorConfig(t *testing.T) {
 	assert.IsType(t, &gorm.DB{}, db)
 }
 
+// captureSystemLog swaps event.SystemLog for a buffer-backed logger, runs fn, and
+// returns what fn wrote to the system log. CleanupTestFolder logs via event.System*
+// (not the DB-hooked default logger), so tests must read the system log to see it.
+func captureSystemLog(t *testing.T, fn func()) string {
+	t.Helper()
+	var buf bytes.Buffer
+	logger := logrus.New()
+	logger.SetOutput(&buf)
+	logger.SetLevel(logrus.DebugLevel)
+	orig := event.SystemLog
+	event.SystemLog = logger
+	t.Cleanup(func() { event.SystemLog = orig })
+	fn()
+	return buf.String()
+}
+
 func TestCleanupTestFolder(t *testing.T) {
 	t.Run("OptionsNil", func(t *testing.T) {
-		// Setup and capture log output
-		buffer := bytes.Buffer{}
-		log.SetOutput(&buffer)
-
 		var c Config
-		c.CleanupTestFolder()
-
-		// Reset logger
-		log.SetOutput(os.Stdout)
-
-		assert.Contains(t, buffer.String(), "config: c.options is nil in CleanupTestFolder")
+		out := captureSystemLog(t, c.CleanupTestFolder)
+		assert.Contains(t, out, "c.options is nil in CleanupTestFolder")
 	})
-
 	t.Run("NotExpectedPath", func(t *testing.T) {
-		// Setup and capture log output
-		buffer := bytes.Buffer{}
-		log.SetOutput(&buffer)
-
 		c := Config{options: &Options{StoragePath: "/tmp/photoprism/testdata"}}
-		c.CleanupTestFolder()
-
-		// Reset logger
-		log.SetOutput(os.Stdout)
-
-		assert.Contains(t, buffer.String(), "config: /tmp/photoprism/testdata not cleaned up")
+		out := captureSystemLog(t, c.CleanupTestFolder)
+		assert.Contains(t, out, "cleanup /tmp/photoprism/testdata")
+		assert.Contains(t, out, "failed")
 	})
-
 	t.Run("Success", func(t *testing.T) {
-		// Setup and capture log output
-		buffer := bytes.Buffer{}
-		log.SetOutput(&buffer)
-
 		c := Config{options: &Options{StoragePath: "/tmp/photoprism/test-photoprism-1394931550/testdata"}}
-		c.CleanupTestFolder()
-
-		// Reset logger
-		log.SetOutput(os.Stdout)
-
-		assert.Contains(t, buffer.String(), "config: cleaned up /tmp/photoprism/test-photoprism-1394931550")
+		out := captureSystemLog(t, c.CleanupTestFolder)
+		assert.Contains(t, out, "cleanup /tmp/photoprism/test-photoprism-1394931550")
+		assert.Contains(t, out, "succeeded")
+	})
+	t.Run("MarkerNotPrefix", func(t *testing.T) {
+		// The isolated-folder marker must be the prefix of the parent directory, not merely
+		// a substring, so a path like ".../my-test-photoprism-x/testdata" is refused.
+		c := Config{options: &Options{StoragePath: "/tmp/photoprism/my-test-photoprism-x/testdata"}}
+		out := captureSystemLog(t, c.CleanupTestFolder)
+		assert.Contains(t, out, "cleanup /tmp/photoprism/my-test-photoprism-x/testdata")
+		assert.Contains(t, out, "failed")
 	})
 }