From 5b5a0983fd26a59d4726b837aa745c624cfa0fe4 Mon Sep 17 00:00:00 2001 From: braydonk Date: Tue, 11 Apr 2023 21:49:08 -0400 Subject: [PATCH] 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. --- path_collector.go | 9 +- path_collector_test.go | 379 +++++++++++++++++++++++++---------------- 2 files changed, 231 insertions(+), 157 deletions(-) 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 } 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 }