diff --git a/internal/discovery/discovery.go b/internal/discovery/discovery.go index d1bfdf9..5a0d410 100644 --- a/internal/discovery/discovery.go +++ b/internal/discovery/discovery.go @@ -32,8 +32,11 @@ const ( var excludedDirs = []string{"node_modules"} -type Excluder struct { - pattern string +type TestFileSetMatcher struct { + includeMatcher utils.PathMatcher + excludeMatcher utils.PathMatcher + explicitFiles map[string]struct{} + matchAll bool } type TestFileSet struct { @@ -43,12 +46,16 @@ type TestFileSet struct { func ResolveTestFiles(pattern, excludePattern string) (TestFileSet, error) { testFiles := TestFileSet{Pattern: pattern} - if utils.NormalizePattern(excludePattern) == "" { + testFileMatcher, err := NewTestFileSetMatcher(TestFileSet{}, excludePattern) + if err != nil { + return TestFileSet{}, err + } + if testFileMatcher.excludeEmpty() { slog.Info("Using test discovery pattern", "pattern", pattern) return testFiles, nil } - filteredFiles, err := DiscoverTestFiles(pattern, excludePattern) + filteredFiles, err := discoverTestFiles(pattern, testFileMatcher) if err != nil { return TestFileSet{}, err } @@ -80,36 +87,87 @@ func (t TestFileSet) Empty() bool { return t.UseExplicitFiles() && len(t.ExplicitFiles) == 0 } -func NewExcluder(pattern string) (Excluder, error) { - normalized := utils.NormalizePattern(pattern) - if normalized == "" { - return Excluder{}, nil +func NewTestFileSetMatcher(testFiles TestFileSet, excludePattern string) (TestFileSetMatcher, error) { + excludeMatcher, err := utils.NewPathMatcher(excludePattern) + if err != nil { + return TestFileSetMatcher{}, fmt.Errorf("invalid tests exclude pattern %q: %w", excludePattern, err) + } + + if testFiles.UseExplicitFiles() { + explicitFiles := make(map[string]struct{}, len(testFiles.ExplicitFiles)) + for _, testFile := range testFiles.ExplicitFiles { + normalized := utils.NormalizePath(testFile) + if normalized != "" { + explicitFiles[normalized] = struct{}{} + } + } + return TestFileSetMatcher{ + excludeMatcher: excludeMatcher, + explicitFiles: explicitFiles, + }, nil + } + + matcher, err := utils.NewPathMatcher(testFiles.Pattern) + if err != nil { + return TestFileSetMatcher{}, fmt.Errorf("invalid tests location pattern %q: %w", testFiles.Pattern, err) } - if !doublestar.ValidatePattern(normalized) { - return Excluder{}, fmt.Errorf("invalid tests exclude pattern %q", pattern) + if matcher.Empty() { + return TestFileSetMatcher{ + excludeMatcher: excludeMatcher, + matchAll: true, + }, nil } - return Excluder{pattern: normalized}, nil + return TestFileSetMatcher{ + includeMatcher: matcher, + excludeMatcher: excludeMatcher, + }, nil } -func (e Excluder) Match(path string) bool { - if e.pattern == "" { +func (m TestFileSetMatcher) Match(path string) bool { + return m.MatchNormalizedPath(utils.NormalizePath(path)) +} + +func (m TestFileSetMatcher) MatchNormalizedPath(normalizedPath string) bool { + if normalizedPath == "" { + return false + } + if m.excludesNormalizedPath(normalizedPath) { return false } - return doublestar.MatchUnvalidated(e.pattern, utils.NormalizePath(path)) + if m.matchAll { + return true + } + if m.explicitFiles != nil { + _, ok := m.explicitFiles[normalizedPath] + return ok + } + return m.includeMatcher.MatchNormalizedPath(normalizedPath) +} + +func (m TestFileSetMatcher) excludeEmpty() bool { + return m.excludeMatcher.Empty() +} + +func (m TestFileSetMatcher) excludesNormalizedPath(normalizedPath string) bool { + return m.excludeMatcher.MatchNormalizedPath(normalizedPath) } func DiscoverTestFiles(includePattern, excludePattern string) ([]string, error) { - excluder, err := NewExcluder(excludePattern) + testFileMatcher, err := NewTestFileSetMatcher(TestFileSet{}, excludePattern) if err != nil { return nil, err } + return discoverTestFiles(includePattern, testFileMatcher) +} +func discoverTestFiles(includePattern string, testFileMatcher TestFileSetMatcher) ([]string, error) { normalizedIncludePattern := normalizeDiscoveryPattern(includePattern) if normalizedIncludePattern == "" { return []string{}, nil } - if !doublestar.ValidatePattern(normalizedIncludePattern) { - return nil, fmt.Errorf("failed to discover test files with pattern %q: %w", includePattern, doublestar.ErrBadPattern) + includeMatcher, err := utils.NewNormalizedPathMatcher(normalizedIncludePattern) + if err != nil { + return nil, fmt.Errorf("failed to discover test files with pattern %q: %w", includePattern, err) } walkRoot := discoveryWalkRoot(normalizedIncludePattern) @@ -136,7 +194,7 @@ func DiscoverTestFiles(includePattern, excludePattern string) ([]string, error) return nil } - if excluder.Match(normalizedPath) { + if testFileMatcher.excludesNormalizedPath(normalizedPath) { if entry.IsDir() { return filepath.SkipDir } @@ -147,7 +205,7 @@ func DiscoverTestFiles(includePattern, excludePattern string) ([]string, error) return nil } - if doublestar.MatchUnvalidated(normalizedIncludePattern, normalizedPath) { + if includeMatcher.MatchNormalizedPath(normalizedPath) { testFiles = append(testFiles, normalizedPath) } return nil diff --git a/internal/discovery/discovery_test.go b/internal/discovery/discovery_test.go index fa0a29d..68828f2 100644 --- a/internal/discovery/discovery_test.go +++ b/internal/discovery/discovery_test.go @@ -236,6 +236,73 @@ func TestResolveTestFilesWithoutExcludePattern(t *testing.T) { } } +func TestTestFileSetMatcherWithPattern(t *testing.T) { + matcher, err := NewTestFileSetMatcher(TestFileSet{Pattern: " ./spec/**/*_spec.rb "}, "") + if err != nil { + t.Fatalf("NewTestFileSetMatcher() returned error: %v", err) + } + + if !matcher.Match("./spec/models/user_spec.rb") { + t.Fatal("expected matcher to match normalized path") + } + if !matcher.MatchNormalizedPath("spec/models/user_spec.rb") { + t.Fatal("expected matcher to match already-normalized path") + } + if matcher.MatchNormalizedPath("gems/lint_rules/spec/rule_spec.rb") { + t.Fatal("expected matcher not to match outside path") + } +} + +func TestTestFileSetMatcherWithExplicitFiles(t *testing.T) { + matcher, err := NewTestFileSetMatcher(TestFileSet{ + ExplicitFiles: []string{ + "./spec/models/user_spec.rb", + "./spec/system/checkout_spec.rb", + }, + }, "spec/system/**/*_spec.rb") + if err != nil { + t.Fatalf("NewTestFileSetMatcher() returned error: %v", err) + } + + if !matcher.MatchNormalizedPath("spec/models/user_spec.rb") { + t.Fatal("expected matcher to match explicit file") + } + if matcher.MatchNormalizedPath("spec/system/checkout_spec.rb") { + t.Fatal("expected matcher not to match excluded explicit file") + } +} + +func TestTestFileSetMatcherWithEmptyPatternMatchesAll(t *testing.T) { + matcher, err := NewTestFileSetMatcher(TestFileSet{}, "") + if err != nil { + t.Fatalf("NewTestFileSetMatcher() returned error: %v", err) + } + + if !matcher.MatchNormalizedPath("spec/models/user_spec.rb") { + t.Fatal("expected empty pattern matcher to match all paths") + } +} + +func TestTestFileSetMatcherWithExcludePattern(t *testing.T) { + matcher, err := NewTestFileSetMatcher( + TestFileSet{Pattern: "spec/**/*_spec.rb"}, + "spec/system/**/*_spec.rb", + ) + if err != nil { + t.Fatalf("NewTestFileSetMatcher() returned error: %v", err) + } + + if !matcher.MatchNormalizedPath("spec/models/user_spec.rb") { + t.Fatal("expected matcher to match included path") + } + if matcher.MatchNormalizedPath("spec/system/checkout_spec.rb") { + t.Fatal("expected matcher not to match excluded path") + } + if matcher.MatchNormalizedPath("gems/lint_rules/spec/rule_spec.rb") { + t.Fatal("expected matcher not to match outside path") + } +} + func TestResolveTestFilesWithExcludePattern(t *testing.T) { root := createDiscoveryFixture(t) t.Chdir(root) diff --git a/internal/planner/discovered_tests.go b/internal/planner/discovered_tests.go index ec3f771..79d343d 100644 --- a/internal/planner/discovered_tests.go +++ b/internal/planner/discovered_tests.go @@ -22,9 +22,10 @@ func (tp *TestPlanner) resetDiscoveryResults() { func (tp *TestPlanner) recordFullDiscoveryResults( discoveredTests []testoptimization.Test, + resolvedTestFiles discovery.TestFileSet, skippableMatcher skippableMatcher, ) error { - excluder, err := discovery.NewExcluder(settings.GetTestsExcludePattern()) + testFileMatcher, err := discovery.NewTestFileSetMatcher(resolvedTestFiles, settings.GetTestsExcludePattern()) if err != nil { return err } @@ -54,10 +55,10 @@ func (tp *TestPlanner) recordFullDiscoveryResults( for _, test := range discoveredTests { normalizedSourceFile := utils.StripCwdSubdirPrefix(test.SuiteSourceFile) normalizedSourceFile = utils.NormalizePath(normalizedSourceFile) - // Full discovery should already receive filtered files when exclude is configured. - // Keep this planner-side guard so normalized tracer-reported paths cannot re-enter - // the runnable file set or suite aggregates. - if normalizedSourceFile != "" && excluder.Match(normalizedSourceFile) { + // Full discovery receives the resolved test selection, but frameworks can still + // report extra tests loaded by process startup. Keep this planner-side guard so + // out-of-selection paths cannot enter the runnable file set or suite aggregates. + if normalizedSourceFile != "" && !testFileMatcher.MatchNormalizedPath(normalizedSourceFile) { excludedTestsCount++ continue } @@ -161,14 +162,14 @@ func (tp *TestPlanner) recordAppliedSkippable(match skippableMatch) { } func (tp *TestPlanner) recordFastDiscoveryFallbackFiles(discoveredTestFiles []string) error { - excluder, err := discovery.NewExcluder(settings.GetTestsExcludePattern()) + testFileMatcher, err := discovery.NewTestFileSetMatcher(discovery.TestFileSet{}, settings.GetTestsExcludePattern()) if err != nil { return err } for _, testFile := range discoveredTestFiles { normalizedTestFile := utils.NormalizePath(testFile) - if normalizedTestFile != "" && !excluder.Match(normalizedTestFile) { + if normalizedTestFile != "" && testFileMatcher.MatchNormalizedPath(normalizedTestFile) { tp.testFiles[normalizedTestFile] = struct{}{} } } diff --git a/internal/planner/planner.go b/internal/planner/planner.go index 34d2230..3f4ac16 100644 --- a/internal/planner/planner.go +++ b/internal/planner/planner.go @@ -435,7 +435,7 @@ func (tp *TestPlanner) PreparePlanningData(ctx context.Context) error { // backend data cancels it, use it even when TIA has no skips: full discovery // is more precise than fast file discovery. if fullDiscoverySucceeded { - if err := tp.recordFullDiscoveryResults(discoveredTests, skipMatcher); err != nil { + if err := tp.recordFullDiscoveryResults(discoveredTests, resolvedTestFiles, skipMatcher); err != nil { return err } selectedDiscoveryMode = discoveryModeFull diff --git a/internal/planner/planner_test.go b/internal/planner/planner_test.go index b0b59e2..ddb356a 100644 --- a/internal/planner/planner_test.go +++ b/internal/planner/planner_test.go @@ -1066,8 +1066,9 @@ func TestTestPlanner_PreparePlanningData_ForceFullDiscoveryKeepsRunningWithNoTIA setPlannerForceFullTestDiscovery(t, true) mockFramework := &MockFramework{ - FrameworkName: "rspec", - TestFiles: []string{"spec/fast_only_spec.rb"}, + FrameworkName: "rspec", + TestPatternValue: "spec/**/*_spec.rb", + TestFiles: []string{"spec/fast_only_spec.rb"}, Tests: []testoptimization.Test{ {Module: "rspec", Suite: "Discovered", Name: "test", SuiteSourceFile: "spec/discovered_spec.rb"}, }, @@ -1099,6 +1100,109 @@ func TestTestPlanner_PreparePlanningData_ForceFullDiscoveryKeepsRunningWithNoTIA } } +func TestTestPlanner_Plan_ForceFullDiscoveryFiltersPlanArtifactsToTestsLocation(t *testing.T) { + t.Chdir(t.TempDir()) + setPlannerForceFullTestDiscovery(t, true) + testsLocation := "spec/engines/billing/**/*_spec.rb" + setPlannerTestsLocation(t, testsLocation) + t.Setenv("DD_TEST_OPTIMIZATION_RUNNER_MIN_PARALLELISM", "2") + t.Setenv("DD_TEST_OPTIMIZATION_RUNNER_MAX_PARALLELISM", "2") + t.Setenv("DD_TEST_OPTIMIZATION_RUNNER_REPORT_ENABLED", "false") + settings.Init() + + insideRunnableFile := "spec/engines/billing/payment_spec.rb" + insideSkippedFile := "spec/engines/billing/skipped_spec.rb" + outsideRunnableFile := "gems/lint_rules/spec/rule_spec.rb" + for _, file := range []string{insideRunnableFile, insideSkippedFile, outsideRunnableFile} { + if err := os.MkdirAll(filepath.Dir(file), 0o755); err != nil { + t.Fatalf("failed to create directory for %s: %v", file, err) + } + if err := os.WriteFile(file, []byte("# spec\n"), 0o644); err != nil { + t.Fatalf("failed to write %s: %v", file, err) + } + } + + skippedSuite := "Billing::Skipped" + mockFramework := &MockFramework{ + FrameworkName: "rspec", + TestPatternValue: testsLocation, + TestFiles: []string{insideRunnableFile, insideSkippedFile, outsideRunnableFile}, + Tests: []testoptimization.Test{ + {Module: "rspec", Suite: "Billing::Payment", Name: "runs", SuiteSourceFile: insideRunnableFile}, + {Module: "rspec", Suite: skippedSuite, Name: "skips", SuiteSourceFile: insideSkippedFile}, + {Module: "rspec", Suite: "LintRules::Rule", Name: "runs", SuiteSourceFile: outsideRunnableFile}, + }, + } + mockPlatform := &MockPlatform{ + PlatformName: "ruby", + Tags: map[string]string{"language": "ruby"}, + Framework: mockFramework, + TestLevel: settings.TestSkippingLevelSuite, + } + mockOptimizationClient := &MockTestOptimizationClient{ + Settings: testOptimizationSettings(true, true, false), + Skippables: api.Skippables{ + Suites: api.SkippableSuites{ + {Module: "rspec", Suite: skippedSuite}: true, + }, + }, + } + + runner := NewWithDependencies( + &MockPlatformDetector{Platform: mockPlatform}, + mockOptimizationClient, + newDefaultMockCIProviderDetector(), + ) + + if err := runner.Plan(context.Background()); err != nil { + t.Fatalf("Plan() should not return error, got: %v", err) + } + + if len(mockFramework.DiscoverTestsFiles) != 1 { + t.Fatalf("expected full discovery to run once, got %d calls", len(mockFramework.DiscoverTestsFiles)) + } + if mockFramework.DiscoverTestsFiles[0].Pattern != testsLocation { + t.Fatalf("expected full discovery pattern %q, got %q", testsLocation, mockFramework.DiscoverTestsFiles[0].Pattern) + } + if _, ok := runner.testFileWeights[insideRunnableFile]; !ok { + t.Fatalf("expected in-location runnable file in weights, got %+v", runner.testFileWeights) + } + if _, ok := runner.testFileWeights[insideSkippedFile]; ok { + t.Fatalf("expected fully skipped in-location file to be omitted, got %+v", runner.testFileWeights) + } + if _, ok := runner.testFileWeights[outsideRunnableFile]; ok { + t.Fatalf("expected out-of-location runnable file to be omitted, got %+v", runner.testFileWeights) + } + + expectedTestFiles := insideRunnableFile + "\n" + assertFileContent(t, constants.TestFilesOutputPath, expectedTestFiles) + + entries, err := os.ReadDir(constants.TestsSplitDir) + if err != nil { + t.Fatalf("failed to read tests split dir: %v", err) + } + foundInsideRunnableFile := false + for _, entry := range entries { + if entry.IsDir() { + continue + } + content, err := os.ReadFile(filepath.Join(constants.TestsSplitDir, entry.Name())) + if err != nil { + t.Fatalf("failed to read split file %s: %v", entry.Name(), err) + } + text := string(content) + if strings.Contains(text, insideRunnableFile) { + foundInsideRunnableFile = true + } + if strings.Contains(text, insideSkippedFile) || strings.Contains(text, outsideRunnableFile) { + t.Fatalf("expected split file %s to only contain files under TESTS_LOCATION after suite skips, got %q", entry.Name(), text) + } + } + if !foundInsideRunnableFile { + t.Fatalf("expected tests split artifacts to contain %s", insideRunnableFile) + } +} + func TestTestPlanner_PreparePlanningData_ForceFullDiscoveryUnsupportedFrameworkUsesSuiteFallback(t *testing.T) { t.Chdir(t.TempDir()) setPlannerForceFullTestDiscovery(t, true) @@ -1224,7 +1328,7 @@ func TestTestPlanner_RecordFullDiscoveryResults_SuiteSkippablesSkipDiscoveredTes }, } - if err := runner.recordFullDiscoveryResults(tests, newSkippableMatcher(skippables, nil)); err != nil { + if err := runner.recordFullDiscoveryResults(tests, discovery.TestFileSet{Pattern: "**/*"}, newSkippableMatcher(skippables, nil)); err != nil { t.Fatalf("recordFullDiscoveryResults() should not fail, got: %v", err) } @@ -1897,7 +2001,8 @@ func TestTestPlanner_PreparePlanningData_Success(t *testing.T) { // Setup mocks mockFramework := &MockFramework{ - FrameworkName: "rspec", + FrameworkName: "rspec", + TestPatternValue: "test/**/*_test.rb", TestFiles: []string{ "test/file1_test.rb", "test/file2_test.rb", @@ -2428,8 +2533,9 @@ func TestTestPlanner_PreparePlanningData_UsesCompletedFullDiscoveryWhenNoTIASkip t.Cleanup(environment.ResetCITags) mockFramework := &MockFramework{ - FrameworkName: "rspec", - TestFiles: []string{"spec/fast_discovery_spec.rb"}, + FrameworkName: "rspec", + TestPatternValue: "spec/**/*_spec.rb", + TestFiles: []string{"spec/fast_discovery_spec.rb"}, Tests: []testoptimization.Test{ {Module: "rspec", Suite: "FullDiscoverySuite", Name: "test1", SuiteSourceFile: "spec/full_discovery_spec.rb"}, }, @@ -3484,7 +3590,7 @@ func TestTestPlanner_RecordFullDiscoveryResults_AppliesExcludeAfterSubdirNormali }, } - err := runner.recordFullDiscoveryResults(tests, newSkippableMatcher(api.NewSkippables(), nil)) + err := runner.recordFullDiscoveryResults(tests, discovery.TestFileSet{Pattern: "**/*"}, newSkippableMatcher(api.NewSkippables(), nil)) if err != nil { t.Fatalf("recordFullDiscoveryResults() should not fail, got: %v", err) } diff --git a/internal/utils/path.go b/internal/utils/path.go index 09f9344..b219c7b 100644 --- a/internal/utils/path.go +++ b/internal/utils/path.go @@ -6,6 +6,7 @@ package utils import ( + "fmt" "log/slog" "os" "path/filepath" @@ -13,6 +14,7 @@ import ( "sync" "github.com/DataDog/ddtest/internal/git" + "github.com/bmatcuk/doublestar/v4" ) var cwdSubdirPrefix = sync.OnceValue(computeCwdSubdirPrefix) @@ -117,6 +119,44 @@ func NormalizePattern(pattern string) string { return trimLeadingCurrentDir(normalized) } +type PathMatcher struct { + pattern string +} + +func NewPathMatcher(pattern string) (PathMatcher, error) { + normalized := NormalizePattern(pattern) + return newPathMatcher(normalized, pattern) +} + +func NewNormalizedPathMatcher(normalizedPattern string) (PathMatcher, error) { + return newPathMatcher(normalizedPattern, normalizedPattern) +} + +func newPathMatcher(normalizedPattern string, originalPattern string) (PathMatcher, error) { + if normalizedPattern == "" { + return PathMatcher{}, nil + } + if !doublestar.ValidatePattern(normalizedPattern) { + return PathMatcher{}, fmt.Errorf("invalid path pattern %q: %w", originalPattern, doublestar.ErrBadPattern) + } + return PathMatcher{pattern: normalizedPattern}, nil +} + +func (m PathMatcher) Empty() bool { + return m.pattern == "" +} + +func (m PathMatcher) Match(path string) bool { + return m.MatchNormalizedPath(NormalizePath(path)) +} + +func (m PathMatcher) MatchNormalizedPath(normalizedPath string) bool { + if m.pattern == "" || normalizedPath == "" { + return false + } + return doublestar.MatchUnvalidated(m.pattern, normalizedPath) +} + func trimLeadingCurrentDir(path string) string { for strings.HasPrefix(path, "./") { path = strings.TrimPrefix(path, "./") diff --git a/internal/utils/path_test.go b/internal/utils/path_test.go index 5868020..bd85a27 100644 --- a/internal/utils/path_test.go +++ b/internal/utils/path_test.go @@ -63,6 +63,54 @@ func TestNormalizePattern(t *testing.T) { } } +func TestPathMatcher(t *testing.T) { + matcher, err := NewPathMatcher(" ./spec/**/*_spec.rb ") + if err != nil { + t.Fatalf("NewPathMatcher() returned error: %v", err) + } + if matcher.Empty() { + t.Fatal("NewPathMatcher() returned empty matcher") + } + if !matcher.Match("./spec/models/user_spec.rb") { + t.Fatal("expected matcher to match normalized path") + } + if !matcher.MatchNormalizedPath("spec/models/user_spec.rb") { + t.Fatal("expected matcher to match already-normalized path") + } + if matcher.MatchNormalizedPath("test/models/user_test.rb") { + t.Fatal("expected matcher not to match outside path") + } +} + +func TestPathMatcherEmptyPattern(t *testing.T) { + matcher, err := NewPathMatcher(" ./ ") + if err != nil { + t.Fatalf("NewPathMatcher() returned error: %v", err) + } + if !matcher.Empty() { + t.Fatal("expected empty matcher") + } + if matcher.MatchNormalizedPath("spec/models/user_spec.rb") { + t.Fatal("empty matcher should not match paths") + } +} + +func TestPathMatcherInvalidPattern(t *testing.T) { + if _, err := NewPathMatcher("["); err == nil { + t.Fatal("expected invalid pattern error") + } +} + +func TestNewNormalizedPathMatcher(t *testing.T) { + matcher, err := NewNormalizedPathMatcher("spec/**/*_spec.rb") + if err != nil { + t.Fatalf("NewNormalizedPathMatcher() returned error: %v", err) + } + if !matcher.MatchNormalizedPath("spec/models/user_spec.rb") { + t.Fatal("expected matcher to match already-normalized path") + } +} + func TestStripCwdSubdirPrefix_SubdirPrefixMatch_StripsPrefix(t *testing.T) { repoRoot := t.TempDir() initGitRepoInDir(t, repoRoot)