From 844a3ec62fe135119e356a09489b8f112a26b001 Mon Sep 17 00:00:00 2001 From: Jaap de Haan <261428+jdehaan@users.noreply.github.com> Date: Fri, 14 Nov 2025 18:47:48 +0000 Subject: [PATCH 01/10] fix: Strip debug symbols from release binaries --- .github/workflows/release.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index aa348db..94fbf20 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -28,7 +28,7 @@ jobs: - name: Build binary run: | mkdir -p dist - GOOS=${{ matrix.goos }} GOARCH=${{ matrix.goarch }} go build -o dist/backup-${{ matrix.goos }}-${{ matrix.goarch }} ./backup/main.go + CGO_ENABLED=0 GOOS=${{ matrix.goos }} GOARCH=${{ matrix.goarch }} go build -ldflags="-s -w" -o dist/backup-${{ matrix.goos }}-${{ matrix.goarch }} ./backup/main.go - name: Generate SHA256 checksum run: | From e1d868fa04670f2182ecb46b796c77cbf14362d7 Mon Sep 17 00:00:00 2001 From: Jaap de Haan <261428+jdehaan@users.noreply.github.com> Date: Sat, 15 Nov 2025 10:40:59 +0000 Subject: [PATCH 02/10] feat: Add sanity checks --- .devcontainer/devcontainer.json | 6 ++-- .github/workflows/go.yml | 12 ++++++- .github/workflows/release.yml | 14 ++++----- .gitignore | 2 +- .vscode/settings.json | 5 +++ Makefile | 55 ++++++++++++++++++++++++++++----- 6 files changed, 75 insertions(+), 19 deletions(-) create mode 100644 .vscode/settings.json diff --git a/.devcontainer/devcontainer.json b/.devcontainer/devcontainer.json index 8f6879e..3caf605 100644 --- a/.devcontainer/devcontainer.json +++ b/.devcontainer/devcontainer.json @@ -1,13 +1,15 @@ { "name": "Go", - "image": "mcr.microsoft.com/devcontainers/go:2-1.24-bookworm", + "image": "mcr.microsoft.com/devcontainers/go:2-1.25-trixie", "features": { "ghcr.io/guiyomh/features/golangci-lint:0": {} }, "customizations": { "vscode": { "extensions": [ - "eamodio.gitlens" + "eamodio.gitlens", + "streetsidesoftware.code-spell-checker", + "esbenp.prettier-vscode" ] } } diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index 63f3f50..2fe17fe 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -19,7 +19,17 @@ jobs: - name: Set up Go uses: actions/setup-go@v4 with: - go-version: '1.24' + go-version: '1.25' + + - name: Sanity check + run: make sanity-check + + - name: Lint + uses: golangci/golangci-lint-action@v8 + with: + version: latest + install-mode: goinstall + args: --timeout=5m --verbose - name: Build run: make build diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 94fbf20..d904683 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -23,18 +23,16 @@ jobs: - name: Set up Go uses: actions/setup-go@v4 with: - go-version: 1.24 + go-version: 1.25 + + - name: Sanity check + run: make sanity-check - name: Build binary - run: | - mkdir -p dist - CGO_ENABLED=0 GOOS=${{ matrix.goos }} GOARCH=${{ matrix.goarch }} go build -ldflags="-s -w" -o dist/backup-${{ matrix.goos }}-${{ matrix.goarch }} ./backup/main.go + run: make release-${{ matrix.goos }}-${{ matrix.goarch }} - name: Generate SHA256 checksum - run: | - for file in dist/*; do - sha256sum "$file" > "$file.sha256"; - done + run: make checksums - name: Upload binaries uses: actions/upload-artifact@v4 diff --git a/.gitignore b/.gitignore index ebfba0f..5bc5994 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,4 @@ BACKLOG/ logs/ *.log -dist/backup +dist/ \ No newline at end of file diff --git a/.vscode/settings.json b/.vscode/settings.json new file mode 100644 index 0000000..0021d94 --- /dev/null +++ b/.vscode/settings.json @@ -0,0 +1,5 @@ +{ + "cSpell.words": [ + "subpaths" + ] +} \ No newline at end of file diff --git a/Makefile b/Makefile index ca548aa..bd56e34 100644 --- a/Makefile +++ b/Makefile @@ -1,16 +1,57 @@ # Makefile to build the project and place the binary in the dist/ directory -.PHONY: build clean deps test +# Build command with common flags +BUILD_CMD = CGO_ENABLED=0 go build -ldflags="-s -w" +PACKAGE = ./backup/main.go -deps: - go mod tidy +.PHONY: build clean test lint tidy checksums release sanity-check check-mod-tidy -build: deps - @mkdir -p dist - go build -o dist/backup ./backup/main.go +check-clean: + @git diff --quiet || (echo "ERROR: Working directory has uncommitted changes." && exit 1) + @echo "OK: Working directory is clean." + +check-mod-tidy: + go mod tidy -diff + @echo "OK: No untidy module files detected." + +sanity-check: check-clean check-mod-tidy + @echo "OK: All sanity checks passed." + +lint: + golangci-lint run ./... test: go test ./... -v +tidy: + go mod tidy + +build: + @mkdir -p dist + $(BUILD_CMD) -o dist/backup $(PACKAGE) + clean: - rm -rf dist \ No newline at end of file + rm -rf dist + +# Build for specific OS and architecture (e.g., make release-linux-amd64) +release-%: + @mkdir -p dist + GOOS=$(word 1,$(subst -, ,$*)) GOARCH=$(word 2,$(subst -, ,$*)) $(BUILD_CMD) -o dist/backup-$* $(PACKAGE) + +checksums: + @for file in dist/*; do \ + if [ "$${file##*.}" != "sha256" ]; then \ + sha256sum "$$file" > "$$file.sha256"; \ + fi; \ + done + +release: release-linux-amd64 release-darwin-amd64 release-windows-amd64 checksums + @echo + @echo "Binaries with sizes and checksums:" + @for file in dist/*; do \ + if [ -f "$$file" ] && [ "$${file##*.}" != "sha256" ]; then \ + size=$$(stat --printf="%s" "$$file"); \ + checksum=$$(cat "$$file.sha256" | awk '{print $$1}'); \ + printf "%-40s %-15s %-64s\n" "$$file" "Size: $$size bytes" "Checksum: $$checksum"; \ + fi; \ + done From d089af3767d6515bbe93aecc414dab794f1122a4 Mon Sep 17 00:00:00 2001 From: Jaap de Haan <261428+jdehaan@users.noreply.github.com> Date: Sat, 15 Nov 2025 11:43:43 +0000 Subject: [PATCH 03/10] feat: Stricter linting --- .github/workflows/go.yml | 10 +++++++--- .golangci.yml | 17 +++++++++++++++++ .vscode/settings.json | 3 +++ Makefile | 15 +++++++++++---- backup/cmd/backup.go | 10 +++++++--- backup/cmd/config.go | 6 +++--- backup/cmd/root.go | 7 ++++--- backup/internal/check.go | 24 +++++++++++++++++++++++- backup/internal/check_test.go | 3 +++ backup/internal/config.go | 27 ++++++++++++++++++++------- backup/internal/config_test.go | 7 +++++++ backup/internal/job.go | 10 ++++++++-- backup/internal/job_test.go | 5 +++++ backup/internal/types.go | 7 ++++--- 14 files changed, 122 insertions(+), 29 deletions(-) create mode 100644 .golangci.yml diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index 2fe17fe..d3fbfab 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -16,6 +16,11 @@ jobs: steps: - uses: actions/checkout@v4 + - name: Setup NodeJS + uses: actions/setup-node@v6 + with: + node-version: 24.x + - name: Set up Go uses: actions/setup-go@v4 with: @@ -25,10 +30,9 @@ jobs: run: make sanity-check - name: Lint - uses: golangci/golangci-lint-action@v8 + uses: golangci/golangci-lint-action@v9 with: - version: latest - install-mode: goinstall + version: 'v2.5.0' args: --timeout=5m --verbose - name: Build diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 0000000..2830a87 --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,17 @@ +version: "2" +linters: + default: all + disable: + - wsl # The linter 'wsl' is deprecated (since v2.2.0) due to: new major version. Replaced by wsl_v5 + settings: + errcheck: + check-type-assertions: true + check-blank: true + staticcheck: + checks: all +formatters: + enable: + - gofmt + settings: + gofmt: + simplify: true diff --git a/.vscode/settings.json b/.vscode/settings.json index 0021d94..3569340 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -1,5 +1,8 @@ { "cSpell.words": [ + "afero", + "golangci", + "gofmt", "subpaths" ] } \ No newline at end of file diff --git a/Makefile b/Makefile index bd56e34..d00a873 100644 --- a/Makefile +++ b/Makefile @@ -6,6 +6,16 @@ PACKAGE = ./backup/main.go .PHONY: build clean test lint tidy checksums release sanity-check check-mod-tidy +format: + go fmt ./... + @echo "OK: Code formatted." + +lint: + golangci-lint run ./... + +lint-fix: + golangci-lint run --fix ./... + check-clean: @git diff --quiet || (echo "ERROR: Working directory has uncommitted changes." && exit 1) @echo "OK: Working directory is clean." @@ -14,12 +24,9 @@ check-mod-tidy: go mod tidy -diff @echo "OK: No untidy module files detected." -sanity-check: check-clean check-mod-tidy +sanity-check: format check-clean check-mod-tidy @echo "OK: All sanity checks passed." -lint: - golangci-lint run ./... - test: go test ./... -v diff --git a/backup/cmd/backup.go b/backup/cmd/backup.go index 0803363..2c98a96 100644 --- a/backup/cmd/backup.go +++ b/backup/cmd/backup.go @@ -12,24 +12,28 @@ import ( ) func getLogPath(create bool) string { - logPath := fmt.Sprintf("logs/sync-%s", time.Now().Format("2006-01-02T15-04-05")) + logPath := "logs/sync-" + time.Now().Format("2006-01-02T15-04-05") if create { - if err := os.MkdirAll(logPath, 0755); err != nil { + err := os.MkdirAll(logPath, 0755) + if err != nil { log.Fatalf("Failed to create log directory: %v", err) } } + return logPath } func executeSyncJobs(cfg internal.Config, simulate bool) { logPath := getLogPath(true) - overallLogPath := fmt.Sprintf("%s/summary.log", logPath) + overallLogPath := logPath + "/summary.log" + overallLogFile, err := os.OpenFile(overallLogPath, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0644) if err != nil { log.Fatalf("Failed to open overall log file: %v", err) } defer overallLogFile.Close() + overallLogger := log.New(overallLogFile, "", log.LstdFlags) for _, job := range cfg.Jobs { diff --git a/backup/cmd/config.go b/backup/cmd/config.go index bf4f31b..7d01cc7 100644 --- a/backup/cmd/config.go +++ b/backup/cmd/config.go @@ -9,7 +9,7 @@ import ( "gopkg.in/yaml.v3" ) -// configCmd represents the config command +// configCmd represents the config command. var configCmd = &cobra.Command{ Use: "config", Short: "Manage configuration", @@ -19,7 +19,7 @@ var configCmd = &cobra.Command{ }, } -// Extend the config subcommand with the show verb +// Extend the config subcommand with the show verb. var showCmd = &cobra.Command{ Use: "show", Short: "Show resolved configuration", @@ -33,7 +33,7 @@ var showCmd = &cobra.Command{ }, } -// Extend the config subcommand with the validate verb +// Extend the config subcommand with the validate verb. var validateCmd = &cobra.Command{ Use: "validate", Short: "Validate configuration", diff --git a/backup/cmd/root.go b/backup/cmd/root.go index 6d1dc57..4e648f1 100644 --- a/backup/cmd/root.go +++ b/backup/cmd/root.go @@ -6,14 +6,14 @@ import ( "github.com/spf13/cobra" ) -// RootCmd represents the base command when called without any subcommands +// RootCmd represents the base command when called without any subcommands. var RootCmd = &cobra.Command{ Use: "backup-tool", Short: "A tool for managing backups", Long: `backup-tool is a CLI tool for managing backups and configurations.`, } -// Define a global configPath variable and flag at the root level +// Define a global configPath variable and flag at the root level. var configPath string func init() { @@ -22,7 +22,8 @@ func init() { // Execute adds all child commands to the root command and sets flags appropriately. func Execute() { - if err := RootCmd.Execute(); err != nil { + err := RootCmd.Execute() + if err != nil { os.Exit(1) } } diff --git a/backup/internal/check.go b/backup/internal/check.go index 29f8160..004c1a9 100644 --- a/backup/internal/check.go +++ b/backup/internal/check.go @@ -16,6 +16,7 @@ func isExcluded(path string, job Job) bool { return true } } + return false } @@ -25,22 +26,28 @@ func isExcludedGlobally(path string, sources []Path) bool { exclusionPath := filepath.Join(source.Path, exclusion) if strings.HasPrefix(NormalizePath(path), exclusionPath) { log.Printf("EXCLUDED: Path '%s' is globally excluded by '%s' in source '%s'", path, exclusion, source.Path) + return true } } } + return false } func isCoveredByJob(path string, job Job) bool { if NormalizePath(job.Source) == NormalizePath(path) { log.Printf("COVERED: Path '%s' is covered by job '%s'", path, job.Name) + return true } + if isExcluded(path, job) { log.Printf("EXCLUDED: Path '%s' is excluded by job '%s'", path, job.Name) + return true } + return false } @@ -50,11 +57,13 @@ func isCovered(path string, jobs []Job) bool { return true } } + return false } func ListUncoveredPaths(fs afero.Fs, cfg Config) []string { var result []string + seen := make(map[string]bool) for _, source := range cfg.Sources { @@ -62,31 +71,37 @@ func ListUncoveredPaths(fs afero.Fs, cfg Config) []string { } sort.Strings(result) // Ensure consistent ordering for test comparison + return result } func checkPath(fs afero.Fs, path string, cfg Config, result *[]string, seen map[string]bool) { if seen[path] { log.Printf("SKIP: Path '%s' already seen", path) + return } + seen[path] = true // Skip if globally excluded if isExcludedGlobally(path, cfg.Sources) { log.Printf("SKIP: Path '%s' is globally excluded", path) + return } // Skip if covered by a job if isCovered(path, cfg.Jobs) { log.Printf("SKIP: Path '%s' is covered by a job", path) + return } // Check if it's effectively covered through descendants if isEffectivelyCovered(fs, path, cfg) { log.Printf("SKIP: Path '%s' is effectively covered", path) + return } @@ -95,23 +110,27 @@ func checkPath(fs afero.Fs, path string, cfg Config, result *[]string, seen map[ *result = append(*result, path) } -// Check if a directory is effectively covered (all its descendants are covered or excluded) +// Check if a directory is effectively covered (all its descendants are covered or excluded). func isEffectivelyCovered(fs afero.Fs, path string, cfg Config) bool { children, err := getChildDirectories(fs, path) if err != nil { log.Printf("ERROR: could not get child directories of '%s': %v", path, err) + return false } if len(children) == 0 { log.Printf("NOT COVERED: Path '%s' has no children", path) + return false // Leaf directories are not effectively covered unless directly covered } allCovered := true + for _, child := range children { if !isExcludedGlobally(child, cfg.Sources) && !isCovered(child, cfg.Jobs) && !isEffectivelyCovered(fs, child, cfg) { log.Printf("UNCOVERED CHILD: Path '%s' has uncovered child '%s'", path, child) + allCovered = false } } @@ -119,11 +138,13 @@ func isEffectivelyCovered(fs afero.Fs, path string, cfg Config) bool { if allCovered { log.Printf("COVERED: Path '%s' is effectively covered", path) } + return allCovered } func getChildDirectories(fs afero.Fs, path string) ([]string, error) { var children []string + fileInfos, err := afero.ReadDir(fs, path) if err != nil { return nil, err @@ -134,5 +155,6 @@ func getChildDirectories(fs afero.Fs, path string) ([]string, error) { children = append(children, filepath.Join(path, info.Name())) } } + return children, nil } diff --git a/backup/internal/check_test.go b/backup/internal/check_test.go index 26d7137..a183f86 100644 --- a/backup/internal/check_test.go +++ b/backup/internal/check_test.go @@ -92,14 +92,17 @@ func runListUncoveredPathsTest(t *testing.T, fakeFS map[string][]string, cfg Con if len(uncoveredPaths) != len(expectedUncoveredPaths) { t.Errorf("Expected uncovered paths length %d, got %d. Expected: %v, Got: %v", len(expectedUncoveredPaths), len(uncoveredPaths), expectedUncoveredPaths, uncoveredPaths) + return } for i, path := range uncoveredPaths { if i >= len(expectedUncoveredPaths) { t.Errorf("Got more uncovered paths than expected. Got: %v", uncoveredPaths) + return } + if path != expectedUncoveredPaths[i] { t.Errorf("Expected uncovered path '%s', got '%s'", expectedUncoveredPaths[i], path) } diff --git a/backup/internal/config.go b/backup/internal/config.go index ab79d90..35f6e9c 100644 --- a/backup/internal/config.go +++ b/backup/internal/config.go @@ -13,7 +13,8 @@ import ( func LoadConfig(reader io.Reader) (Config, error) { var cfg Config - if err := yaml.NewDecoder(reader).Decode(&cfg); err != nil { + err := yaml.NewDecoder(reader).Decode(&cfg) + if err != nil { return Config{}, err } @@ -27,6 +28,7 @@ func substituteVariables(input string, variables map[string]string) string { placeholder := fmt.Sprintf("${%s}", key) input = strings.ReplaceAll(input, placeholder, value) } + return input } @@ -36,6 +38,7 @@ func resolveConfig(cfg Config) Config { resolvedCfg.Jobs[i].Source = substituteVariables(job.Source, cfg.Variables) resolvedCfg.Jobs[i].Target = substituteVariables(job.Target, cfg.Variables) } + return resolvedCfg } @@ -45,14 +48,15 @@ func validateJobNames(jobs []Job) error { for _, job := range jobs { if nameSet[job.Name] { - invalidNames = append(invalidNames, fmt.Sprintf("duplicate job name: %s", job.Name)) + invalidNames = append(invalidNames, "duplicate job name: "+job.Name) } else { nameSet[job.Name] = true } for _, r := range job.Name { if r > 127 || r == ' ' { - invalidNames = append(invalidNames, fmt.Sprintf("invalid characters in job name: %s", job.Name)) + invalidNames = append(invalidNames, "invalid characters in job name: "+job.Name) + break } } @@ -61,6 +65,7 @@ func validateJobNames(jobs []Job) error { if len(invalidNames) > 0 { return fmt.Errorf("job validation errors: %v", invalidNames) } + return nil } @@ -70,6 +75,7 @@ func validatePath(jobPath string, paths []Path, pathType string, jobName string) return nil } } + return fmt.Errorf("invalid %s path for job '%s': %s", pathType, jobName, jobPath) } @@ -77,17 +83,21 @@ func validatePaths(cfg Config) error { invalidPaths := []string{} for _, job := range cfg.Jobs { - if err := validatePath(job.Source, cfg.Sources, "source", job.Name); err != nil { - invalidPaths = append(invalidPaths, err.Error()) + err1 := validatePath(job.Source, cfg.Sources, "source", job.Name) + if err1 != nil { + invalidPaths = append(invalidPaths, err1.Error()) } - if err := validatePath(job.Target, cfg.Targets, "target", job.Name); err != nil { - invalidPaths = append(invalidPaths, err.Error()) + err2 := validatePath(job.Target, cfg.Targets, "target", job.Name) + + if err2 != nil { + invalidPaths = append(invalidPaths, err2.Error()) } } if len(invalidPaths) > 0 { return fmt.Errorf("path validation errors: %v", invalidPaths) } + return nil } @@ -99,12 +109,14 @@ func validateJobPaths(jobs []Job, pathType string, getPath func(job Job) string) // Check if path2 is part of job1's exclusions excluded := false + if pathType == "source" { for _, exclusion := range job2.Exclusions { exclusionPath := NormalizePath(filepath.Join(job2.Source, exclusion)) // log.Printf("job2: %s %s\n", job2.Name, exclusionPath) if strings.HasPrefix(path1, exclusionPath) { excluded = true + break } } @@ -116,6 +128,7 @@ func validateJobPaths(jobs []Job, pathType string, getPath func(job Job) string) } } } + return nil } diff --git a/backup/internal/config_test.go b/backup/internal/config_test.go index c67d921..bdbd88d 100644 --- a/backup/internal/config_test.go +++ b/backup/internal/config_test.go @@ -21,6 +21,7 @@ jobs: enabled: true ` reader := bytes.NewReader([]byte(yamlData)) + cfg, err := LoadConfig(reader) if err != nil { t.Fatalf("Failed to load config: %v", err) @@ -38,9 +39,11 @@ jobs: if job.Name != "test_job" { t.Errorf("Expected job name test_job, got %s", job.Name) } + if job.Source != "/home/test/" { t.Errorf("Expected source /home/test/, got %s", job.Source) } + if job.Target != "${target_base}/test/" { t.Errorf("Expected target ${target_base}/test/, got %s", job.Target) } @@ -61,6 +64,7 @@ jobs: // Use a reader instead of a mock file reader := bytes.NewReader([]byte(yamlData)) + cfg, err := LoadConfig(reader) if err != nil { t.Fatalf("Failed to load config: %v", err) @@ -147,10 +151,12 @@ delete: false for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { var job Job + err := yaml.Unmarshal([]byte(tt.yamlData), &job) if err != nil { t.Fatalf("Failed to unmarshal YAML: %v", err) } + if !reflect.DeepEqual(job, tt.expected) { t.Errorf("got %+v, want %+v", job, tt.expected) } @@ -164,6 +170,7 @@ func TestSubstituteVariables(t *testing.T) { } input := "${target_base}/user/music/home" expected := "/mnt/backup1/user/music/home" + result := substituteVariables(input, variables) if result != expected { t.Errorf("Expected %s, got %s", expected, result) diff --git a/backup/internal/job.go b/backup/internal/job.go index 650b9e8..af6ac53 100644 --- a/backup/internal/job.go +++ b/backup/internal/job.go @@ -13,16 +13,20 @@ func buildRsyncCmd(job Job, simulate bool, logPath string) []string { if job.Delete { args = append(args, "--delete") } + if logPath != "" { - args = append(args, fmt.Sprintf("--log-file=%s", logPath)) + args = append(args, "--log-file="+logPath) } + for _, excl := range job.Exclusions { - args = append(args, fmt.Sprintf("--exclude=%s", excl)) + args = append(args, "--exclude="+excl) } + args = append(args, job.Source, job.Target) if simulate { args = append([]string{"--dry-run"}, args...) } + return args } @@ -42,8 +46,10 @@ func ExecuteJob(job Job, simulate bool, show bool, logPath string) string { cmd := execCommand("rsync", args...) out, err := cmd.CombinedOutput() fmt.Printf("Output:\n%s\n", string(out)) + if err != nil { return "FAILURE" } + return "SUCCESS" } diff --git a/backup/internal/job_test.go b/backup/internal/job_test.go index e9485c5..4f8fdd6 100644 --- a/backup/internal/job_test.go +++ b/backup/internal/job_test.go @@ -17,8 +17,10 @@ var mockExecCommand = func(name string, args ...string) *exec.Cmd { if strings.Contains(strings.Join(args, " "), "/invalid/source/path") { return exec.Command("false") // Simulate failure for invalid paths } + return exec.Command("echo", "mocked rsync success") // Simulate general success } + return exec.Command(name, args...) } @@ -96,6 +98,7 @@ func TestJobSkippedEnabledTrue(t *testing.T) { Target: "/mnt/backup1/test/", Enabled: true, } + status := ExecuteJob(job, true, false, "") if status != "SUCCESS" { t.Errorf("Expected status SUCCESS, got %s", status) @@ -109,6 +112,7 @@ func TestJobSkippedEnabledFalse(t *testing.T) { Target: "/mnt/backup1/disabled/", Enabled: false, } + status := ExecuteJob(disabledJob, true, false, "") if status != "SKIPPED" { t.Errorf("Expected status SKIPPED, got %s", status) @@ -123,6 +127,7 @@ func TestJobSkippedEnabledOmitted(t *testing.T) { Delete: true, Enabled: true, } + status := ExecuteJob(job, true, false, "") if status != "SUCCESS" { t.Errorf("Expected status SUCCESS, got %s", status) diff --git a/backup/internal/types.go b/backup/internal/types.go index 1549261..408646e 100644 --- a/backup/internal/types.go +++ b/backup/internal/types.go @@ -27,7 +27,7 @@ type Job struct { Exclusions []string `yaml:"exclusions,omitempty"` } -// JobYAML is a helper struct for proper YAML unmarshaling with defaults +// JobYAML is a helper struct for proper YAML unmarshaling with defaults. type JobYAML struct { Name string `yaml:"name"` Source string `yaml:"source"` @@ -37,10 +37,11 @@ type JobYAML struct { Exclusions []string `yaml:"exclusions,omitempty"` } -// UnmarshalYAML implements custom YAML unmarshaling to handle defaults properly +// UnmarshalYAML implements custom YAML unmarshaling to handle defaults properly. func (j *Job) UnmarshalYAML(node *yaml.Node) error { var jobYAML JobYAML - if err := node.Decode(&jobYAML); err != nil { + err := node.Decode(&jobYAML) + if err != nil { return err } From 19275bd7cc8d02670d8be70c69f8883ce41e1fdf Mon Sep 17 00:00:00 2001 From: Jaap de Haan <261428+jdehaan@users.noreply.github.com> Date: Sat, 15 Nov 2025 12:09:28 +0000 Subject: [PATCH 04/10] fix: Shift tests into internal_test package --- backup/cmd/root.go | 1 + backup/internal/check.go | 6 ++-- backup/internal/check_test.go | 60 ++++++++++--------------------- backup/internal/config.go | 21 +++++------ backup/internal/config_test.go | 66 +++++++++++++++++----------------- backup/internal/helper_test.go | 9 +++-- backup/internal/job.go | 8 ++--- backup/internal/job_test.go | 39 ++++++++++---------- backup/internal/types.go | 1 + 9 files changed, 100 insertions(+), 111 deletions(-) diff --git a/backup/cmd/root.go b/backup/cmd/root.go index 4e648f1..1a3cbd5 100644 --- a/backup/cmd/root.go +++ b/backup/cmd/root.go @@ -1,3 +1,4 @@ +// Commands for backup-tool package cmd import ( diff --git a/backup/internal/check.go b/backup/internal/check.go index 004c1a9..1f08726 100644 --- a/backup/internal/check.go +++ b/backup/internal/check.go @@ -20,7 +20,7 @@ func isExcluded(path string, job Job) bool { return false } -func isExcludedGlobally(path string, sources []Path) bool { +func IsExcludedGlobally(path string, sources []Path) bool { for _, source := range sources { for _, exclusion := range source.Exclusions { exclusionPath := filepath.Join(source.Path, exclusion) @@ -85,7 +85,7 @@ func checkPath(fs afero.Fs, path string, cfg Config, result *[]string, seen map[ seen[path] = true // Skip if globally excluded - if isExcludedGlobally(path, cfg.Sources) { + if IsExcludedGlobally(path, cfg.Sources) { log.Printf("SKIP: Path '%s' is globally excluded", path) return @@ -128,7 +128,7 @@ func isEffectivelyCovered(fs afero.Fs, path string, cfg Config) bool { allCovered := true for _, child := range children { - if !isExcludedGlobally(child, cfg.Sources) && !isCovered(child, cfg.Jobs) && !isEffectivelyCovered(fs, child, cfg) { + if !IsExcludedGlobally(child, cfg.Sources) && !isCovered(child, cfg.Jobs) && !isEffectivelyCovered(fs, child, cfg) { log.Printf("UNCOVERED CHILD: Path '%s' has uncovered child '%s'", path, child) allCovered = false diff --git a/backup/internal/check_test.go b/backup/internal/check_test.go index a183f86..ae6868e 100644 --- a/backup/internal/check_test.go +++ b/backup/internal/check_test.go @@ -1,20 +1,20 @@ -package internal +package internal_test import ( "bytes" "log" - "os" "path/filepath" "sort" "strings" "testing" - "time" + + "backup-rsync/backup/internal" "github.com/spf13/afero" ) func TestIsExcludedGlobally(t *testing.T) { - sources := []Path{ + sources := []internal.Path{ { Path: "/home/data/", Exclusions: []string{"/projects/P1/", "/media/"}, @@ -55,7 +55,7 @@ func TestIsExcludedGlobally(t *testing.T) { var logBuffer bytes.Buffer log.SetOutput(&logBuffer) - result := isExcludedGlobally(test.path, sources) + result := internal.IsExcludedGlobally(test.path, sources) if result != test.expectsError { t.Errorf("Expected exclusion result %v, got %v", test.expectsError, result) } @@ -69,7 +69,7 @@ func TestIsExcludedGlobally(t *testing.T) { } } -func runListUncoveredPathsTest(t *testing.T, fakeFS map[string][]string, cfg Config, expectedUncoveredPaths []string) { +func runListUncoveredPathsTest(t *testing.T, fakeFS map[string][]string, cfg internal.Config, expectedUncoveredPaths []string) { // Create an in-memory filesystem using Afero fs := afero.NewMemMapFs() @@ -83,7 +83,7 @@ func runListUncoveredPathsTest(t *testing.T, fakeFS map[string][]string, cfg Con } // Call the function - uncoveredPaths := ListUncoveredPaths(fs, cfg) + uncoveredPaths := internal.ListUncoveredPaths(fs, cfg) // Assertions sort.Strings(uncoveredPaths) @@ -116,12 +116,12 @@ func TestListUncoveredPathsVariations(t *testing.T) { "/var/log": {"app1", "app2"}, "/tmp": {"cache", "temp"}, }, - Config{ - Sources: []Path{ + internal.Config{ + Sources: []internal.Path{ {Path: "/var/log"}, {Path: "/tmp"}, }, - Jobs: []Job{ + Jobs: []internal.Job{ {Name: "Job1", Source: "/var/log"}, {Name: "Job2", Source: "/tmp"}, }, @@ -137,12 +137,12 @@ func TestListUncoveredPathsVariations(t *testing.T) { "/home/user/cache": {}, "/home/user/npm": {}, }, - Config{ - Sources: []Path{ + internal.Config{ + Sources: []internal.Path{ {Path: "/home/data"}, {Path: "/home/user"}, }, - Jobs: []Job{ + Jobs: []internal.Job{ {Name: "Job1", Source: "/home/data"}, }, }, @@ -154,11 +154,11 @@ func TestListUncoveredPathsVariations(t *testing.T) { map[string][]string{ "/home/data": {"projects", "media"}, }, - Config{ - Sources: []Path{ + internal.Config{ + Sources: []internal.Path{ {Path: "/home/data", Exclusions: []string{"media"}}, }, - Jobs: []Job{ + Jobs: []internal.Job{ {Name: "Job1", Source: "/home/data/projects"}, }, }, @@ -173,11 +173,11 @@ func TestListUncoveredPathsVariations(t *testing.T) { "/home/data/family/me": {"a"}, "/home/data/family/you": {"a"}, }, - Config{ - Sources: []Path{ + internal.Config{ + Sources: []internal.Path{ {Path: "/home/data"}, }, - Jobs: []Job{ + Jobs: []internal.Job{ {Name: "JobMe", Source: "/home/data/family/me"}, {Name: "JobYou", Source: "/home/data/family/you"}, }, @@ -204,25 +204,3 @@ func TestListUncoveredPathsVariations(t *testing.T) { // []string{"/home/data/family/you"}, // ) } - -type mockDirEntry struct { - name string - isDir bool -} - -func (m mockDirEntry) Name() string { return m.name } -func (m mockDirEntry) IsDir() bool { return m.isDir } -func (m mockDirEntry) Type() os.FileMode { return 0 } -func (m mockDirEntry) Info() (os.FileInfo, error) { return nil, nil } - -type mockFileInfo struct { - name string - isDir bool -} - -func (m mockFileInfo) Name() string { return m.name } -func (m mockFileInfo) Size() int64 { return 0 } -func (m mockFileInfo) Mode() os.FileMode { return 0 } -func (m mockFileInfo) ModTime() time.Time { return time.Time{} } -func (m mockFileInfo) IsDir() bool { return m.isDir } -func (m mockFileInfo) Sys() interface{} { return nil } diff --git a/backup/internal/config.go b/backup/internal/config.go index 35f6e9c..6eb26a7 100644 --- a/backup/internal/config.go +++ b/backup/internal/config.go @@ -13,6 +13,7 @@ import ( func LoadConfig(reader io.Reader) (Config, error) { var cfg Config + err := yaml.NewDecoder(reader).Decode(&cfg) if err != nil { return Config{}, err @@ -23,7 +24,7 @@ func LoadConfig(reader io.Reader) (Config, error) { return cfg, nil } -func substituteVariables(input string, variables map[string]string) string { +func SubstituteVariables(input string, variables map[string]string) string { for key, value := range variables { placeholder := fmt.Sprintf("${%s}", key) input = strings.ReplaceAll(input, placeholder, value) @@ -35,14 +36,14 @@ func substituteVariables(input string, variables map[string]string) string { func resolveConfig(cfg Config) Config { resolvedCfg := cfg for i, job := range resolvedCfg.Jobs { - resolvedCfg.Jobs[i].Source = substituteVariables(job.Source, cfg.Variables) - resolvedCfg.Jobs[i].Target = substituteVariables(job.Target, cfg.Variables) + resolvedCfg.Jobs[i].Source = SubstituteVariables(job.Source, cfg.Variables) + resolvedCfg.Jobs[i].Target = SubstituteVariables(job.Target, cfg.Variables) } return resolvedCfg } -func validateJobNames(jobs []Job) error { +func ValidateJobNames(jobs []Job) error { invalidNames := []string{} nameSet := make(map[string]bool) @@ -69,7 +70,7 @@ func validateJobNames(jobs []Job) error { return nil } -func validatePath(jobPath string, paths []Path, pathType string, jobName string) error { +func ValidatePath(jobPath string, paths []Path, pathType string, jobName string) error { for _, path := range paths { if strings.HasPrefix(jobPath, path.Path) { return nil @@ -79,16 +80,16 @@ func validatePath(jobPath string, paths []Path, pathType string, jobName string) return fmt.Errorf("invalid %s path for job '%s': %s", pathType, jobName, jobPath) } -func validatePaths(cfg Config) error { +func ValidatePaths(cfg Config) error { invalidPaths := []string{} for _, job := range cfg.Jobs { - err1 := validatePath(job.Source, cfg.Sources, "source", job.Name) + err1 := ValidatePath(job.Source, cfg.Sources, "source", job.Name) if err1 != nil { invalidPaths = append(invalidPaths, err1.Error()) } - err2 := validatePath(job.Target, cfg.Targets, "target", job.Name) + err2 := ValidatePath(job.Target, cfg.Targets, "target", job.Name) if err2 != nil { invalidPaths = append(invalidPaths, err2.Error()) } @@ -144,13 +145,13 @@ func LoadResolvedConfig(configPath string) Config { log.Fatalf("Failed to parse YAML: %v", err) } - if err := validateJobNames(cfg.Jobs); err != nil { + if err := ValidateJobNames(cfg.Jobs); err != nil { log.Fatalf("Job validation failed: %v", err) } resolvedCfg := resolveConfig(cfg) - if err := validatePaths(resolvedCfg); err != nil { + if err := ValidatePaths(resolvedCfg); err != nil { log.Fatalf("Path validation failed: %v", err) } diff --git a/backup/internal/config_test.go b/backup/internal/config_test.go index bdbd88d..e51f49f 100644 --- a/backup/internal/config_test.go +++ b/backup/internal/config_test.go @@ -1,4 +1,4 @@ -package internal +package internal_test import ( "bytes" @@ -7,6 +7,8 @@ import ( "testing" "gopkg.in/yaml.v3" + + "backup-rsync/backup/internal" ) func TestLoadConfig1(t *testing.T) { @@ -22,7 +24,7 @@ jobs: ` reader := bytes.NewReader([]byte(yamlData)) - cfg, err := LoadConfig(reader) + cfg, err := internal.LoadConfig(reader) if err != nil { t.Fatalf("Failed to load config: %v", err) } @@ -65,12 +67,12 @@ jobs: // Use a reader instead of a mock file reader := bytes.NewReader([]byte(yamlData)) - cfg, err := LoadConfig(reader) + cfg, err := internal.LoadConfig(reader) if err != nil { t.Fatalf("Failed to load config: %v", err) } - expected := []Job{ + expected := []internal.Job{ { Name: "job1", Source: "/source1", @@ -96,7 +98,7 @@ func TestYAMLUnmarshalingDefaults(t *testing.T) { tests := []struct { name string yamlData string - expected Job + expected internal.Job }{ { name: "Defaults applied when fields omitted", @@ -105,7 +107,7 @@ name: "test_job" source: "/source" target: "/target" `, - expected: Job{ + expected: internal.Job{ Name: "test_job", Source: "/source", Target: "/target", @@ -122,7 +124,7 @@ target: "/target" delete: false enabled: false `, - expected: Job{ + expected: internal.Job{ Name: "test_job", Source: "/source", Target: "/target", @@ -138,7 +140,7 @@ source: "/source" target: "/target" delete: false `, - expected: Job{ + expected: internal.Job{ Name: "test_job", Source: "/source", Target: "/target", @@ -150,7 +152,7 @@ delete: false for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - var job Job + var job internal.Job err := yaml.Unmarshal([]byte(tt.yamlData), &job) if err != nil { @@ -171,7 +173,7 @@ func TestSubstituteVariables(t *testing.T) { input := "${target_base}/user/music/home" expected := "/mnt/backup1/user/music/home" - result := substituteVariables(input, variables) + result := internal.SubstituteVariables(input, variables) if result != expected { t.Errorf("Expected %s, got %s", expected, result) } @@ -180,13 +182,13 @@ func TestSubstituteVariables(t *testing.T) { func TestValidateJobNames(t *testing.T) { tests := []struct { name string - jobs []Job + jobs []internal.Job expectsError bool errorMessage string }{ { name: "Valid job names", - jobs: []Job{ + jobs: []internal.Job{ {Name: "job1"}, {Name: "job2"}, }, @@ -194,7 +196,7 @@ func TestValidateJobNames(t *testing.T) { }, { name: "Duplicate job names", - jobs: []Job{ + jobs: []internal.Job{ {Name: "job1"}, {Name: "job1"}, }, @@ -203,7 +205,7 @@ func TestValidateJobNames(t *testing.T) { }, { name: "Invalid characters in job name", - jobs: []Job{ + jobs: []internal.Job{ {Name: "job 1"}, }, expectsError: true, @@ -211,7 +213,7 @@ func TestValidateJobNames(t *testing.T) { }, { name: "Mixed errors", - jobs: []Job{ + jobs: []internal.Job{ {Name: "job1"}, {Name: "job 1"}, {Name: "job1"}, @@ -223,7 +225,7 @@ func TestValidateJobNames(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - err := validateJobNames(test.jobs) + err := internal.ValidateJobNames(test.jobs) if test.expectsError { if err == nil { t.Errorf("Expected error but got none") @@ -243,7 +245,7 @@ func TestValidatePath(t *testing.T) { tests := []struct { name string jobPath string - paths []Path + paths []internal.Path pathType string jobName string expectsError bool @@ -252,7 +254,7 @@ func TestValidatePath(t *testing.T) { { name: "Valid source path", jobPath: "/home/user/documents", - paths: []Path{{Path: "/home/user"}}, + paths: []internal.Path{{Path: "/home/user"}}, pathType: "source", jobName: "job1", expectsError: false, @@ -260,7 +262,7 @@ func TestValidatePath(t *testing.T) { { name: "Invalid source path", jobPath: "/invalid/source", - paths: []Path{{Path: "/home/user"}}, + paths: []internal.Path{{Path: "/home/user"}}, pathType: "source", jobName: "job1", expectsError: true, @@ -269,7 +271,7 @@ func TestValidatePath(t *testing.T) { { name: "Valid target path", jobPath: "/mnt/backup/documents", - paths: []Path{{Path: "/mnt/backup"}}, + paths: []internal.Path{{Path: "/mnt/backup"}}, pathType: "target", jobName: "job1", expectsError: false, @@ -277,7 +279,7 @@ func TestValidatePath(t *testing.T) { { name: "Invalid target path", jobPath: "/invalid/target", - paths: []Path{{Path: "/mnt/backup"}}, + paths: []internal.Path{{Path: "/mnt/backup"}}, pathType: "target", jobName: "job1", expectsError: true, @@ -287,7 +289,7 @@ func TestValidatePath(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - err := validatePath(test.jobPath, test.paths, test.pathType, test.jobName) + err := internal.ValidatePath(test.jobPath, test.paths, test.pathType, test.jobName) if test.expectsError { if err == nil { t.Errorf("Expected error but got none") @@ -306,20 +308,20 @@ func TestValidatePath(t *testing.T) { func TestValidatePaths(t *testing.T) { tests := []struct { name string - cfg Config + cfg internal.Config expectsError bool errorMessage string }{ { name: "Valid paths", - cfg: Config{ - Sources: []Path{ + cfg: internal.Config{ + Sources: []internal.Path{ {Path: "/home/user"}, }, - Targets: []Path{ + Targets: []internal.Path{ {Path: "/mnt/backup"}, }, - Jobs: []Job{ + Jobs: []internal.Job{ {Name: "job1", Source: "/home/user/documents", Target: "/mnt/backup/documents"}, }, }, @@ -327,14 +329,14 @@ func TestValidatePaths(t *testing.T) { }, { name: "Invalid paths", - cfg: Config{ - Sources: []Path{ + cfg: internal.Config{ + Sources: []internal.Path{ {Path: "/home/user"}, }, - Targets: []Path{ + Targets: []internal.Path{ {Path: "/mnt/backup"}, }, - Jobs: []Job{ + Jobs: []internal.Job{ {Name: "job1", Source: "/invalid/source", Target: "/invalid/target"}, }, }, @@ -345,7 +347,7 @@ func TestValidatePaths(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - err := validatePaths(test.cfg) + err := internal.ValidatePaths(test.cfg) if test.expectsError { if err == nil { t.Errorf("Expected error but got none") diff --git a/backup/internal/helper_test.go b/backup/internal/helper_test.go index 80b6c47..e5d2559 100644 --- a/backup/internal/helper_test.go +++ b/backup/internal/helper_test.go @@ -1,6 +1,9 @@ -package internal +package internal_test -import "testing" +import ( + "backup-rsync/backup/internal" + "testing" +) func TestNormalizePath(t *testing.T) { tests := []struct { @@ -14,7 +17,7 @@ func TestNormalizePath(t *testing.T) { } for _, test := range tests { - result := NormalizePath(test.input) + result := internal.NormalizePath(test.input) if result != test.expected { t.Errorf("NormalizePath(%q) = %q; want %q", test.input, result, test.expected) } diff --git a/backup/internal/job.go b/backup/internal/job.go index af6ac53..7689220 100644 --- a/backup/internal/job.go +++ b/backup/internal/job.go @@ -6,9 +6,9 @@ import ( "strings" ) -var execCommand = exec.Command +var ExecCommand = exec.Command -func buildRsyncCmd(job Job, simulate bool, logPath string) []string { +func BuildRsyncCmd(job Job, simulate bool, logPath string) []string { args := []string{"-aiv", "--stats"} if job.Delete { args = append(args, "--delete") @@ -35,7 +35,7 @@ func ExecuteJob(job Job, simulate bool, show bool, logPath string) string { return "SKIPPED" } - args := buildRsyncCmd(job, simulate, logPath) + args := BuildRsyncCmd(job, simulate, logPath) fmt.Printf("Job: %s\n", job.Name) fmt.Printf("Command: rsync %s\n", strings.Join(args, " ")) @@ -43,7 +43,7 @@ func ExecuteJob(job Job, simulate bool, show bool, logPath string) string { return "SUCCESS" } - cmd := execCommand("rsync", args...) + cmd := ExecCommand("rsync", args...) out, err := cmd.CombinedOutput() fmt.Printf("Output:\n%s\n", string(out)) diff --git a/backup/internal/job_test.go b/backup/internal/job_test.go index 4f8fdd6..28007b1 100644 --- a/backup/internal/job_test.go +++ b/backup/internal/job_test.go @@ -1,9 +1,11 @@ -package internal +package internal_test import ( "os/exec" "strings" "testing" + + "backup-rsync/backup/internal" ) var capturedArgs []string @@ -25,17 +27,17 @@ var mockExecCommand = func(name string, args ...string) *exec.Cmd { } func init() { - execCommand = mockExecCommand + internal.ExecCommand = mockExecCommand } func TestBuildRsyncCmd(t *testing.T) { - job := Job{ + job := internal.Job{ Source: "/home/user/Music/", Target: "/target/user/music/home", Delete: true, Exclusions: []string{"*.tmp", "node_modules/"}, } - args := buildRsyncCmd(job, true, "") + args := internal.BuildRsyncCmd(job, true, "") expectedArgs := []string{ "--dry-run", "-aiv", "--stats", "--delete", @@ -49,7 +51,7 @@ func TestBuildRsyncCmd(t *testing.T) { } func TestExecuteJob(t *testing.T) { - job := Job{ + job := internal.Job{ Name: "test_job", Source: "/home/test/", Target: "/mnt/backup1/test/", @@ -59,25 +61,25 @@ func TestExecuteJob(t *testing.T) { } simulate := true - status := ExecuteJob(job, simulate, false, "") + status := internal.ExecuteJob(job, simulate, false, "") if status != "SUCCESS" { t.Errorf("Expected status SUCCESS, got %s", status) } - disabledJob := Job{ + disabledJob := internal.Job{ Name: "disabled_job", Source: "/home/disabled/", Target: "/mnt/backup1/disabled/", Enabled: false, } - status = ExecuteJob(disabledJob, simulate, false, "") + status = internal.ExecuteJob(disabledJob, simulate, false, "") if status != "SKIPPED" { t.Errorf("Expected status SKIPPED, got %s", status) } // Test case for failure (simulate by providing invalid source path) - invalidJob := Job{ + invalidJob := internal.Job{ Name: "invalid_job", Source: "/invalid/source/path", Target: "/mnt/backup1/invalid/", @@ -85,42 +87,43 @@ func TestExecuteJob(t *testing.T) { Enabled: true, } - status = ExecuteJob(invalidJob, false, false, "") + status = internal.ExecuteJob(invalidJob, false, false, "") if status != "FAILURE" { t.Errorf("Expected status FAILURE, got %s", status) } } +// Ensure all references to ExecuteJob are prefixed with internal func TestJobSkippedEnabledTrue(t *testing.T) { - job := Job{ + job := internal.Job{ Name: "test_job", Source: "/home/test/", Target: "/mnt/backup1/test/", Enabled: true, } - status := ExecuteJob(job, true, false, "") + status := internal.ExecuteJob(job, true, false, "") if status != "SUCCESS" { t.Errorf("Expected status SUCCESS, got %s", status) } } func TestJobSkippedEnabledFalse(t *testing.T) { - disabledJob := Job{ + disabledJob := internal.Job{ Name: "disabled_job", Source: "/home/disabled/", Target: "/mnt/backup1/disabled/", Enabled: false, } - status := ExecuteJob(disabledJob, true, false, "") + status := internal.ExecuteJob(disabledJob, true, false, "") if status != "SKIPPED" { t.Errorf("Expected status SKIPPED, got %s", status) } } func TestJobSkippedEnabledOmitted(t *testing.T) { - job := Job{ + job := internal.Job{ Name: "omitted_enabled_job", Source: "/home/omitted/", Target: "/mnt/backup1/omitted/", @@ -128,7 +131,7 @@ func TestJobSkippedEnabledOmitted(t *testing.T) { Enabled: true, } - status := ExecuteJob(job, true, false, "") + status := internal.ExecuteJob(job, true, false, "") if status != "SUCCESS" { t.Errorf("Expected status SUCCESS, got %s", status) } @@ -138,7 +141,7 @@ func TestExecuteJobWithMockedRsync(t *testing.T) { // Reset capturedArgs before the test capturedArgs = nil - job := Job{ + job := internal.Job{ Name: "test_job", Source: "/home/test/", Target: "/mnt/backup1/test/", @@ -146,7 +149,7 @@ func TestExecuteJobWithMockedRsync(t *testing.T) { Enabled: true, Exclusions: []string{"*.tmp"}, } - status := ExecuteJob(job, true, false, "") + status := internal.ExecuteJob(job, true, false, "") if status != "SUCCESS" { t.Errorf("Expected status SUCCESS, got %s", status) diff --git a/backup/internal/types.go b/backup/internal/types.go index 408646e..1473dfe 100644 --- a/backup/internal/types.go +++ b/backup/internal/types.go @@ -40,6 +40,7 @@ type JobYAML struct { // UnmarshalYAML implements custom YAML unmarshaling to handle defaults properly. func (j *Job) UnmarshalYAML(node *yaml.Node) error { var jobYAML JobYAML + err := node.Decode(&jobYAML) if err != nil { return err From cdcea4f92b37a5d93b4cc565ee72cd06f7d258dd Mon Sep 17 00:00:00 2001 From: Jaap de Haan <261428+jdehaan@users.noreply.github.com> Date: Sat, 15 Nov 2025 14:29:02 +0000 Subject: [PATCH 05/10] fix: Linter messages cleanup --- .golangci.yml | 18 ++++++++++++++++++ backup/cmd/backup.go | 7 +++++-- backup/cmd/root.go | 2 +- backup/internal/check_test.go | 8 +++++++- backup/internal/config.go | 24 ++++++++++++++---------- backup/internal/config_test.go | 4 +++- backup/internal/helper.go | 2 +- backup/internal/job_test.go | 12 +++++++----- backup/internal/types.go | 3 +++ 9 files changed, 59 insertions(+), 21 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 2830a87..e54f58b 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -3,12 +3,30 @@ linters: default: all disable: - wsl # The linter 'wsl' is deprecated (since v2.2.0) due to: new major version. Replaced by wsl_v5 + - paralleltest # not used in this project + - revive # temporarily disable + - exhaustruct # temporarily disable + - gochecknoglobals # temporarily disable + - depguard + - err113 + - errcheck + - forbidigo + - funlen + - gochecknoinits + - gocritic + - godoclint + - gosec + - varnamelen + - wrapcheck settings: errcheck: check-type-assertions: true check-blank: true staticcheck: checks: all + varnamelen: + ignore-decls: + fs afero.Fs formatters: enable: - gofmt diff --git a/backup/cmd/backup.go b/backup/cmd/backup.go index 2c98a96..d9d5188 100644 --- a/backup/cmd/backup.go +++ b/backup/cmd/backup.go @@ -11,10 +11,13 @@ import ( "github.com/spf13/cobra" ) +const filePermission = 0644 +const logDirPermission = 0755 + func getLogPath(create bool) string { logPath := "logs/sync-" + time.Now().Format("2006-01-02T15-04-05") if create { - err := os.MkdirAll(logPath, 0755) + err := os.MkdirAll(logPath, logDirPermission) if err != nil { log.Fatalf("Failed to create log directory: %v", err) } @@ -28,7 +31,7 @@ func executeSyncJobs(cfg internal.Config, simulate bool) { overallLogPath := logPath + "/summary.log" - overallLogFile, err := os.OpenFile(overallLogPath, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0644) + overallLogFile, err := os.OpenFile(overallLogPath, os.O_CREATE|os.O_WRONLY|os.O_APPEND, filePermission) if err != nil { log.Fatalf("Failed to open overall log file: %v", err) } diff --git a/backup/cmd/root.go b/backup/cmd/root.go index 1a3cbd5..1de7cbb 100644 --- a/backup/cmd/root.go +++ b/backup/cmd/root.go @@ -1,4 +1,4 @@ -// Commands for backup-tool +// Package cmd contains the commands for the backup-tool CLI application. package cmd import ( diff --git a/backup/internal/check_test.go b/backup/internal/check_test.go index ae6868e..5fb8872 100644 --- a/backup/internal/check_test.go +++ b/backup/internal/check_test.go @@ -69,7 +69,13 @@ func TestIsExcludedGlobally(t *testing.T) { } } -func runListUncoveredPathsTest(t *testing.T, fakeFS map[string][]string, cfg internal.Config, expectedUncoveredPaths []string) { +func runListUncoveredPathsTest( + t *testing.T, + fakeFS map[string][]string, + cfg internal.Config, + expectedUncoveredPaths []string, +) { + t.Helper() // Create an in-memory filesystem using Afero fs := afero.NewMemMapFs() diff --git a/backup/internal/config.go b/backup/internal/config.go index 6eb26a7..5a46c2e 100644 --- a/backup/internal/config.go +++ b/backup/internal/config.go @@ -84,14 +84,14 @@ func ValidatePaths(cfg Config) error { invalidPaths := []string{} for _, job := range cfg.Jobs { - err1 := ValidatePath(job.Source, cfg.Sources, "source", job.Name) - if err1 != nil { - invalidPaths = append(invalidPaths, err1.Error()) + err := ValidatePath(job.Source, cfg.Sources, "source", job.Name) + if err != nil { + invalidPaths = append(invalidPaths, err.Error()) } - err2 := ValidatePath(job.Target, cfg.Targets, "target", job.Name) - if err2 != nil { - invalidPaths = append(invalidPaths, err2.Error()) + err = ValidatePath(job.Target, cfg.Targets, "target", job.Name) + if err != nil { + invalidPaths = append(invalidPaths, err.Error()) } } @@ -145,21 +145,25 @@ func LoadResolvedConfig(configPath string) Config { log.Fatalf("Failed to parse YAML: %v", err) } - if err := ValidateJobNames(cfg.Jobs); err != nil { + err = ValidateJobNames(cfg.Jobs) + if err != nil { log.Fatalf("Job validation failed: %v", err) } resolvedCfg := resolveConfig(cfg) - if err := ValidatePaths(resolvedCfg); err != nil { + err = ValidatePaths(resolvedCfg) + if err != nil { log.Fatalf("Path validation failed: %v", err) } - if err := validateJobPaths(resolvedCfg.Jobs, "source", func(job Job) string { return job.Source }); err != nil { + err = validateJobPaths(resolvedCfg.Jobs, "source", func(job Job) string { return job.Source }) + if err != nil { log.Fatalf("Job source path validation failed: %v", err) } - if err := validateJobPaths(resolvedCfg.Jobs, "target", func(job Job) string { return job.Target }); err != nil { + err = validateJobPaths(resolvedCfg.Jobs, "target", func(job Job) string { return job.Target }) + if err != nil { log.Fatalf("Job target path validation failed: %v", err) } diff --git a/backup/internal/config_test.go b/backup/internal/config_test.go index e51f49f..018ecb0 100644 --- a/backup/internal/config_test.go +++ b/backup/internal/config_test.go @@ -341,7 +341,9 @@ func TestValidatePaths(t *testing.T) { }, }, expectsError: true, - errorMessage: "path validation errors: [invalid source path for job 'job1': /invalid/source invalid target path for job 'job1': /invalid/target]", + errorMessage: "path validation errors: [" + + "invalid source path for job 'job1': /invalid/source " + + "invalid target path for job 'job1': /invalid/target]", }, } diff --git a/backup/internal/helper.go b/backup/internal/helper.go index 1f4c426..2089042 100644 --- a/backup/internal/helper.go +++ b/backup/internal/helper.go @@ -1,4 +1,4 @@ -// Correct the package declaration +// Package internal provides helper functions for internal use within the application. package internal import "strings" diff --git a/backup/internal/job_test.go b/backup/internal/job_test.go index 28007b1..9ddb2b5 100644 --- a/backup/internal/job_test.go +++ b/backup/internal/job_test.go @@ -8,6 +8,8 @@ import ( "backup-rsync/backup/internal" ) +const statusSuccess = "SUCCESS" + var capturedArgs []string var mockExecCommand = func(name string, args ...string) *exec.Cmd { @@ -62,7 +64,7 @@ func TestExecuteJob(t *testing.T) { simulate := true status := internal.ExecuteJob(job, simulate, false, "") - if status != "SUCCESS" { + if status != statusSuccess { t.Errorf("Expected status SUCCESS, got %s", status) } @@ -93,7 +95,7 @@ func TestExecuteJob(t *testing.T) { } } -// Ensure all references to ExecuteJob are prefixed with internal +// Ensure all references to ExecuteJob are prefixed with internal. func TestJobSkippedEnabledTrue(t *testing.T) { job := internal.Job{ Name: "test_job", @@ -103,7 +105,7 @@ func TestJobSkippedEnabledTrue(t *testing.T) { } status := internal.ExecuteJob(job, true, false, "") - if status != "SUCCESS" { + if status != statusSuccess { t.Errorf("Expected status SUCCESS, got %s", status) } } @@ -132,7 +134,7 @@ func TestJobSkippedEnabledOmitted(t *testing.T) { } status := internal.ExecuteJob(job, true, false, "") - if status != "SUCCESS" { + if status != statusSuccess { t.Errorf("Expected status SUCCESS, got %s", status) } } @@ -151,7 +153,7 @@ func TestExecuteJobWithMockedRsync(t *testing.T) { } status := internal.ExecuteJob(job, true, false, "") - if status != "SUCCESS" { + if status != statusSuccess { t.Errorf("Expected status SUCCESS, got %s", status) } diff --git a/backup/internal/types.go b/backup/internal/types.go index 1473dfe..5883c55 100644 --- a/backup/internal/types.go +++ b/backup/internal/types.go @@ -6,11 +6,13 @@ import ( // Centralized type definitions +// Path represents a source or target path with optional exclusions. type Path struct { Path string `yaml:"path"` Exclusions []string `yaml:"exclusions"` } +// Config represents the overall backup configuration. type Config struct { Sources []Path `yaml:"sources"` Targets []Path `yaml:"targets"` @@ -18,6 +20,7 @@ type Config struct { Jobs []Job `yaml:"jobs"` } +// Job represents a backup job configuration for a source/target pair. type Job struct { Name string `yaml:"name"` Source string `yaml:"source"` From 6162d86dff5fc3050c3df9328d6d6ed27a47594a Mon Sep 17 00:00:00 2001 From: Jaap de Haan <261428+jdehaan@users.noreply.github.com> Date: Sat, 15 Nov 2025 14:57:53 +0000 Subject: [PATCH 06/10] fix: Cleanup init and globals for commands --- .golangci.yml | 2 -- backup/cmd/backup.go | 56 +++++++++++++++++------------------ backup/cmd/check.go | 30 ++++++++++--------- backup/cmd/config.go | 69 +++++++++++++++++++++++--------------------- backup/cmd/root.go | 31 +++++++++++--------- 5 files changed, 97 insertions(+), 91 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index e54f58b..1b9fd79 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -6,13 +6,11 @@ linters: - paralleltest # not used in this project - revive # temporarily disable - exhaustruct # temporarily disable - - gochecknoglobals # temporarily disable - depguard - err113 - errcheck - forbidigo - funlen - - gochecknoinits - gocritic - godoclint - gosec diff --git a/backup/cmd/backup.go b/backup/cmd/backup.go index d9d5188..c736565 100644 --- a/backup/cmd/backup.go +++ b/backup/cmd/backup.go @@ -55,35 +55,35 @@ func listCommands(cfg internal.Config) { } } -var runCmd = &cobra.Command{ - Use: "run", - Short: "Execute the sync jobs", - Run: func(cmd *cobra.Command, args []string) { - cfg := internal.LoadResolvedConfig(configPath) - executeSyncJobs(cfg, false) - }, -} +func AddBackupCommands(rootCmd *cobra.Command, configPath string) { + var runCmd = &cobra.Command{ + Use: "run", + Short: "Execute the sync jobs", + Run: func(cmd *cobra.Command, args []string) { + cfg := internal.LoadResolvedConfig(configPath) + executeSyncJobs(cfg, false) + }, + } -var simulateCmd = &cobra.Command{ - Use: "simulate", - Short: "Simulate the sync jobs", - Run: func(cmd *cobra.Command, args []string) { - cfg := internal.LoadResolvedConfig(configPath) - executeSyncJobs(cfg, true) - }, -} + var simulateCmd = &cobra.Command{ + Use: "simulate", + Short: "Simulate the sync jobs", + Run: func(cmd *cobra.Command, args []string) { + cfg := internal.LoadResolvedConfig(configPath) + executeSyncJobs(cfg, true) + }, + } -var listCmd = &cobra.Command{ - Use: "list", - Short: "List the commands that will be executed", - Run: func(cmd *cobra.Command, args []string) { - cfg := internal.LoadResolvedConfig(configPath) - listCommands(cfg) - }, -} + var listCmd = &cobra.Command{ + Use: "list", + Short: "List the commands that will be executed", + Run: func(cmd *cobra.Command, args []string) { + cfg := internal.LoadResolvedConfig(configPath) + listCommands(cfg) + }, + } -func init() { - RootCmd.AddCommand(runCmd) - RootCmd.AddCommand(simulateCmd) - RootCmd.AddCommand(listCmd) + rootCmd.AddCommand(runCmd) + rootCmd.AddCommand(simulateCmd) + rootCmd.AddCommand(listCmd) } diff --git a/backup/cmd/check.go b/backup/cmd/check.go index 54f9071..9945de0 100644 --- a/backup/cmd/check.go +++ b/backup/cmd/check.go @@ -11,19 +11,21 @@ import ( var AppFs = afero.NewOsFs() -var checkCmd = &cobra.Command{ - Use: "check-coverage", - Short: "Check path coverage", - Run: func(cmd *cobra.Command, args []string) { - cfg := internal.LoadResolvedConfig(configPath) - uncoveredPaths := internal.ListUncoveredPaths(AppFs, cfg) - fmt.Println("Uncovered paths:") - for _, path := range uncoveredPaths { - fmt.Println(path) - } - }, -} +func AddCheckCommands(rootCmd *cobra.Command, configPath string) { + var checkCmd = &cobra.Command{ + Use: "check-coverage", + Short: "Check path coverage", + Run: func(cmd *cobra.Command, args []string) { + cfg := internal.LoadResolvedConfig(configPath) + uncoveredPaths := internal.ListUncoveredPaths(AppFs, cfg) + + fmt.Println("Uncovered paths:") + + for _, path := range uncoveredPaths { + fmt.Println(path) + } + }, + } -func init() { - RootCmd.AddCommand(checkCmd) + rootCmd.AddCommand(checkCmd) } diff --git a/backup/cmd/config.go b/backup/cmd/config.go index 7d01cc7..d3ba07c 100644 --- a/backup/cmd/config.go +++ b/backup/cmd/config.go @@ -9,42 +9,45 @@ import ( "gopkg.in/yaml.v3" ) -// configCmd represents the config command. -var configCmd = &cobra.Command{ - Use: "config", - Short: "Manage configuration", - Run: func(cmd *cobra.Command, args []string) { - // Implementation for the config command - fmt.Println("Config command executed") - }, -} +// AddConfigCommands binds the config command and its subcommands to the root command. +func AddConfigCommands(rootCmd *cobra.Command, configPath string) { + // configCmd represents the config command. + var configCmd = &cobra.Command{ + Use: "config", + Short: "Manage configuration", + Run: func(cmd *cobra.Command, args []string) { + // Implementation for the config command + fmt.Println("Config command executed") + }, + } -// Extend the config subcommand with the show verb. -var showCmd = &cobra.Command{ - Use: "show", - Short: "Show resolved configuration", - Run: func(cmd *cobra.Command, args []string) { - cfg := internal.LoadResolvedConfig(configPath) - out, err := yaml.Marshal(cfg) - if err != nil { - log.Fatalf("Failed to marshal resolved configuration: %v", err) - } - fmt.Printf("Resolved Configuration:\n%s\n", string(out)) - }, -} + // Extend the config subcommand with the show verb. + var showCmd = &cobra.Command{ + Use: "show", + Short: "Show resolved configuration", + Run: func(cmd *cobra.Command, args []string) { + cfg := internal.LoadResolvedConfig(configPath) -// Extend the config subcommand with the validate verb. -var validateCmd = &cobra.Command{ - Use: "validate", - Short: "Validate configuration", - Run: func(cmd *cobra.Command, args []string) { - internal.LoadResolvedConfig(configPath) - fmt.Println("Configuration is valid.") - }, -} + out, err := yaml.Marshal(cfg) + if err != nil { + log.Fatalf("Failed to marshal resolved configuration: %v", err) + } + + fmt.Printf("Resolved Configuration:\n%s\n", string(out)) + }, + } + + // Extend the config subcommand with the validate verb. + var validateCmd = &cobra.Command{ + Use: "validate", + Short: "Validate configuration", + Run: func(cmd *cobra.Command, args []string) { + internal.LoadResolvedConfig(configPath) + fmt.Println("Configuration is valid.") + }, + } -func init() { - RootCmd.AddCommand(configCmd) + rootCmd.AddCommand(configCmd) configCmd.AddCommand(showCmd) configCmd.AddCommand(validateCmd) } diff --git a/backup/cmd/root.go b/backup/cmd/root.go index 1de7cbb..71a1f64 100644 --- a/backup/cmd/root.go +++ b/backup/cmd/root.go @@ -7,23 +7,26 @@ import ( "github.com/spf13/cobra" ) -// RootCmd represents the base command when called without any subcommands. -var RootCmd = &cobra.Command{ - Use: "backup-tool", - Short: "A tool for managing backups", - Long: `backup-tool is a CLI tool for managing backups and configurations.`, -} +// Execute adds all child commands to the root command and sets flags appropriately. +func Execute() { + var configPath string -// Define a global configPath variable and flag at the root level. -var configPath string + rootCmd := &cobra.Command{ + Use: "backup-tool", + Short: "A tool for managing backups", + Long: `backup-tool is a CLI tool for managing backups and configurations.`, + } -func init() { - RootCmd.PersistentFlags().StringVar(&configPath, "config", "config.yaml", "Path to the configuration file") -} + rootCmd.PersistentFlags().StringVar(&configPath, "config", "config.yaml", "Path to the configuration file") -// Execute adds all child commands to the root command and sets flags appropriately. -func Execute() { - err := RootCmd.Execute() + // Parse flags before adding commands to ensure configPath is available. + rootCmd.ParseFlags(os.Args[1:]) + + AddConfigCommands(rootCmd, configPath) + AddBackupCommands(rootCmd, configPath) + AddCheckCommands(rootCmd, configPath) + + err := rootCmd.Execute() if err != nil { os.Exit(1) } From 4f3880198b9f570170ec0f421a5122d90d8e953a Mon Sep 17 00:00:00 2001 From: Jaap de Haan <261428+jdehaan@users.noreply.github.com> Date: Sat, 15 Nov 2025 15:07:47 +0000 Subject: [PATCH 07/10] fix: Be explicit for the execution mock during tests --- backup/cmd/check.go | 6 +-- backup/internal/job.go | 26 +++++++++-- backup/internal/job_test.go | 89 ++++++++++++++++++++++++++----------- 3 files changed, 89 insertions(+), 32 deletions(-) diff --git a/backup/cmd/check.go b/backup/cmd/check.go index 9945de0..b611c5e 100644 --- a/backup/cmd/check.go +++ b/backup/cmd/check.go @@ -9,15 +9,15 @@ import ( "github.com/spf13/cobra" ) -var AppFs = afero.NewOsFs() - func AddCheckCommands(rootCmd *cobra.Command, configPath string) { + var fs = afero.NewOsFs() + var checkCmd = &cobra.Command{ Use: "check-coverage", Short: "Check path coverage", Run: func(cmd *cobra.Command, args []string) { cfg := internal.LoadResolvedConfig(configPath) - uncoveredPaths := internal.ListUncoveredPaths(AppFs, cfg) + uncoveredPaths := internal.ListUncoveredPaths(fs, cfg) fmt.Println("Uncovered paths:") diff --git a/backup/internal/job.go b/backup/internal/job.go index 7689220..a9aaf51 100644 --- a/backup/internal/job.go +++ b/backup/internal/job.go @@ -1,12 +1,27 @@ package internal import ( + "context" "fmt" "os/exec" "strings" ) -var ExecCommand = exec.Command +// CommandExecutor interface for executing commands. +type CommandExecutor interface { + Execute(name string, args ...string) ([]byte, error) +} + +// RealCommandExecutor implements CommandExecutor using actual os/exec. +type RealCommandExecutor struct{} + +// Execute runs the actual command. +func (r *RealCommandExecutor) Execute(name string, args ...string) ([]byte, error) { + ctx := context.Background() + cmd := exec.CommandContext(ctx, name, args...) + + return cmd.CombinedOutput() +} func BuildRsyncCmd(job Job, simulate bool, logPath string) []string { args := []string{"-aiv", "--stats"} @@ -31,6 +46,12 @@ func BuildRsyncCmd(job Job, simulate bool, logPath string) []string { } func ExecuteJob(job Job, simulate bool, show bool, logPath string) string { + var osExec CommandExecutor = &RealCommandExecutor{} + + return ExecuteJobWithExecutor(job, simulate, show, logPath, osExec) +} + +func ExecuteJobWithExecutor(job Job, simulate bool, show bool, logPath string, executor CommandExecutor) string { if !job.Enabled { return "SKIPPED" } @@ -43,8 +64,7 @@ func ExecuteJob(job Job, simulate bool, show bool, logPath string) string { return "SUCCESS" } - cmd := ExecCommand("rsync", args...) - out, err := cmd.CombinedOutput() + out, err := executor.Execute("rsync", args...) fmt.Printf("Output:\n%s\n", string(out)) if err != nil { diff --git a/backup/internal/job_test.go b/backup/internal/job_test.go index 9ddb2b5..3bf77f9 100644 --- a/backup/internal/job_test.go +++ b/backup/internal/job_test.go @@ -1,7 +1,7 @@ package internal_test import ( - "os/exec" + "errors" "strings" "testing" @@ -10,29 +10,42 @@ import ( const statusSuccess = "SUCCESS" -var capturedArgs []string +// MockCommandExecutor implements CommandExecutor for testing. +type MockCommandExecutor struct { + CapturedCommands []MockCommand +} + +// MockCommand represents a captured command execution. +type MockCommand struct { + Name string + Args []string +} + +// Execute captures the command and simulates execution. +func (m *MockCommandExecutor) Execute(name string, args ...string) ([]byte, error) { + m.CapturedCommands = append(m.CapturedCommands, MockCommand{ + Name: name, + Args: append([]string{}, args...), // Make a copy of args + }) -var mockExecCommand = func(name string, args ...string) *exec.Cmd { if name == "rsync" { - capturedArgs = append(capturedArgs, args...) // Append arguments for assertions - if strings.Contains(strings.Join(args, " "), "--dry-run") { - return exec.Command("echo", "mocked rsync success") // Simulate success for dry-run - } - if strings.Contains(strings.Join(args, " "), "/invalid/source/path") { - return exec.Command("false") // Simulate failure for invalid paths + // Simulate different scenarios based on arguments + argsStr := strings.Join(args, " ") + + if strings.Contains(argsStr, "/invalid/source/path") { + errMsg := "rsync: link_stat \"/invalid/source/path\" failed: No such file or directory" + + return []byte(errMsg), errors.New("exit status 23") } - return exec.Command("echo", "mocked rsync success") // Simulate general success + return []byte("mocked rsync success"), nil } - return exec.Command(name, args...) -} - -func init() { - internal.ExecCommand = mockExecCommand + return []byte("command not mocked"), nil } func TestBuildRsyncCmd(t *testing.T) { + // This test doesn't need mocking since it only builds args job := internal.Job{ Source: "/home/user/Music/", Target: "/target/user/music/home", @@ -53,6 +66,9 @@ func TestBuildRsyncCmd(t *testing.T) { } func TestExecuteJob(t *testing.T) { + // Create mock executor + mockExecutor := &MockCommandExecutor{} + job := internal.Job{ Name: "test_job", Source: "/home/test/", @@ -63,7 +79,7 @@ func TestExecuteJob(t *testing.T) { } simulate := true - status := internal.ExecuteJob(job, simulate, false, "") + status := internal.ExecuteJobWithExecutor(job, simulate, false, "", mockExecutor) if status != statusSuccess { t.Errorf("Expected status SUCCESS, got %s", status) } @@ -75,7 +91,7 @@ func TestExecuteJob(t *testing.T) { Enabled: false, } - status = internal.ExecuteJob(disabledJob, simulate, false, "") + status = internal.ExecuteJobWithExecutor(disabledJob, simulate, false, "", mockExecutor) if status != "SKIPPED" { t.Errorf("Expected status SKIPPED, got %s", status) } @@ -89,7 +105,7 @@ func TestExecuteJob(t *testing.T) { Enabled: true, } - status = internal.ExecuteJob(invalidJob, false, false, "") + status = internal.ExecuteJobWithExecutor(invalidJob, false, false, "", mockExecutor) if status != "FAILURE" { t.Errorf("Expected status FAILURE, got %s", status) } @@ -97,6 +113,9 @@ func TestExecuteJob(t *testing.T) { // Ensure all references to ExecuteJob are prefixed with internal. func TestJobSkippedEnabledTrue(t *testing.T) { + // Create mock executor + mockExecutor := &MockCommandExecutor{} + job := internal.Job{ Name: "test_job", Source: "/home/test/", @@ -104,13 +123,16 @@ func TestJobSkippedEnabledTrue(t *testing.T) { Enabled: true, } - status := internal.ExecuteJob(job, true, false, "") + status := internal.ExecuteJobWithExecutor(job, true, false, "", mockExecutor) if status != statusSuccess { t.Errorf("Expected status SUCCESS, got %s", status) } } func TestJobSkippedEnabledFalse(t *testing.T) { + // Create mock executor (won't be used since job is disabled) + mockExecutor := &MockCommandExecutor{} + disabledJob := internal.Job{ Name: "disabled_job", Source: "/home/disabled/", @@ -118,13 +140,16 @@ func TestJobSkippedEnabledFalse(t *testing.T) { Enabled: false, } - status := internal.ExecuteJob(disabledJob, true, false, "") + status := internal.ExecuteJobWithExecutor(disabledJob, true, false, "", mockExecutor) if status != "SKIPPED" { t.Errorf("Expected status SKIPPED, got %s", status) } } func TestJobSkippedEnabledOmitted(t *testing.T) { + // Create mock executor + mockExecutor := &MockCommandExecutor{} + job := internal.Job{ Name: "omitted_enabled_job", Source: "/home/omitted/", @@ -133,15 +158,15 @@ func TestJobSkippedEnabledOmitted(t *testing.T) { Enabled: true, } - status := internal.ExecuteJob(job, true, false, "") + status := internal.ExecuteJobWithExecutor(job, true, false, "", mockExecutor) if status != statusSuccess { t.Errorf("Expected status SUCCESS, got %s", status) } } func TestExecuteJobWithMockedRsync(t *testing.T) { - // Reset capturedArgs before the test - capturedArgs = nil + // Create mock executor + mockExecutor := &MockCommandExecutor{} job := internal.Job{ Name: "test_job", @@ -151,13 +176,25 @@ func TestExecuteJobWithMockedRsync(t *testing.T) { Enabled: true, Exclusions: []string{"*.tmp"}, } - status := internal.ExecuteJob(job, true, false, "") + status := internal.ExecuteJobWithExecutor(job, true, false, "", mockExecutor) if status != statusSuccess { t.Errorf("Expected status SUCCESS, got %s", status) } - if len(capturedArgs) == 0 || capturedArgs[0] != "--dry-run" { - t.Errorf("Expected --dry-run flag, got %v", capturedArgs) + // Check that rsync was called with the expected arguments + if len(mockExecutor.CapturedCommands) == 0 { + t.Errorf("Expected at least one command to be executed") + + return + } + + cmd := mockExecutor.CapturedCommands[0] + if cmd.Name != "rsync" { + t.Errorf("Expected command to be 'rsync', got %s", cmd.Name) + } + + if len(cmd.Args) == 0 || cmd.Args[0] != "--dry-run" { + t.Errorf("Expected --dry-run flag, got %v", cmd.Args) } } From 514b2da834415310e5151474887ea1caf4248139 Mon Sep 17 00:00:00 2001 From: Jaap de Haan <261428+jdehaan@users.noreply.github.com> Date: Sat, 15 Nov 2025 15:29:43 +0000 Subject: [PATCH 08/10] fix: DRY test code --- .golangci.yml | 1 - backup/internal/check_test.go | 19 ++- backup/internal/config_test.go | 261 ++++++++++++++++++--------------- 3 files changed, 158 insertions(+), 123 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 1b9fd79..dd25486 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -10,7 +10,6 @@ linters: - err113 - errcheck - forbidigo - - funlen - gocritic - godoclint - gosec diff --git a/backup/internal/check_test.go b/backup/internal/check_test.go index 5fb8872..ff522ed 100644 --- a/backup/internal/check_test.go +++ b/backup/internal/check_test.go @@ -115,8 +115,8 @@ func runListUncoveredPathsTest( } } -func TestListUncoveredPathsVariations(t *testing.T) { - // Variation: all paths used +// Variation: all paths used. +func TestListUncoveredPathsVariationsAllCovered(t *testing.T) { runListUncoveredPathsTest(t, map[string][]string{ "/var/log": {"app1", "app2"}, @@ -134,8 +134,10 @@ func TestListUncoveredPathsVariations(t *testing.T) { }, []string{}, ) +} - // Variation: one source covered, one uncovered +// Variation: one source covered, one uncovered. +func TestListUncoveredPathsVariationsOneCoveredOneUncovered(t *testing.T) { runListUncoveredPathsTest(t, map[string][]string{ "/home/data": {"projects", "media"}, @@ -154,8 +156,10 @@ func TestListUncoveredPathsVariations(t *testing.T) { }, []string{"/home/user"}, ) +} - // Variation: one source covered, one uncovered but excluded +// Variation: one source covered, one uncovered but excluded. +func TestListUncoveredPathsVariationsUncoveredExcluded(t *testing.T) { runListUncoveredPathsTest(t, map[string][]string{ "/home/data": {"projects", "media"}, @@ -170,8 +174,10 @@ func TestListUncoveredPathsVariations(t *testing.T) { }, []string{}, ) +} - // Variation: one source covered, subfolders covered +// Variation: one source covered, subfolders covered. +func TestListUncoveredPathsVariationsSubfoldersCovered(t *testing.T) { runListUncoveredPathsTest(t, map[string][]string{ "/home/data": {"family"}, @@ -190,7 +196,10 @@ func TestListUncoveredPathsVariations(t *testing.T) { }, []string{}, ) +} +func TestListUncoveredPathsVariationsSubfoldersPartiallyCovered(t *testing.T) { + t.Skip("Skipping test for partially covered subfolders") // // Variation: one source covered, one uncovered subfolder // runListUncoveredPathsTest(t, // map[string][]string{ diff --git a/backup/internal/config_test.go b/backup/internal/config_test.go index 018ecb0..3183a24 100644 --- a/backup/internal/config_test.go +++ b/backup/internal/config_test.go @@ -94,75 +94,84 @@ jobs: } } -func TestYAMLUnmarshalingDefaults(t *testing.T) { - tests := []struct { - name string - yamlData string - expected internal.Job - }{ - { - name: "Defaults applied when fields omitted", - yamlData: ` +func TestYAMLUnmarshalingDefaults_FieldsOmitted(t *testing.T) { + yamlData := ` name: "test_job" source: "/source" target: "/target" -`, - expected: internal.Job{ - Name: "test_job", - Source: "/source", - Target: "/target", - Delete: true, - Enabled: true, - }, - }, - { - name: "Explicit false values preserved", - yamlData: ` +` + expected := internal.Job{ + Name: "test_job", + Source: "/source", + Target: "/target", + Delete: true, + Enabled: true, + } + + var job internal.Job + + err := yaml.Unmarshal([]byte(yamlData), &job) + if err != nil { + t.Fatalf("Failed to unmarshal YAML: %v", err) + } + + if !reflect.DeepEqual(job, expected) { + t.Errorf("got %+v, want %+v", job, expected) + } +} + +func TestYAMLUnmarshalingDefaults_ExplicitFalseValues(t *testing.T) { + yamlData := ` name: "test_job" source: "/source" target: "/target" delete: false enabled: false -`, - expected: internal.Job{ - Name: "test_job", - Source: "/source", - Target: "/target", - Delete: false, - Enabled: false, - }, - }, - { - name: "Mixed explicit and default values", - yamlData: ` +` + expected := internal.Job{ + Name: "test_job", + Source: "/source", + Target: "/target", + Delete: false, + Enabled: false, + } + + var job internal.Job + + err := yaml.Unmarshal([]byte(yamlData), &job) + if err != nil { + t.Fatalf("Failed to unmarshal YAML: %v", err) + } + + if !reflect.DeepEqual(job, expected) { + t.Errorf("got %+v, want %+v", job, expected) + } +} + +func TestYAMLUnmarshalingDefaults_MixedValues(t *testing.T) { + yamlData := ` name: "test_job" source: "/source" target: "/target" delete: false -`, - expected: internal.Job{ - Name: "test_job", - Source: "/source", - Target: "/target", - Delete: false, - Enabled: true, // default - }, - }, +` + expected := internal.Job{ + Name: "test_job", + Source: "/source", + Target: "/target", + Delete: false, + Enabled: true, // default } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - var job internal.Job + var job internal.Job - err := yaml.Unmarshal([]byte(tt.yamlData), &job) - if err != nil { - t.Fatalf("Failed to unmarshal YAML: %v", err) - } + err := yaml.Unmarshal([]byte(yamlData), &job) + if err != nil { + t.Fatalf("Failed to unmarshal YAML: %v", err) + } - if !reflect.DeepEqual(job, tt.expected) { - t.Errorf("got %+v, want %+v", job, tt.expected) - } - }) + if !reflect.DeepEqual(job, expected) { + t.Errorf("got %+v, want %+v", job, expected) } } @@ -233,76 +242,100 @@ func TestValidateJobNames(t *testing.T) { t.Errorf("Expected error message to contain '%s', but got '%s'", test.errorMessage, err.Error()) } } else { - if err != nil { - t.Errorf("Expected no error but got: %v", err) - } + expectNoError(t, err) } }) } } -func TestValidatePath(t *testing.T) { - tests := []struct { - name string +func expectNoError(t *testing.T, err error) { + t.Helper() + + if err != nil { + t.Errorf("Expected no error but got: %v", err) + } +} + +func expectError(t *testing.T, err error, expectedMessage string) { + t.Helper() + + if err == nil { + t.Errorf("Expected error but got none") + } else if err.Error() != expectedMessage { + t.Errorf("Expected error message '%s', but got '%s'", expectedMessage, err.Error()) + } +} + +func TestValidatePath_ValidSourcePath(t *testing.T) { + test := struct { + jobPath string + paths []internal.Path + pathType string + jobName string + }{ + jobPath: "/home/user/documents", + paths: []internal.Path{{Path: "/home/user"}}, + pathType: "source", + jobName: "job1", + } + + err := internal.ValidatePath(test.jobPath, test.paths, test.pathType, test.jobName) + expectNoError(t, err) +} + +func TestValidatePath_InvalidSourcePath(t *testing.T) { + test := struct { jobPath string paths []internal.Path pathType string jobName string - expectsError bool errorMessage string }{ - { - name: "Valid source path", - jobPath: "/home/user/documents", - paths: []internal.Path{{Path: "/home/user"}}, - pathType: "source", - jobName: "job1", - expectsError: false, - }, - { - name: "Invalid source path", - jobPath: "/invalid/source", - paths: []internal.Path{{Path: "/home/user"}}, - pathType: "source", - jobName: "job1", - expectsError: true, - errorMessage: "invalid source path for job 'job1': /invalid/source", - }, - { - name: "Valid target path", - jobPath: "/mnt/backup/documents", - paths: []internal.Path{{Path: "/mnt/backup"}}, - pathType: "target", - jobName: "job1", - expectsError: false, - }, - { - name: "Invalid target path", - jobPath: "/invalid/target", - paths: []internal.Path{{Path: "/mnt/backup"}}, - pathType: "target", - jobName: "job1", - expectsError: true, - errorMessage: "invalid target path for job 'job1': /invalid/target", - }, + jobPath: "/invalid/source", + paths: []internal.Path{{Path: "/home/user"}}, + pathType: "source", + jobName: "job1", + errorMessage: "invalid source path for job 'job1': /invalid/source", } - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - err := internal.ValidatePath(test.jobPath, test.paths, test.pathType, test.jobName) - if test.expectsError { - if err == nil { - t.Errorf("Expected error but got none") - } else if err.Error() != test.errorMessage { - t.Errorf("Expected error message '%s', but got '%s'", test.errorMessage, err.Error()) - } - } else { - if err != nil { - t.Errorf("Expected no error but got: %v", err) - } - } - }) + err := internal.ValidatePath(test.jobPath, test.paths, test.pathType, test.jobName) + expectError(t, err, test.errorMessage) +} + +func TestValidatePath_ValidTargetPath(t *testing.T) { + test := struct { + jobPath string + paths []internal.Path + pathType string + jobName string + }{ + jobPath: "/mnt/backup/documents", + paths: []internal.Path{{Path: "/mnt/backup"}}, + pathType: "target", + jobName: "job1", + } + + err := internal.ValidatePath(test.jobPath, test.paths, test.pathType, test.jobName) + expectNoError(t, err) +} + +func TestValidatePath_InvalidTargetPath(t *testing.T) { + test := struct { + jobPath string + paths []internal.Path + pathType string + jobName string + errorMessage string + }{ + jobPath: "/invalid/target", + paths: []internal.Path{{Path: "/mnt/backup"}}, + pathType: "target", + jobName: "job1", + errorMessage: "invalid target path for job 'job1': /invalid/target", } + + err := internal.ValidatePath(test.jobPath, test.paths, test.pathType, test.jobName) + expectError(t, err, test.errorMessage) } func TestValidatePaths(t *testing.T) { @@ -351,15 +384,9 @@ func TestValidatePaths(t *testing.T) { t.Run(test.name, func(t *testing.T) { err := internal.ValidatePaths(test.cfg) if test.expectsError { - if err == nil { - t.Errorf("Expected error but got none") - } else if err.Error() != test.errorMessage { - t.Errorf("Expected error message '%s', but got '%s'", test.errorMessage, err.Error()) - } + expectError(t, err, test.errorMessage) } else { - if err != nil { - t.Errorf("Expected no error but got: %v", err) - } + expectNoError(t, err) } }) } From ca5b48c8fc0392b11088a8eb1e607555b59538d2 Mon Sep 17 00:00:00 2001 From: Jaap de Haan <261428+jdehaan@users.noreply.github.com> Date: Sat, 15 Nov 2025 16:00:42 +0000 Subject: [PATCH 09/10] fix: Cleanup and guard dependencies --- .golangci.yml | 31 +++++++++++++++------ Makefile | 9 ++++-- backup/internal/check.go | 3 +- backup/internal/check_test.go | 8 +++--- backup/internal/config.go | 26 +++++++++++------ backup/internal/config_test.go | 51 ++++++++++++++++++++++------------ backup/internal/job.go | 7 ++++- backup/internal/job_test.go | 5 +++- backup/internal/types.go | 4 ++- 9 files changed, 101 insertions(+), 43 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index dd25486..8af7327 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -5,25 +5,40 @@ linters: - wsl # The linter 'wsl' is deprecated (since v2.2.0) due to: new major version. Replaced by wsl_v5 - paralleltest # not used in this project - revive # temporarily disable - - exhaustruct # temporarily disable - - depguard - - err113 - errcheck - forbidigo - gocritic - godoclint - gosec - - varnamelen - - wrapcheck settings: + depguard: + rules: + main: + list-mode: strict + allow: + - bytes + - context + - errors + - fmt + - io + - log + - os + - path/filepath + - sort + - strings + - testing + - time + - github.com/spf13/cobra + - github.com/spf13/afero + - gopkg.in/yaml.v3 + - backup-rsync/backup/internal + - backup-rsync/backup/cmd errcheck: check-type-assertions: true check-blank: true - staticcheck: - checks: all varnamelen: ignore-decls: - fs afero.Fs + - fs afero.Fs formatters: enable: - gofmt diff --git a/Makefile b/Makefile index d00a873..7af44c7 100644 --- a/Makefile +++ b/Makefile @@ -4,13 +4,18 @@ BUILD_CMD = CGO_ENABLED=0 go build -ldflags="-s -w" PACKAGE = ./backup/main.go -.PHONY: build clean test lint tidy checksums release sanity-check check-mod-tidy +.PHONY: build clean test lint tidy checksums release sanity-check check-mod-tidy lint-config-check lint-fix format check-clean format: go fmt ./... @echo "OK: Code formatted." -lint: +lint-config-check: + @golangci-lint config path + @golangci-lint config verify + @echo "OK: Lint configuration is valid." + +lint: lint-config-check golangci-lint run ./... lint-fix: diff --git a/backup/internal/check.go b/backup/internal/check.go index 1f08726..362b53c 100644 --- a/backup/internal/check.go +++ b/backup/internal/check.go @@ -1,6 +1,7 @@ package internal import ( + "fmt" "log" "path/filepath" "sort" @@ -147,7 +148,7 @@ func getChildDirectories(fs afero.Fs, path string) ([]string, error) { fileInfos, err := afero.ReadDir(fs, path) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to read directory '%s': %w", path, err) } for _, info := range fileInfos { diff --git a/backup/internal/check_test.go b/backup/internal/check_test.go index ff522ed..a755049 100644 --- a/backup/internal/check_test.go +++ b/backup/internal/check_test.go @@ -102,15 +102,15 @@ func runListUncoveredPathsTest( return } - for i, path := range uncoveredPaths { - if i >= len(expectedUncoveredPaths) { + for count, path := range uncoveredPaths { + if count >= len(expectedUncoveredPaths) { t.Errorf("Got more uncovered paths than expected. Got: %v", uncoveredPaths) return } - if path != expectedUncoveredPaths[i] { - t.Errorf("Expected uncovered path '%s', got '%s'", expectedUncoveredPaths[i], path) + if path != expectedUncoveredPaths[count] { + t.Errorf("Expected uncovered path '%s', got '%s'", expectedUncoveredPaths[count], path) } } } diff --git a/backup/internal/config.go b/backup/internal/config.go index 5a46c2e..74c482e 100644 --- a/backup/internal/config.go +++ b/backup/internal/config.go @@ -1,6 +1,7 @@ package internal import ( + "errors" "fmt" "io" "log" @@ -11,12 +12,20 @@ import ( "gopkg.in/yaml.v3" ) +// Static errors for wrapping.. +var ( + ErrJobValidation = errors.New("job validation failed") + ErrInvalidPath = errors.New("invalid path") + ErrPathValidation = errors.New("path validation failed") + ErrOverlappingPath = errors.New("overlapping path detected") +) + func LoadConfig(reader io.Reader) (Config, error) { var cfg Config err := yaml.NewDecoder(reader).Decode(&cfg) if err != nil { - return Config{}, err + return Config{}, fmt.Errorf("failed to decode YAML: %w", err) } // Defaults are handled in Job.UnmarshalYAML @@ -64,7 +73,7 @@ func ValidateJobNames(jobs []Job) error { } if len(invalidNames) > 0 { - return fmt.Errorf("job validation errors: %v", invalidNames) + return fmt.Errorf("%w: %v", ErrJobValidation, invalidNames) } return nil @@ -77,7 +86,7 @@ func ValidatePath(jobPath string, paths []Path, pathType string, jobName string) } } - return fmt.Errorf("invalid %s path for job '%s': %s", pathType, jobName, jobPath) + return fmt.Errorf("%w for job '%s': %s %s", ErrInvalidPath, jobName, pathType, jobPath) } func ValidatePaths(cfg Config) error { @@ -96,7 +105,7 @@ func ValidatePaths(cfg Config) error { } if len(invalidPaths) > 0 { - return fmt.Errorf("path validation errors: %v", invalidPaths) + return fmt.Errorf("%w: %v", ErrPathValidation, invalidPaths) } return nil @@ -124,7 +133,8 @@ func validateJobPaths(jobs []Job, pathType string, getPath func(job Job) string) } if !excluded && strings.HasPrefix(path1, path2) { - return fmt.Errorf("Job '%s' has a %s path overlapping with job '%s'", job1.Name, pathType, job2.Name) + return fmt.Errorf("%w: job '%s' has a %s path overlapping with job '%s'", + ErrOverlappingPath, job1.Name, pathType, job2.Name) } } } @@ -134,13 +144,13 @@ func validateJobPaths(jobs []Job, pathType string, getPath func(job Job) string) } func LoadResolvedConfig(configPath string) Config { - f, err := os.Open(configPath) + configFile, err := os.Open(configPath) if err != nil { log.Fatalf("Failed to open config: %v", err) } - defer f.Close() + defer configFile.Close() - cfg, err := LoadConfig(f) + cfg, err := LoadConfig(configFile) if err != nil { log.Fatalf("Failed to parse YAML: %v", err) } diff --git a/backup/internal/config_test.go b/backup/internal/config_test.go index 3183a24..7981190 100644 --- a/backup/internal/config_test.go +++ b/backup/internal/config_test.go @@ -2,7 +2,6 @@ package internal_test import ( "bytes" - "reflect" "strings" "testing" @@ -89,8 +88,8 @@ jobs: }, } - if !reflect.DeepEqual(cfg.Jobs, expected) { - t.Errorf("got %+v, want %+v", cfg.Jobs, expected) + for i, job := range cfg.Jobs { + assertJobEqual(t, job, expected[i]) } } @@ -115,9 +114,7 @@ target: "/target" t.Fatalf("Failed to unmarshal YAML: %v", err) } - if !reflect.DeepEqual(job, expected) { - t.Errorf("got %+v, want %+v", job, expected) - } + assertJobEqual(t, job, expected) } func TestYAMLUnmarshalingDefaults_ExplicitFalseValues(t *testing.T) { @@ -143,9 +140,7 @@ enabled: false t.Fatalf("Failed to unmarshal YAML: %v", err) } - if !reflect.DeepEqual(job, expected) { - t.Errorf("got %+v, want %+v", job, expected) - } + assertJobEqual(t, job, expected) } func TestYAMLUnmarshalingDefaults_MixedValues(t *testing.T) { @@ -170,9 +165,7 @@ delete: false t.Fatalf("Failed to unmarshal YAML: %v", err) } - if !reflect.DeepEqual(job, expected) { - t.Errorf("got %+v, want %+v", job, expected) - } + assertJobEqual(t, job, expected) } func TestSubstituteVariables(t *testing.T) { @@ -295,7 +288,7 @@ func TestValidatePath_InvalidSourcePath(t *testing.T) { paths: []internal.Path{{Path: "/home/user"}}, pathType: "source", jobName: "job1", - errorMessage: "invalid source path for job 'job1': /invalid/source", + errorMessage: "invalid path for job 'job1': source /invalid/source", } err := internal.ValidatePath(test.jobPath, test.paths, test.pathType, test.jobName) @@ -331,7 +324,7 @@ func TestValidatePath_InvalidTargetPath(t *testing.T) { paths: []internal.Path{{Path: "/mnt/backup"}}, pathType: "target", jobName: "job1", - errorMessage: "invalid target path for job 'job1': /invalid/target", + errorMessage: "invalid path for job 'job1': target /invalid/target", } err := internal.ValidatePath(test.jobPath, test.paths, test.pathType, test.jobName) @@ -374,9 +367,9 @@ func TestValidatePaths(t *testing.T) { }, }, expectsError: true, - errorMessage: "path validation errors: [" + - "invalid source path for job 'job1': /invalid/source " + - "invalid target path for job 'job1': /invalid/target]", + errorMessage: "path validation failed: [" + + "invalid path for job 'job1': source /invalid/source " + + "invalid path for job 'job1': target /invalid/target]", }, } @@ -391,3 +384,27 @@ func TestValidatePaths(t *testing.T) { }) } } + +func assertJobEqual(t *testing.T, got, expected internal.Job) { + t.Helper() + + if got.Name != expected.Name { + t.Errorf("Job name mismatch: got %s, want %s", got.Name, expected.Name) + } + + if got.Source != expected.Source { + t.Errorf("Job source mismatch: got %s, want %s", got.Source, expected.Source) + } + + if got.Target != expected.Target { + t.Errorf("Job target mismatch: got %s, want %s", got.Target, expected.Target) + } + + if got.Delete != expected.Delete { + t.Errorf("Job delete flag mismatch: got %v, want %v", got.Delete, expected.Delete) + } + + if got.Enabled != expected.Enabled { + t.Errorf("Job enabled flag mismatch: got %v, want %v", got.Enabled, expected.Enabled) + } +} diff --git a/backup/internal/job.go b/backup/internal/job.go index a9aaf51..3a907fd 100644 --- a/backup/internal/job.go +++ b/backup/internal/job.go @@ -20,7 +20,12 @@ func (r *RealCommandExecutor) Execute(name string, args ...string) ([]byte, erro ctx := context.Background() cmd := exec.CommandContext(ctx, name, args...) - return cmd.CombinedOutput() + output, err := cmd.CombinedOutput() + if err != nil { + return nil, fmt.Errorf("failed to execute command '%s %s': %w", name, strings.Join(args, " "), err) + } + + return output, nil } func BuildRsyncCmd(job Job, simulate bool, logPath string) []string { diff --git a/backup/internal/job_test.go b/backup/internal/job_test.go index 3bf77f9..6449e91 100644 --- a/backup/internal/job_test.go +++ b/backup/internal/job_test.go @@ -8,6 +8,9 @@ import ( "backup-rsync/backup/internal" ) +// Static error for testing. +var ErrExitStatus23 = errors.New("exit status 23") + const statusSuccess = "SUCCESS" // MockCommandExecutor implements CommandExecutor for testing. @@ -35,7 +38,7 @@ func (m *MockCommandExecutor) Execute(name string, args ...string) ([]byte, erro if strings.Contains(argsStr, "/invalid/source/path") { errMsg := "rsync: link_stat \"/invalid/source/path\" failed: No such file or directory" - return []byte(errMsg), errors.New("exit status 23") + return []byte(errMsg), ErrExitStatus23 } return []byte("mocked rsync success"), nil diff --git a/backup/internal/types.go b/backup/internal/types.go index 5883c55..9b553b3 100644 --- a/backup/internal/types.go +++ b/backup/internal/types.go @@ -1,6 +1,8 @@ package internal import ( + "fmt" + "gopkg.in/yaml.v3" ) @@ -46,7 +48,7 @@ func (j *Job) UnmarshalYAML(node *yaml.Node) error { err := node.Decode(&jobYAML) if err != nil { - return err + return fmt.Errorf("failed to decode YAML node: %w", err) } // Copy basic fields From 7b933f8cab164e972a709b276c7844c454d2641b Mon Sep 17 00:00:00 2001 From: Jaap de Haan <261428+jdehaan@users.noreply.github.com> Date: Sat, 15 Nov 2025 23:53:34 +0000 Subject: [PATCH 10/10] fix: Cleanup code --- .golangci.yml | 6 +- Makefile | 2 +- backup/cmd/root.go | 7 +- backup/internal/config_test.go | 48 ++++----- backup/internal/job_test.go | 187 ++++++++++++++++++++------------- 5 files changed, 140 insertions(+), 110 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 8af7327..ca0e978 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -5,11 +5,10 @@ linters: - wsl # The linter 'wsl' is deprecated (since v2.2.0) due to: new major version. Replaced by wsl_v5 - paralleltest # not used in this project - revive # temporarily disable + - exhaustruct # temporarily disable - errcheck - forbidigo - gocritic - - godoclint - - gosec settings: depguard: rules: @@ -36,6 +35,9 @@ linters: errcheck: check-type-assertions: true check-blank: true + gosec: + excludes: + - G304 # ignore Potential file inclusion via variable varnamelen: ignore-decls: - fs afero.Fs diff --git a/Makefile b/Makefile index 7af44c7..3e89421 100644 --- a/Makefile +++ b/Makefile @@ -15,7 +15,7 @@ lint-config-check: @golangci-lint config verify @echo "OK: Lint configuration is valid." -lint: lint-config-check +lint: golangci-lint run ./... lint-fix: diff --git a/backup/cmd/root.go b/backup/cmd/root.go index 71a1f64..1505d84 100644 --- a/backup/cmd/root.go +++ b/backup/cmd/root.go @@ -20,13 +20,16 @@ func Execute() { rootCmd.PersistentFlags().StringVar(&configPath, "config", "config.yaml", "Path to the configuration file") // Parse flags before adding commands to ensure configPath is available. - rootCmd.ParseFlags(os.Args[1:]) + err := rootCmd.ParseFlags(os.Args[1:]) + if err != nil { + os.Exit(1) + } AddConfigCommands(rootCmd, configPath) AddBackupCommands(rootCmd, configPath) AddCheckCommands(rootCmd, configPath) - err := rootCmd.Execute() + err = rootCmd.Execute() if err != nil { os.Exit(1) } diff --git a/backup/internal/config_test.go b/backup/internal/config_test.go index 7981190..a6058ee 100644 --- a/backup/internal/config_test.go +++ b/backup/internal/config_test.go @@ -264,35 +264,29 @@ func TestValidatePath_ValidSourcePath(t *testing.T) { jobPath string paths []internal.Path pathType string - jobName string }{ jobPath: "/home/user/documents", paths: []internal.Path{{Path: "/home/user"}}, pathType: "source", - jobName: "job1", } - err := internal.ValidatePath(test.jobPath, test.paths, test.pathType, test.jobName) + err := internal.ValidatePath(test.jobPath, test.paths, test.pathType, "job1") expectNoError(t, err) } func TestValidatePath_InvalidSourcePath(t *testing.T) { test := struct { - jobPath string - paths []internal.Path - pathType string - jobName string - errorMessage string + jobPath string + paths []internal.Path + pathType string }{ - jobPath: "/invalid/source", - paths: []internal.Path{{Path: "/home/user"}}, - pathType: "source", - jobName: "job1", - errorMessage: "invalid path for job 'job1': source /invalid/source", + jobPath: "/invalid/source", + paths: []internal.Path{{Path: "/home/user"}}, + pathType: "source", } - err := internal.ValidatePath(test.jobPath, test.paths, test.pathType, test.jobName) - expectError(t, err, test.errorMessage) + err := internal.ValidatePath(test.jobPath, test.paths, test.pathType, "job1") + expectError(t, err, "invalid path for job 'job1': source /invalid/source") } func TestValidatePath_ValidTargetPath(t *testing.T) { @@ -300,35 +294,29 @@ func TestValidatePath_ValidTargetPath(t *testing.T) { jobPath string paths []internal.Path pathType string - jobName string }{ jobPath: "/mnt/backup/documents", paths: []internal.Path{{Path: "/mnt/backup"}}, pathType: "target", - jobName: "job1", } - err := internal.ValidatePath(test.jobPath, test.paths, test.pathType, test.jobName) + err := internal.ValidatePath(test.jobPath, test.paths, test.pathType, "job1") expectNoError(t, err) } func TestValidatePath_InvalidTargetPath(t *testing.T) { test := struct { - jobPath string - paths []internal.Path - pathType string - jobName string - errorMessage string + jobPath string + paths []internal.Path + pathType string }{ - jobPath: "/invalid/target", - paths: []internal.Path{{Path: "/mnt/backup"}}, - pathType: "target", - jobName: "job1", - errorMessage: "invalid path for job 'job1': target /invalid/target", + jobPath: "/invalid/target", + paths: []internal.Path{{Path: "/mnt/backup"}}, + pathType: "target", } - err := internal.ValidatePath(test.jobPath, test.paths, test.pathType, test.jobName) - expectError(t, err, test.errorMessage) + err := internal.ValidatePath(test.jobPath, test.paths, test.pathType, "job1") + expectError(t, err, "invalid path for job 'job1': target /invalid/target") } func TestValidatePaths(t *testing.T) { diff --git a/backup/internal/job_test.go b/backup/internal/job_test.go index 6449e91..18490d5 100644 --- a/backup/internal/job_test.go +++ b/backup/internal/job_test.go @@ -24,6 +24,59 @@ type MockCommand struct { Args []string } +// Option defines a function that modifies a Job. +type Option func(*internal.Job) + +// NewJob is a job factory with defaults. +func NewJob(opts ...Option) *internal.Job { + // Default values + job := &internal.Job{ + Name: "job", + Source: "", + Target: "", + Delete: true, + Enabled: true, + Exclusions: []string{}, + } + + // Apply all options (overrides defaults) + for _, opt := range opts { + opt(job) + } + + return job +} + +func WithName(name string) Option { + return func(p *internal.Job) { + p.Name = name + } +} + +func WithSource(source string) Option { + return func(p *internal.Job) { + p.Source = source + } +} + +func WithTarget(target string) Option { + return func(p *internal.Job) { + p.Target = target + } +} + +func WithEnabled(enabled bool) Option { + return func(p *internal.Job) { + p.Enabled = enabled + } +} + +func WithExclusions(exclusions []string) Option { + return func(p *internal.Job) { + p.Exclusions = exclusions + } +} + // Execute captures the command and simulates execution. func (m *MockCommandExecutor) Execute(name string, args ...string) ([]byte, error) { m.CapturedCommands = append(m.CapturedCommands, MockCommand{ @@ -49,12 +102,11 @@ func (m *MockCommandExecutor) Execute(name string, args ...string) ([]byte, erro func TestBuildRsyncCmd(t *testing.T) { // This test doesn't need mocking since it only builds args - job := internal.Job{ - Source: "/home/user/Music/", - Target: "/target/user/music/home", - Delete: true, - Exclusions: []string{"*.tmp", "node_modules/"}, - } + job := *NewJob( + WithSource("/home/user/Music/"), + WithTarget("/target/user/music/home"), + WithExclusions([]string{"*.tmp", "node_modules/"}), + ) args := internal.BuildRsyncCmd(job, true, "") expectedArgs := []string{ @@ -72,46 +124,36 @@ func TestExecuteJob(t *testing.T) { // Create mock executor mockExecutor := &MockCommandExecutor{} - job := internal.Job{ - Name: "test_job", - Source: "/home/test/", - Target: "/mnt/backup1/test/", - Delete: true, - Enabled: true, - Exclusions: []string{"*.tmp"}, - } + job := *NewJob( + WithName("test_job"), + WithSource("/home/test/"), + WithTarget("/mnt/backup1/test/"), + WithExclusions([]string{"*.tmp"}), + ) simulate := true status := internal.ExecuteJobWithExecutor(job, simulate, false, "", mockExecutor) - if status != statusSuccess { - t.Errorf("Expected status SUCCESS, got %s", status) - } + expectStatus(t, status, statusSuccess) - disabledJob := internal.Job{ - Name: "disabled_job", - Source: "/home/disabled/", - Target: "/mnt/backup1/disabled/", - Enabled: false, - } + disabledJob := *NewJob( + WithName("disabled_job"), + WithSource("/home/disabled/"), + WithTarget("/mnt/backup1/disabled/"), + WithEnabled(false), + ) status = internal.ExecuteJobWithExecutor(disabledJob, simulate, false, "", mockExecutor) - if status != "SKIPPED" { - t.Errorf("Expected status SKIPPED, got %s", status) - } + expectStatus(t, status, "SKIPPED") // Test case for failure (simulate by providing invalid source path) - invalidJob := internal.Job{ - Name: "invalid_job", - Source: "/invalid/source/path", - Target: "/mnt/backup1/invalid/", - Delete: true, - Enabled: true, - } + invalidJob := *NewJob( + WithName("invalid_job"), + WithSource("/invalid/source/path"), + WithTarget("/mnt/backup1/invalid/"), + ) status = internal.ExecuteJobWithExecutor(invalidJob, false, false, "", mockExecutor) - if status != "FAILURE" { - t.Errorf("Expected status FAILURE, got %s", status) - } + expectStatus(t, status, "FAILURE") } // Ensure all references to ExecuteJob are prefixed with internal. @@ -119,71 +161,58 @@ func TestJobSkippedEnabledTrue(t *testing.T) { // Create mock executor mockExecutor := &MockCommandExecutor{} - job := internal.Job{ - Name: "test_job", - Source: "/home/test/", - Target: "/mnt/backup1/test/", - Enabled: true, - } + job := *NewJob( + WithName("test_job"), + WithSource("/home/test/"), + WithTarget("/mnt/backup1/test/"), + ) status := internal.ExecuteJobWithExecutor(job, true, false, "", mockExecutor) - if status != statusSuccess { - t.Errorf("Expected status SUCCESS, got %s", status) - } + expectStatus(t, status, statusSuccess) } func TestJobSkippedEnabledFalse(t *testing.T) { // Create mock executor (won't be used since job is disabled) mockExecutor := &MockCommandExecutor{} - disabledJob := internal.Job{ - Name: "disabled_job", - Source: "/home/disabled/", - Target: "/mnt/backup1/disabled/", - Enabled: false, - } + disabledJob := *NewJob( + WithName("disabled_job"), + WithSource("/home/disabled/"), + WithTarget("/mnt/backup1/disabled/"), + WithEnabled(false), + ) status := internal.ExecuteJobWithExecutor(disabledJob, true, false, "", mockExecutor) - if status != "SKIPPED" { - t.Errorf("Expected status SKIPPED, got %s", status) - } + expectStatus(t, status, "SKIPPED") } func TestJobSkippedEnabledOmitted(t *testing.T) { // Create mock executor mockExecutor := &MockCommandExecutor{} - job := internal.Job{ - Name: "omitted_enabled_job", - Source: "/home/omitted/", - Target: "/mnt/backup1/omitted/", - Delete: true, - Enabled: true, - } + job := *NewJob( + WithName("omitted_enabled_job"), + WithSource("/home/omitted/"), + WithTarget("/mnt/backup1/omitted/"), + ) status := internal.ExecuteJobWithExecutor(job, true, false, "", mockExecutor) - if status != statusSuccess { - t.Errorf("Expected status SUCCESS, got %s", status) - } + expectStatus(t, status, statusSuccess) } func TestExecuteJobWithMockedRsync(t *testing.T) { // Create mock executor mockExecutor := &MockCommandExecutor{} - job := internal.Job{ - Name: "test_job", - Source: "/home/test/", - Target: "/mnt/backup1/test/", - Delete: true, - Enabled: true, - Exclusions: []string{"*.tmp"}, - } + job := *NewJob( + WithName("test_job"), + WithSource("/home/test/"), + WithTarget("/mnt/backup1/test/"), + WithExclusions([]string{"*.tmp"}), + ) status := internal.ExecuteJobWithExecutor(job, true, false, "", mockExecutor) - if status != statusSuccess { - t.Errorf("Expected status SUCCESS, got %s", status) - } + expectStatus(t, status, statusSuccess) // Check that rsync was called with the expected arguments if len(mockExecutor.CapturedCommands) == 0 { @@ -201,3 +230,11 @@ func TestExecuteJobWithMockedRsync(t *testing.T) { t.Errorf("Expected --dry-run flag, got %v", cmd.Args) } } + +func expectStatus(t *testing.T, status, expectedStatus string) { + t.Helper() + + if status != expectedStatus { + t.Errorf("Expected status %s, got %s", expectedStatus, status) + } +}