From 93bd384ca874b0ccc94699b046a963071871e4ed Mon Sep 17 00:00:00 2001 From: Gabriel Radureau Date: Sun, 3 May 2026 16:28:57 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20fix(bdd):=20revert=20PR=20#26=20?= =?UTF-8?q?schema=20isolation=20+=20cache=20flush=20+=20sequential=20CI=20?= =?UTF-8?q?tests=20(#28)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reverts PR #26 (BDD_SCHEMA_ISOLATION caused empty schemas with no tables, 500 errors). Adds sequential package execution (-p 1) + cache flush AfterScenario. AuthBDD goes from 0/5 PASS to 5/5 PASS deterministically. Parallel BDD deferred as architectural follow-up (requires per-schema migrations + dedicated connection pools). Co-authored-by: Gabriel Radureau Co-committed-by: Gabriel Radureau --- .gitea/workflows/ci-cd.yaml | 8 ++++---- pkg/bdd/suite.go | 9 +++++++++ pkg/bdd/testserver/server.go | 24 +++++++++++++++++++++++- pkg/bdd/testserver/state_tracer.go | 5 +++++ pkg/server/server.go | 6 ++++++ scripts/run-bdd-tests.sh | 4 +++- 6 files changed, 50 insertions(+), 6 deletions(-) diff --git a/.gitea/workflows/ci-cd.yaml b/.gitea/workflows/ci-cd.yaml index 1be0859..b5af3d7 100644 --- a/.gitea/workflows/ci-cd.yaml +++ b/.gitea/workflows/ci-cd.yaml @@ -219,10 +219,10 @@ jobs: export DLC_DATABASE_PASSWORD=postgres export DLC_DATABASE_NAME=dance_lessons_coach_bdd_test export DLC_DATABASE_SSL_MODE=disable - # Enable per-scenario schema isolation (ADR-0025) to prevent flaky AuthBDD failures. - # Without this, scenarios share the public schema and pollute each other's state. - # Observed flakiness: same code passes in #605, fails in #606 on TestAuthBDD/*. - export BDD_SCHEMA_ISOLATION=true + # NOTE: BDD_SCHEMA_ISOLATION was tried (PR #26) but creates empty per-scenario schemas + # without table migrations, causing 500 errors on registration. Reverted in PR #28. + # The default mode (CleanupDatabase truncates between scenarios) works fine when tests + # run sequentially (Go BDD doesn't use t.Parallel by default). ./scripts/run-bdd-tests.sh # Generate BDD coverage report diff --git a/pkg/bdd/suite.go b/pkg/bdd/suite.go index 08b1cda..eca9452 100644 --- a/pkg/bdd/suite.go +++ b/pkg/bdd/suite.go @@ -115,6 +115,15 @@ func InitializeTestSuite(ctx *godog.TestSuiteContext) { testserver.TraceStateJWTSecretOperation(feature, scenarioKey, "RESET", "ok") } + // Flush cache after every scenario to prevent cache pollution + if flushErr := sharedServer.FlushCache(); flushErr != nil { + if isCleanupLoggingEnabled() { + log.Warn().Err(flushErr).Msg("CLEANUP: Failed to flush cache after scenario") + } + } else { + testserver.TraceStateCacheOperation(feature, scenarioKey, "FLUSH", "ok") + } + // Clean database after every scenario (only if schema isolation is disabled) if !isSchemaIsolationEnabled() { if cleanupErr := sharedServer.CleanupDatabase(); cleanupErr != nil { diff --git a/pkg/bdd/testserver/server.go b/pkg/bdd/testserver/server.go index 059c9e5..38bb117 100644 --- a/pkg/bdd/testserver/server.go +++ b/pkg/bdd/testserver/server.go @@ -15,6 +15,7 @@ import ( "sync" "time" + "dance-lessons-coach/pkg/cache" "dance-lessons-coach/pkg/config" "dance-lessons-coach/pkg/server" "dance-lessons-coach/pkg/user" @@ -48,6 +49,7 @@ type Server struct { baseURL string db *sql.DB authService user.AuthService // Reference to auth service for cleanup + cacheService cache.Service // Reference to cache service for cleanup schemaMutex sync.Mutex // Protects schema operations currentSchema string // Current schema being used originalSearchPath string // Original search_path to restore @@ -153,6 +155,9 @@ func (s *Server) Start() error { // Store auth service for cleanup s.authService = realServer.GetAuthService() + // Store cache service for cleanup + s.cacheService = realServer.GetCacheService() + // Initialize database connection for cleanup if err := s.initDBConnection(); err != nil { return fmt.Errorf("failed to initialize database connection: %w", err) @@ -409,6 +414,23 @@ func (s *Server) ResetJWTSecrets() error { return nil } +// FlushCache clears all cached data to prevent cache pollution between scenarios +// This prevents cached responses from affecting subsequent test scenarios +func (s *Server) FlushCache() error { + if s.cacheService == nil { + if isCleanupLoggingEnabled() { + log.Info().Msg("CLEANUP: No cache service available, skipping cache flush") + } + return nil + } + + s.cacheService.Flush() + if isCleanupLoggingEnabled() { + log.Info().Msg("CLEANUP: Cache flushed successfully") + } + return nil +} + // CleanupDatabase deletes all test data from all tables // This uses raw SQL to avoid dependency on repositories and handles foreign keys properly // Uses SET CONSTRAINTS ALL DEFERRED to temporarily disable foreign key checks @@ -555,7 +577,7 @@ func (s *Server) SetupScenarioSchema(feature, scenario string) error { return fmt.Errorf("failed to create schema %s: %w", schemaName, err) } - // Set search path to use the new schema + // Set search path to use the new schema (testserver's own connection) searchPathSQL := fmt.Sprintf("SET search_path = %s, %s", schemaName, s.originalSearchPath) if _, err := s.db.Exec(searchPathSQL); err != nil { return fmt.Errorf("failed to set search_path: %w", err) diff --git a/pkg/bdd/testserver/state_tracer.go b/pkg/bdd/testserver/state_tracer.go index 916aaf1..2f58be1 100644 --- a/pkg/bdd/testserver/state_tracer.go +++ b/pkg/bdd/testserver/state_tracer.go @@ -31,6 +31,11 @@ func TraceStateJWTSecretOperation(feature, scenario, operation, details string) writeTraceLine(feature, scenario, "JWT_"+operation, details) } +// TraceStateCacheOperation logs a cache operation +func TraceStateCacheOperation(feature, scenario, operation, details string) { + writeTraceLine(feature, scenario, "CACHE_"+operation, details) +} + // TraceStateSchemaIsolation logs a schema isolation operation func TraceStateSchemaIsolation(feature, scenario, operation, details string) { writeTraceLine(feature, scenario, "SCHEMA_"+operation, details) diff --git a/pkg/server/server.go b/pkg/server/server.go index 1e2936f..750f590 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -118,6 +118,12 @@ func (s *Server) GetAuthService() user.AuthService { return s.userService } +// GetCacheService returns the cache service for test cleanup +// This allows test suites to flush cache between tests +func (s *Server) GetCacheService() cache.Service { + return s.cacheService +} + // initializeUserServices initializes the user repository and unified user service func initializeUserServices(cfg *config.Config) (user.UserRepository, user.UserService, error) { // Create user repository using PostgreSQL diff --git a/scripts/run-bdd-tests.sh b/scripts/run-bdd-tests.sh index d392277..8b4f65a 100755 --- a/scripts/run-bdd-tests.sh +++ b/scripts/run-bdd-tests.sh @@ -144,7 +144,9 @@ run_tests_with_tags() { # Note: -tags flag in go test is for Go build tags, NOT Godog feature tags # We use GODOG_TAGS env var which is read by the test framework echo "🚀 Running: GODOG_TAGS=\"${DEFAULT_TAGS}\" go test ./features/..." - GODOG_TAGS="$DEFAULT_TAGS" go test ./features/... -v -cover -coverpkg=./... -coverprofile=coverage.out 2>&1 | tee /tmp/bdd_test_output.txt && test_output=$(cat /tmp/bdd_test_output.txt) && rm -f /tmp/bdd_test_output.txt || test_output=$(cat /tmp/bdd_test_output.txt 2>/dev/null || echo "") + # -p 1 forces sequential package execution to avoid DB-state contention between feature packages + # (different packages would otherwise spawn concurrent test servers sharing the same Postgres DB). + GODOG_TAGS="$DEFAULT_TAGS" go test ./features/... -v -p 1 -cover -coverpkg=./... -coverprofile=coverage.out 2>&1 | tee /tmp/bdd_test_output.txt && test_output=$(cat /tmp/bdd_test_output.txt) && rm -f /tmp/bdd_test_output.txt || test_output=$(cat /tmp/bdd_test_output.txt 2>/dev/null || echo "") test_exit_code=${PIPESTATUS[0]} fi