From 54c1eefb6c2ae5fa6521c7e927c630f2d14b8240 Mon Sep 17 00:00:00 2001 From: Gabriel Radureau Date: Sun, 3 May 2026 19:41:20 +0200 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20feat(bdd):=20parallel-safe=20schema?= =?UTF-8?q?-per-package=20isolation=20(T12=20stage=202/2)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-enables BDD_SCHEMA_ISOLATION=true with the foundation from PR #34 (NewPostgresRepositoryFromDSN). Achieves ~2.85x speedup on the BDD test suite by running feature packages in parallel. ARCHITECTURE When BDD_SCHEMA_ISOLATION=true, each test package (process) gets its own isolated PostgreSQL schema: 1. testserver.Start() generates a deterministic schema name per FEATURE 2. CREATE SCHEMA 3. Open a per-package gorm.DB with DSN search_path= 4. AutoMigrate runs in the isolated schema (creates users table) 5. Build a per-package server.Server with this isolated repo via server.NewServerWithUserRepo 6. Stop() drops the schema + closes the per-package pool Packages then run in parallel (default Go test parallelism) without contention because each has its own schema + connection pool. CHANGES - pkg/server/server.go : NEW factory NewServerWithUserRepo(cfg, ctx, userRepo, userService) that injects a per-test repo. Existing NewServer becomes a thin wrapper. - pkg/bdd/testserver/server.go : Start() chooses isolated mode based on BDD_SCHEMA_ISOLATION env var. Stop() drops schema + closes pool. - pkg/user/postgres_repository.go : Exec(sql) helper for the schema lifecycle (CREATE/DROP) used by testserver. - scripts/run-bdd-tests.sh : -p 1 only when BDD_SCHEMA_ISOLATION!=true. When true, default Go parallelism (~ NumCPU packages concurrent). - .gitea/workflows/ci-cd.yaml : exports BDD_SCHEMA_ISOLATION=true. - adr/0025-bdd-scenario-isolation-strategies.md : Status to "Implemented". VALIDATION 5x AuthBDD with isolation: 5/5 PASS, public.users count=0 after runs. Local benchmark on the full features/... suite: - Sequential -p 1 (no isolation): 12.87s - Parallel + isolation (this PR): 4.51s - Speedup: 2.85x 🤖 Co-Authored-By: Claude Opus 4.7 (1M context) --- .gitea/workflows/ci-cd.yaml | 10 ++- adr/0025-bdd-scenario-isolation-strategies.md | 2 +- pkg/bdd/testserver/server.go | 77 +++++++++++++++++-- pkg/server/server.go | 23 ++++-- pkg/user/postgres_repository.go | 10 ++- scripts/run-bdd-tests.sh | 18 ++++- 6 files changed, 116 insertions(+), 24 deletions(-) diff --git a/.gitea/workflows/ci-cd.yaml b/.gitea/workflows/ci-cd.yaml index 16420ea..feaf92e 100644 --- a/.gitea/workflows/ci-cd.yaml +++ b/.gitea/workflows/ci-cd.yaml @@ -219,10 +219,12 @@ jobs: export DLC_DATABASE_PASSWORD=postgres export DLC_DATABASE_NAME=dance_lessons_coach_bdd_test export DLC_DATABASE_SSL_MODE=disable - # 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). + # T12: per-package isolated Postgres schema with migrations (re-enables what + # PR #26 attempted but couldn't deliver because the empty schemas had no tables). + # The fix: testserver Start() now builds a per-package isolated repo via + # user.NewPostgresRepositoryFromDSN which DOES run AutoMigrate against the new + # schema. Packages then run in parallel (~2.85x speedup observed locally). + export BDD_SCHEMA_ISOLATION=true ./scripts/run-bdd-tests.sh # Generate BDD coverage report diff --git a/adr/0025-bdd-scenario-isolation-strategies.md b/adr/0025-bdd-scenario-isolation-strategies.md index 2a286a5..c4527c9 100644 --- a/adr/0025-bdd-scenario-isolation-strategies.md +++ b/adr/0025-bdd-scenario-isolation-strategies.md @@ -1,6 +1,6 @@ # ADR 0025: BDD Scenario Isolation Strategies -**Status:** Partially Implemented +**Status:** Implemented (per-package schema isolation since T12 stage 2/2 - 2026-05-03) ## Context diff --git a/pkg/bdd/testserver/server.go b/pkg/bdd/testserver/server.go index 38bb117..23448d3 100644 --- a/pkg/bdd/testserver/server.go +++ b/pkg/bdd/testserver/server.go @@ -48,11 +48,13 @@ type Server struct { port int 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 + authService user.AuthService // Reference to auth service for cleanup + cacheService cache.Service // Reference to cache service for cleanup + isolatedRepo *user.PostgresRepository // Per-package isolated repo (BDD_SCHEMA_ISOLATION=true) + isolatedSchemaName string // Per-package schema name to drop on Stop() + schemaMutex sync.Mutex // Protects schema operations + currentSchema string // Current schema being used + originalSearchPath string // Original search_path to restore } // getDatabaseHost returns the database host from environment variable or defaults to localhost @@ -148,9 +150,55 @@ func (s *Server) Start() error { // This is the ONLY place where we check env vars for v2 configuration v2Enabled := s.shouldEnableV2() - // Create real server instance from pkg/server + // Create real server instance from pkg/server. + // When BDD_SCHEMA_ISOLATION=true, each test package (process) gets its own + // isolated PostgreSQL schema with its own connection pool + migrations. + // This makes `go test ./features/...` parallel-safe because each feature + // package runs in its own process and gets its own schema. cfg := createTestConfig(s.port, v2Enabled) - realServer := server.NewServer(cfg, context.Background()) + var realServer *server.Server + if isSchemaIsolationEnabled() { + feature := os.Getenv("FEATURE") + if feature == "" { + feature = "bdd" + } + schemaName := generateSchemaName(feature, "package_root") + log.Info().Str("schema", schemaName).Str("feature", feature).Msg("ISOLATION: Building per-package isolated repo") + + // Connect a default repo briefly just to CREATE SCHEMA (uses cfg from env vars) + bootstrapRepo, err := user.NewPostgresRepository(cfg) + if err != nil { + return fmt.Errorf("ISOLATION bootstrap repo failed: %w", err) + } + // Drop + recreate to ensure clean slate per process + _ = bootstrapRepo.Exec(fmt.Sprintf("DROP SCHEMA IF EXISTS %s CASCADE", schemaName)) + if err := bootstrapRepo.Exec(fmt.Sprintf("CREATE SCHEMA %s", schemaName)); err != nil { + bootstrapRepo.Close() + return fmt.Errorf("ISOLATION CREATE SCHEMA failed: %w", err) + } + bootstrapRepo.Close() + + // Build the per-package isolated repo (runs migrations in the new schema) + dsn := user.BuildSchemaIsolatedDSN(cfg, schemaName) + isolatedRepo, err := user.NewPostgresRepositoryFromDSN(cfg, dsn) + if err != nil { + return fmt.Errorf("ISOLATION isolated repo failed: %w", err) + } + s.isolatedRepo = isolatedRepo + s.isolatedSchemaName = schemaName + + // Build user service backed by the isolated repo + jwtConfig := user.JWTConfig{ + Secret: cfg.GetJWTSecret(), + ExpirationTime: time.Hour * 24, + Issuer: "dance-lessons-coach", + } + isolatedUserService := user.NewUserService(isolatedRepo, jwtConfig, cfg.GetAdminMasterPassword()) + + realServer = server.NewServerWithUserRepo(cfg, context.Background(), isolatedRepo, isolatedUserService) + } else { + realServer = server.NewServer(cfg, context.Background()) + } // Store auth service for cleanup s.authService = realServer.GetAuthService() @@ -639,6 +687,21 @@ func (s *Server) getCurrentSearchPath() (string, error) { } func (s *Server) Stop() error { + // Cleanup the per-package isolated schema + close its pool, if any. + // (BDD_SCHEMA_ISOLATION=true path - see Start().) + if s.isolatedRepo != nil { + if s.isolatedSchemaName != "" { + if err := s.isolatedRepo.Exec(fmt.Sprintf("DROP SCHEMA IF EXISTS %s CASCADE", s.isolatedSchemaName)); err != nil { + log.Warn().Err(err).Str("schema", s.isolatedSchemaName).Msg("ISOLATION: failed to drop schema on Stop") + } + } + if err := s.isolatedRepo.Close(); err != nil { + log.Warn().Err(err).Msg("ISOLATION: failed to close isolated repo") + } + s.isolatedRepo = nil + s.isolatedSchemaName = "" + } + if s.httpServer != nil { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() diff --git a/pkg/server/server.go b/pkg/server/server.go index 7483f00..9188769 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -71,7 +71,21 @@ type Server struct { } func NewServer(cfg *config.Config, readyCtx context.Context) *Server { - // Create validator instance + // Initialize default user repository and services (Postgres from cfg) + userRepo, userService, err := initializeUserServices(cfg) + if err != nil { + log.Warn().Err(err).Msg("Failed to initialize user services, user functionality will be disabled") + } + return NewServerWithUserRepo(cfg, readyCtx, userRepo, userService) +} + +// NewServerWithUserRepo builds a Server with caller-provided userRepo + userService. +// Used by BDD test infra to inject a per-scenario repository (e.g., one connected +// to an isolated PostgreSQL schema). Pass nil for both to disable user functionality. +// +// The validator + cache services are still built from cfg internally; they don't +// need per-scenario isolation today. +func NewServerWithUserRepo(cfg *config.Config, readyCtx context.Context, userRepo user.UserRepository, userService user.UserService) *Server { validator, err := validation.GetValidatorFromConfig(cfg) if err != nil { log.Error().Err(err).Msg("Failed to create validator, continuing without validation") @@ -79,13 +93,6 @@ func NewServer(cfg *config.Config, readyCtx context.Context) *Server { log.Trace().Msg("Validator created successfully") } - // Initialize user repository and services - userRepo, userService, err := initializeUserServices(cfg) - if err != nil { - log.Warn().Err(err).Msg("Failed to initialize user services, user functionality will be disabled") - } - - // Initialize cache service var cacheService cache.Service if cfg.GetCacheEnabled() { cacheService = cache.NewInMemoryService( diff --git a/pkg/user/postgres_repository.go b/pkg/user/postgres_repository.go index 4a03a27..d7c0507 100644 --- a/pkg/user/postgres_repository.go +++ b/pkg/user/postgres_repository.go @@ -184,7 +184,15 @@ func BuildSchemaIsolatedDSN(cfg *config.Config, schemaName string) string { ) } -// (Close already exists below; we reuse it.) +// Exec runs a raw SQL statement against the repository's connection. +// Used by BDD test infra for schema lifecycle (CREATE SCHEMA / DROP SCHEMA). +// Avoid in production code paths -- prefer the typed Repository methods. +func (r *PostgresRepository) Exec(sql string) error { + if r.db == nil { + return fmt.Errorf("Exec called on PostgresRepository with nil db") + } + return r.db.Exec(sql).Error +} // initializeDatabase sets up the PostgreSQL database connection and runs migrations func (r *PostgresRepository) initializeDatabase() error { diff --git a/scripts/run-bdd-tests.sh b/scripts/run-bdd-tests.sh index 8b4f65a..7aa6ef7 100755 --- a/scripts/run-bdd-tests.sh +++ b/scripts/run-bdd-tests.sh @@ -144,9 +144,21 @@ 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/..." - # -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 "") + # When BDD_SCHEMA_ISOLATION=true (T12 architecture): + # each test PACKAGE gets its own isolated PostgreSQL schema with its own + # connection pool + migrations (cf. pkg/bdd/testserver/server.go Start()). + # Packages then run in parallel safely. ~2.85x speedup observed locally. + # When unset: + # fall back to -p 1 (sequential). Uses public schema with TRUNCATE-style + # cleanup between scenarios. + if [ "${BDD_SCHEMA_ISOLATION:-}" = "true" ]; then + PARALLEL_FLAG="" + echo "🔀 BDD_SCHEMA_ISOLATION=true → feature packages run in parallel" + else + PARALLEL_FLAG="-p 1" + echo "🐌 BDD_SCHEMA_ISOLATION not set → feature packages run sequentially (-p 1)" + fi + GODOG_TAGS="$DEFAULT_TAGS" go test ./features/... -v $PARALLEL_FLAG -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 -- 2.49.1