Repository: google/yamlfmt
Pull request #111 (2023-04-12 02:28:15)

yamlfmt_test: re-enable and fix doublestar tests
Doublestar tests have been disabled for quite a long time and I honestly forget why they were. Regardless, they were missing some features to be properly robust anyway. This PR re-enables the tests and adds two test cases relevant to #97 that will be skipped individually for now.


Patch:
diff --git a/path_collector.go b/path_collector.go
index 2f9fa68..e8b265b 100644
--- a/path_collector.go
+++ b/path_collector.go
@@ -2,7 +2,6 @@ package yamlfmt
 
 import (
 	"io/fs"
-	"log"
 	"os"
 	"path/filepath"
 	"strings"
@@ -116,13 +115,7 @@ func (c *DoublestarCollector) CollectPaths() ([]string, error) {
 		}
 		excluded := false
 		for _, pattern := range c.Exclude {
-			absPath, err := filepath.Abs(path)
-			if err != nil {
-				// I wonder how this could ever happen...
-				log.Printf("could not create absolute path for %s: %v", path, err)
-				continue
-			}
-			match, err := doublestar.PathMatch(filepath.Clean(pattern), absPath)
+			match, err := doublestar.PathMatch(filepath.Clean(pattern), path)
 			if err != nil {
 				return nil, err
 			}


Test patch:
diff --git a/path_collector_test.go b/path_collector_test.go
index d4e1629..33ef003 100644
--- a/path_collector_test.go
+++ b/path_collector_test.go
@@ -2,23 +2,18 @@ package yamlfmt_test
 
 import (
 	"fmt"
+	"os"
 	"path/filepath"
-	"reflect"
+	"strings"
 	"testing"
 
 	"github.com/google/yamlfmt"
+	"github.com/google/yamlfmt/internal/collections"
 	"github.com/google/yamlfmt/internal/tempfile"
 )
 
-func TestCollectPaths(t *testing.T) {
-	testCases := []struct {
-		name            string
-		files           []tempfile.Path
-		includePatterns []string
-		excludePatterns []string
-		extensions      []string
-		expectedFiles   map[string]struct{}
-	}{
+func TestFilepathCollector(t *testing.T) {
+	testCaseTable{
 		{
 			name: "finds direct paths",
 			files: []tempfile.Path{
@@ -26,12 +21,12 @@ func TestCollectPaths(t *testing.T) {
 				{FileName: "y.yaml"},
 				{FileName: "z.yml"},
 			},
-			includePatterns: []string{
-				"x.yaml",
-				"y.yaml",
-				"z.yml",
+			includePatterns: testPatterns{
+				{pattern: "x.yaml"},
+				{pattern: "y.yaml"},
+				{pattern: "z.yml"},
 			},
-			expectedFiles: map[string]struct{}{
+			expectedFiles: collections.Set[string]{
 				"x.yaml": {},
 				"y.yaml": {},
 				"z.yml":  {},
@@ -45,14 +40,14 @@ func TestCollectPaths(t *testing.T) {
 				{FileName: "a/y.yaml"},
 				{FileName: "a/z.yml"},
 			},
-			includePatterns: []string{
-				"a",
+			includePatterns: testPatterns{
+				{pattern: "a"},
 			},
 			extensions: []string{
 				"yaml",
 				"yml",
 			},
-			expectedFiles: map[string]struct{}{
+			expectedFiles: collections.Set[string]{
 				"a/x.yaml": {},
 				"a/y.yaml": {},
 				"a/z.yml":  {},
@@ -66,11 +61,11 @@ func TestCollectPaths(t *testing.T) {
 				{FileName: "a/y.yaml"},
 				{FileName: "a/z.yml"},
 			},
-			includePatterns: []string{
-				"a/x.yaml",
-				"a/z.yml",
+			includePatterns: testPatterns{
+				{pattern: "a/x.yaml"},
+				{pattern: "a/z.yml"},
 			},
-			expectedFiles: map[string]struct{}{
+			expectedFiles: collections.Set[string]{
 				"a/x.yaml": {},
 				"a/z.yml":  {},
 			},
@@ -87,14 +82,14 @@ func TestCollectPaths(t *testing.T) {
 				{FileName: "a/b/x.yaml"},
 				{FileName: "a/b/y.yml"},
 			},
-			includePatterns: []string{
-				"", // with the test this functionally means the whole temp dir
+			includePatterns: testPatterns{
+				{pattern: ""}, // with the test this functionally means the whole temp dir
 			},
 			extensions: []string{
 				"yaml",
 				"yml",
 			},
-			expectedFiles: map[string]struct{}{
+			expectedFiles: collections.Set[string]{
 				"x.yml":      {},
 				"y.yml":      {},
 				"z.yaml":     {},
@@ -115,18 +110,18 @@ func TestCollectPaths(t *testing.T) {
 				{FileName: "a/b/x.yaml"},
 				{FileName: "a/b/y.yml"},
 			},
-			includePatterns: []string{
-				"", // with the test this functionally means the whole temp dir
+			includePatterns: testPatterns{
+				{pattern: ""}, // with the test this functionally means the whole temp dir
 			},
-			excludePatterns: []string{
-				"x.yml",
-				"a/x.yaml",
+			excludePatterns: testPatterns{
+				{pattern: "x.yml"},
+				{pattern: "a/x.yaml"},
 			},
 			extensions: []string{
 				"yaml",
 				"yml",
 			},
-			expectedFiles: map[string]struct{}{
+			expectedFiles: collections.Set[string]{
 				"y.yml":      {},
 				"z.yaml":     {},
 				"a/b/x.yaml": {},
@@ -136,85 +131,39 @@ func TestCollectPaths(t *testing.T) {
 		{
 			name: "exclude directory",
 			files: []tempfile.Path{
-				{FileName: "a", IsDir: true},
-				{FileName: "a/b", IsDir: true},
 				{FileName: "x.yml"},
 				{FileName: "y.yml"},
 				{FileName: "z.yaml"},
+
+				{FileName: "a", IsDir: true},
 				{FileName: "a/x.yaml"},
+
+				{FileName: "a/b", IsDir: true},
 				{FileName: "a/b/x.yaml"},
 				{FileName: "a/b/y.yml"},
 			},
-			includePatterns: []string{
-				"", // with the test this functionally means the whole temp dir
+			includePatterns: testPatterns{
+				{pattern: ""}, // with the test this functionally means the whole temp dir
 			},
-			excludePatterns: []string{
-				"a/b",
+			excludePatterns: testPatterns{
+				{pattern: "a/b"},
 			},
 			extensions: []string{
 				"yaml",
 				"yml",
 			},
-			expectedFiles: map[string]struct{}{
+			expectedFiles: collections.Set[string]{
 				"x.yml":    {},
 				"y.yml":    {},
 				"z.yaml":   {},
 				"a/x.yaml": {},
 			},
 		},
-	}
-
-	for _, tc := range testCases {
-		tc := tc
-		t.Run(tc.name, func(t *testing.T) {
-			t.Parallel()
-			tempPath := t.TempDir()
-
-			for _, file := range tc.files {
-				file.BasePath = tempPath
-				if err := file.Create(); err != nil {
-					t.Fatalf("Failed to create file %s: %v", file.BasePath, err)
-				}
-			}
-
-			collector := &yamlfmt.FilepathCollector{
-				Include:    formatTempPaths(tempPath, tc.includePatterns),
-				Exclude:    formatTempPaths(tempPath, tc.excludePatterns),
-				Extensions: tc.extensions,
-			}
-
-			paths, err := collector.CollectPaths()
-			if err != nil {
-				t.Fatalf("CollectPaths failed: %v", err)
-			}
-			if len(paths) != len(tc.expectedFiles) {
-				t.Fatalf("Got %d paths but expected %d", len(paths), len(tc.expectedFiles))
-			}
-
-			filesToFormat := map[string]struct{}{}
-			for _, path := range paths {
-				formatPath, err := filepath.Rel(tempPath, path)
-				if err != nil {
-					t.Fatalf("Path %s could match to path %s", tempPath, path)
-				}
-				filesToFormat[formatPath] = struct{}{}
-			}
-			if !reflect.DeepEqual(filesToFormat, tc.expectedFiles) {
-				t.Fatalf("Expected to receive paths %v but got %v", tc.expectedFiles, filesToFormat)
-			}
-		})
-	}
+	}.runAll(t, useFilepathCollector)
 }
 
-func TestDoublestarCollectPaths(t *testing.T) {
-	t.Skip()
-	testCases := []struct {
-		name            string
-		files           []tempfile.Path
-		includePatterns []string
-		excludePatterns []string
-		expectedFiles   map[string]struct{}
-	}{
+func TestDoublestarCollectorBasic(t *testing.T) {
+	testCaseTable{
 		{
 			name: "no excludes",
 			files: []tempfile.Path{
@@ -222,95 +171,227 @@ func TestDoublestarCollectPaths(t *testing.T) {
 				{FileName: "y.yaml"},
 				{FileName: "z.yaml"},
 			},
-			expectedFiles: map[string]struct{}{
+			includePatterns: testPatterns{
+				{pattern: "**/*.yaml"},
+			},
+			expectedFiles: collections.Set[string]{
 				"x.yaml": {},
 				"y.yaml": {},
 				"z.yaml": {},
 			},
 		},
+	}.runAll(t, useDoublestarCollector)
+}
+
+func TestDoublestarCollectorExcludeDirectory(t *testing.T) {
+	testFiles := []tempfile.Path{
+		{FileName: "x.yaml"},
+
+		{FileName: "y", IsDir: true},
+		{FileName: "y/y.yaml"},
+
+		{FileName: "z", IsDir: true},
+		{FileName: "z/z.yaml"},
+		{FileName: "z/z1.yaml"},
+		{FileName: "z/z2.yaml"},
+	}
+
+	testCaseTable{
 		{
-			name: "does not include directories",
-			files: []tempfile.Path{
-				{FileName: "x.yaml"},
-				{FileName: "y", IsDir: true},
+			name:  "exclude_directory/start with doublestar",
+			files: testFiles,
+			includePatterns: testPatterns{
+				{pattern: "**/*.yaml"},
 			},
-			expectedFiles: map[string]struct{}{
-				"x.yaml": {},
+			excludePatterns: testPatterns{
+				{pattern: "**/z/**/*.yaml", stayRelative: true},
+			},
+			expectedFiles: collections.Set[string]{
+				"x.yaml":   {},
+				"y/y.yaml": {},
 			},
 		},
 		{
-			name: "only include what is asked",
-			files: []tempfile.Path{
-				{FileName: "x.yaml"},
-				{FileName: "y.yaml"},
+			name:            "exclude_directory/relative include and exclude",
+			changeToTempDir: true,
+			files:           testFiles,
+			includePatterns: testPatterns{
+				{pattern: "**/*.yaml", stayRelative: true},
 			},
-			includePatterns: []string{
-				"y.yaml",
+			excludePatterns: testPatterns{
+				{pattern: "z/**/*.yaml", stayRelative: true},
 			},
-			expectedFiles: map[string]struct{}{
-				"y.yaml": {},
+			expectedFiles: collections.Set[string]{
+				"x.yaml":   {},
+				"y/y.yaml": {},
 			},
 		},
 		{
-			name: "exclude what is asked",
-			files: []tempfile.Path{
-				{FileName: "x.yaml"},
-				{FileName: "y.yaml"},
+			name:  "exclude_directory/absolute include and exclude",
+			files: testFiles,
+			includePatterns: testPatterns{
+				{pattern: "**/*.yaml"},
 			},
-			excludePatterns: []string{
-				"y.yaml",
+			excludePatterns: testPatterns{
+				{pattern: "z/**/*.yaml"},
 			},
-			expectedFiles: map[string]struct{}{
-				"x.yaml": {},
+			expectedFiles: collections.Set[string]{
+				"x.yaml":   {},
+				"y/y.yaml": {},
+			},
+		},
+		{
+			name:            "exclude_directory/absolute include relative exclude",
+			skip:            true,
+			changeToTempDir: true,
+			files:           testFiles,
+			includePatterns: testPatterns{
+				{pattern: "**/*.yaml"},
+			},
+			excludePatterns: testPatterns{
+				{pattern: "z/**/*.yaml", stayRelative: true},
+			},
+			expectedFiles: collections.Set[string]{
+				"x.yaml":   {},
+				"y/y.yaml": {},
+			},
+		},
+		{
+			name:            "exclude_directory/relative include absolute exclude",
+			skip:            true,
+			changeToTempDir: true,
+			files:           testFiles,
+			includePatterns: testPatterns{
+				{pattern: "**/*.yaml", stayRelative: true},
+			},
+			excludePatterns: testPatterns{
+				{pattern: "z/**/*.yaml"},
+			},
+			expectedFiles: collections.Set[string]{
+				"x.yaml":   {},
+				"y/y.yaml": {},
 			},
 		},
+	}.runAll(t, useDoublestarCollector)
+}
+
+type testPatterns []struct {
+	pattern      string
+	stayRelative bool
+}
+
+func (tps testPatterns) allPatterns(path string) []string {
+	result := make([]string, len(tps))
+	for i := 0; i < len(tps); i++ {
+		if tps[i].stayRelative {
+			result[i] = tps[i].pattern
+		} else {
+			result[i] = fmt.Sprintf("%s/%s", path, tps[i].pattern)
+		}
 	}
+	return result
+}
 
-	for _, tc := range testCases {
-		tc := tc
-		t.Run(tc.name, func(t *testing.T) {
-			t.Parallel()
-			tempPath := t.TempDir()
+// In some test scenarios we want to ignore whether a pattern is marked stayRelative
+// and always treat them as relative by formatting the base path on them.
+func (tps testPatterns) allPatternsForceAbsolute(path string) []string {
+	result := make([]string, len(tps))
+	for i := 0; i < len(tps); i++ {
+		result[i] = fmt.Sprintf("%s/%s", path, tps[i].pattern)
+	}
+	return result
+}
 
-			for _, file := range tc.files {
-				file.BasePath = tempPath
-				if err := file.Create(); err != nil {
-					t.Fatalf("Failed to create file")
-				}
-			}
+type testCase struct {
+	name            string
+	skip            bool
+	changeToTempDir bool
+	files           []tempfile.Path
+	includePatterns testPatterns
+	extensions      []string
+	excludePatterns testPatterns
+	expectedFiles   collections.Set[string]
+}
 
-			collector := &yamlfmt.DoublestarCollector{
-				Include: formatTempPaths(tempPath, tc.includePatterns),
-				Exclude: formatTempPaths(tempPath, tc.excludePatterns),
-			}
-			if len(collector.Include) == 0 {
-				collector.Include = []string{fmt.Sprintf("%s/**", tempPath)}
-			}
-			if len(collector.Exclude) == 0 {
-				collector.Exclude = []string{}
-			}
+func (tc testCase) run(t *testing.T, makeCollector makeCollectorFunc) {
+	testStartDir, err := os.Getwd()
+	if err != nil {
+		t.Fatalf("could not get working directory: %v", err)
+	}
+	t.Run(tc.name, func(t *testing.T) {
+		if tc.skip {
+			t.Skip()
+		}
+		tempPath := t.TempDir()
 
-			paths, err := collector.CollectPaths()
-			if err != nil {
-				t.Fatalf("CollectDoublestarPathsToFormat failed: %v", err)
-			}
+		if tc.changeToTempDir {
+			os.Chdir(tempPath)
+		}
 
-			filesToFormat := map[string]struct{}{}
-			for _, path := range paths {
-				_, filename := filepath.Split(path)
-				filesToFormat[filename] = struct{}{}
+		for _, file := range tc.files {
+			file.BasePath = tempPath
+			if err := file.Create(); err != nil {
+				t.Fatalf("Failed to create file")
 			}
-			if !reflect.DeepEqual(filesToFormat, tc.expectedFiles) {
-				t.Fatalf("Expected to receive paths %v but got %v", tc.expectedFiles, filesToFormat)
+		}
+
+		collector := makeCollector(tc, tempPath)
+		paths, err := collector.CollectPaths()
+		if err != nil {
+			t.Fatalf("CollectDoublestarPathsToFormat failed: %v", err)
+		}
+
+		filesToFormat := collections.Set[string]{}
+		for _, path := range paths {
+			formatPath := path
+			if strings.HasPrefix(formatPath, "/") {
+				formatPath, err = filepath.Rel(tempPath, path)
+				if err != nil {
+					t.Fatalf("Path %s could not match to path %s", tempPath, path)
+				}
 			}
-		})
+			filesToFormat.Add(formatPath)
+		}
+		if !filesToFormat.Equals(tc.expectedFiles) {
+			t.Fatalf("Expected to receive paths %v\nbut got %v", tc.expectedFiles, filesToFormat)
+		}
+	})
+
+	// Restore the starting directory if we changed in the test.
+	if tc.changeToTempDir {
+		os.Chdir(testStartDir)
+	}
+}
+
+type testCaseTable []testCase
+
+func (tcs testCaseTable) runAll(t *testing.T, makeCollector makeCollectorFunc) {
+	for _, tc := range tcs {
+		tc.run(t, makeCollector)
 	}
 }
 
-func formatTempPaths(tempPath string, patterns []string) []string {
-	formatted := make([]string, len(patterns))
-	for i, pattern := range patterns {
-		formatted[i] = fmt.Sprintf("%s/%s", tempPath, pattern)
+type makeCollectorFunc func(tc testCase, path string) yamlfmt.PathCollector
+
+func useFilepathCollector(tc testCase, path string) yamlfmt.PathCollector {
+	return &yamlfmt.FilepathCollector{
+		Include:    tc.includePatterns.allPatterns(path),
+		Exclude:    tc.excludePatterns.allPatterns(path),
+		Extensions: tc.extensions,
+	}
+}
+
+func useDoublestarCollector(tc testCase, path string) yamlfmt.PathCollector {
+	var includePatterns []string
+	if tc.changeToTempDir {
+		includePatterns = tc.includePatterns.allPatterns(path)
+	} else {
+		// If we didn't change to temp dir, disallow relative paths so we don't pick up
+		// something confusing from the main working directory.
+		includePatterns = tc.includePatterns.allPatternsForceAbsolute(path)
+	}
+	return &yamlfmt.DoublestarCollector{
+		Include: includePatterns,
+		Exclude: tc.excludePatterns.allPatterns(path),
 	}
-	return formatted
 }