From 49256468eb82dceda7256c797d767fe2bbe7cafc Mon Sep 17 00:00:00 2001 From: Andrey Marchenko Date: Thu, 12 Feb 2026 19:18:24 +0100 Subject: [PATCH 1/4] Fix monorepo subdirectory path handling when ITR is enabled (#33) When running ddtest from a monorepo subdirectory (e.g., `cd core && ddtest run`), full test discovery returns repo-root-relative paths like `core/spec/models/order_spec.rb`. These paths were passed unchanged to workers running from the `core/` CWD, causing them to resolve `core/core/spec/...` which does not exist. Add conservative path normalization that detects the CWD's position relative to the git root and strips the matching prefix from repo-root-relative paths before storing them. The normalization is fail-safe: paths are returned unchanged if git root is unavailable, the prefix doesn't match, or CWD is already the repo root. Co-authored-by: Cursor --- internal/runner/dd_test_optimization.go | 14 +- internal/runner/dd_test_optimization_test.go | 255 +++++++++++++++++++ internal/runner/path_normalization.go | 105 ++++++++ internal/runner/path_normalization_test.go | 163 ++++++++++++ internal/runner/runner_test.go | 118 ++++++++- 5 files changed, 648 insertions(+), 7 deletions(-) create mode 100644 internal/runner/path_normalization.go create mode 100644 internal/runner/path_normalization_test.go diff --git a/internal/runner/dd_test_optimization.go b/internal/runner/dd_test_optimization.go index 0b5573a..557ba61 100644 --- a/internal/runner/dd_test_optimization.go +++ b/internal/runner/dd_test_optimization.go @@ -133,14 +133,26 @@ func (tr *TestRunner) PrepareTestOptimization(ctx context.Context) error { discoveredTestsCount := len(discoveredTests) skippableTestsCount := 0 + // Compute subdirectory prefix once for all paths. + // When running from a monorepo subdirectory (e.g., "cd core && ddtest plan"), + // full discovery may return repo-root-relative paths (e.g., "core/spec/..."). + // We normalize them to CWD-relative paths so workers can find the files. + subdirPrefix := getCwdSubdirPrefix() + if subdirPrefix != "" { + slog.Info("Running from subdirectory, will normalize repo-root-relative paths", "subdirPrefix", subdirPrefix) + } + tr.testFiles = make(map[string]int) for _, test := range discoveredTests { if !skippableTests[test.FQN()] { slog.Debug("Test is not skipped", "test", test.FQN(), "sourceFile", test.SuiteSourceFile) if test.SuiteSourceFile != "" { + // Normalize repo-root-relative path to CWD-relative path. + // No-op when running from repo root or when path doesn't match subdir prefix. + normalizedPath := normalizeTestFilePathWithPrefix(test.SuiteSourceFile, subdirPrefix) // increment the number of tests in the file // it should track the test suite's duration here in the future - tr.testFiles[test.SuiteSourceFile]++ + tr.testFiles[normalizedPath]++ } } else { skippableTestsCount++ diff --git a/internal/runner/dd_test_optimization_test.go b/internal/runner/dd_test_optimization_test.go index cedd0d1..92c1c9d 100644 --- a/internal/runner/dd_test_optimization_test.go +++ b/internal/runner/dd_test_optimization_test.go @@ -4,6 +4,8 @@ import ( "context" "errors" "os" + "os/exec" + "path/filepath" "strings" "testing" @@ -493,3 +495,256 @@ func TestTestRunner_PrepareTestOptimization_NoRuntimeTagsOverride(t *testing.T) t.Errorf("Expected runtime.version to be '3.3.0' from platform, got %q", mockOptimizationClient.Tags["runtime.version"]) } } + +// initGitRepo initializes a bare git repo in the given directory so that +// `git rev-parse --show-toplevel` resolves correctly during tests. +func initGitRepo(t *testing.T, dir string) { + t.Helper() + cmd := exec.Command("git", "init") + cmd.Dir = dir + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("failed to init git repo in %s: %v\n%s", dir, err, string(out)) + } + // Need at least one commit for some git operations to work + cmd = exec.Command("git", "commit", "--allow-empty", "-m", "init") + cmd.Dir = dir + cmd.Env = append(os.Environ(), + "GIT_AUTHOR_NAME=test", "GIT_AUTHOR_EMAIL=test@test.com", + "GIT_COMMITTER_NAME=test", "GIT_COMMITTER_EMAIL=test@test.com", + ) + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("failed to create initial commit in %s: %v\n%s", dir, err, string(out)) + } +} + +// TestPrepareTestOptimization_ITRFullDiscovery_SubdirRootRelativePath_NormalizesToCwdRelative +// reproduces issue #33: full discovery returns repo-root-relative SuiteSourceFile paths +// (e.g. "core/spec/...") but workers run from subdirectory "core/", causing double-prefix. +func TestPrepareTestOptimization_ITRFullDiscovery_SubdirRootRelativePath_NormalizesToCwdRelative(t *testing.T) { + ctx := context.Background() + + // Create a temp monorepo: repoRoot/core/spec/models/order_spec.rb + repoRoot := t.TempDir() + initGitRepo(t, repoRoot) + + coreDir := filepath.Join(repoRoot, "core") + _ = os.MkdirAll(filepath.Join(coreDir, "spec", "models"), 0755) + _ = os.WriteFile(filepath.Join(coreDir, "spec", "models", "order_spec.rb"), []byte("# spec"), 0644) + _ = os.MkdirAll(filepath.Join(coreDir, "spec", "finders"), 0755) + _ = os.WriteFile(filepath.Join(coreDir, "spec", "finders", "find_spec.rb"), []byte("# spec"), 0644) + + // chdir into the subdirectory (simulating: cd core && ddtest plan) + oldWd, _ := os.Getwd() + defer func() { _ = os.Chdir(oldWd) }() + _ = os.Chdir(coreDir) + + // Full discovery returns repo-root-relative paths (the bug scenario) + mockFramework := &MockFramework{ + FrameworkName: "rspec", + Tests: []testoptimization.Test{ + {Suite: "Order", Name: "should be valid", Parameters: "", SuiteSourceFile: "core/spec/models/order_spec.rb"}, + {Suite: "AddressFinder", Name: "finds addresses", Parameters: "", SuiteSourceFile: "core/spec/finders/find_spec.rb"}, + }, + } + + mockPlatform := &MockPlatform{ + PlatformName: "ruby", + Tags: map[string]string{"platform": "ruby"}, + Framework: mockFramework, + } + + mockPlatformDetector := &MockPlatformDetector{Platform: mockPlatform} + mockOptimizationClient := &MockTestOptimizationClient{ + SkippableTests: map[string]bool{}, // No tests skipped (ITR enabled but all tests need to run) + } + + runner := NewWithDependencies(mockPlatformDetector, mockOptimizationClient, newDefaultMockCIProviderDetector()) + + err := runner.PrepareTestOptimization(ctx) + if err != nil { + t.Fatalf("PrepareTestOptimization() should not return error, got: %v", err) + } + + // The key assertion: testFiles should contain CWD-relative paths, not repo-root-relative paths + // i.e. "spec/models/order_spec.rb" not "core/spec/models/order_spec.rb" + expectedFiles := map[string]bool{ + "spec/models/order_spec.rb": true, + "spec/finders/find_spec.rb": true, + } + + if len(runner.testFiles) != 2 { + t.Fatalf("Expected 2 test files, got %d: %v", len(runner.testFiles), runner.testFiles) + } + + for file := range runner.testFiles { + if !expectedFiles[file] { + t.Errorf("Unexpected test file path %q - should be CWD-relative, not repo-root-relative", file) + } + // Explicitly check for the double-prefix bug + if strings.HasPrefix(file, "core/") { + t.Errorf("Test file path %q still has repo-root prefix 'core/' - this is the bug from issue #33", file) + } + } +} + +// TestPrepareTestOptimization_RepoRootRun_LeavesRepoRelativePathsUnchanged +// ensures that when running from the repo root (not a subdirectory), paths are not modified. +func TestPrepareTestOptimization_RepoRootRun_LeavesRepoRelativePathsUnchanged(t *testing.T) { + ctx := context.Background() + + // Create a temp repo root with spec files + repoRoot := t.TempDir() + initGitRepo(t, repoRoot) + + _ = os.MkdirAll(filepath.Join(repoRoot, "spec", "models"), 0755) + _ = os.WriteFile(filepath.Join(repoRoot, "spec", "models", "user_spec.rb"), []byte("# spec"), 0644) + + // chdir to repo root (normal case) + oldWd, _ := os.Getwd() + defer func() { _ = os.Chdir(oldWd) }() + _ = os.Chdir(repoRoot) + + mockFramework := &MockFramework{ + FrameworkName: "rspec", + Tests: []testoptimization.Test{ + {Suite: "User", Name: "should be valid", Parameters: "", SuiteSourceFile: "spec/models/user_spec.rb"}, + }, + } + + mockPlatform := &MockPlatform{ + PlatformName: "ruby", + Tags: map[string]string{"platform": "ruby"}, + Framework: mockFramework, + } + + mockPlatformDetector := &MockPlatformDetector{Platform: mockPlatform} + mockOptimizationClient := &MockTestOptimizationClient{ + SkippableTests: map[string]bool{}, + } + + runner := NewWithDependencies(mockPlatformDetector, mockOptimizationClient, newDefaultMockCIProviderDetector()) + + err := runner.PrepareTestOptimization(ctx) + if err != nil { + t.Fatalf("PrepareTestOptimization() should not return error, got: %v", err) + } + + // Paths should remain unchanged when running from repo root + if _, ok := runner.testFiles["spec/models/user_spec.rb"]; !ok { + t.Errorf("Expected test file 'spec/models/user_spec.rb' to remain unchanged, got: %v", runner.testFiles) + } +} + +// TestPrepareTestOptimization_FastDiscovery_PathsRemainUnchanged +// ensures the fast discovery path (ITR disabled) does not modify paths. +func TestPrepareTestOptimization_FastDiscovery_PathsRemainUnchanged(t *testing.T) { + ctx := context.Background() + + // Fast discovery returns CWD-relative paths directly from glob + mockFramework := &MockFramework{ + FrameworkName: "rspec", + Tests: nil, // No full discovery results + TestFiles: []string{"spec/models/user_spec.rb", "spec/controllers/admin_spec.rb"}, // Fast discovery + DiscoverTestsErr: errors.New("context canceled"), // Simulate full discovery being cancelled + } + + mockPlatform := &MockPlatform{ + PlatformName: "ruby", + Tags: map[string]string{"platform": "ruby"}, + Framework: mockFramework, + } + + mockPlatformDetector := &MockPlatformDetector{Platform: mockPlatform} + mockOptimizationClient := &MockTestOptimizationClient{ + SkippableTests: map[string]bool{}, + } + + runner := NewWithDependencies(mockPlatformDetector, mockOptimizationClient, newDefaultMockCIProviderDetector()) + + err := runner.PrepareTestOptimization(ctx) + if err != nil { + t.Fatalf("PrepareTestOptimization() should not return error, got: %v", err) + } + + // Fast discovery paths should be used as-is + expectedFiles := map[string]bool{ + "spec/models/user_spec.rb": true, + "spec/controllers/admin_spec.rb": true, + } + + if len(runner.testFiles) != 2 { + t.Fatalf("Expected 2 test files, got %d: %v", len(runner.testFiles), runner.testFiles) + } + + for file := range runner.testFiles { + if !expectedFiles[file] { + t.Errorf("Unexpected test file path %q in fast discovery", file) + } + } +} + +// TestPrepareTestOptimization_ITRPathNormalization_PrefixMismatchUnchanged +// ensures that when SuiteSourceFile does not match the current subdir prefix, +// the path is not modified (conservative behavior). +func TestPrepareTestOptimization_ITRPathNormalization_PrefixMismatchUnchanged(t *testing.T) { + ctx := context.Background() + + // Create monorepo with "api" and "core" subdirs + repoRoot := t.TempDir() + initGitRepo(t, repoRoot) + + apiDir := filepath.Join(repoRoot, "api") + _ = os.MkdirAll(filepath.Join(apiDir, "spec", "models"), 0755) + _ = os.WriteFile(filepath.Join(apiDir, "spec", "models", "endpoint_spec.rb"), []byte("# spec"), 0644) + + // We're in "api/" subdir but discovery returns "core/" paths (shouldn't happen in practice, + // but tests the safety of the normalization) + oldWd, _ := os.Getwd() + defer func() { _ = os.Chdir(oldWd) }() + _ = os.Chdir(apiDir) + + mockFramework := &MockFramework{ + FrameworkName: "rspec", + Tests: []testoptimization.Test{ + // Path with different prefix than CWD subdir - should be left unchanged + {Suite: "Endpoint", Name: "should respond", Parameters: "", SuiteSourceFile: "core/spec/models/order_spec.rb"}, + // Path that does match CWD subdir prefix + {Suite: "ApiEndpoint", Name: "should work", Parameters: "", SuiteSourceFile: "api/spec/models/endpoint_spec.rb"}, + }, + } + + mockPlatform := &MockPlatform{ + PlatformName: "ruby", + Tags: map[string]string{"platform": "ruby"}, + Framework: mockFramework, + } + + mockPlatformDetector := &MockPlatformDetector{Platform: mockPlatform} + mockOptimizationClient := &MockTestOptimizationClient{ + SkippableTests: map[string]bool{}, + } + + runner := NewWithDependencies(mockPlatformDetector, mockOptimizationClient, newDefaultMockCIProviderDetector()) + + err := runner.PrepareTestOptimization(ctx) + if err != nil { + t.Fatalf("PrepareTestOptimization() should not return error, got: %v", err) + } + + // "core/spec/..." doesn't match "api" subdir prefix, should be unchanged + // "api/spec/..." matches "api" subdir prefix, should be normalized to "spec/..." + expectedFiles := map[string]bool{ + "core/spec/models/order_spec.rb": true, // Mismatched prefix - unchanged + "spec/models/endpoint_spec.rb": true, // Matched "api/" prefix - stripped + } + + if len(runner.testFiles) != 2 { + t.Fatalf("Expected 2 test files, got %d: %v", len(runner.testFiles), runner.testFiles) + } + + for file := range runner.testFiles { + if !expectedFiles[file] { + t.Errorf("Unexpected test file path %q", file) + } + } +} diff --git a/internal/runner/path_normalization.go b/internal/runner/path_normalization.go new file mode 100644 index 0000000..f628abd --- /dev/null +++ b/internal/runner/path_normalization.go @@ -0,0 +1,105 @@ +package runner + +import ( + "log/slog" + "os" + "os/exec" + "path/filepath" + "strings" +) + +// getGitRootDir returns the absolute path to the git repository root. +// Returns empty string if git is not available or CWD is not in a git repo. +func getGitRootDir() string { + cmd := exec.Command("git", "rev-parse", "--show-toplevel") + out, err := cmd.Output() + if err != nil { + return "" + } + return strings.TrimSpace(string(out)) +} + +// getCwdSubdirPrefix calculates the relative path from the git root to the +// current working directory. Returns empty string if CWD is the git root +// or if the git root cannot be determined. +// +// Example: git root = /repo, CWD = /repo/core -> returns "core" +// Example: git root = /repo, CWD = /repo/packages/core -> returns "packages/core" +// Example: git root = /repo, CWD = /repo -> returns "" +func getCwdSubdirPrefix() string { + gitRoot := getGitRootDir() + if gitRoot == "" { + return "" + } + + cwd, err := os.Getwd() + if err != nil { + return "" + } + + // Resolve symlinks for both to ensure correct comparison + gitRootResolved, err := filepath.EvalSymlinks(gitRoot) + if err != nil { + gitRootResolved = gitRoot + } + cwdResolved, err := filepath.EvalSymlinks(cwd) + if err != nil { + cwdResolved = cwd + } + + rel, err := filepath.Rel(gitRootResolved, cwdResolved) + if err != nil { + return "" + } + + // "." means CWD == git root + if rel == "." { + return "" + } + + // Use forward slashes for consistent path matching + return filepath.ToSlash(rel) +} + +// normalizeTestFilePath converts a test file path that may be repo-root-relative +// to a CWD-relative path. This is needed when running ddtest from a monorepo +// subdirectory (e.g., "cd core && ddtest plan") where full test discovery returns +// paths relative to the git root (e.g., "core/spec/...") but workers need +// paths relative to CWD (e.g., "spec/..."). +// +// Safety rules: +// - If the path does not start with the CWD subdir prefix, it is returned unchanged +// - If CWD is the git root, the path is returned unchanged +// - If the git root cannot be determined, the path is returned unchanged (fail-safe) +// - Absolute paths are returned unchanged +// - Empty paths are returned unchanged +func normalizeTestFilePath(path string) string { + return normalizeTestFilePathWithPrefix(path, getCwdSubdirPrefix()) +} + +// normalizeTestFilePathWithPrefix is the pure version of normalizeTestFilePath +// that accepts a pre-computed subdir prefix. This avoids calling git once per file. +func normalizeTestFilePathWithPrefix(path string, subdirPrefix string) string { + if path == "" || subdirPrefix == "" { + return path + } + + // Don't modify absolute paths + if filepath.IsAbs(path) { + return path + } + + // Use forward slashes for matching + normalizedPath := filepath.ToSlash(path) + prefixWithSlash := subdirPrefix + "/" + + if strings.HasPrefix(normalizedPath, prefixWithSlash) { + stripped := strings.TrimPrefix(normalizedPath, prefixWithSlash) + slog.Debug("Normalized test file path for subdirectory execution", + "original", path, "normalized", stripped, "subdirPrefix", subdirPrefix) + return stripped + } + + // Path doesn't match CWD subdir prefix - return unchanged + return path +} diff --git a/internal/runner/path_normalization_test.go b/internal/runner/path_normalization_test.go new file mode 100644 index 0000000..3ee9617 --- /dev/null +++ b/internal/runner/path_normalization_test.go @@ -0,0 +1,163 @@ +package runner + +import ( + "os" + "os/exec" + "path/filepath" + "testing" +) + +func TestNormalizePath_SubdirPrefixMatch_StripsPrefix(t *testing.T) { + // Simulates: git root = /repo, CWD = /repo/core + // Path "core/spec/models/order_spec.rb" -> "spec/models/order_spec.rb" + repoRoot := t.TempDir() + initGitRepoInDir(t, repoRoot) + + coreDir := filepath.Join(repoRoot, "core") + _ = os.MkdirAll(coreDir, 0755) + + oldWd, _ := os.Getwd() + defer func() { _ = os.Chdir(oldWd) }() + _ = os.Chdir(coreDir) + + result := normalizeTestFilePath("core/spec/models/order_spec.rb") + expected := "spec/models/order_spec.rb" + if result != expected { + t.Errorf("Expected %q, got %q", expected, result) + } +} + +func TestNormalizePath_NestedSubdirPrefixMatch_StripsFullPrefix(t *testing.T) { + // Simulates: git root = /repo, CWD = /repo/packages/core + // Path "packages/core/spec/user_spec.rb" -> "spec/user_spec.rb" + repoRoot := t.TempDir() + initGitRepoInDir(t, repoRoot) + + nestedDir := filepath.Join(repoRoot, "packages", "core") + _ = os.MkdirAll(nestedDir, 0755) + + oldWd, _ := os.Getwd() + defer func() { _ = os.Chdir(oldWd) }() + _ = os.Chdir(nestedDir) + + result := normalizeTestFilePath("packages/core/spec/user_spec.rb") + expected := "spec/user_spec.rb" + if result != expected { + t.Errorf("Expected %q, got %q", expected, result) + } +} + +func TestNormalizePath_AlreadyRelative_NoChange(t *testing.T) { + // When path doesn't start with subdir prefix, it's already CWD-relative + repoRoot := t.TempDir() + initGitRepoInDir(t, repoRoot) + + coreDir := filepath.Join(repoRoot, "core") + _ = os.MkdirAll(coreDir, 0755) + + oldWd, _ := os.Getwd() + defer func() { _ = os.Chdir(oldWd) }() + _ = os.Chdir(coreDir) + + // This path is already CWD-relative (doesn't start with "core/") + result := normalizeTestFilePath("spec/models/order_spec.rb") + expected := "spec/models/order_spec.rb" + if result != expected { + t.Errorf("Expected %q (unchanged), got %q", expected, result) + } +} + +func TestNormalizePath_PrefixMismatch_NoChange(t *testing.T) { + // CWD is "api/" but path has "core/" prefix - should NOT be stripped + repoRoot := t.TempDir() + initGitRepoInDir(t, repoRoot) + + apiDir := filepath.Join(repoRoot, "api") + _ = os.MkdirAll(apiDir, 0755) + + oldWd, _ := os.Getwd() + defer func() { _ = os.Chdir(oldWd) }() + _ = os.Chdir(apiDir) + + result := normalizeTestFilePath("core/spec/models/order_spec.rb") + expected := "core/spec/models/order_spec.rb" + if result != expected { + t.Errorf("Expected %q (unchanged), got %q", expected, result) + } +} + +func TestNormalizePath_AtRepoRoot_NoChange(t *testing.T) { + // When CWD == git root, no prefix to strip + repoRoot := t.TempDir() + initGitRepoInDir(t, repoRoot) + + oldWd, _ := os.Getwd() + defer func() { _ = os.Chdir(oldWd) }() + _ = os.Chdir(repoRoot) + + result := normalizeTestFilePath("spec/models/order_spec.rb") + expected := "spec/models/order_spec.rb" + if result != expected { + t.Errorf("Expected %q (unchanged), got %q", expected, result) + } +} + +func TestNormalizePath_GitRootUnavailable_NoChange(t *testing.T) { + // When not in a git repo, normalization should be a no-op (fail-safe) + tempDir := t.TempDir() + + oldWd, _ := os.Getwd() + defer func() { _ = os.Chdir(oldWd) }() + _ = os.Chdir(tempDir) + + result := normalizeTestFilePath("core/spec/models/order_spec.rb") + expected := "core/spec/models/order_spec.rb" + if result != expected { + t.Errorf("Expected %q (unchanged when git root unavailable), got %q", expected, result) + } +} + +func TestNormalizePath_AbsolutePath_NoChange(t *testing.T) { + // Absolute paths should not be modified + repoRoot := t.TempDir() + initGitRepoInDir(t, repoRoot) + + coreDir := filepath.Join(repoRoot, "core") + _ = os.MkdirAll(coreDir, 0755) + + oldWd, _ := os.Getwd() + defer func() { _ = os.Chdir(oldWd) }() + _ = os.Chdir(coreDir) + + absPath := "/absolute/path/to/spec.rb" + result := normalizeTestFilePath(absPath) + if result != absPath { + t.Errorf("Expected %q (absolute path unchanged), got %q", absPath, result) + } +} + +func TestNormalizePath_EmptyPath_NoChange(t *testing.T) { + result := normalizeTestFilePath("") + if result != "" { + t.Errorf("Expected empty string, got %q", result) + } +} + +// initGitRepoInDir initializes a git repo in the specified directory. +func initGitRepoInDir(t *testing.T, dir string) { + t.Helper() + cmd := exec.Command("git", "init") + cmd.Dir = dir + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("failed to init git repo in %s: %v\n%s", dir, err, string(out)) + } + cmd = exec.Command("git", "commit", "--allow-empty", "-m", "init") + cmd.Dir = dir + cmd.Env = append(os.Environ(), + "GIT_AUTHOR_NAME=test", "GIT_AUTHOR_EMAIL=test@test.com", + "GIT_COMMITTER_NAME=test", "GIT_COMMITTER_EMAIL=test@test.com", + ) + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("failed to create initial commit in %s: %v\n%s", dir, err, string(out)) + } +} diff --git a/internal/runner/runner_test.go b/internal/runner/runner_test.go index 44519c2..b417fc9 100644 --- a/internal/runner/runner_test.go +++ b/internal/runner/runner_test.go @@ -6,6 +6,7 @@ import ( "fmt" "maps" "os" + "os/exec" "path/filepath" "slices" "strconv" @@ -62,12 +63,14 @@ func (m *MockPlatform) SanityCheck() error { // MockFramework mocks a testing framework type MockFramework struct { - FrameworkName string - Tests []testoptimization.Test - TestFiles []string - Err error - RunTestsCalls []RunTestsCall - mu sync.Mutex + FrameworkName string + Tests []testoptimization.Test + TestFiles []string + Err error // Used by both DiscoverTests and DiscoverTestFiles if specific errors are nil + DiscoverTestsErr error // If set, overrides Err for DiscoverTests + DiscoverTestFilesErr error // If set, overrides Err for DiscoverTestFiles + RunTestsCalls []RunTestsCall + mu sync.Mutex } type RunTestsCall struct { @@ -80,10 +83,16 @@ func (m *MockFramework) Name() string { } func (m *MockFramework) DiscoverTests(ctx context.Context) ([]testoptimization.Test, error) { + if m.DiscoverTestsErr != nil { + return m.Tests, m.DiscoverTestsErr + } return m.Tests, m.Err } func (m *MockFramework) DiscoverTestFiles() ([]string, error) { + if m.DiscoverTestFilesErr != nil { + return m.TestFiles, m.DiscoverTestFilesErr + } return m.TestFiles, m.Err } @@ -644,3 +653,100 @@ func TestTestRunner_Setup_WithTestSplit(t *testing.T) { } }) } + +// TestTestRunner_Plan_SubdirRootRelativeDiscovery_WritesNormalizedPaths +// reproduces the end-to-end bug from issue #33: Plan writes repo-root-relative paths +// that become invalid for workers running from a monorepo subdirectory. +func TestTestRunner_Plan_SubdirRootRelativeDiscovery_WritesNormalizedPaths(t *testing.T) { + // Create a temp monorepo: repoRoot/core/spec/... + repoRoot := t.TempDir() + + // Initialize git repo at the root + cmd := exec.Command("git", "init") + cmd.Dir = repoRoot + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("failed to init git repo: %v\n%s", err, string(out)) + } + cmd = exec.Command("git", "commit", "--allow-empty", "-m", "init") + cmd.Dir = repoRoot + cmd.Env = append(os.Environ(), + "GIT_AUTHOR_NAME=test", "GIT_AUTHOR_EMAIL=test@test.com", + "GIT_COMMITTER_NAME=test", "GIT_COMMITTER_EMAIL=test@test.com", + ) + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("failed to create initial commit: %v\n%s", err, string(out)) + } + + coreDir := filepath.Join(repoRoot, "core") + _ = os.MkdirAll(filepath.Join(coreDir, "spec", "models"), 0755) + _ = os.WriteFile(filepath.Join(coreDir, "spec", "models", "order_spec.rb"), []byte("# spec"), 0644) + _ = os.WriteFile(filepath.Join(coreDir, "spec", "models", "payment_spec.rb"), []byte("# spec"), 0644) + + // chdir into subdirectory + oldWd, _ := os.Getwd() + defer func() { _ = os.Chdir(oldWd) }() + _ = os.Chdir(coreDir) + + // Set parallelism to 1 + _ = os.Setenv("DD_TEST_OPTIMIZATION_RUNNER_MIN_PARALLELISM", "1") + _ = os.Setenv("DD_TEST_OPTIMIZATION_RUNNER_MAX_PARALLELISM", "1") + defer func() { + _ = os.Unsetenv("DD_TEST_OPTIMIZATION_RUNNER_MIN_PARALLELISM") + _ = os.Unsetenv("DD_TEST_OPTIMIZATION_RUNNER_MAX_PARALLELISM") + }() + settings.Init() + + // Full discovery returns repo-root-relative paths (the bug) + mockFramework := &MockFramework{ + FrameworkName: "rspec", + Tests: []testoptimization.Test{ + {Suite: "Order", Name: "should be valid", Parameters: "", SuiteSourceFile: "core/spec/models/order_spec.rb"}, + {Suite: "Payment", Name: "should process", Parameters: "", SuiteSourceFile: "core/spec/models/payment_spec.rb"}, + }, + } + + mockPlatform := &MockPlatform{ + PlatformName: "ruby", + Tags: map[string]string{"platform": "ruby"}, + Framework: mockFramework, + } + + mockPlatformDetector := &MockPlatformDetector{Platform: mockPlatform} + mockOptimizationClient := &MockTestOptimizationClient{ + SkippableTests: map[string]bool{}, + } + + runner := NewWithDependencies(mockPlatformDetector, mockOptimizationClient, newDefaultMockCIProviderDetector()) + + err := runner.Plan(context.Background()) + if err != nil { + t.Fatalf("Plan() should not return error, got: %v", err) + } + + // Verify test-files.txt contains CWD-relative paths + testFilesContent, err := os.ReadFile(constants.TestFilesOutputPath) + if err != nil { + t.Fatalf("Failed to read test-files.txt: %v", err) + } + + testFilesStr := string(testFilesContent) + if strings.Contains(testFilesStr, "core/") { + t.Errorf("test-files.txt should not contain repo-root prefix 'core/', got:\n%s", testFilesStr) + } + + expectedContent := "spec/models/order_spec.rb\nspec/models/payment_spec.rb\n" + if testFilesStr != expectedContent { + t.Errorf("Expected test-files.txt content:\n%s\nGot:\n%s", expectedContent, testFilesStr) + } + + // Verify runner-0 split file also contains CWD-relative paths + runnerContent, err := os.ReadFile(filepath.Join(constants.TestsSplitDir, "runner-0")) + if err != nil { + t.Fatalf("Failed to read runner-0: %v", err) + } + + runnerStr := string(runnerContent) + if strings.Contains(runnerStr, "core/") { + t.Errorf("runner-0 should not contain repo-root prefix 'core/', got:\n%s", runnerStr) + } +} From 5684e3276ce6ada5c2508caa28adbbb331147715 Mon Sep 17 00:00:00 2001 From: Andrey Marchenko Date: Mon, 16 Feb 2026 13:40:28 +0100 Subject: [PATCH 2/4] reuse existing function to determine repo root --- civisibility/utils/git.go | 14 ++++++++++++++ internal/runner/path_normalization.go | 16 +++------------- 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/civisibility/utils/git.go b/civisibility/utils/git.go index 1fd2fec..b0b7d70 100644 --- a/civisibility/utils/git.go +++ b/civisibility/utils/git.go @@ -210,6 +210,20 @@ func getGitVersion() (major int, minor int, patch int, err error) { return gitVersionValue.major, gitVersionValue.minor, gitVersionValue.patch, gitVersionValue.err } +// GetSourceRoot returns the absolute path to the git repository root +// (the result of `git rev-parse --show-toplevel`). +// Returns empty string if git is not available or the current directory is not in a git repo. +func GetSourceRoot() string { + if !isGitFound() { + return "" + } + out, err := execGitString("rev-parse", "--show-toplevel") + if err != nil { + return "" + } + return out +} + // getLocalGitData retrieves information about the local Git repository from the current HEAD. // It gathers details such as the repository URL, current branch, latest commit SHA, author and committer details, and commit message. // diff --git a/internal/runner/path_normalization.go b/internal/runner/path_normalization.go index f628abd..bd1a471 100644 --- a/internal/runner/path_normalization.go +++ b/internal/runner/path_normalization.go @@ -3,21 +3,11 @@ package runner import ( "log/slog" "os" - "os/exec" "path/filepath" "strings" -) -// getGitRootDir returns the absolute path to the git repository root. -// Returns empty string if git is not available or CWD is not in a git repo. -func getGitRootDir() string { - cmd := exec.Command("git", "rev-parse", "--show-toplevel") - out, err := cmd.Output() - if err != nil { - return "" - } - return strings.TrimSpace(string(out)) -} + ciutils "github.com/DataDog/ddtest/civisibility/utils" +) // getCwdSubdirPrefix calculates the relative path from the git root to the // current working directory. Returns empty string if CWD is the git root @@ -27,7 +17,7 @@ func getGitRootDir() string { // Example: git root = /repo, CWD = /repo/packages/core -> returns "packages/core" // Example: git root = /repo, CWD = /repo -> returns "" func getCwdSubdirPrefix() string { - gitRoot := getGitRootDir() + gitRoot := ciutils.GetSourceRoot() if gitRoot == "" { return "" } From 980cffed658e3e7490a8144ae94aa0496eaf1dd3 Mon Sep 17 00:00:00 2001 From: Andrey Marchenko Date: Mon, 16 Feb 2026 13:47:25 +0100 Subject: [PATCH 3/4] make it clear that getCwdSubdirPrefix func is called once --- internal/runner/path_normalization.go | 14 +++++-------- internal/runner/path_normalization_test.go | 23 ++++++++++++++-------- 2 files changed, 20 insertions(+), 17 deletions(-) diff --git a/internal/runner/path_normalization.go b/internal/runner/path_normalization.go index bd1a471..a1ebb17 100644 --- a/internal/runner/path_normalization.go +++ b/internal/runner/path_normalization.go @@ -51,24 +51,20 @@ func getCwdSubdirPrefix() string { return filepath.ToSlash(rel) } -// normalizeTestFilePath converts a test file path that may be repo-root-relative +// normalizeTestFilePathWithPrefix converts a test file path that may be repo-root-relative // to a CWD-relative path. This is needed when running ddtest from a monorepo // subdirectory (e.g., "cd core && ddtest plan") where full test discovery returns // paths relative to the git root (e.g., "core/spec/...") but workers need // paths relative to CWD (e.g., "spec/..."). // +// The subdirPrefix should be computed once via getCwdSubdirPrefix() and reused +// across all paths to avoid repeated git calls. +// // Safety rules: // - If the path does not start with the CWD subdir prefix, it is returned unchanged -// - If CWD is the git root, the path is returned unchanged -// - If the git root cannot be determined, the path is returned unchanged (fail-safe) +// - If CWD is the git root (subdirPrefix is ""), the path is returned unchanged // - Absolute paths are returned unchanged // - Empty paths are returned unchanged -func normalizeTestFilePath(path string) string { - return normalizeTestFilePathWithPrefix(path, getCwdSubdirPrefix()) -} - -// normalizeTestFilePathWithPrefix is the pure version of normalizeTestFilePath -// that accepts a pre-computed subdir prefix. This avoids calling git once per file. func normalizeTestFilePathWithPrefix(path string, subdirPrefix string) string { if path == "" || subdirPrefix == "" { return path diff --git a/internal/runner/path_normalization_test.go b/internal/runner/path_normalization_test.go index 3ee9617..2990171 100644 --- a/internal/runner/path_normalization_test.go +++ b/internal/runner/path_normalization_test.go @@ -20,7 +20,8 @@ func TestNormalizePath_SubdirPrefixMatch_StripsPrefix(t *testing.T) { defer func() { _ = os.Chdir(oldWd) }() _ = os.Chdir(coreDir) - result := normalizeTestFilePath("core/spec/models/order_spec.rb") + prefix := getCwdSubdirPrefix() + result := normalizeTestFilePathWithPrefix("core/spec/models/order_spec.rb", prefix) expected := "spec/models/order_spec.rb" if result != expected { t.Errorf("Expected %q, got %q", expected, result) @@ -40,7 +41,8 @@ func TestNormalizePath_NestedSubdirPrefixMatch_StripsFullPrefix(t *testing.T) { defer func() { _ = os.Chdir(oldWd) }() _ = os.Chdir(nestedDir) - result := normalizeTestFilePath("packages/core/spec/user_spec.rb") + prefix := getCwdSubdirPrefix() + result := normalizeTestFilePathWithPrefix("packages/core/spec/user_spec.rb", prefix) expected := "spec/user_spec.rb" if result != expected { t.Errorf("Expected %q, got %q", expected, result) @@ -60,7 +62,8 @@ func TestNormalizePath_AlreadyRelative_NoChange(t *testing.T) { _ = os.Chdir(coreDir) // This path is already CWD-relative (doesn't start with "core/") - result := normalizeTestFilePath("spec/models/order_spec.rb") + prefix := getCwdSubdirPrefix() + result := normalizeTestFilePathWithPrefix("spec/models/order_spec.rb", prefix) expected := "spec/models/order_spec.rb" if result != expected { t.Errorf("Expected %q (unchanged), got %q", expected, result) @@ -79,7 +82,8 @@ func TestNormalizePath_PrefixMismatch_NoChange(t *testing.T) { defer func() { _ = os.Chdir(oldWd) }() _ = os.Chdir(apiDir) - result := normalizeTestFilePath("core/spec/models/order_spec.rb") + prefix := getCwdSubdirPrefix() + result := normalizeTestFilePathWithPrefix("core/spec/models/order_spec.rb", prefix) expected := "core/spec/models/order_spec.rb" if result != expected { t.Errorf("Expected %q (unchanged), got %q", expected, result) @@ -95,7 +99,8 @@ func TestNormalizePath_AtRepoRoot_NoChange(t *testing.T) { defer func() { _ = os.Chdir(oldWd) }() _ = os.Chdir(repoRoot) - result := normalizeTestFilePath("spec/models/order_spec.rb") + prefix := getCwdSubdirPrefix() + result := normalizeTestFilePathWithPrefix("spec/models/order_spec.rb", prefix) expected := "spec/models/order_spec.rb" if result != expected { t.Errorf("Expected %q (unchanged), got %q", expected, result) @@ -110,7 +115,8 @@ func TestNormalizePath_GitRootUnavailable_NoChange(t *testing.T) { defer func() { _ = os.Chdir(oldWd) }() _ = os.Chdir(tempDir) - result := normalizeTestFilePath("core/spec/models/order_spec.rb") + prefix := getCwdSubdirPrefix() + result := normalizeTestFilePathWithPrefix("core/spec/models/order_spec.rb", prefix) expected := "core/spec/models/order_spec.rb" if result != expected { t.Errorf("Expected %q (unchanged when git root unavailable), got %q", expected, result) @@ -130,14 +136,15 @@ func TestNormalizePath_AbsolutePath_NoChange(t *testing.T) { _ = os.Chdir(coreDir) absPath := "/absolute/path/to/spec.rb" - result := normalizeTestFilePath(absPath) + prefix := getCwdSubdirPrefix() + result := normalizeTestFilePathWithPrefix(absPath, prefix) if result != absPath { t.Errorf("Expected %q (absolute path unchanged), got %q", absPath, result) } } func TestNormalizePath_EmptyPath_NoChange(t *testing.T) { - result := normalizeTestFilePath("") + result := normalizeTestFilePathWithPrefix("", "core") if result != "" { t.Errorf("Expected empty string, got %q", result) } From b83e10a8436454562b071884a0bee296860634ec Mon Sep 17 00:00:00 2001 From: Andrey Marchenko Date: Mon, 16 Feb 2026 14:43:58 +0100 Subject: [PATCH 4/4] add one more integration test to confirm that test skipping logic works correctly when test suites are returned from the backend with same names as they were traced by the library --- internal/runner/dd_test_optimization_test.go | 87 ++++++++++++++++++++ 1 file changed, 87 insertions(+) diff --git a/internal/runner/dd_test_optimization_test.go b/internal/runner/dd_test_optimization_test.go index 92c1c9d..f178fc6 100644 --- a/internal/runner/dd_test_optimization_test.go +++ b/internal/runner/dd_test_optimization_test.go @@ -748,3 +748,90 @@ func TestPrepareTestOptimization_ITRPathNormalization_PrefixMismatchUnchanged(t } } } + +// TestPrepareTestOptimization_ITRSubdir_SkipMatching_WithSuitePathsMatchingCwd +// verifies that when running from a monorepo subdirectory, skip matching works +// correctly: both the API (skippable tests) and framework discovery use the same +// CWD-relative Suite names (e.g. "Spree::Role at ./spec/models/role_spec.rb"), +// while SuiteSourceFile is repo-root-relative (e.g. "core/spec/models/role_spec.rb") +// and needs normalization for worker splitting. +func TestPrepareTestOptimization_ITRSubdir_SkipMatching_WithSuitePathsMatchingCwd(t *testing.T) { + ctx := context.Background() + + // Create a temp monorepo: repoRoot/core/spec/models/ + repoRoot := t.TempDir() + initGitRepo(t, repoRoot) + + coreDir := filepath.Join(repoRoot, "core") + _ = os.MkdirAll(filepath.Join(coreDir, "spec", "models"), 0755) + _ = os.WriteFile(filepath.Join(coreDir, "spec", "models", "role_spec.rb"), []byte("# spec"), 0644) + _ = os.WriteFile(filepath.Join(coreDir, "spec", "models", "order_spec.rb"), []byte("# spec"), 0644) + + // chdir into the subdirectory (simulating: cd core && ddtest plan) + oldWd, _ := os.Getwd() + defer func() { _ = os.Chdir(oldWd) }() + _ = os.Chdir(coreDir) + + // Both framework discovery and API use CWD-relative Suite names. + // SuiteSourceFile is repo-root-relative (comes from tracer's test discovery mode). + mockFramework := &MockFramework{ + FrameworkName: "rspec", + Tests: []testoptimization.Test{ + {Suite: "Spree::Role at ./spec/models/role_spec.rb", Name: "should be valid", Parameters: "", SuiteSourceFile: "core/spec/models/role_spec.rb"}, + {Suite: "Spree::Role at ./spec/models/role_spec.rb", Name: "should have permissions", Parameters: "", SuiteSourceFile: "core/spec/models/role_spec.rb"}, + {Suite: "Order at ./spec/models/order_spec.rb", Name: "should be valid", Parameters: "", SuiteSourceFile: "core/spec/models/order_spec.rb"}, + }, + } + + mockPlatform := &MockPlatform{ + PlatformName: "ruby", + Tags: map[string]string{"platform": "ruby"}, + Framework: mockFramework, + } + + mockPlatformDetector := &MockPlatformDetector{Platform: mockPlatform} + + // API returns skippable tests with the same CWD-relative Suite names + roleTest1 := testoptimization.Test{ + Suite: "Spree::Role at ./spec/models/role_spec.rb", Name: "should be valid", Parameters: "", + } + roleTest2 := testoptimization.Test{ + Suite: "Spree::Role at ./spec/models/role_spec.rb", Name: "should have permissions", Parameters: "", + } + mockOptimizationClient := &MockTestOptimizationClient{ + SkippableTests: map[string]bool{ + roleTest1.FQN(): true, + roleTest2.FQN(): true, + }, + } + + runner := NewWithDependencies(mockPlatformDetector, mockOptimizationClient, newDefaultMockCIProviderDetector()) + + err := runner.PrepareTestOptimization(ctx) + if err != nil { + t.Fatalf("PrepareTestOptimization() should not return error, got: %v", err) + } + + // 2 of 3 tests should be skipped (the role_spec.rb tests) + expectedSkippablePercentage := float64(2) / float64(3) * 100.0 + if runner.skippablePercentage != expectedSkippablePercentage { + t.Errorf("Expected skippablePercentage=%.2f%%, got %.2f%%", + expectedSkippablePercentage, runner.skippablePercentage) + } + + // Only order_spec.rb should remain in testFiles (role_spec.rb tests were skipped). + // The SuiteSourceFile path should be normalized from "core/spec/..." to "spec/..." (CWD-relative). + expectedFiles := map[string]bool{ + "spec/models/order_spec.rb": true, + } + + if len(runner.testFiles) != 1 { + t.Fatalf("Expected 1 test file (only order_spec.rb), got %d: %v", len(runner.testFiles), runner.testFiles) + } + + for file := range runner.testFiles { + if !expectedFiles[file] { + t.Errorf("Unexpected test file path %q", file) + } + } +}