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/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..f178fc6 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,343 @@ 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) + } + } +} + +// 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) + } + } +} diff --git a/internal/runner/path_normalization.go b/internal/runner/path_normalization.go new file mode 100644 index 0000000..a1ebb17 --- /dev/null +++ b/internal/runner/path_normalization.go @@ -0,0 +1,91 @@ +package runner + +import ( + "log/slog" + "os" + "path/filepath" + "strings" + + 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 +// 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 := ciutils.GetSourceRoot() + 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) +} + +// 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 (subdirPrefix is ""), the path is returned unchanged +// - Absolute paths are returned unchanged +// - Empty paths are returned unchanged +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..2990171 --- /dev/null +++ b/internal/runner/path_normalization_test.go @@ -0,0 +1,170 @@ +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) + + 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) + } +} + +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) + + 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) + } +} + +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/") + 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) + } +} + +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) + + 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) + } +} + +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) + + 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) + } +} + +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) + + 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) + } +} + +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" + 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 := normalizeTestFilePathWithPrefix("", "core") + 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) + } +}