From 3c1ade03efb17965e3231c365e44b8dd289434df Mon Sep 17 00:00:00 2001 From: Jaap de Haan <261428+jdehaan@users.noreply.github.com> Date: Fri, 20 Mar 2026 08:01:00 +0000 Subject: [PATCH 01/25] chore: Add AGENTS.md --- AGENTS.md | 61 +++++++++++++++++ MOCKERY_INTEGRATION.md | 149 +++++++++++++++++++++++++++++++++++++++++ TESTING_GUIDE.md | 132 ++++++++++++++++++++++++++++++++++++ 3 files changed, 342 insertions(+) create mode 100644 AGENTS.md create mode 100644 MOCKERY_INTEGRATION.md create mode 100644 TESTING_GUIDE.md diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000..8835711 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,61 @@ +# Project Guidelines + +## Overview + +CLI tool for managing local backups using `rsync` as the engine. Built in Go with `cobra` for CLI, `afero` for filesystem abstraction, and YAML for configuration. Local-only — no remote rsync support. + +## Code Style + +- Go 1.24+; follow idiomatic Go conventions +- Format with `go fmt`; lint with `golangci-lint` (config in `.golangci.yml`) +- All linters enabled by default — check `.golangci.yml` for disabled ones +- Keep packages focused: `cmd/` for CLI wiring, `internal/` for core logic +- Prefer dependency injection over global state for testability +- Use interfaces at consumption boundaries (see `internal/exec.go`, `internal/job_command.go`) + +## Architecture + +``` +backup/ + main.go # Entrypoint — calls cmd.BuildRootCommand().Execute() + cmd/ # Cobra commands: list, run, simulate, config, check-coverage, version + internal/ # Core logic: config loading, job execution, rsync wrapper +``` + +- **Config**: YAML-based (`Config`, `Job`, sources, targets, variables with `${var}` substitution) +- **Jobs**: Each job maps a source to a target with optional exclusions; jobs run independently +- **Rsync**: Wrapped via `SharedCommand` struct in `internal/rsync.go` +- YAML config files at repo root (e.g., `sync.yaml`) define backup configurations + +## Build and Test + +```sh +make build # Build to dist/backup +make test # go test ./... -v +make lint # golangci-lint run ./... +make lint-fix # Auto-fix lint issues +make format # go fmt ./... +make tidy # gofmt -s + go mod tidy +make sanity-check # format + clean + tidy +``` + +## Testing Conventions + +- See `TESTING_GUIDE.md` for patterns and examples +- Use **dependency injection** — inject interfaces, not concrete types +- **Mocks**: Generated with [mockery](https://github.com/vektra/mockery) (config: `.mockery.yml`) + - Mock files live in `internal/test/` as `mock__test.go` + - Mock structs named `Mock` + - See `MOCKERY_INTEGRATION.md` for setup details +- Use `testify` for assertions (`require` / `assert`) +- Test files live in `/test/` subdirectories +- Prefer table-driven tests for multiple input scenarios + +## Conventions + +- No remote rsync — only locally mounted paths +- Job-level granularity: each backup job can be listed, simulated, or run independently +- Dry-run/simulate mode available for all operations +- Logging goes to both stdout and timestamped log directories under `logs/` +- Custom YAML unmarshaling handles job defaults (see `internal/job.go`) +- CI runs sanity checks, lint, and build on every push/PR (`.github/workflows/go.yml`) diff --git a/MOCKERY_INTEGRATION.md b/MOCKERY_INTEGRATION.md new file mode 100644 index 0000000..8e6e21b --- /dev/null +++ b/MOCKERY_INTEGRATION.md @@ -0,0 +1,149 @@ +# Mockery Integration Guide + +This document explains how mockery has been integrated into this Go project for generating mocks from interfaces. + +## Installation + +Mockery v3.6.1 has been installed in the project: + +```bash +go install github.com/vektra/mockery/v3@v3.6.1 +``` + +## Configuration + +The project uses a `.mockery.yml` configuration file to control mock generation: + +```yaml +all: false +dir: '{{.InterfaceDir}}/test' +filename: mock_{{.InterfaceName | lower}}_test.go +force-file-write: true +formatter: goimports +generate: true +include-auto-generated: false +log-level: info +structname: 'Mock{{.InterfaceName}}' +pkgname: 'internal_test' +recursive: false +template: testify +packages: + backup-rsync/backup/internal: + interfaces: + Exec: + JobCommand: +``` + +Key configuration points: +- **Filename pattern**: `mock_{{.InterfaceName | lower}}_test.go` for standard naming +- **Struct naming**: `Mock{{.InterfaceName}}` for standard mock naming +- **Package**: `internal_test` to match the test package +- **Template**: Uses the `testify` template for rich mock functionality + +## Generated Mocks + +Running `mockery` generates the following mock files: + +- `backup/internal/test/mock_exec_test.go` - Mock for the `Exec` interface +- `backup/internal/test/mock_jobcommand_test.go` - Mock for the `JobCommand` interface + +These mocks provide: +- **Type-safe mocking** with compile-time verification +- **Expectation-based testing** with automatic verification +- **Fluent interface** for setting up complex expectations +- **Automatic cleanup** via `t.Cleanup()` + +## Usage Examples + +### Basic Usage + +```go +func TestJobApply_WithMockeryJobCommand_Success(t *testing.T) { + // Create mock using mockery's generated constructor + mockJobCommand := NewMockJobCommand(t) + + // Create an enabled job + enabledJob := NewJob( + WithName("success_job"), + WithSource("/home/success/"), + WithTarget("/mnt/backup1/success/"), + WithEnabled(true), + ) + + // Set expectation that Run will be called and return Success + mockJobCommand.EXPECT().Run(mock.MatchedBy(func(job Job) bool { + return job.Name == "success_job" && + job.Source == "/home/success/" && + job.Target == "/mnt/backup1/success/" && + job.Enabled == true + })).Return(Success).Once() + + // Apply the job + status := enabledJob.Apply(mockJobCommand) + + // Assert that the status is Success + assert.Equal(t, Success, status) + + // mockery automatically verifies expectations via t.Cleanup() +} +``` + +### Testing Disabled Jobs + +```go +func TestJobApply_WithMockeryJobCommand_DisabledJob(t *testing.T) { + // Create mock using mockery's generated constructor + mockJobCommand := NewMockJobCommand(t) + + // Create a disabled job + disabledJob := NewJob( + WithName("disabled_job"), + WithEnabled(false), + ) + + // No expectations set - the Run method should NOT be called for disabled jobs + + // Apply the job + status := disabledJob.Apply(mockJobCommand) + + // Assert that the status is Skipped + assert.Equal(t, Skipped, status) + + // The mock will automatically verify that Run was NOT called +} +``` + +## Replacement of Simple Mocks + +The project has fully migrated from simple mocks to mockery-generated mocks: + +- **Old simple mocks**: Removed (used field-based APIs like `CapturedCommands`, `Output`, `Error`) +- **New mockery mocks** (`MockExec`, `MockJobCommand`): Used for all testing scenarios with rich expectations and automatic verification + +## Benefits of Mockery Integration + +1. **Type Safety**: Compile-time verification of mock method signatures +2. **Rich Expectations**: Support for complex argument matching and call counting +3. **Automatic Verification**: Expectations are automatically verified at test completion +4. **Fluent Interface**: Easy-to-read test setup with method chaining +5. **Maintenance**: Mocks stay in sync with interface changes automatically + +## Regenerating Mocks + +When interfaces change, regenerate mocks with: + +```bash +mockery +``` + +This will update all configured mocks according to the `.mockery.yml` configuration. + +## Dependencies + +The project now includes the testify mock dependency: + +```go +github.com/stretchr/testify/mock v1.11.1 +``` + +This enables the full mockery feature set including expectations, matchers, and automatic verification. \ No newline at end of file diff --git a/TESTING_GUIDE.md b/TESTING_GUIDE.md new file mode 100644 index 0000000..f1388be --- /dev/null +++ b/TESTING_GUIDE.md @@ -0,0 +1,132 @@ +# Testing Guide + +## Better Go Testing Practices + +Instead of using `init()` functions to set up mocks, this codebase now uses dependency injection with interfaces for better testability. + +### The Old Way (Problematic) + +```go +// DON'T DO THIS - Global state mutation in init() +var mockExecCommand = func(name string, args ...string) *exec.Cmd { + // mock implementation +} + +func init() { + internal.ExecCommand = mockExecCommand // Mutates global state +} +``` + +**Problems with this approach:** +- Global state mutation affects all tests +- Tests can interfere with each other +- Difficult to reset between tests +- Not explicit about what's being mocked +- Makes parallel testing unsafe + +### The New Way (Recommended) + +```go +// Define interface for testability +type JobRunner interface { + Execute(name string, args ...string) ([]byte, error) +} + +// Real implementation +type RealSync struct{} +func (r *RealSync) Execute(name string, args ...string) ([]byte, error) { + cmd := exec.Command(name, args...) + return cmd.CombinedOutput() +} + +// Test implementation +type MockCommandExecutor struct { + CapturedCommands []MockCommand +} +func (m *MockCommandExecutor) Execute(name string, args ...string) ([]byte, error) { + // Mock logic here +} +``` + +### Testing Examples + +#### 1. Simple Test with Mock + +```go +func TestExecuteJob(t *testing.T) { + // Create mock executor for this test only + mockExecutor := &MockCommandExecutor{} + + job := internal.Job{ + Name: "test_job", + Source: "/home/test/", + Target: "/mnt/backup1/test/", + Enabled: true, + } + + // Use the executor-aware function + status := internal.ExecuteJobWithExecutor(job, true, false, "", mockExecutor) + + if status != "SUCCESS" { + t.Errorf("Expected SUCCESS, got %s", status) + } + + // Verify the mock was called correctly + if len(mockExecutor.CapturedCommands) == 0 { + t.Error("Expected command to be executed") + } +} +``` + +#### 2. Test Setup with t.Cleanup() (Alternative Pattern) + +```go +func setupMockExecutor(t *testing.T) *MockCommandExecutor { + t.Helper() + + // Store original to restore later + originalExecutor := internal.DefaultExecutor + + // Create mock + mockExecutor := &MockCommandExecutor{} + + // Set mock globally + internal.DefaultExecutor = mockExecutor + + // Restore original after test + t.Cleanup(func() { + internal.DefaultExecutor = originalExecutor + }) + + return mockExecutor +} + +func TestWithSetup(t *testing.T) { + mock := setupMockExecutor(t) + + // Use regular ExecuteJob function - it uses DefaultExecutor + status := internal.ExecuteJob(job, true, false, "") + + // Verify mock was used + assert.Equal(t, 1, len(mock.CapturedCommands)) +} +``` + +### Benefits of the New Approach + +1. **Test Isolation**: Each test gets its own mock instance +2. **Explicit Dependencies**: Clear what's being mocked in each test +3. **Better Assertions**: Can inspect captured calls, arguments, etc. +4. **Parallel Safe**: Tests don't interfere with each other +5. **Easier Debugging**: Clearer test failures and state +6. **Production Safety**: Real code unchanged, only test behavior modified + +### Key Principles + +1. **Use interfaces for external dependencies** (file system, network, exec, etc.) +2. **Inject dependencies rather than using globals** +3. **Keep mocks scoped to individual tests** +4. **Use `t.Cleanup()` for setup/teardown patterns** +5. **Make test intentions explicit in the test code** + +This approach follows Go testing best practices and makes the codebase more maintainable and reliable. \ No newline at end of file From 562f5d5458c3e2479abbe79c391cb54c3d7cc781 Mon Sep 17 00:00:00 2001 From: Jaap de Haan <261428+jdehaan@users.noreply.github.com> Date: Fri, 20 Mar 2026 08:16:06 +0000 Subject: [PATCH 02/25] fix: Eliminate `log.Fatalf` - return Errors Instead --- backup/cmd/check-coverage.go | 11 ++- backup/cmd/config.go | 22 +++++- backup/cmd/list.go | 12 ++- backup/cmd/run.go | 17 ++++- backup/cmd/simulate.go | 17 ++++- backup/cmd/version.go | 8 +- backup/internal/config.go | 19 +++-- backup/internal/helper.go | 9 ++- backup/internal/test/config_test.go | 110 ++++++++++++++++++++++++++++ backup/internal/test/helper_test.go | 16 ++-- 10 files changed, 204 insertions(+), 37 deletions(-) diff --git a/backup/cmd/check-coverage.go b/backup/cmd/check-coverage.go index b7f7738..4993bec 100644 --- a/backup/cmd/check-coverage.go +++ b/backup/cmd/check-coverage.go @@ -15,9 +15,14 @@ func buildCheckCoverageCommand() *cobra.Command { return &cobra.Command{ Use: "check-coverage", Short: "Check path coverage", - Run: func(cmd *cobra.Command, args []string) { + RunE: func(cmd *cobra.Command, args []string) error { configPath, _ := cmd.Flags().GetString("config") - cfg := internal.LoadResolvedConfig(configPath) + + cfg, err := internal.LoadResolvedConfig(configPath) + if err != nil { + return fmt.Errorf("loading config: %w", err) + } + uncoveredPaths := internal.ListUncoveredPaths(fs, cfg) fmt.Println("Uncovered paths:") @@ -25,6 +30,8 @@ func buildCheckCoverageCommand() *cobra.Command { for _, path := range uncoveredPaths { fmt.Println(path) } + + return nil }, } } diff --git a/backup/cmd/config.go b/backup/cmd/config.go index c3ecdd3..580b5bc 100644 --- a/backup/cmd/config.go +++ b/backup/cmd/config.go @@ -16,20 +16,34 @@ func buildConfigCommand() *cobra.Command { var showVerb = &cobra.Command{ Use: "show", Short: "Show resolved configuration", - Run: func(cmd *cobra.Command, args []string) { + RunE: func(cmd *cobra.Command, args []string) error { configPath, _ := cmd.Flags().GetString("config") - cfg := internal.LoadResolvedConfig(configPath) + + cfg, err := internal.LoadResolvedConfig(configPath) + if err != nil { + return fmt.Errorf("loading config: %w", err) + } + fmt.Printf("Resolved Configuration:\n%s\n", cfg) + + return nil }, } var validateVerb = &cobra.Command{ Use: "validate", Short: "Validate configuration", - Run: func(cmd *cobra.Command, args []string) { + RunE: func(cmd *cobra.Command, args []string) error { configPath, _ := cmd.Flags().GetString("config") - internal.LoadResolvedConfig(configPath) + + _, err := internal.LoadResolvedConfig(configPath) + if err != nil { + return fmt.Errorf("validating config: %w", err) + } + fmt.Println("Configuration is valid.") + + return nil }, } diff --git a/backup/cmd/list.go b/backup/cmd/list.go index 2e957a3..71f1e40 100644 --- a/backup/cmd/list.go +++ b/backup/cmd/list.go @@ -2,6 +2,7 @@ package cmd import ( "backup-rsync/backup/internal" + "fmt" "io" "log" @@ -12,14 +13,21 @@ func buildListCommand() *cobra.Command { return &cobra.Command{ Use: "list", Short: "List the commands that will be executed", - Run: func(cmd *cobra.Command, args []string) { + RunE: func(cmd *cobra.Command, args []string) error { configPath, _ := cmd.Flags().GetString("config") rsyncPath, _ := cmd.Flags().GetString("rsync-path") - cfg := internal.LoadResolvedConfig(configPath) + + cfg, err := internal.LoadResolvedConfig(configPath) + if err != nil { + return fmt.Errorf("loading config: %w", err) + } + command := internal.NewListCommand(rsyncPath) logger := log.New(io.Discard, "", 0) cfg.Apply(command, logger) + + return nil }, } } diff --git a/backup/cmd/run.go b/backup/cmd/run.go index cc133ea..ee2fe81 100644 --- a/backup/cmd/run.go +++ b/backup/cmd/run.go @@ -2,6 +2,7 @@ package cmd import ( "backup-rsync/backup/internal" + "fmt" "github.com/spf13/cobra" ) @@ -10,15 +11,25 @@ func buildRunCommand() *cobra.Command { return &cobra.Command{ Use: "run", Short: "Execute the sync jobs", - Run: func(cmd *cobra.Command, args []string) { + RunE: func(cmd *cobra.Command, args []string) error { configPath, _ := cmd.Flags().GetString("config") rsyncPath, _ := cmd.Flags().GetString("rsync-path") - cfg := internal.LoadResolvedConfig(configPath) - logger, logPath := internal.CreateMainLogger(configPath, false) + cfg, err := internal.LoadResolvedConfig(configPath) + if err != nil { + return fmt.Errorf("loading config: %w", err) + } + + logger, logPath, err := internal.CreateMainLogger(configPath, false) + if err != nil { + return fmt.Errorf("creating logger: %w", err) + } + command := internal.NewSyncCommand(rsyncPath, logPath) cfg.Apply(command, logger) + + return nil }, } } diff --git a/backup/cmd/simulate.go b/backup/cmd/simulate.go index eb4ab83..ed82a83 100644 --- a/backup/cmd/simulate.go +++ b/backup/cmd/simulate.go @@ -2,6 +2,7 @@ package cmd import ( "backup-rsync/backup/internal" + "fmt" "github.com/spf13/cobra" ) @@ -10,15 +11,25 @@ func buildSimulateCommand() *cobra.Command { return &cobra.Command{ Use: "simulate", Short: "Simulate the sync jobs", - Run: func(cmd *cobra.Command, args []string) { + RunE: func(cmd *cobra.Command, args []string) error { configPath, _ := cmd.Flags().GetString("config") rsyncPath, _ := cmd.Flags().GetString("rsync-path") - cfg := internal.LoadResolvedConfig(configPath) - logger, logPath := internal.CreateMainLogger(configPath, true) + cfg, err := internal.LoadResolvedConfig(configPath) + if err != nil { + return fmt.Errorf("loading config: %w", err) + } + + logger, logPath, err := internal.CreateMainLogger(configPath, true) + if err != nil { + return fmt.Errorf("creating logger: %w", err) + } + command := internal.NewSimulateCommand(rsyncPath, logPath) cfg.Apply(command, logger) + + return nil }, } } diff --git a/backup/cmd/version.go b/backup/cmd/version.go index e6e4c69..36904e0 100644 --- a/backup/cmd/version.go +++ b/backup/cmd/version.go @@ -12,19 +12,19 @@ func buildVersionCommand() *cobra.Command { return &cobra.Command{ Use: "version", Short: "Prints the rsync version, protocol version, and full path to the rsync binary.", - Run: func(cmd *cobra.Command, args []string) { + RunE: func(cmd *cobra.Command, args []string) error { rsyncPath, _ := cmd.Flags().GetString("rsync-path") rsync := internal.NewSyncCommand(rsyncPath, "") output, _, err := rsync.GetVersionInfo() if err != nil { - fmt.Printf("%v\n", err) - - return + return fmt.Errorf("getting version info: %w", err) } fmt.Printf("Rsync Binary Path: %s\n", rsyncPath) fmt.Printf("Version Info: %s", output) + + return nil }, } } diff --git a/backup/internal/config.go b/backup/internal/config.go index 8c8f1e9..58d386c 100644 --- a/backup/internal/config.go +++ b/backup/internal/config.go @@ -173,40 +173,39 @@ func validateJobPaths(jobs []Job, pathType string, getPath func(job Job) string) return nil } -func LoadResolvedConfig(configPath string) Config { +func LoadResolvedConfig(configPath string) (Config, error) { configFile, err := os.Open(configPath) if err != nil { - log.Fatalf("Failed to open config: %v", err) + return Config{}, fmt.Errorf("failed to open config: %w", err) } + defer configFile.Close() cfg, err := LoadConfig(configFile) - _ = configFile.Close() - if err != nil { - log.Fatalf("Failed to parse YAML: %v", err) + return Config{}, fmt.Errorf("failed to parse YAML: %w", err) } err = ValidateJobNames(cfg.Jobs) if err != nil { - log.Fatalf("Job validation failed: %v", err) + return Config{}, fmt.Errorf("job validation failed: %w", err) } resolvedCfg := ResolveConfig(cfg) err = ValidatePaths(resolvedCfg) if err != nil { - log.Fatalf("Path validation failed: %v", err) + return Config{}, fmt.Errorf("path validation failed: %w", err) } err = validateJobPaths(resolvedCfg.Jobs, "source", func(job Job) string { return job.Source }) if err != nil { - log.Fatalf("Job source path validation failed: %v", err) + return Config{}, fmt.Errorf("job source path validation failed: %w", err) } err = validateJobPaths(resolvedCfg.Jobs, "target", func(job Job) string { return job.Target }) if err != nil { - log.Fatalf("Job target path validation failed: %v", err) + return Config{}, fmt.Errorf("job target path validation failed: %w", err) } - return resolvedCfg + return resolvedCfg, nil } diff --git a/backup/internal/helper.go b/backup/internal/helper.go index ce5d0da..6fb57ce 100644 --- a/backup/internal/helper.go +++ b/backup/internal/helper.go @@ -2,6 +2,7 @@ package internal import ( + "fmt" "log" "os" "path/filepath" @@ -34,21 +35,21 @@ func getLogPath(simulate bool, configPath string) string { return logPath } -func CreateMainLogger(configPath string, simulate bool) (*log.Logger, string) { +func CreateMainLogger(configPath string, simulate bool) (*log.Logger, string, error) { logPath := getLogPath(simulate, configPath) overallLogPath := logPath + "/summary.log" err := os.MkdirAll(logPath, LogDirPermission) if err != nil { - log.Fatalf("Failed to create log directory: %v", err) + return nil, "", fmt.Errorf("failed to create log directory: %w", err) } overallLogFile, err := os.OpenFile(overallLogPath, os.O_CREATE|os.O_WRONLY|os.O_APPEND, LogFilePermission) if err != nil { - log.Fatalf("Failed to open overall log file: %v", err) + return nil, "", fmt.Errorf("failed to open overall log file: %w", err) } logger := log.New(overallLogFile, "", log.LstdFlags) - return logger, logPath + return logger, logPath, nil } diff --git a/backup/internal/test/config_test.go b/backup/internal/test/config_test.go index a3ae015..e6663ec 100644 --- a/backup/internal/test/config_test.go +++ b/backup/internal/test/config_test.go @@ -2,6 +2,8 @@ package internal_test import ( "bytes" + "os" + "path/filepath" "testing" "github.com/stretchr/testify/assert" @@ -360,3 +362,111 @@ func TestResolveConfig(t *testing.T) { assert.Equal(t, "/home/user/Pictures", resolvedCfg.Jobs[1].Source) assert.Equal(t, "/backup/user/Pictures", resolvedCfg.Jobs[1].Target) } + +// writeTestConfig writes YAML content to a temp file and returns its path. +func writeTestConfig(t *testing.T, content string) string { + t.Helper() + + dir := t.TempDir() + path := filepath.Join(dir, "test.yaml") + err := os.WriteFile(path, []byte(content), 0600) + require.NoError(t, err) + + return path +} + +func TestLoadResolvedConfig_FileNotFound(t *testing.T) { + _, err := LoadResolvedConfig("/nonexistent/path/config.yaml") + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to open config") +} + +func TestLoadResolvedConfig_InvalidYAML(t *testing.T) { + path := writeTestConfig(t, "{{invalid yaml") + + _, err := LoadResolvedConfig(path) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to parse YAML") +} + +func TestLoadResolvedConfig_DuplicateJobNames(t *testing.T) { + yaml := ` +sources: + - path: "/src" +targets: + - path: "/tgt" +jobs: + - name: "dup" + source: "/src/a" + target: "/tgt/a" + - name: "dup" + source: "/src/b" + target: "/tgt/b" +` + path := writeTestConfig(t, yaml) + + _, err := LoadResolvedConfig(path) + require.Error(t, err) + assert.Contains(t, err.Error(), "job validation failed") + assert.Contains(t, err.Error(), "duplicate job name: dup") +} + +func TestLoadResolvedConfig_InvalidSourcePath(t *testing.T) { + yaml := ` +sources: + - path: "/home" +targets: + - path: "/backup" +jobs: + - name: "job1" + source: "/invalid/source" + target: "/backup/stuff" +` + path := writeTestConfig(t, yaml) + + _, err := LoadResolvedConfig(path) + require.Error(t, err) + assert.Contains(t, err.Error(), "path validation failed") +} + +func TestLoadResolvedConfig_OverlappingSourcePaths(t *testing.T) { + yaml := ` +sources: + - path: "/home" +targets: + - path: "/backup" +jobs: + - name: "parent" + source: "/home/user" + target: "/backup/user" + - name: "child" + source: "/home/user/docs" + target: "/backup/docs" +` + path := writeTestConfig(t, yaml) + + _, err := LoadResolvedConfig(path) + require.Error(t, err) + assert.Contains(t, err.Error(), "job source path validation failed") +} + +func TestLoadResolvedConfig_ValidConfig(t *testing.T) { + yaml := ` +sources: + - path: "/home" +targets: + - path: "/backup" +variables: + base: "/backup" +jobs: + - name: "docs" + source: "/home/docs" + target: "${base}/docs" +` + path := writeTestConfig(t, yaml) + + cfg, err := LoadResolvedConfig(path) + require.NoError(t, err) + assert.Len(t, cfg.Jobs, 1) + assert.Equal(t, "/backup/docs", cfg.Jobs[0].Target) +} diff --git a/backup/internal/test/helper_test.go b/backup/internal/test/helper_test.go index 906bce4..39cdd0b 100644 --- a/backup/internal/test/helper_test.go +++ b/backup/internal/test/helper_test.go @@ -6,6 +6,7 @@ import ( . "backup-rsync/backup/internal" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestNormalizePath(t *testing.T) { @@ -26,32 +27,37 @@ func TestNormalizePath(t *testing.T) { } func TestCreateMainLogger_Title_IsPresent(t *testing.T) { - logger, logPath := CreateMainLogger("title", true) + logger, logPath, err := CreateMainLogger("title", true) + require.NoError(t, err) assert.Contains(t, logPath, "title") assert.NotNil(t, logger) } func TestCreateMainLogger_IsSimulate_HasSimSuffix(t *testing.T) { - logger, logPath := CreateMainLogger("", true) + logger, logPath, err := CreateMainLogger("", true) + require.NoError(t, err) assert.Contains(t, logPath, "-sim") assert.NotNil(t, logger) } func TestCreateMainLogger_NotSimulate_HasNoSimSuffix(t *testing.T) { - logger, logPath := CreateMainLogger("", false) + logger, logPath, err := CreateMainLogger("", false) + require.NoError(t, err) assert.NotContains(t, logPath, "-sim") assert.NotNil(t, logger) } func TestCreateLogPath_IsSimulate_ContainsTimestamp(t *testing.T) { - _, logPath := CreateMainLogger("", true) + _, logPath, err := CreateMainLogger("", true) + require.NoError(t, err) // Check if the logPath contains a timestamp in the format '2006-01-02T15-04-05' timestampRegex := `\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}` assert.Regexp(t, timestampRegex, logPath) } func TestCreateLogPath_NotSimulate_ContainsTimestamp(t *testing.T) { - _, logPath := CreateMainLogger("", false) + _, logPath, err := CreateMainLogger("", false) + require.NoError(t, err) // Check if the logPath contains a timestamp in the format '2006-01-02T15-04-05' timestampRegex := `\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}` assert.Regexp(t, timestampRegex, logPath) From 201655a4c3adc76eeac54751d378ac4c8907e273 Mon Sep 17 00:00:00 2001 From: Jaap de Haan <261428+jdehaan@users.noreply.github.com> Date: Fri, 20 Mar 2026 08:19:08 +0000 Subject: [PATCH 03/25] refactor: Reduce Duplication in Command Types --- backup/internal/rsync.go | 12 ++++++++++++ backup/internal/rsync_list.go | 13 +++---------- backup/internal/rsync_simulate.go | 12 ++---------- backup/internal/rsync_sync.go | 12 ++---------- 4 files changed, 19 insertions(+), 30 deletions(-) diff --git a/backup/internal/rsync.go b/backup/internal/rsync.go index 03b12a6..06db3f1 100644 --- a/backup/internal/rsync.go +++ b/backup/internal/rsync.go @@ -20,6 +20,18 @@ type SharedCommand struct { Shell Exec } +func NewSharedCommand(binPath string, logPath string, shell Exec) SharedCommand { + return SharedCommand{ + BinPath: binPath, + BaseLogPath: logPath, + Shell: shell, + } +} + +func (c SharedCommand) JobLogPath(job Job) string { + return fmt.Sprintf("%s/job-%s.log", c.BaseLogPath, job.Name) +} + func (c SharedCommand) PrintArgs(job Job, args []string) { fmt.Printf("Job: %s\n", job.Name) fmt.Printf("Command: %s %s\n", c.BinPath, strings.Join(args, " ")) diff --git a/backup/internal/rsync_list.go b/backup/internal/rsync_list.go index b181af0..7e0065b 100644 --- a/backup/internal/rsync_list.go +++ b/backup/internal/rsync_list.go @@ -1,24 +1,17 @@ package internal -import ( - "fmt" -) - type ListCommand struct { SharedCommand } func NewListCommand(binPath string) ListCommand { return ListCommand{ - SharedCommand: SharedCommand{ - BinPath: binPath, - BaseLogPath: "", - Shell: &OsExec{}, - }, + SharedCommand: NewSharedCommand(binPath, "", &OsExec{}), } } + func (c ListCommand) Run(job Job) JobStatus { - logPath := fmt.Sprintf("%s/job-%s.log", c.BaseLogPath, job.Name) + logPath := c.JobLogPath(job) args := ArgumentsForJob(job, logPath, false) c.PrintArgs(job, args) diff --git a/backup/internal/rsync_simulate.go b/backup/internal/rsync_simulate.go index ddcb527..ae4a7b3 100644 --- a/backup/internal/rsync_simulate.go +++ b/backup/internal/rsync_simulate.go @@ -1,25 +1,17 @@ package internal -import ( - "fmt" -) - type SimulateCommand struct { SharedCommand } func NewSimulateCommand(binPath string, logPath string) SimulateCommand { return SimulateCommand{ - SharedCommand: SharedCommand{ - BinPath: binPath, - BaseLogPath: logPath, - Shell: &OsExec{}, - }, + SharedCommand: NewSharedCommand(binPath, logPath, &OsExec{}), } } func (c SimulateCommand) Run(job Job) JobStatus { - logPath := fmt.Sprintf("%s/job-%s.log", c.BaseLogPath, job.Name) + logPath := c.JobLogPath(job) // Don't use --log-file in simulate mode as rsync doesn't log file changes to it in dry-run args := ArgumentsForJob(job, "", true) diff --git a/backup/internal/rsync_sync.go b/backup/internal/rsync_sync.go index ea22044..7ce6a16 100644 --- a/backup/internal/rsync_sync.go +++ b/backup/internal/rsync_sync.go @@ -1,25 +1,17 @@ package internal -import ( - "fmt" -) - type SyncCommand struct { SharedCommand } func NewSyncCommand(binPath string, logPath string) SyncCommand { return SyncCommand{ - SharedCommand: SharedCommand{ - BinPath: binPath, - BaseLogPath: logPath, - Shell: &OsExec{}, - }, + SharedCommand: NewSharedCommand(binPath, logPath, &OsExec{}), } } func (c SyncCommand) Run(job Job) JobStatus { - logPath := fmt.Sprintf("%s/job-%s.log", c.BaseLogPath, job.Name) + logPath := c.JobLogPath(job) args := ArgumentsForJob(job, logPath, false) return c.RunWithArgs(job, args) From 320138d5167a7f092ec380afb89935284142e8c3 Mon Sep 17 00:00:00 2001 From: Jaap de Haan <261428+jdehaan@users.noreply.github.com> Date: Fri, 20 Mar 2026 08:21:31 +0000 Subject: [PATCH 04/25] fix: Inject Exec via constructors (Testability) --- backup/cmd/list.go | 2 +- backup/cmd/run.go | 2 +- backup/cmd/simulate.go | 2 +- backup/cmd/version.go | 2 +- backup/internal/rsync_list.go | 4 ++-- backup/internal/rsync_simulate.go | 4 ++-- backup/internal/rsync_sync.go | 4 ++-- 7 files changed, 10 insertions(+), 10 deletions(-) diff --git a/backup/cmd/list.go b/backup/cmd/list.go index 71f1e40..7cd4ba9 100644 --- a/backup/cmd/list.go +++ b/backup/cmd/list.go @@ -22,7 +22,7 @@ func buildListCommand() *cobra.Command { return fmt.Errorf("loading config: %w", err) } - command := internal.NewListCommand(rsyncPath) + command := internal.NewListCommand(rsyncPath, &internal.OsExec{}) logger := log.New(io.Discard, "", 0) cfg.Apply(command, logger) diff --git a/backup/cmd/run.go b/backup/cmd/run.go index ee2fe81..8694790 100644 --- a/backup/cmd/run.go +++ b/backup/cmd/run.go @@ -25,7 +25,7 @@ func buildRunCommand() *cobra.Command { return fmt.Errorf("creating logger: %w", err) } - command := internal.NewSyncCommand(rsyncPath, logPath) + command := internal.NewSyncCommand(rsyncPath, logPath, &internal.OsExec{}) cfg.Apply(command, logger) diff --git a/backup/cmd/simulate.go b/backup/cmd/simulate.go index ed82a83..25b4bb1 100644 --- a/backup/cmd/simulate.go +++ b/backup/cmd/simulate.go @@ -25,7 +25,7 @@ func buildSimulateCommand() *cobra.Command { return fmt.Errorf("creating logger: %w", err) } - command := internal.NewSimulateCommand(rsyncPath, logPath) + command := internal.NewSimulateCommand(rsyncPath, logPath, &internal.OsExec{}) cfg.Apply(command, logger) diff --git a/backup/cmd/version.go b/backup/cmd/version.go index 36904e0..dbceb41 100644 --- a/backup/cmd/version.go +++ b/backup/cmd/version.go @@ -14,7 +14,7 @@ func buildVersionCommand() *cobra.Command { Short: "Prints the rsync version, protocol version, and full path to the rsync binary.", RunE: func(cmd *cobra.Command, args []string) error { rsyncPath, _ := cmd.Flags().GetString("rsync-path") - rsync := internal.NewSyncCommand(rsyncPath, "") + rsync := internal.NewSyncCommand(rsyncPath, "", &internal.OsExec{}) output, _, err := rsync.GetVersionInfo() if err != nil { diff --git a/backup/internal/rsync_list.go b/backup/internal/rsync_list.go index 7e0065b..72caaaa 100644 --- a/backup/internal/rsync_list.go +++ b/backup/internal/rsync_list.go @@ -4,9 +4,9 @@ type ListCommand struct { SharedCommand } -func NewListCommand(binPath string) ListCommand { +func NewListCommand(binPath string, shell Exec) ListCommand { return ListCommand{ - SharedCommand: NewSharedCommand(binPath, "", &OsExec{}), + SharedCommand: NewSharedCommand(binPath, "", shell), } } diff --git a/backup/internal/rsync_simulate.go b/backup/internal/rsync_simulate.go index ae4a7b3..5bc43df 100644 --- a/backup/internal/rsync_simulate.go +++ b/backup/internal/rsync_simulate.go @@ -4,9 +4,9 @@ type SimulateCommand struct { SharedCommand } -func NewSimulateCommand(binPath string, logPath string) SimulateCommand { +func NewSimulateCommand(binPath string, logPath string, shell Exec) SimulateCommand { return SimulateCommand{ - SharedCommand: NewSharedCommand(binPath, logPath, &OsExec{}), + SharedCommand: NewSharedCommand(binPath, logPath, shell), } } diff --git a/backup/internal/rsync_sync.go b/backup/internal/rsync_sync.go index 7ce6a16..562414c 100644 --- a/backup/internal/rsync_sync.go +++ b/backup/internal/rsync_sync.go @@ -4,9 +4,9 @@ type SyncCommand struct { SharedCommand } -func NewSyncCommand(binPath string, logPath string) SyncCommand { +func NewSyncCommand(binPath string, logPath string, shell Exec) SyncCommand { return SyncCommand{ - SharedCommand: NewSharedCommand(binPath, logPath, &OsExec{}), + SharedCommand: NewSharedCommand(binPath, logPath, shell), } } From 52b19e4650c8516ea49342a751e760abdb2ba139 Mon Sep 17 00:00:00 2001 From: Jaap de Haan <261428+jdehaan@users.noreply.github.com> Date: Fri, 20 Mar 2026 08:25:23 +0000 Subject: [PATCH 05/25] fix: Add tests for Run commands --- backup/internal/test/rsync_test.go | 121 +++++++++++++++++++++++++++++ 1 file changed, 121 insertions(+) diff --git a/backup/internal/test/rsync_test.go b/backup/internal/test/rsync_test.go index 249864d..f7874bb 100644 --- a/backup/internal/test/rsync_test.go +++ b/backup/internal/test/rsync_test.go @@ -142,3 +142,124 @@ func TestGetVersionInfo_IncompletePath(t *testing.T) { assert.Empty(t, versionInfo) assert.Empty(t, fullpath) } + +func newTestJob() Job { + return Job{ + Name: "test-job", + Source: "/home/user/docs/", + Target: "/backup/user/docs/", + Delete: true, + Enabled: true, + Exclusions: []string{"*.tmp"}, + } +} + +func TestNewSharedCommand(t *testing.T) { + mockExec := NewMockExec(t) + cmd := NewSharedCommand(rsyncPath, "/logs/base", mockExec) + + assert.Equal(t, rsyncPath, cmd.BinPath) + assert.Equal(t, "/logs/base", cmd.BaseLogPath) + assert.Equal(t, mockExec, cmd.Shell) +} + +func TestJobLogPath(t *testing.T) { + cmd := NewSharedCommand(rsyncPath, "/logs/sync-2025", nil) + job := newTestJob() + + logPath := cmd.JobLogPath(job) + + assert.Equal(t, "/logs/sync-2025/job-test-job.log", logPath) +} + +func TestNewListCommand(t *testing.T) { + mockExec := NewMockExec(t) + cmd := NewListCommand(rsyncPath, mockExec) + + assert.Equal(t, rsyncPath, cmd.BinPath) + assert.Empty(t, cmd.BaseLogPath) + assert.Equal(t, mockExec, cmd.Shell) +} + +func TestListCommand_Run_ReturnsSuccess(t *testing.T) { + mockExec := NewMockExec(t) + cmd := NewListCommand(rsyncPath, mockExec) + job := newTestJob() + + // ListCommand.Run only prints args, doesn't call Shell.Execute + status := cmd.Run(job) + + assert.Equal(t, Success, status) +} + +func TestNewSyncCommand(t *testing.T) { + mockExec := NewMockExec(t) + cmd := NewSyncCommand(rsyncPath, "/logs/base", mockExec) + + assert.Equal(t, rsyncPath, cmd.BinPath) + assert.Equal(t, "/logs/base", cmd.BaseLogPath) + assert.Equal(t, mockExec, cmd.Shell) +} + +func TestSyncCommand_Run_Success(t *testing.T) { + mockExec := NewMockExec(t) + cmd := NewSyncCommand(rsyncPath, "/logs/base", mockExec) + job := newTestJob() + + mockExec.EXPECT().Execute(rsyncPath, mock.AnythingOfType("[]string")). + Return([]byte("sync output"), nil).Once() + + status := cmd.Run(job) + + assert.Equal(t, Success, status) +} + +func TestSyncCommand_Run_Failure(t *testing.T) { + mockExec := NewMockExec(t) + cmd := NewSyncCommand(rsyncPath, "/logs/base", mockExec) + job := newTestJob() + + mockExec.EXPECT().Execute(rsyncPath, mock.AnythingOfType("[]string")). + Return(nil, errCommandNotFound).Once() + + status := cmd.Run(job) + + assert.Equal(t, Failure, status) +} + +func TestNewSimulateCommand(t *testing.T) { + mockExec := NewMockExec(t) + cmd := NewSimulateCommand(rsyncPath, "/logs/base", mockExec) + + assert.Equal(t, rsyncPath, cmd.BinPath) + assert.Equal(t, "/logs/base", cmd.BaseLogPath) + assert.Equal(t, mockExec, cmd.Shell) +} + +func TestSimulateCommand_Run_Success(t *testing.T) { + mockExec := NewMockExec(t) + logDir := t.TempDir() + cmd := NewSimulateCommand(rsyncPath, logDir, mockExec) + job := newTestJob() + + mockExec.EXPECT().Execute(rsyncPath, mock.AnythingOfType("[]string")). + Return([]byte("simulated output"), nil).Once() + + status := cmd.Run(job) + + assert.Equal(t, Success, status) +} + +func TestSimulateCommand_Run_Failure(t *testing.T) { + mockExec := NewMockExec(t) + logDir := t.TempDir() + cmd := NewSimulateCommand(rsyncPath, logDir, mockExec) + job := newTestJob() + + mockExec.EXPECT().Execute(rsyncPath, mock.AnythingOfType("[]string")). + Return(nil, errCommandNotFound).Once() + + status := cmd.Run(job) + + assert.Equal(t, Failure, status) +} From 37208054d0099a9b70a2d756943f097db1e8000b Mon Sep 17 00:00:00 2001 From: Jaap de Haan <261428+jdehaan@users.noreply.github.com> Date: Fri, 20 Mar 2026 08:31:00 +0000 Subject: [PATCH 06/25] refactor: Replace `fmt.Printf` with structured output --- backup/cmd/list.go | 5 ++-- backup/cmd/run.go | 5 ++-- backup/cmd/simulate.go | 5 ++-- backup/cmd/version.go | 3 ++- backup/internal/config.go | 4 +-- backup/internal/rsync.go | 15 ++++++----- backup/internal/rsync_list.go | 6 +++-- backup/internal/rsync_simulate.go | 6 +++-- backup/internal/rsync_sync.go | 6 +++-- backup/internal/test/rsync_test.go | 43 ++++++++++++++++++++++-------- 10 files changed, 66 insertions(+), 32 deletions(-) diff --git a/backup/cmd/list.go b/backup/cmd/list.go index 7cd4ba9..cc5f7f1 100644 --- a/backup/cmd/list.go +++ b/backup/cmd/list.go @@ -5,6 +5,7 @@ import ( "fmt" "io" "log" + "os" "github.com/spf13/cobra" ) @@ -22,10 +23,10 @@ func buildListCommand() *cobra.Command { return fmt.Errorf("loading config: %w", err) } - command := internal.NewListCommand(rsyncPath, &internal.OsExec{}) + command := internal.NewListCommand(rsyncPath, &internal.OsExec{}, os.Stdout) logger := log.New(io.Discard, "", 0) - cfg.Apply(command, logger) + cfg.Apply(command, logger, os.Stdout) return nil }, diff --git a/backup/cmd/run.go b/backup/cmd/run.go index 8694790..0f52834 100644 --- a/backup/cmd/run.go +++ b/backup/cmd/run.go @@ -3,6 +3,7 @@ package cmd import ( "backup-rsync/backup/internal" "fmt" + "os" "github.com/spf13/cobra" ) @@ -25,9 +26,9 @@ func buildRunCommand() *cobra.Command { return fmt.Errorf("creating logger: %w", err) } - command := internal.NewSyncCommand(rsyncPath, logPath, &internal.OsExec{}) + command := internal.NewSyncCommand(rsyncPath, logPath, &internal.OsExec{}, os.Stdout) - cfg.Apply(command, logger) + cfg.Apply(command, logger, os.Stdout) return nil }, diff --git a/backup/cmd/simulate.go b/backup/cmd/simulate.go index 25b4bb1..530fdb9 100644 --- a/backup/cmd/simulate.go +++ b/backup/cmd/simulate.go @@ -3,6 +3,7 @@ package cmd import ( "backup-rsync/backup/internal" "fmt" + "os" "github.com/spf13/cobra" ) @@ -25,9 +26,9 @@ func buildSimulateCommand() *cobra.Command { return fmt.Errorf("creating logger: %w", err) } - command := internal.NewSimulateCommand(rsyncPath, logPath, &internal.OsExec{}) + command := internal.NewSimulateCommand(rsyncPath, logPath, &internal.OsExec{}, os.Stdout) - cfg.Apply(command, logger) + cfg.Apply(command, logger, os.Stdout) return nil }, diff --git a/backup/cmd/version.go b/backup/cmd/version.go index dbceb41..e5aa12e 100644 --- a/backup/cmd/version.go +++ b/backup/cmd/version.go @@ -2,6 +2,7 @@ package cmd import ( "fmt" + "os" "backup-rsync/backup/internal" @@ -14,7 +15,7 @@ func buildVersionCommand() *cobra.Command { Short: "Prints the rsync version, protocol version, and full path to the rsync binary.", RunE: func(cmd *cobra.Command, args []string) error { rsyncPath, _ := cmd.Flags().GetString("rsync-path") - rsync := internal.NewSyncCommand(rsyncPath, "", &internal.OsExec{}) + rsync := internal.NewSyncCommand(rsyncPath, "", &internal.OsExec{}, os.Stdout) output, _, err := rsync.GetVersionInfo() if err != nil { diff --git a/backup/internal/config.go b/backup/internal/config.go index 58d386c..14f4fbf 100644 --- a/backup/internal/config.go +++ b/backup/internal/config.go @@ -34,7 +34,7 @@ func (cfg Config) String() string { return string(out) } -func (cfg Config) Apply(rsync JobCommand, logger *log.Logger) { +func (cfg Config) Apply(rsync JobCommand, logger *log.Logger, output io.Writer) { versionInfo, fullpath, err := rsync.GetVersionInfo() if err != nil { logger.Printf("Failed to fetch rsync version: %v", err) @@ -46,7 +46,7 @@ func (cfg Config) Apply(rsync JobCommand, logger *log.Logger) { for _, job := range cfg.Jobs { status := job.Apply(rsync) logger.Printf("STATUS [%s]: %s", job.Name, status) - fmt.Printf("Status [%s]: %s\n", job.Name, status) + fmt.Fprintf(output, "Status [%s]: %s\n", job.Name, status) } } diff --git a/backup/internal/rsync.go b/backup/internal/rsync.go index 06db3f1..39298a2 100644 --- a/backup/internal/rsync.go +++ b/backup/internal/rsync.go @@ -3,6 +3,7 @@ package internal import ( "errors" "fmt" + "io" "os" "path/filepath" "strings" @@ -17,14 +18,16 @@ type SharedCommand struct { BinPath string BaseLogPath string - Shell Exec + Shell Exec + Output io.Writer } -func NewSharedCommand(binPath string, logPath string, shell Exec) SharedCommand { +func NewSharedCommand(binPath string, logPath string, shell Exec, output io.Writer) SharedCommand { return SharedCommand{ BinPath: binPath, BaseLogPath: logPath, Shell: shell, + Output: output, } } @@ -33,15 +36,15 @@ func (c SharedCommand) JobLogPath(job Job) string { } func (c SharedCommand) PrintArgs(job Job, args []string) { - fmt.Printf("Job: %s\n", job.Name) - fmt.Printf("Command: %s %s\n", c.BinPath, strings.Join(args, " ")) + fmt.Fprintf(c.Output, "Job: %s\n", job.Name) + fmt.Fprintf(c.Output, "Command: %s %s\n", c.BinPath, strings.Join(args, " ")) } func (c SharedCommand) RunWithArgs(job Job, args []string) JobStatus { c.PrintArgs(job, args) out, err := c.Shell.Execute(c.BinPath, args...) - fmt.Printf("Output:\n%s\n", string(out)) + fmt.Fprintf(c.Output, "Output:\n%s\n", string(out)) if err != nil { return Failure @@ -59,7 +62,7 @@ func (c SharedCommand) RunWithArgsAndCaptureOutput(job Job, args []string, logPa if logPath != "" { writeErr := os.WriteFile(logPath, out, LogFilePermission) if writeErr != nil { - fmt.Printf("Warning: Failed to write output to log file %s: %v\n", logPath, writeErr) + fmt.Fprintf(c.Output, "Warning: Failed to write output to log file %s: %v\n", logPath, writeErr) } } diff --git a/backup/internal/rsync_list.go b/backup/internal/rsync_list.go index 72caaaa..018e2f8 100644 --- a/backup/internal/rsync_list.go +++ b/backup/internal/rsync_list.go @@ -1,12 +1,14 @@ package internal +import "io" + type ListCommand struct { SharedCommand } -func NewListCommand(binPath string, shell Exec) ListCommand { +func NewListCommand(binPath string, shell Exec, output io.Writer) ListCommand { return ListCommand{ - SharedCommand: NewSharedCommand(binPath, "", shell), + SharedCommand: NewSharedCommand(binPath, "", shell, output), } } diff --git a/backup/internal/rsync_simulate.go b/backup/internal/rsync_simulate.go index 5bc43df..1c5a8be 100644 --- a/backup/internal/rsync_simulate.go +++ b/backup/internal/rsync_simulate.go @@ -1,12 +1,14 @@ package internal +import "io" + type SimulateCommand struct { SharedCommand } -func NewSimulateCommand(binPath string, logPath string, shell Exec) SimulateCommand { +func NewSimulateCommand(binPath string, logPath string, shell Exec, output io.Writer) SimulateCommand { return SimulateCommand{ - SharedCommand: NewSharedCommand(binPath, logPath, shell), + SharedCommand: NewSharedCommand(binPath, logPath, shell, output), } } diff --git a/backup/internal/rsync_sync.go b/backup/internal/rsync_sync.go index 562414c..2f32b7a 100644 --- a/backup/internal/rsync_sync.go +++ b/backup/internal/rsync_sync.go @@ -1,12 +1,14 @@ package internal +import "io" + type SyncCommand struct { SharedCommand } -func NewSyncCommand(binPath string, logPath string, shell Exec) SyncCommand { +func NewSyncCommand(binPath string, logPath string, shell Exec, output io.Writer) SyncCommand { return SyncCommand{ - SharedCommand: NewSharedCommand(binPath, logPath, shell), + SharedCommand: NewSharedCommand(binPath, logPath, shell, output), } } diff --git a/backup/internal/test/rsync_test.go b/backup/internal/test/rsync_test.go index f7874bb..37c8585 100644 --- a/backup/internal/test/rsync_test.go +++ b/backup/internal/test/rsync_test.go @@ -2,7 +2,9 @@ package internal_test import ( . "backup-rsync/backup/internal" + "bytes" "errors" + "io" "strings" "testing" @@ -57,6 +59,7 @@ func TestGetVersionInfo_Success(t *testing.T) { rsync := SharedCommand{ BinPath: rsyncPath, Shell: mockExec, + Output: io.Discard, } // Set expectation for Execute call @@ -76,6 +79,7 @@ func TestGetVersionInfo_CommandError(t *testing.T) { rsync := SharedCommand{ BinPath: rsyncPath, Shell: mockExec, + Output: io.Discard, } // Set expectation for Execute call to return error @@ -95,6 +99,7 @@ func TestGetVersionInfo_InvalidOutput(t *testing.T) { rsync := SharedCommand{ BinPath: rsyncPath, Shell: mockExec, + Output: io.Discard, } // Set expectation for Execute call to return invalid output @@ -114,6 +119,7 @@ func TestGetVersionInfo_EmptyPath(t *testing.T) { rsync := SharedCommand{ BinPath: "", Shell: mockExec, + Output: io.Discard, } // No expectations set - should fail before calling Execute due to path validation @@ -131,6 +137,7 @@ func TestGetVersionInfo_IncompletePath(t *testing.T) { rsync := SharedCommand{ BinPath: "bin/rsync", Shell: mockExec, + Output: io.Discard, } // No expectations set - should fail before calling Execute due to path validation @@ -156,15 +163,16 @@ func newTestJob() Job { func TestNewSharedCommand(t *testing.T) { mockExec := NewMockExec(t) - cmd := NewSharedCommand(rsyncPath, "/logs/base", mockExec) + cmd := NewSharedCommand(rsyncPath, "/logs/base", mockExec, io.Discard) assert.Equal(t, rsyncPath, cmd.BinPath) assert.Equal(t, "/logs/base", cmd.BaseLogPath) assert.Equal(t, mockExec, cmd.Shell) + assert.Equal(t, io.Discard, cmd.Output) } func TestJobLogPath(t *testing.T) { - cmd := NewSharedCommand(rsyncPath, "/logs/sync-2025", nil) + cmd := NewSharedCommand(rsyncPath, "/logs/sync-2025", nil, io.Discard) job := newTestJob() logPath := cmd.JobLogPath(job) @@ -174,7 +182,7 @@ func TestJobLogPath(t *testing.T) { func TestNewListCommand(t *testing.T) { mockExec := NewMockExec(t) - cmd := NewListCommand(rsyncPath, mockExec) + cmd := NewListCommand(rsyncPath, mockExec, io.Discard) assert.Equal(t, rsyncPath, cmd.BinPath) assert.Empty(t, cmd.BaseLogPath) @@ -183,18 +191,22 @@ func TestNewListCommand(t *testing.T) { func TestListCommand_Run_ReturnsSuccess(t *testing.T) { mockExec := NewMockExec(t) - cmd := NewListCommand(rsyncPath, mockExec) + + var buf bytes.Buffer + + cmd := NewListCommand(rsyncPath, mockExec, &buf) job := newTestJob() - // ListCommand.Run only prints args, doesn't call Shell.Execute status := cmd.Run(job) assert.Equal(t, Success, status) + assert.Contains(t, buf.String(), "Job: test-job") + assert.Contains(t, buf.String(), rsyncPath) } func TestNewSyncCommand(t *testing.T) { mockExec := NewMockExec(t) - cmd := NewSyncCommand(rsyncPath, "/logs/base", mockExec) + cmd := NewSyncCommand(rsyncPath, "/logs/base", mockExec, io.Discard) assert.Equal(t, rsyncPath, cmd.BinPath) assert.Equal(t, "/logs/base", cmd.BaseLogPath) @@ -203,7 +215,10 @@ func TestNewSyncCommand(t *testing.T) { func TestSyncCommand_Run_Success(t *testing.T) { mockExec := NewMockExec(t) - cmd := NewSyncCommand(rsyncPath, "/logs/base", mockExec) + + var buf bytes.Buffer + + cmd := NewSyncCommand(rsyncPath, "/logs/base", mockExec, &buf) job := newTestJob() mockExec.EXPECT().Execute(rsyncPath, mock.AnythingOfType("[]string")). @@ -212,11 +227,13 @@ func TestSyncCommand_Run_Success(t *testing.T) { status := cmd.Run(job) assert.Equal(t, Success, status) + assert.Contains(t, buf.String(), "Job: test-job") + assert.Contains(t, buf.String(), "Output:\nsync output") } func TestSyncCommand_Run_Failure(t *testing.T) { mockExec := NewMockExec(t) - cmd := NewSyncCommand(rsyncPath, "/logs/base", mockExec) + cmd := NewSyncCommand(rsyncPath, "/logs/base", mockExec, io.Discard) job := newTestJob() mockExec.EXPECT().Execute(rsyncPath, mock.AnythingOfType("[]string")). @@ -229,7 +246,7 @@ func TestSyncCommand_Run_Failure(t *testing.T) { func TestNewSimulateCommand(t *testing.T) { mockExec := NewMockExec(t) - cmd := NewSimulateCommand(rsyncPath, "/logs/base", mockExec) + cmd := NewSimulateCommand(rsyncPath, "/logs/base", mockExec, io.Discard) assert.Equal(t, rsyncPath, cmd.BinPath) assert.Equal(t, "/logs/base", cmd.BaseLogPath) @@ -239,7 +256,10 @@ func TestNewSimulateCommand(t *testing.T) { func TestSimulateCommand_Run_Success(t *testing.T) { mockExec := NewMockExec(t) logDir := t.TempDir() - cmd := NewSimulateCommand(rsyncPath, logDir, mockExec) + + var buf bytes.Buffer + + cmd := NewSimulateCommand(rsyncPath, logDir, mockExec, &buf) job := newTestJob() mockExec.EXPECT().Execute(rsyncPath, mock.AnythingOfType("[]string")). @@ -248,12 +268,13 @@ func TestSimulateCommand_Run_Success(t *testing.T) { status := cmd.Run(job) assert.Equal(t, Success, status) + assert.Contains(t, buf.String(), "Job: test-job") } func TestSimulateCommand_Run_Failure(t *testing.T) { mockExec := NewMockExec(t) logDir := t.TempDir() - cmd := NewSimulateCommand(rsyncPath, logDir, mockExec) + cmd := NewSimulateCommand(rsyncPath, logDir, mockExec, io.Discard) job := newTestJob() mockExec.EXPECT().Execute(rsyncPath, mock.AnythingOfType("[]string")). From b62c3ccaa578f641c89aa8c0435fd4a77e7d2c12 Mon Sep 17 00:00:00 2001 From: Jaap de Haan <261428+jdehaan@users.noreply.github.com> Date: Fri, 20 Mar 2026 08:37:33 +0000 Subject: [PATCH 07/25] fix: Add tests using mocks for capturing output --- backup/internal/test/config_test.go | 58 +++++++++++++++++++++++++++++ backup/internal/test/rsync_test.go | 18 +++++++++ 2 files changed, 76 insertions(+) diff --git a/backup/internal/test/config_test.go b/backup/internal/test/config_test.go index e6663ec..e20d4a5 100644 --- a/backup/internal/test/config_test.go +++ b/backup/internal/test/config_test.go @@ -2,11 +2,14 @@ package internal_test import ( "bytes" + "errors" + "log" "os" "path/filepath" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "gopkg.in/yaml.v3" @@ -470,3 +473,58 @@ jobs: assert.Len(t, cfg.Jobs, 1) assert.Equal(t, "/backup/docs", cfg.Jobs[0].Target) } + +func TestConfigApply_VersionInfoSuccess(t *testing.T) { + mockCmd := NewMockJobCommand(t) + + var output bytes.Buffer + + var logBuf bytes.Buffer + + logger := log.New(&logBuf, "", 0) + + cfg := Config{ + Jobs: []Job{ + {Name: "job1", Source: "/src/", Target: "/dst/", Enabled: true}, + {Name: "job2", Source: "/src2/", Target: "/dst2/", Enabled: false}, + }, + } + + mockCmd.EXPECT().GetVersionInfo().Return("rsync version 3.2.3", "/usr/bin/rsync", nil).Once() + mockCmd.EXPECT().Run(mock.AnythingOfType("internal.Job")).Return(Success).Once() + + cfg.Apply(mockCmd, logger, &output) + + assert.Contains(t, logBuf.String(), "Rsync Binary Path: /usr/bin/rsync") + assert.Contains(t, logBuf.String(), "Rsync Version Info: rsync version 3.2.3") + assert.Contains(t, logBuf.String(), "STATUS [job1]: SUCCESS") + assert.Contains(t, logBuf.String(), "STATUS [job2]: SKIPPED") + assert.Contains(t, output.String(), "Status [job1]: SUCCESS") + assert.Contains(t, output.String(), "Status [job2]: SKIPPED") +} + +func TestConfigApply_VersionInfoError(t *testing.T) { + mockCmd := NewMockJobCommand(t) + + var output bytes.Buffer + + var logBuf bytes.Buffer + + logger := log.New(&logBuf, "", 0) + + cfg := Config{ + Jobs: []Job{ + {Name: "backup", Source: "/data/", Target: "/bak/", Enabled: true}, + }, + } + + mockCmd.EXPECT().GetVersionInfo().Return("", "", errors.New("not found")).Once() + mockCmd.EXPECT().Run(mock.AnythingOfType("internal.Job")).Return(Failure).Once() + + cfg.Apply(mockCmd, logger, &output) + + assert.Contains(t, logBuf.String(), "Failed to fetch rsync version: not found") + assert.NotContains(t, logBuf.String(), "Rsync Binary Path") + assert.Contains(t, logBuf.String(), "STATUS [backup]: FAILURE") + assert.Contains(t, output.String(), "Status [backup]: FAILURE") +} diff --git a/backup/internal/test/rsync_test.go b/backup/internal/test/rsync_test.go index 37c8585..90598b9 100644 --- a/backup/internal/test/rsync_test.go +++ b/backup/internal/test/rsync_test.go @@ -284,3 +284,21 @@ func TestSimulateCommand_Run_Failure(t *testing.T) { assert.Equal(t, Failure, status) } + +func TestSimulateCommand_Run_LogWriteError(t *testing.T) { + mockExec := NewMockExec(t) + + var buf bytes.Buffer + + // Use a non-existent directory so WriteFile fails + cmd := NewSimulateCommand(rsyncPath, "/nonexistent/path", mockExec, &buf) + job := newTestJob() + + mockExec.EXPECT().Execute(rsyncPath, mock.AnythingOfType("[]string")). + Return([]byte("output"), nil).Once() + + status := cmd.Run(job) + + assert.Equal(t, Success, status) + assert.Contains(t, buf.String(), "Warning: Failed to write output to log file") +} From 060470966d4dc1614101897cecb0b9517c40370f Mon Sep 17 00:00:00 2001 From: Jaap de Haan <261428+jdehaan@users.noreply.github.com> Date: Fri, 20 Mar 2026 08:45:57 +0000 Subject: [PATCH 08/25] refactor: Introduce `CoverageChecker` and inject logger --- backup/cmd/check-coverage.go | 9 ++- backup/internal/check.go | 103 +++++++++++++++------------- backup/internal/test/check_test.go | 43 ++++++++---- backup/internal/test/config_test.go | 5 +- 4 files changed, 94 insertions(+), 66 deletions(-) diff --git a/backup/cmd/check-coverage.go b/backup/cmd/check-coverage.go index 4993bec..ceaf4f6 100644 --- a/backup/cmd/check-coverage.go +++ b/backup/cmd/check-coverage.go @@ -2,6 +2,8 @@ package cmd import ( "fmt" + "log" + "os" "backup-rsync/backup/internal" @@ -23,7 +25,12 @@ func buildCheckCoverageCommand() *cobra.Command { return fmt.Errorf("loading config: %w", err) } - uncoveredPaths := internal.ListUncoveredPaths(fs, cfg) + checker := &internal.CoverageChecker{ + Logger: log.New(os.Stderr, "", log.LstdFlags), + Fs: fs, + } + + uncoveredPaths := checker.ListUncoveredPaths(cfg) fmt.Println("Uncovered paths:") diff --git a/backup/internal/check.go b/backup/internal/check.go index 362b53c..104599a 100644 --- a/backup/internal/check.go +++ b/backup/internal/check.go @@ -10,23 +10,18 @@ import ( "github.com/spf13/afero" ) -func isExcluded(path string, job Job) bool { - for _, exclusion := range job.Exclusions { - exclusionPath := filepath.Join(job.Source, exclusion) - if strings.HasPrefix(NormalizePath(path), exclusionPath) { - return true - } - } - - return false +// CoverageChecker analyzes path coverage against a configuration. +type CoverageChecker struct { + Logger *log.Logger + Fs afero.Fs } -func IsExcludedGlobally(path string, sources []Path) bool { +func (c *CoverageChecker) IsExcludedGlobally(path string, sources []Path) bool { for _, source := range sources { for _, exclusion := range source.Exclusions { 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) + c.Logger.Printf("EXCLUDED: Path '%s' is globally excluded by '%s' in source '%s'", path, exclusion, source.Path) return true } @@ -36,25 +31,24 @@ func IsExcludedGlobally(path string, sources []Path) bool { 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 - } +func (c *CoverageChecker) ListUncoveredPaths(cfg Config) []string { + var result []string - if isExcluded(path, job) { - log.Printf("EXCLUDED: Path '%s' is excluded by job '%s'", path, job.Name) + seen := make(map[string]bool) - return true + for _, source := range cfg.Sources { + c.checkPath(source.Path, cfg, &result, seen) } - return false + sort.Strings(result) // Ensure consistent ordering for test comparison + + return result } -func isCovered(path string, jobs []Job) bool { - for _, job := range jobs { - if isCoveredByJob(path, job) { +func (c *CoverageChecker) isExcluded(path string, job Job) bool { + for _, exclusion := range job.Exclusions { + exclusionPath := filepath.Join(job.Source, exclusion) + if strings.HasPrefix(NormalizePath(path), exclusionPath) { return true } } @@ -62,23 +56,35 @@ func isCovered(path string, jobs []Job) bool { return false } -func ListUncoveredPaths(fs afero.Fs, cfg Config) []string { - var result []string +func (c *CoverageChecker) isCoveredByJob(path string, job Job) bool { + if NormalizePath(job.Source) == NormalizePath(path) { + c.Logger.Printf("COVERED: Path '%s' is covered by job '%s'", path, job.Name) - seen := make(map[string]bool) + return true + } - for _, source := range cfg.Sources { - checkPath(fs, source.Path, cfg, &result, seen) + if c.isExcluded(path, job) { + c.Logger.Printf("EXCLUDED: Path '%s' is excluded by job '%s'", path, job.Name) + + return true } - sort.Strings(result) // Ensure consistent ordering for test comparison + return false +} - return result +func (c *CoverageChecker) isCovered(path string, jobs []Job) bool { + for _, job := range jobs { + if c.isCoveredByJob(path, job) { + return true + } + } + + return false } -func checkPath(fs afero.Fs, path string, cfg Config, result *[]string, seen map[string]bool) { +func (c *CoverageChecker) checkPath(path string, cfg Config, result *[]string, seen map[string]bool) { if seen[path] { - log.Printf("SKIP: Path '%s' already seen", path) + c.Logger.Printf("SKIP: Path '%s' already seen", path) return } @@ -86,42 +92,43 @@ 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) { - log.Printf("SKIP: Path '%s' is globally excluded", path) + if c.IsExcludedGlobally(path, cfg.Sources) { + c.Logger.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) + if c.isCovered(path, cfg.Jobs) { + c.Logger.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) + if c.isEffectivelyCovered(path, cfg) { + c.Logger.Printf("SKIP: Path '%s' is effectively covered", path) return } // Add uncovered path - log.Printf("ADD: Path '%s' is uncovered", path) + c.Logger.Printf("ADD: Path '%s' is uncovered", path) *result = append(*result, path) } -// 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) +// isEffectivelyCovered checks if a directory is effectively covered +// (all its descendants are covered or excluded). +func (c *CoverageChecker) isEffectivelyCovered(path string, cfg Config) bool { + children, err := getChildDirectories(c.Fs, path) if err != nil { - log.Printf("ERROR: could not get child directories of '%s': %v", path, err) + c.Logger.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) + c.Logger.Printf("NOT COVERED: Path '%s' has no children", path) return false // Leaf directories are not effectively covered unless directly covered } @@ -129,15 +136,15 @@ 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) { - log.Printf("UNCOVERED CHILD: Path '%s' has uncovered child '%s'", path, child) + if !c.IsExcludedGlobally(child, cfg.Sources) && !c.isCovered(child, cfg.Jobs) && !c.isEffectivelyCovered(child, cfg) { + c.Logger.Printf("UNCOVERED CHILD: Path '%s' has uncovered child '%s'", path, child) allCovered = false } } if allCovered { - log.Printf("COVERED: Path '%s' is effectively covered", path) + c.Logger.Printf("COVERED: Path '%s' is effectively covered", path) } return allCovered diff --git a/backup/internal/test/check_test.go b/backup/internal/test/check_test.go index 3c4a139..aae1492 100644 --- a/backup/internal/test/check_test.go +++ b/backup/internal/test/check_test.go @@ -2,6 +2,7 @@ package internal_test import ( "bytes" + "io" "log" "path/filepath" "sort" @@ -13,6 +14,20 @@ import ( "github.com/stretchr/testify/assert" ) +func newTestChecker(fs afero.Fs, logBuf *bytes.Buffer) *CoverageChecker { + return &CoverageChecker{ + Logger: log.New(logBuf, "", 0), + Fs: fs, + } +} + +func newSilentChecker(fs afero.Fs) *CoverageChecker { + return &CoverageChecker{ + Logger: log.New(io.Discard, "", 0), + Fs: fs, + } +} + func TestIsExcludedGlobally_PathGloballyExcluded(t *testing.T) { sources := []Path{ { @@ -26,14 +41,14 @@ func TestIsExcludedGlobally_PathGloballyExcluded(t *testing.T) { } var logBuffer bytes.Buffer - log.SetOutput(&logBuffer) + + checker := newTestChecker(nil, &logBuffer) path := "/home/data/projects/P1" - expectsError := true expectedLog := "Path '/home/data/projects/P1' is globally excluded by '/projects/P1/' in source '/home/data/'" - result := IsExcludedGlobally(path, sources) - assert.Equal(t, expectsError, result) + result := checker.IsExcludedGlobally(path, sources) + assert.True(t, result) assert.Contains(t, logBuffer.String(), expectedLog) } @@ -49,14 +64,12 @@ func TestIsExcludedGlobally_PathNotExcluded(t *testing.T) { }, } - var logBuffer bytes.Buffer - log.SetOutput(&logBuffer) + checker := newSilentChecker(nil) path := "/home/data/projects/Other" - expectsError := false - result := IsExcludedGlobally(path, sources) - assert.Equal(t, expectsError, result) + result := checker.IsExcludedGlobally(path, sources) + assert.False(t, result) } func TestIsExcludedGlobally_PathExcludedInAnotherSource(t *testing.T) { @@ -72,14 +85,14 @@ func TestIsExcludedGlobally_PathExcludedInAnotherSource(t *testing.T) { } var logBuffer bytes.Buffer - log.SetOutput(&logBuffer) + + checker := newTestChecker(nil, &logBuffer) path := "/home/user/cache" - expectsError := true expectedLog := "Path '/home/user/cache' is globally excluded by '/cache/' in source '/home/user/'" - result := IsExcludedGlobally(path, sources) - assert.Equal(t, expectsError, result) + result := checker.IsExcludedGlobally(path, sources) + assert.True(t, result) assert.Contains(t, logBuffer.String(), expectedLog) } @@ -102,8 +115,10 @@ func runListUncoveredPathsTest( } } + checker := newSilentChecker(fs) + // Call the function - uncoveredPaths := ListUncoveredPaths(fs, cfg) + uncoveredPaths := checker.ListUncoveredPaths(cfg) // Assertions sort.Strings(uncoveredPaths) diff --git a/backup/internal/test/config_test.go b/backup/internal/test/config_test.go index e20d4a5..a0f5cd0 100644 --- a/backup/internal/test/config_test.go +++ b/backup/internal/test/config_test.go @@ -2,7 +2,6 @@ package internal_test import ( "bytes" - "errors" "log" "os" "path/filepath" @@ -518,12 +517,12 @@ func TestConfigApply_VersionInfoError(t *testing.T) { }, } - mockCmd.EXPECT().GetVersionInfo().Return("", "", errors.New("not found")).Once() + mockCmd.EXPECT().GetVersionInfo().Return("", "", errCommandNotFound).Once() mockCmd.EXPECT().Run(mock.AnythingOfType("internal.Job")).Return(Failure).Once() cfg.Apply(mockCmd, logger, &output) - assert.Contains(t, logBuf.String(), "Failed to fetch rsync version: not found") + assert.Contains(t, logBuf.String(), "Failed to fetch rsync version: command not found") assert.NotContains(t, logBuf.String(), "Rsync Binary Path") assert.Contains(t, logBuf.String(), "STATUS [backup]: FAILURE") assert.Contains(t, output.String(), "Status [backup]: FAILURE") From b22ce60aa7d1b93884e06542f52c2f8e9653a1d6 Mon Sep 17 00:00:00 2001 From: Jaap de Haan <261428+jdehaan@users.noreply.github.com> Date: Fri, 20 Mar 2026 08:47:35 +0000 Subject: [PATCH 09/25] fix: Add tests for coverage check --- backup/internal/test/check_test.go | 85 +++++++++++++++++++++++------- 1 file changed, 67 insertions(+), 18 deletions(-) diff --git a/backup/internal/test/check_test.go b/backup/internal/test/check_test.go index aae1492..db49cae 100644 --- a/backup/internal/test/check_test.go +++ b/backup/internal/test/check_test.go @@ -213,22 +213,71 @@ func TestListUncoveredPathsVariationsSubfoldersCovered(t *testing.T) { 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{ - // "/home/data": {"family"}, - // "/home/data/family": {"me", "you"}, - // "/home/data/family/me": {"a"}, - // "/home/data/family/you": {"a"}, - // }, - // Config{ - // Sources: []Path{ - // {Path: "/home/data"}, - // }, - // Jobs: []Job{ - // {Name: "JobMe", Source: "/home/data/family/me"}, - // }, - // }, - // []string{"/home/data/family/you"}, - // ) +} + +// Test that a job with exclusions properly marks child paths as excluded. +func TestListUncoveredPaths_JobExclusion(t *testing.T) { + runListUncoveredPathsTest(t, + map[string][]string{ + "/data": {"docs", "cache"}, + "/data/docs": {}, + "/data/cache": {}, + }, + Config{ + Sources: []Path{ + {Path: "/data"}, + }, + Jobs: []Job{ + {Name: "backup", Source: "/data/", Exclusions: []string{"cache"}}, + }, + }, + []string{}, + ) +} + +// Test that duplicate source paths are processed only once. +func TestListUncoveredPaths_DuplicateSourcesSkipped(t *testing.T) { + fs := afero.NewMemMapFs() + _ = fs.MkdirAll("/data", 0755) + + var logBuf bytes.Buffer + + checker := newTestChecker(fs, &logBuf) + + cfg := Config{ + Sources: []Path{ + {Path: "/data"}, + {Path: "/data"}, + }, + Jobs: []Job{ + {Name: "backup", Source: "/data"}, + }, + } + + result := checker.ListUncoveredPaths(cfg) + + assert.Empty(t, result) + assert.Contains(t, logBuf.String(), "SKIP: Path '/data' already seen") +} + +// Test getChildDirectories error path (unreadable directory). +func TestListUncoveredPaths_UnreadableDirectory(t *testing.T) { + fs := afero.NewMemMapFs() + // Don't create /data, so ReadDir will fail + + var logBuf bytes.Buffer + + checker := newTestChecker(fs, &logBuf) + + cfg := Config{ + Sources: []Path{ + {Path: "/data"}, + }, + Jobs: []Job{}, + } + + result := checker.ListUncoveredPaths(cfg) + + assert.Equal(t, []string{"/data"}, result) + assert.Contains(t, logBuf.String(), "ADD: Path '/data' is uncovered") } From 088b13e0d01e8eb7056acad02c4e5f2d6ba11e04 Mon Sep 17 00:00:00 2001 From: Jaap de Haan <261428+jdehaan@users.noreply.github.com> Date: Fri, 20 Mar 2026 08:58:07 +0000 Subject: [PATCH 10/25] fix: Inject filesystem for tests and add command tests --- backup/cmd/check-coverage.go | 9 +- backup/cmd/config.go | 4 +- backup/cmd/root.go | 7 +- backup/cmd/test/commands_test.go | 244 +++++++++++++++++++++++++++++++ backup/cmd/version.go | 5 +- 5 files changed, 259 insertions(+), 10 deletions(-) create mode 100644 backup/cmd/test/commands_test.go diff --git a/backup/cmd/check-coverage.go b/backup/cmd/check-coverage.go index ceaf4f6..2af8113 100644 --- a/backup/cmd/check-coverage.go +++ b/backup/cmd/check-coverage.go @@ -11,9 +11,7 @@ import ( "github.com/spf13/cobra" ) -func buildCheckCoverageCommand() *cobra.Command { - var fs = afero.NewOsFs() - +func buildCheckCoverageCommand(fs afero.Fs) *cobra.Command { return &cobra.Command{ Use: "check-coverage", Short: "Check path coverage", @@ -32,10 +30,11 @@ func buildCheckCoverageCommand() *cobra.Command { uncoveredPaths := checker.ListUncoveredPaths(cfg) - fmt.Println("Uncovered paths:") + out := cmd.OutOrStdout() + fmt.Fprintln(out, "Uncovered paths:") for _, path := range uncoveredPaths { - fmt.Println(path) + fmt.Fprintln(out, path) } return nil diff --git a/backup/cmd/config.go b/backup/cmd/config.go index 580b5bc..95c684f 100644 --- a/backup/cmd/config.go +++ b/backup/cmd/config.go @@ -24,7 +24,7 @@ func buildConfigCommand() *cobra.Command { return fmt.Errorf("loading config: %w", err) } - fmt.Printf("Resolved Configuration:\n%s\n", cfg) + fmt.Fprintf(cmd.OutOrStdout(), "Resolved Configuration:\n%s\n", cfg) return nil }, @@ -41,7 +41,7 @@ func buildConfigCommand() *cobra.Command { return fmt.Errorf("validating config: %w", err) } - fmt.Println("Configuration is valid.") + fmt.Fprintln(cmd.OutOrStdout(), "Configuration is valid.") return nil }, diff --git a/backup/cmd/root.go b/backup/cmd/root.go index 2e2d794..6412a03 100644 --- a/backup/cmd/root.go +++ b/backup/cmd/root.go @@ -1,10 +1,15 @@ package cmd import ( + "github.com/spf13/afero" "github.com/spf13/cobra" ) func BuildRootCommand() *cobra.Command { + return BuildRootCommandWithFs(afero.NewOsFs()) +} + +func BuildRootCommandWithFs(fs afero.Fs) *cobra.Command { rootCmd := &cobra.Command{ Use: "backup", Short: "A tool for managing backups", @@ -19,7 +24,7 @@ func BuildRootCommand() *cobra.Command { buildRunCommand(), buildSimulateCommand(), buildConfigCommand(), - buildCheckCoverageCommand(), + buildCheckCoverageCommand(fs), buildVersionCommand(), ) diff --git a/backup/cmd/test/commands_test.go b/backup/cmd/test/commands_test.go new file mode 100644 index 0000000..1685ea4 --- /dev/null +++ b/backup/cmd/test/commands_test.go @@ -0,0 +1,244 @@ +package cmd_test + +import ( + "bytes" + "os" + "path/filepath" + "testing" + + "backup-rsync/backup/cmd" + + "github.com/spf13/afero" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func writeConfigFile(t *testing.T, content string) string { + t.Helper() + + dir := t.TempDir() + path := filepath.Join(dir, "test.yaml") + + err := os.WriteFile(path, []byte(content), 0600) + require.NoError(t, err) + + return path +} + +func executeCommand(t *testing.T, args ...string) (string, error) { + t.Helper() + + return executeCommandWithFs(t, afero.NewMemMapFs(), args...) +} + +func executeCommandWithFs(t *testing.T, fs afero.Fs, args ...string) (string, error) { + t.Helper() + + rootCmd := cmd.BuildRootCommandWithFs(fs) + + var stdout bytes.Buffer + + rootCmd.SetOut(&stdout) + rootCmd.SetErr(&bytes.Buffer{}) + rootCmd.SetArgs(args) + + err := rootCmd.Execute() + + return stdout.String(), err +} + +// --- config show --- + +func TestConfigShow_ValidConfig(t *testing.T) { + cfgPath := writeConfigFile(t, ` +sources: + - path: "/home" +targets: + - path: "/backup" +jobs: + - name: "docs" + source: "/home/docs/" + target: "/backup/docs/" +`) + + stdout, err := executeCommand(t, "config", "show", "--config", cfgPath) + + require.NoError(t, err) + assert.Contains(t, stdout, "docs") + assert.Contains(t, stdout, "/home/docs") + assert.Contains(t, stdout, "/backup/docs") +} + +func TestConfigShow_MissingFile(t *testing.T) { + _, err := executeCommand(t, "config", "show", "--config", "/nonexistent/config.yaml") + + require.Error(t, err) + assert.Contains(t, err.Error(), "loading config") +} + +func TestConfigShow_InvalidYAML(t *testing.T) { + cfgPath := writeConfigFile(t, `{{{invalid yaml`) + + _, err := executeCommand(t, "config", "show", "--config", cfgPath) + + require.Error(t, err) + assert.Contains(t, err.Error(), "loading config") +} + +// --- config validate --- + +func TestConfigValidate_ValidConfig(t *testing.T) { + cfgPath := writeConfigFile(t, ` +sources: + - path: "/home" +targets: + - path: "/backup" +jobs: + - name: "docs" + source: "/home/docs/" + target: "/backup/docs/" +`) + + stdout, err := executeCommand(t, "config", "validate", "--config", cfgPath) + + require.NoError(t, err) + assert.Contains(t, stdout, "Configuration is valid.") +} + +func TestConfigValidate_MissingFile(t *testing.T) { + _, err := executeCommand(t, "config", "validate", "--config", "/nonexistent/config.yaml") + + require.Error(t, err) + assert.Contains(t, err.Error(), "validating config") +} + +func TestConfigValidate_DuplicateJobNames(t *testing.T) { + cfgPath := writeConfigFile(t, ` +sources: + - path: "/home" +targets: + - path: "/backup" +jobs: + - name: "same" + source: "/home/a/" + target: "/backup/a/" + - name: "same" + source: "/home/b/" + target: "/backup/b/" +`) + + _, err := executeCommand(t, "config", "validate", "--config", cfgPath) + + require.Error(t, err) + assert.Contains(t, err.Error(), "validating config") +} + +// --- list --- + +func TestList_MissingConfig(t *testing.T) { + _, err := executeCommand(t, "list", "--config", "/nonexistent/config.yaml") + + require.Error(t, err) + assert.Contains(t, err.Error(), "loading config") +} + +// --- run --- + +func TestRun_MissingConfig(t *testing.T) { + _, err := executeCommand(t, "run", "--config", "/nonexistent/config.yaml") + + require.Error(t, err) + assert.Contains(t, err.Error(), "loading config") +} + +// --- simulate --- + +func TestSimulate_MissingConfig(t *testing.T) { + _, err := executeCommand(t, "simulate", "--config", "/nonexistent/config.yaml") + + require.Error(t, err) + assert.Contains(t, err.Error(), "loading config") +} + +// --- version --- + +func TestVersion_InvalidRsyncPath(t *testing.T) { + _, err := executeCommand(t, "version", "--rsync-path", "not-absolute") + + require.Error(t, err) + assert.Contains(t, err.Error(), "getting version info") +} + +func TestVersion_NonExistentRsyncPath(t *testing.T) { + _, err := executeCommand(t, "version", "--rsync-path", "/nonexistent/rsync") + + require.Error(t, err) + assert.Contains(t, err.Error(), "getting version info") +} + +// --- check-coverage --- + +func TestCheckCoverage_MissingConfig(t *testing.T) { + _, err := executeCommand(t, "check-coverage", "--config", "/nonexistent/config.yaml") + + require.Error(t, err) + assert.Contains(t, err.Error(), "loading config") +} + +func TestCheckCoverage_ValidConfig(t *testing.T) { + cfgPath := writeConfigFile(t, ` +sources: + - path: "/src" +targets: + - path: "/dst" +jobs: + - name: "docs" + source: "/src/docs/" + target: "/dst/docs/" +`) + + fs := afero.NewMemMapFs() + _ = fs.MkdirAll("/src/docs", 0755) + + stdout, err := executeCommandWithFs(t, fs, "check-coverage", "--config", cfgPath) + + require.NoError(t, err) + assert.Contains(t, stdout, "Uncovered paths:") +} + +// --- version (positive path) --- + +func TestVersion_ValidRsync(t *testing.T) { + // Only run if rsync is actually installed + _, err := os.Stat("/usr/bin/rsync") + if os.IsNotExist(err) { + t.Skip("rsync not installed") + } + + stdout, err := executeCommand(t, "version", "--rsync-path", "/usr/bin/rsync") + + require.NoError(t, err) + assert.Contains(t, stdout, "Rsync Binary Path: /usr/bin/rsync") + assert.Contains(t, stdout, "Version Info:") +} + +// --- list (positive path) --- + +func TestList_ValidConfig(t *testing.T) { + cfgPath := writeConfigFile(t, ` +sources: + - path: "/home" +targets: + - path: "/backup" +jobs: + - name: "docs" + source: "/home/docs/" + target: "/backup/docs/" + enabled: true +`) + + // list doesn't call rsync, it just prints job info + _, err := executeCommand(t, "list", "--config", cfgPath) + + require.NoError(t, err) +} diff --git a/backup/cmd/version.go b/backup/cmd/version.go index e5aa12e..4cf61c1 100644 --- a/backup/cmd/version.go +++ b/backup/cmd/version.go @@ -22,8 +22,9 @@ func buildVersionCommand() *cobra.Command { return fmt.Errorf("getting version info: %w", err) } - fmt.Printf("Rsync Binary Path: %s\n", rsyncPath) - fmt.Printf("Version Info: %s", output) + out := cmd.OutOrStdout() + fmt.Fprintf(out, "Rsync Binary Path: %s\n", rsyncPath) + fmt.Fprintf(out, "Version Info: %s", output) return nil }, From 8dd1b4e147cbb374583ca842da159acbd184dc2a Mon Sep 17 00:00:00 2001 From: Jaap de Haan <261428+jdehaan@users.noreply.github.com> Date: Fri, 20 Mar 2026 08:59:13 +0000 Subject: [PATCH 11/25] chore: Use go 1.25 --- go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go.mod b/go.mod index 1f51c78..d0c05f1 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module backup-rsync -go 1.24.9 +go 1.25 require ( github.com/spf13/afero v1.15.0 From e6218c0cba17682dc2f71e82002890b0f91c8388 Mon Sep 17 00:00:00 2001 From: Jaap de Haan <261428+jdehaan@users.noreply.github.com> Date: Fri, 20 Mar 2026 09:01:20 +0000 Subject: [PATCH 12/25] feat: Add flag for data race detection --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 48404b3..71912b8 100644 --- a/Makefile +++ b/Makefile @@ -33,7 +33,7 @@ sanity-check: format check-clean check-mod-tidy @echo "OK: All sanity checks passed." test: - go test ./... -v + go test -race ./... -v tidy: gofmt -s -w . From 151367b67713fe4d119f5aba2e2d597085b8d788 Mon Sep 17 00:00:00 2001 From: Jaap de Haan <261428+jdehaan@users.noreply.github.com> Date: Fri, 20 Mar 2026 09:04:23 +0000 Subject: [PATCH 13/25] ci: Add check coverage with 90% --- .github/workflows/go.yml | 3 +++ Makefile | 14 +++++++++++++- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index d3fbfab..33b3794 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -40,3 +40,6 @@ jobs: - name: Test run: make test + + - name: Check coverage + run: make check-coverage diff --git a/Makefile b/Makefile index 71912b8..2cab072 100644 --- a/Makefile +++ b/Makefile @@ -3,8 +3,9 @@ # Build command with common flags BUILD_CMD = CGO_ENABLED=0 go build -trimpath -ldflags="-s -w" -tags=prod PACKAGE = ./backup/main.go +COVERAGE_THRESHOLD = 90 -.PHONY: build clean test lint tidy checksums release sanity-check check-mod-tidy lint-config-check lint-fix format check-clean +.PHONY: build clean test lint tidy checksums release sanity-check check-mod-tidy lint-config-check lint-fix format check-clean check-coverage format: go fmt ./... @@ -73,6 +74,17 @@ report-size: build go install github.com/Zxilly/go-size-analyzer/cmd/gsa@latest gsa --web --listen=":8910" --open dist/backup +check-coverage: + @go test ./... -count=1 -coverprofile=/tmp/coverage.out -coverpkg=./backup/... + @COVERAGE=$$(go tool cover -func=/tmp/coverage.out | grep '^total:' | awk '{print int($$3)}'); \ + echo "Total coverage: $${COVERAGE}%"; \ + if [ "$${COVERAGE}" -lt "$(COVERAGE_THRESHOLD)" ]; then \ + echo "FAIL: Coverage $${COVERAGE}% is below threshold $(COVERAGE_THRESHOLD)%"; \ + exit 1; \ + else \ + echo "OK: Coverage $${COVERAGE}% meets threshold $(COVERAGE_THRESHOLD)%"; \ + fi + report-coverage: @mkdir -p coverage @go test ./... -count=1 -coverprofile=coverage/coverage.out -coverpkg=./backup/... From d11f53f3782bf28a238ae820248429dc4643ff50 Mon Sep 17 00:00:00 2001 From: Jaap de Haan <261428+jdehaan@users.noreply.github.com> Date: Fri, 20 Mar 2026 09:06:25 +0000 Subject: [PATCH 14/25] fix: Remove spurious comment --- backup/internal/config.go | 1 - 1 file changed, 1 deletion(-) diff --git a/backup/internal/config.go b/backup/internal/config.go index 14f4fbf..da791fb 100644 --- a/backup/internal/config.go +++ b/backup/internal/config.go @@ -153,7 +153,6 @@ func validateJobPaths(jobs []Job, pathType string, getPath func(job Job) string) 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 From 4e12d9c567f63dee271c9ea5e3a0a6a35f0969c0 Mon Sep 17 00:00:00 2001 From: Jaap de Haan <261428+jdehaan@users.noreply.github.com> Date: Fri, 20 Mar 2026 09:19:43 +0000 Subject: [PATCH 15/25] chore: Update AGENTS.md --- AGENTS.md | 67 ++++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 49 insertions(+), 18 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 8835711..2db1fcf 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -2,41 +2,60 @@ ## Overview -CLI tool for managing local backups using `rsync` as the engine. Built in Go with `cobra` for CLI, `afero` for filesystem abstraction, and YAML for configuration. Local-only — no remote rsync support. +CLI tool for managing local backups using `rsync` as the engine. +Built in Go with `cobra` for CLI, `afero` for filesystem abstraction, and YAML for configuration. Local-only — no remote rsync support. ## Code Style -- Go 1.24+; follow idiomatic Go conventions +- Go 1.25; follow idiomatic Go conventions - Format with `go fmt`; lint with `golangci-lint` (config in `.golangci.yml`) - All linters enabled by default — check `.golangci.yml` for disabled ones - Keep packages focused: `cmd/` for CLI wiring, `internal/` for core logic - Prefer dependency injection over global state for testability - Use interfaces at consumption boundaries (see `internal/exec.go`, `internal/job_command.go`) +- All output in commands routed through `cmd.OutOrStdout()` or injected `io.Writer` — never raw `fmt.Printf` +- All commands use `RunE` with wrapped errors ## Architecture ``` backup/ - main.go # Entrypoint — calls cmd.BuildRootCommand().Execute() - cmd/ # Cobra commands: list, run, simulate, config, check-coverage, version - internal/ # Core logic: config loading, job execution, rsync wrapper + main.go # Entrypoint — calls cmd.BuildRootCommand().Execute() + cmd/ # Cobra commands: list, run, simulate, config (show/validate), check-coverage, version + root.go # BuildRootCommand() / BuildRootCommandWithFs(fs) — injects afero.Fs + test/ # Cobra command integration tests + internal/ # Core logic: config, job execution, rsync wrapper, coverage checker + test/ # Unit tests + mockery-generated mocks ``` -- **Config**: YAML-based (`Config`, `Job`, sources, targets, variables with `${var}` substitution) -- **Jobs**: Each job maps a source to a target with optional exclusions; jobs run independently -- **Rsync**: Wrapped via `SharedCommand` struct in `internal/rsync.go` -- YAML config files at repo root (e.g., `sync.yaml`) define backup configurations +### Key Types & Interfaces + +- **`Exec`** (interface): Command execution abstraction (`OsExec` for real, `MockExec` for tests) +- **`JobCommand`** (interface): `Run(job Job) JobStatus` + `GetVersionInfo()` — implemented by `ListCommand`, `SimulateCommand`, `SyncCommand` +- **`SharedCommand`** (struct): Base for all commands — holds `BinPath`, `BaseLogPath`, `Shell Exec`, `Output io.Writer` +- **`CoverageChecker`** (struct): Analyzes path coverage with injected `*log.Logger` and `afero.Fs` +- **`Config`**: YAML-based (`Config`, `Job`, `Path`, variables with `${var}` substitution) + +### Dependency Injection + +- `Exec` injected into all command constructors (`NewListCommand`, `NewSimulateCommand`, `NewSyncCommand`) +- `io.Writer` injected into `SharedCommand` for output capture +- `afero.Fs` injected into `BuildRootCommandWithFs()` → `buildCheckCoverageCommand(fs)` +- `*log.Logger` injected into `CoverageChecker` and `Config.Apply()` +- Commands use `cmd.OutOrStdout()` for testable output ## Build and Test ```sh -make build # Build to dist/backup -make test # go test ./... -v -make lint # golangci-lint run ./... -make lint-fix # Auto-fix lint issues -make format # go fmt ./... -make tidy # gofmt -s + go mod tidy -make sanity-check # format + clean + tidy +make build # Build to dist/backup +make test # go test -race ./... -v +make lint # golangci-lint run ./... +make lint-fix # Auto-fix lint issues +make format # go fmt ./... +make tidy # gofmt -s + go mod tidy +make sanity-check # format + clean + tidy +make check-coverage # Fail if coverage < 90% +make report-coverage # Generate HTML coverage report ``` ## Testing Conventions @@ -45,17 +64,29 @@ make sanity-check # format + clean + tidy - Use **dependency injection** — inject interfaces, not concrete types - **Mocks**: Generated with [mockery](https://github.com/vektra/mockery) (config: `.mockery.yml`) - Mock files live in `internal/test/` as `mock__test.go` - - Mock structs named `Mock` + - Mock structs named `Mock` (e.g., `MockExec`, `MockJobCommand`) - See `MOCKERY_INTEGRATION.md` for setup details - Use `testify` for assertions (`require` / `assert`) - Test files live in `/test/` subdirectories - Prefer table-driven tests for multiple input scenarios +- Use `afero.NewMemMapFs()` in tests — never hit the real filesystem +- Use `bytes.Buffer` or `io.Discard` for output capture in tests +- CI enforces coverage threshold via `make check-coverage` + +## CI Pipeline + +CI runs on every push/PR to `main` (`.github/workflows/go.yml`): +1. Sanity check (format + clean + mod tidy) +2. Lint (golangci-lint) +3. Build +4. Test (with `-race` flag) +5. Coverage threshold enforcement (90%) ## Conventions - No remote rsync — only locally mounted paths - Job-level granularity: each backup job can be listed, simulated, or run independently - Dry-run/simulate mode available for all operations -- Logging goes to both stdout and timestamped log directories under `logs/` +- Logging goes to both an injected `io.Writer` (user output) and `*log.Logger` (file logging) under `logs/` - Custom YAML unmarshaling handles job defaults (see `internal/job.go`) - CI runs sanity checks, lint, and build on every push/PR (`.github/workflows/go.yml`) From e674792a22674b2de3b9a2ac545a3017a3ea0bce Mon Sep 17 00:00:00 2001 From: Jaap de Haan <261428+jdehaan@users.noreply.github.com> Date: Fri, 20 Mar 2026 11:39:25 +0000 Subject: [PATCH 16/25] fix: Use cobra's cmd.OutOrStdout --- backup/cmd/list.go | 6 +++--- backup/cmd/run.go | 6 +++--- backup/cmd/simulate.go | 6 +++--- backup/cmd/test/commands_test.go | 5 +++-- backup/cmd/version.go | 5 ++--- 5 files changed, 14 insertions(+), 14 deletions(-) diff --git a/backup/cmd/list.go b/backup/cmd/list.go index cc5f7f1..04009af 100644 --- a/backup/cmd/list.go +++ b/backup/cmd/list.go @@ -5,7 +5,6 @@ import ( "fmt" "io" "log" - "os" "github.com/spf13/cobra" ) @@ -23,10 +22,11 @@ func buildListCommand() *cobra.Command { return fmt.Errorf("loading config: %w", err) } - command := internal.NewListCommand(rsyncPath, &internal.OsExec{}, os.Stdout) + out := cmd.OutOrStdout() + command := internal.NewListCommand(rsyncPath, &internal.OsExec{}, out) logger := log.New(io.Discard, "", 0) - cfg.Apply(command, logger, os.Stdout) + cfg.Apply(command, logger, out) return nil }, diff --git a/backup/cmd/run.go b/backup/cmd/run.go index 0f52834..c2de9b9 100644 --- a/backup/cmd/run.go +++ b/backup/cmd/run.go @@ -3,7 +3,6 @@ package cmd import ( "backup-rsync/backup/internal" "fmt" - "os" "github.com/spf13/cobra" ) @@ -26,9 +25,10 @@ func buildRunCommand() *cobra.Command { return fmt.Errorf("creating logger: %w", err) } - command := internal.NewSyncCommand(rsyncPath, logPath, &internal.OsExec{}, os.Stdout) + out := cmd.OutOrStdout() + command := internal.NewSyncCommand(rsyncPath, logPath, &internal.OsExec{}, out) - cfg.Apply(command, logger, os.Stdout) + cfg.Apply(command, logger, out) return nil }, diff --git a/backup/cmd/simulate.go b/backup/cmd/simulate.go index 530fdb9..76447aa 100644 --- a/backup/cmd/simulate.go +++ b/backup/cmd/simulate.go @@ -3,7 +3,6 @@ package cmd import ( "backup-rsync/backup/internal" "fmt" - "os" "github.com/spf13/cobra" ) @@ -26,9 +25,10 @@ func buildSimulateCommand() *cobra.Command { return fmt.Errorf("creating logger: %w", err) } - command := internal.NewSimulateCommand(rsyncPath, logPath, &internal.OsExec{}, os.Stdout) + out := cmd.OutOrStdout() + command := internal.NewSimulateCommand(rsyncPath, logPath, &internal.OsExec{}, out) - cfg.Apply(command, logger, os.Stdout) + cfg.Apply(command, logger, out) return nil }, diff --git a/backup/cmd/test/commands_test.go b/backup/cmd/test/commands_test.go index 1685ea4..2952231 100644 --- a/backup/cmd/test/commands_test.go +++ b/backup/cmd/test/commands_test.go @@ -237,8 +237,9 @@ jobs: enabled: true `) - // list doesn't call rsync, it just prints job info - _, err := executeCommand(t, "list", "--config", cfgPath) + stdout, err := executeCommand(t, "list", "--config", cfgPath) require.NoError(t, err) + assert.Contains(t, stdout, "Job: docs") + assert.Contains(t, stdout, "Status [docs]: SUCCESS") } diff --git a/backup/cmd/version.go b/backup/cmd/version.go index 4cf61c1..4d64dec 100644 --- a/backup/cmd/version.go +++ b/backup/cmd/version.go @@ -2,7 +2,6 @@ package cmd import ( "fmt" - "os" "backup-rsync/backup/internal" @@ -15,14 +14,14 @@ func buildVersionCommand() *cobra.Command { Short: "Prints the rsync version, protocol version, and full path to the rsync binary.", RunE: func(cmd *cobra.Command, args []string) error { rsyncPath, _ := cmd.Flags().GetString("rsync-path") - rsync := internal.NewSyncCommand(rsyncPath, "", &internal.OsExec{}, os.Stdout) + out := cmd.OutOrStdout() + rsync := internal.NewSyncCommand(rsyncPath, "", &internal.OsExec{}, out) output, _, err := rsync.GetVersionInfo() if err != nil { return fmt.Errorf("getting version info: %w", err) } - out := cmd.OutOrStdout() fmt.Fprintf(out, "Rsync Binary Path: %s\n", rsyncPath) fmt.Fprintf(out, "Version Info: %s", output) From 46a618afb04104c7d5107cb058aa3adfe1964d1d Mon Sep 17 00:00:00 2001 From: Jaap de Haan <261428+jdehaan@users.noreply.github.com> Date: Fri, 20 Mar 2026 11:40:56 +0000 Subject: [PATCH 17/25] fix: Improve test coverage --- backup/cmd/test/commands_test.go | 38 ++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/backup/cmd/test/commands_test.go b/backup/cmd/test/commands_test.go index 2952231..5d6c6f6 100644 --- a/backup/cmd/test/commands_test.go +++ b/backup/cmd/test/commands_test.go @@ -151,6 +151,25 @@ func TestRun_MissingConfig(t *testing.T) { assert.Contains(t, err.Error(), "loading config") } +func TestRun_ValidConfig(t *testing.T) { + cfgPath := writeConfigFile(t, ` +sources: + - path: "/home" +targets: + - path: "/backup" +jobs: + - name: "docs" + source: "/home/docs/" + target: "/backup/docs/" + enabled: true +`) + + stdout, err := executeCommand(t, "run", "--config", cfgPath) + + require.NoError(t, err) + assert.Contains(t, stdout, "Status [docs]:") +} + // --- simulate --- func TestSimulate_MissingConfig(t *testing.T) { @@ -160,6 +179,25 @@ func TestSimulate_MissingConfig(t *testing.T) { assert.Contains(t, err.Error(), "loading config") } +func TestSimulate_ValidConfig(t *testing.T) { + cfgPath := writeConfigFile(t, ` +sources: + - path: "/home" +targets: + - path: "/backup" +jobs: + - name: "docs" + source: "/home/docs/" + target: "/backup/docs/" + enabled: true +`) + + stdout, err := executeCommand(t, "simulate", "--config", cfgPath) + + require.NoError(t, err) + assert.Contains(t, stdout, "Status [docs]:") +} + // --- version --- func TestVersion_InvalidRsyncPath(t *testing.T) { From eb9307411090ab039986bbf304ba912d569f6333 Mon Sep 17 00:00:00 2001 From: Jaap de Haan <261428+jdehaan@users.noreply.github.com> Date: Fri, 20 Mar 2026 11:49:18 +0000 Subject: [PATCH 18/25] refactor: Inject Exec into cmd for full testability --- backup/cmd/list.go | 4 +-- backup/cmd/root.go | 16 +++++++--- backup/cmd/run.go | 4 +-- backup/cmd/simulate.go | 4 +-- backup/cmd/test/commands_test.go | 54 +++++++++++++++++++++++++++++--- backup/cmd/version.go | 4 +-- 6 files changed, 68 insertions(+), 18 deletions(-) diff --git a/backup/cmd/list.go b/backup/cmd/list.go index 04009af..14bab48 100644 --- a/backup/cmd/list.go +++ b/backup/cmd/list.go @@ -9,7 +9,7 @@ import ( "github.com/spf13/cobra" ) -func buildListCommand() *cobra.Command { +func buildListCommand(shell internal.Exec) *cobra.Command { return &cobra.Command{ Use: "list", Short: "List the commands that will be executed", @@ -23,7 +23,7 @@ func buildListCommand() *cobra.Command { } out := cmd.OutOrStdout() - command := internal.NewListCommand(rsyncPath, &internal.OsExec{}, out) + command := internal.NewListCommand(rsyncPath, shell, out) logger := log.New(io.Discard, "", 0) cfg.Apply(command, logger, out) diff --git a/backup/cmd/root.go b/backup/cmd/root.go index 6412a03..18d5b90 100644 --- a/backup/cmd/root.go +++ b/backup/cmd/root.go @@ -1,15 +1,21 @@ package cmd import ( + "backup-rsync/backup/internal" + "github.com/spf13/afero" "github.com/spf13/cobra" ) func BuildRootCommand() *cobra.Command { - return BuildRootCommandWithFs(afero.NewOsFs()) + return BuildRootCommandWithDeps(afero.NewOsFs(), &internal.OsExec{}) } func BuildRootCommandWithFs(fs afero.Fs) *cobra.Command { + return BuildRootCommandWithDeps(fs, &internal.OsExec{}) +} + +func BuildRootCommandWithDeps(fs afero.Fs, shell internal.Exec) *cobra.Command { rootCmd := &cobra.Command{ Use: "backup", Short: "A tool for managing backups", @@ -20,12 +26,12 @@ func BuildRootCommandWithFs(fs afero.Fs) *cobra.Command { rootCmd.PersistentFlags().String("rsync-path", "/usr/bin/rsync", "Path to the rsync binary") rootCmd.AddCommand( - buildListCommand(), - buildRunCommand(), - buildSimulateCommand(), + buildListCommand(shell), + buildRunCommand(shell), + buildSimulateCommand(shell), buildConfigCommand(), buildCheckCoverageCommand(fs), - buildVersionCommand(), + buildVersionCommand(shell), ) return rootCmd diff --git a/backup/cmd/run.go b/backup/cmd/run.go index c2de9b9..a1af492 100644 --- a/backup/cmd/run.go +++ b/backup/cmd/run.go @@ -7,7 +7,7 @@ import ( "github.com/spf13/cobra" ) -func buildRunCommand() *cobra.Command { +func buildRunCommand(shell internal.Exec) *cobra.Command { return &cobra.Command{ Use: "run", Short: "Execute the sync jobs", @@ -26,7 +26,7 @@ func buildRunCommand() *cobra.Command { } out := cmd.OutOrStdout() - command := internal.NewSyncCommand(rsyncPath, logPath, &internal.OsExec{}, out) + command := internal.NewSyncCommand(rsyncPath, logPath, shell, out) cfg.Apply(command, logger, out) diff --git a/backup/cmd/simulate.go b/backup/cmd/simulate.go index 76447aa..b19f786 100644 --- a/backup/cmd/simulate.go +++ b/backup/cmd/simulate.go @@ -7,7 +7,7 @@ import ( "github.com/spf13/cobra" ) -func buildSimulateCommand() *cobra.Command { +func buildSimulateCommand(shell internal.Exec) *cobra.Command { return &cobra.Command{ Use: "simulate", Short: "Simulate the sync jobs", @@ -26,7 +26,7 @@ func buildSimulateCommand() *cobra.Command { } out := cmd.OutOrStdout() - command := internal.NewSimulateCommand(rsyncPath, logPath, &internal.OsExec{}, out) + command := internal.NewSimulateCommand(rsyncPath, logPath, shell, out) cfg.Apply(command, logger, out) diff --git a/backup/cmd/test/commands_test.go b/backup/cmd/test/commands_test.go index 5d6c6f6..2e76807 100644 --- a/backup/cmd/test/commands_test.go +++ b/backup/cmd/test/commands_test.go @@ -7,12 +7,22 @@ import ( "testing" "backup-rsync/backup/cmd" + "backup-rsync/backup/internal" "github.com/spf13/afero" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +type stubExec struct { + output []byte + err error +} + +func (s *stubExec) Execute(_ string, _ ...string) ([]byte, error) { + return s.output, s.err +} + func writeConfigFile(t *testing.T, content string) string { t.Helper() @@ -47,6 +57,22 @@ func executeCommandWithFs(t *testing.T, fs afero.Fs, args ...string) (string, er return stdout.String(), err } +func executeCommandWithDeps(t *testing.T, fs afero.Fs, shell internal.Exec, args ...string) (string, error) { + t.Helper() + + rootCmd := cmd.BuildRootCommandWithDeps(fs, shell) + + var stdout bytes.Buffer + + rootCmd.SetOut(&stdout) + rootCmd.SetErr(&bytes.Buffer{}) + rootCmd.SetArgs(args) + + err := rootCmd.Execute() + + return stdout.String(), err +} + // --- config show --- func TestConfigShow_ValidConfig(t *testing.T) { @@ -164,10 +190,13 @@ jobs: enabled: true `) - stdout, err := executeCommand(t, "run", "--config", cfgPath) + shell := &stubExec{output: []byte("rsync version 3.2.7 protocol version 31\n")} + + stdout, err := executeCommandWithDeps(t, afero.NewMemMapFs(), shell, "run", "--config", cfgPath) require.NoError(t, err) - assert.Contains(t, stdout, "Status [docs]:") + assert.Contains(t, stdout, "Job: docs") + assert.Contains(t, stdout, "Status [docs]: SUCCESS") } // --- simulate --- @@ -192,10 +221,13 @@ jobs: enabled: true `) - stdout, err := executeCommand(t, "simulate", "--config", cfgPath) + shell := &stubExec{output: []byte("rsync version 3.2.7 protocol version 31\n")} + + stdout, err := executeCommandWithDeps(t, afero.NewMemMapFs(), shell, "simulate", "--config", cfgPath) require.NoError(t, err) - assert.Contains(t, stdout, "Status [docs]:") + assert.Contains(t, stdout, "Job: docs") + assert.Contains(t, stdout, "Status [docs]: SUCCESS") } // --- version --- @@ -260,6 +292,16 @@ func TestVersion_ValidRsync(t *testing.T) { assert.Contains(t, stdout, "Version Info:") } +func TestVersion_WithMockExec(t *testing.T) { + shell := &stubExec{output: []byte("rsync version 3.2.7 protocol version 31\n")} + + stdout, err := executeCommandWithDeps(t, afero.NewMemMapFs(), shell, "version", "--rsync-path", "/usr/bin/rsync") + + require.NoError(t, err) + assert.Contains(t, stdout, "Rsync Binary Path: /usr/bin/rsync") + assert.Contains(t, stdout, "rsync version 3.2.7") +} + // --- list (positive path) --- func TestList_ValidConfig(t *testing.T) { @@ -275,7 +317,9 @@ jobs: enabled: true `) - stdout, err := executeCommand(t, "list", "--config", cfgPath) + shell := &stubExec{output: []byte("rsync version 3.2.7 protocol version 31\n")} + + stdout, err := executeCommandWithDeps(t, afero.NewMemMapFs(), shell, "list", "--config", cfgPath) require.NoError(t, err) assert.Contains(t, stdout, "Job: docs") diff --git a/backup/cmd/version.go b/backup/cmd/version.go index 4d64dec..06af080 100644 --- a/backup/cmd/version.go +++ b/backup/cmd/version.go @@ -8,14 +8,14 @@ import ( "github.com/spf13/cobra" ) -func buildVersionCommand() *cobra.Command { +func buildVersionCommand(shell internal.Exec) *cobra.Command { return &cobra.Command{ Use: "version", Short: "Prints the rsync version, protocol version, and full path to the rsync binary.", RunE: func(cmd *cobra.Command, args []string) error { rsyncPath, _ := cmd.Flags().GetString("rsync-path") out := cmd.OutOrStdout() - rsync := internal.NewSyncCommand(rsyncPath, "", &internal.OsExec{}, out) + rsync := internal.NewSyncCommand(rsyncPath, "", shell, out) output, _, err := rsync.GetVersionInfo() if err != nil { From 2d8031496416981bb985fffdbc6b954ba4ac0086 Mon Sep 17 00:00:00 2001 From: Jaap de Haan <261428+jdehaan@users.noreply.github.com> Date: Fri, 20 Mar 2026 11:54:04 +0000 Subject: [PATCH 19/25] chore: Improve test coverage --- backup/cmd/test/commands_test.go | 74 ++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) diff --git a/backup/cmd/test/commands_test.go b/backup/cmd/test/commands_test.go index 2e76807..b8b72f8 100644 --- a/backup/cmd/test/commands_test.go +++ b/backup/cmd/test/commands_test.go @@ -177,6 +177,32 @@ func TestRun_MissingConfig(t *testing.T) { assert.Contains(t, err.Error(), "loading config") } +func TestRun_CreateLoggerError(t *testing.T) { + cfgPath := writeConfigFile(t, ` +sources: + - path: "/home" +targets: + - path: "/backup" +jobs: + - name: "docs" + source: "/home/docs/" + target: "/backup/docs/" +`) + + // Block log directory creation by placing a file named "logs" + tmpDir := t.TempDir() + t.Chdir(tmpDir) + + require.NoError(t, os.WriteFile(filepath.Join(tmpDir, "logs"), []byte("block"), 0600)) + + shell := &stubExec{output: []byte("rsync version 3.2.7 protocol version 31\n")} + + _, err := executeCommandWithDeps(t, afero.NewMemMapFs(), shell, "run", "--config", cfgPath) + + require.Error(t, err) + assert.Contains(t, err.Error(), "creating logger") +} + func TestRun_ValidConfig(t *testing.T) { cfgPath := writeConfigFile(t, ` sources: @@ -208,6 +234,31 @@ func TestSimulate_MissingConfig(t *testing.T) { assert.Contains(t, err.Error(), "loading config") } +func TestSimulate_CreateLoggerError(t *testing.T) { + cfgPath := writeConfigFile(t, ` +sources: + - path: "/home" +targets: + - path: "/backup" +jobs: + - name: "docs" + source: "/home/docs/" + target: "/backup/docs/" +`) + + tmpDir := t.TempDir() + t.Chdir(tmpDir) + + require.NoError(t, os.WriteFile(filepath.Join(tmpDir, "logs"), []byte("block"), 0600)) + + shell := &stubExec{output: []byte("rsync version 3.2.7 protocol version 31\n")} + + _, err := executeCommandWithDeps(t, afero.NewMemMapFs(), shell, "simulate", "--config", cfgPath) + + require.Error(t, err) + assert.Contains(t, err.Error(), "creating logger") +} + func TestSimulate_ValidConfig(t *testing.T) { cfgPath := writeConfigFile(t, ` sources: @@ -255,6 +306,29 @@ func TestCheckCoverage_MissingConfig(t *testing.T) { assert.Contains(t, err.Error(), "loading config") } +func TestCheckCoverage_WithUncoveredPaths(t *testing.T) { + cfgPath := writeConfigFile(t, ` +sources: + - path: "/src" +targets: + - path: "/dst" +jobs: + - name: "docs" + source: "/src/docs/" + target: "/dst/docs/" +`) + + fs := afero.NewMemMapFs() + _ = fs.MkdirAll("/src/docs", 0755) + _ = fs.MkdirAll("/src/photos", 0755) + + stdout, err := executeCommandWithFs(t, fs, "check-coverage", "--config", cfgPath) + + require.NoError(t, err) + assert.Contains(t, stdout, "Uncovered paths:") + assert.Contains(t, stdout, "/src") +} + func TestCheckCoverage_ValidConfig(t *testing.T) { cfgPath := writeConfigFile(t, ` sources: From c017285e95b8740bf6dc5d5d786dfa7fbdc2d5ab Mon Sep 17 00:00:00 2001 From: Jaap de Haan <261428+jdehaan@users.noreply.github.com> Date: Fri, 20 Mar 2026 11:56:52 +0000 Subject: [PATCH 20/25] fix: Deterministic tests by injecting timestamp --- backup/cmd/run.go | 3 ++- backup/cmd/simulate.go | 3 ++- backup/internal/helper.go | 8 ++++---- backup/internal/test/helper_test.go | 27 ++++++++++++++------------- 4 files changed, 22 insertions(+), 19 deletions(-) diff --git a/backup/cmd/run.go b/backup/cmd/run.go index a1af492..3b00eab 100644 --- a/backup/cmd/run.go +++ b/backup/cmd/run.go @@ -3,6 +3,7 @@ package cmd import ( "backup-rsync/backup/internal" "fmt" + "time" "github.com/spf13/cobra" ) @@ -20,7 +21,7 @@ func buildRunCommand(shell internal.Exec) *cobra.Command { return fmt.Errorf("loading config: %w", err) } - logger, logPath, err := internal.CreateMainLogger(configPath, false) + logger, logPath, err := internal.CreateMainLogger(configPath, false, time.Now()) if err != nil { return fmt.Errorf("creating logger: %w", err) } diff --git a/backup/cmd/simulate.go b/backup/cmd/simulate.go index b19f786..bab87c2 100644 --- a/backup/cmd/simulate.go +++ b/backup/cmd/simulate.go @@ -3,6 +3,7 @@ package cmd import ( "backup-rsync/backup/internal" "fmt" + "time" "github.com/spf13/cobra" ) @@ -20,7 +21,7 @@ func buildSimulateCommand(shell internal.Exec) *cobra.Command { return fmt.Errorf("loading config: %w", err) } - logger, logPath, err := internal.CreateMainLogger(configPath, true) + logger, logPath, err := internal.CreateMainLogger(configPath, true, time.Now()) if err != nil { return fmt.Errorf("creating logger: %w", err) } diff --git a/backup/internal/helper.go b/backup/internal/helper.go index 6fb57ce..b9baaa1 100644 --- a/backup/internal/helper.go +++ b/backup/internal/helper.go @@ -23,10 +23,10 @@ func NormalizePath(path string) string { const LogFilePermission = 0644 const LogDirPermission = 0755 -func getLogPath(simulate bool, configPath string) string { +func getLogPath(simulate bool, configPath string, now time.Time) string { filename := filepath.Base(configPath) filename = strings.TrimSuffix(filename, ".yaml") - logPath := "logs/sync-" + time.Now().Format("2006-01-02T15-04-05") + "-" + filename + logPath := "logs/sync-" + now.Format("2006-01-02T15-04-05") + "-" + filename if simulate { logPath += "-sim" @@ -35,8 +35,8 @@ func getLogPath(simulate bool, configPath string) string { return logPath } -func CreateMainLogger(configPath string, simulate bool) (*log.Logger, string, error) { - logPath := getLogPath(simulate, configPath) +func CreateMainLogger(configPath string, simulate bool, now time.Time) (*log.Logger, string, error) { + logPath := getLogPath(simulate, configPath, now) overallLogPath := logPath + "/summary.log" err := os.MkdirAll(logPath, LogDirPermission) diff --git a/backup/internal/test/helper_test.go b/backup/internal/test/helper_test.go index 39cdd0b..cb1321b 100644 --- a/backup/internal/test/helper_test.go +++ b/backup/internal/test/helper_test.go @@ -2,6 +2,7 @@ package internal_test import ( "testing" + "time" . "backup-rsync/backup/internal" @@ -26,39 +27,39 @@ func TestNormalizePath(t *testing.T) { } } +func fixedTime() time.Time { + return time.Date(2025, 6, 15, 14, 30, 45, 0, time.UTC) +} + func TestCreateMainLogger_Title_IsPresent(t *testing.T) { - logger, logPath, err := CreateMainLogger("title", true) + logger, logPath, err := CreateMainLogger("title", true, fixedTime()) require.NoError(t, err) assert.Contains(t, logPath, "title") assert.NotNil(t, logger) } func TestCreateMainLogger_IsSimulate_HasSimSuffix(t *testing.T) { - logger, logPath, err := CreateMainLogger("", true) + logger, logPath, err := CreateMainLogger("", true, fixedTime()) require.NoError(t, err) assert.Contains(t, logPath, "-sim") assert.NotNil(t, logger) } func TestCreateMainLogger_NotSimulate_HasNoSimSuffix(t *testing.T) { - logger, logPath, err := CreateMainLogger("", false) + logger, logPath, err := CreateMainLogger("", false, fixedTime()) require.NoError(t, err) assert.NotContains(t, logPath, "-sim") assert.NotNil(t, logger) } -func TestCreateLogPath_IsSimulate_ContainsTimestamp(t *testing.T) { - _, logPath, err := CreateMainLogger("", true) +func TestCreateMainLogger_DeterministicLogPath(t *testing.T) { + _, logPath, err := CreateMainLogger("backup.yaml", true, fixedTime()) require.NoError(t, err) - // Check if the logPath contains a timestamp in the format '2006-01-02T15-04-05' - timestampRegex := `\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}` - assert.Regexp(t, timestampRegex, logPath) + assert.Equal(t, "logs/sync-2025-06-15T14-30-45-backup-sim", logPath) } -func TestCreateLogPath_NotSimulate_ContainsTimestamp(t *testing.T) { - _, logPath, err := CreateMainLogger("", false) +func TestCreateMainLogger_DeterministicLogPath_NoSimulate(t *testing.T) { + _, logPath, err := CreateMainLogger("sync.yaml", false, fixedTime()) require.NoError(t, err) - // Check if the logPath contains a timestamp in the format '2006-01-02T15-04-05' - timestampRegex := `\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}` - assert.Regexp(t, timestampRegex, logPath) + assert.Equal(t, "logs/sync-2025-06-15T14-30-45-sync", logPath) } From 75b12ec832ecf1ac63b5ae4570d2eb75ff68857a Mon Sep 17 00:00:00 2001 From: Jaap de Haan <261428+jdehaan@users.noreply.github.com> Date: Fri, 20 Mar 2026 11:59:11 +0000 Subject: [PATCH 21/25] fix: Close log file handle properly --- backup/cmd/run.go | 4 +++- backup/cmd/simulate.go | 4 +++- backup/internal/helper.go | 12 ++++++++---- backup/internal/test/helper_test.go | 25 ++++++++++++++++++++----- 4 files changed, 34 insertions(+), 11 deletions(-) diff --git a/backup/cmd/run.go b/backup/cmd/run.go index 3b00eab..ba0f96d 100644 --- a/backup/cmd/run.go +++ b/backup/cmd/run.go @@ -21,11 +21,13 @@ func buildRunCommand(shell internal.Exec) *cobra.Command { return fmt.Errorf("loading config: %w", err) } - logger, logPath, err := internal.CreateMainLogger(configPath, false, time.Now()) + logger, logPath, cleanup, err := internal.CreateMainLogger(configPath, false, time.Now()) if err != nil { return fmt.Errorf("creating logger: %w", err) } + defer cleanup() + out := cmd.OutOrStdout() command := internal.NewSyncCommand(rsyncPath, logPath, shell, out) diff --git a/backup/cmd/simulate.go b/backup/cmd/simulate.go index bab87c2..0a5d923 100644 --- a/backup/cmd/simulate.go +++ b/backup/cmd/simulate.go @@ -21,11 +21,13 @@ func buildSimulateCommand(shell internal.Exec) *cobra.Command { return fmt.Errorf("loading config: %w", err) } - logger, logPath, err := internal.CreateMainLogger(configPath, true, time.Now()) + logger, logPath, cleanup, err := internal.CreateMainLogger(configPath, true, time.Now()) if err != nil { return fmt.Errorf("creating logger: %w", err) } + defer cleanup() + out := cmd.OutOrStdout() command := internal.NewSimulateCommand(rsyncPath, logPath, shell, out) diff --git a/backup/internal/helper.go b/backup/internal/helper.go index b9baaa1..080576c 100644 --- a/backup/internal/helper.go +++ b/backup/internal/helper.go @@ -35,21 +35,25 @@ func getLogPath(simulate bool, configPath string, now time.Time) string { return logPath } -func CreateMainLogger(configPath string, simulate bool, now time.Time) (*log.Logger, string, error) { +func CreateMainLogger(configPath string, simulate bool, now time.Time) (*log.Logger, string, func() error, error) { logPath := getLogPath(simulate, configPath, now) overallLogPath := logPath + "/summary.log" err := os.MkdirAll(logPath, LogDirPermission) if err != nil { - return nil, "", fmt.Errorf("failed to create log directory: %w", err) + return nil, "", nil, fmt.Errorf("failed to create log directory: %w", err) } overallLogFile, err := os.OpenFile(overallLogPath, os.O_CREATE|os.O_WRONLY|os.O_APPEND, LogFilePermission) if err != nil { - return nil, "", fmt.Errorf("failed to open overall log file: %w", err) + return nil, "", nil, fmt.Errorf("failed to open overall log file: %w", err) } logger := log.New(overallLogFile, "", log.LstdFlags) - return logger, logPath, nil + cleanup := func() error { + return overallLogFile.Close() + } + + return logger, logPath, cleanup, nil } diff --git a/backup/internal/test/helper_test.go b/backup/internal/test/helper_test.go index cb1321b..d450003 100644 --- a/backup/internal/test/helper_test.go +++ b/backup/internal/test/helper_test.go @@ -32,34 +32,49 @@ func fixedTime() time.Time { } func TestCreateMainLogger_Title_IsPresent(t *testing.T) { - logger, logPath, err := CreateMainLogger("title", true, fixedTime()) + logger, logPath, cleanup, err := CreateMainLogger("title", true, fixedTime()) require.NoError(t, err) + + defer cleanup() + assert.Contains(t, logPath, "title") assert.NotNil(t, logger) } func TestCreateMainLogger_IsSimulate_HasSimSuffix(t *testing.T) { - logger, logPath, err := CreateMainLogger("", true, fixedTime()) + logger, logPath, cleanup, err := CreateMainLogger("", true, fixedTime()) require.NoError(t, err) + + defer cleanup() + assert.Contains(t, logPath, "-sim") assert.NotNil(t, logger) } func TestCreateMainLogger_NotSimulate_HasNoSimSuffix(t *testing.T) { - logger, logPath, err := CreateMainLogger("", false, fixedTime()) + logger, logPath, cleanup, err := CreateMainLogger("", false, fixedTime()) require.NoError(t, err) + + defer cleanup() + assert.NotContains(t, logPath, "-sim") assert.NotNil(t, logger) } func TestCreateMainLogger_DeterministicLogPath(t *testing.T) { - _, logPath, err := CreateMainLogger("backup.yaml", true, fixedTime()) + _, logPath, cleanup, err := CreateMainLogger("backup.yaml", true, fixedTime()) require.NoError(t, err) + + defer cleanup() + assert.Equal(t, "logs/sync-2025-06-15T14-30-45-backup-sim", logPath) } func TestCreateMainLogger_DeterministicLogPath_NoSimulate(t *testing.T) { - _, logPath, err := CreateMainLogger("sync.yaml", false, fixedTime()) + _, logPath, cleanup, err := CreateMainLogger("sync.yaml", false, fixedTime()) require.NoError(t, err) + + defer cleanup() + assert.Equal(t, "logs/sync-2025-06-15T14-30-45-sync", logPath) } From 76f50e486b6efe877b7daa3990a5d50c3aebc1e5 Mon Sep 17 00:00:00 2001 From: Jaap de Haan <261428+jdehaan@users.noreply.github.com> Date: Fri, 20 Mar 2026 12:02:23 +0000 Subject: [PATCH 22/25] fix: Defensive programming for Marshal Although currently no problem should ever occur with the basic types used --- backup/internal/config.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/backup/internal/config.go b/backup/internal/config.go index da791fb..916cf58 100644 --- a/backup/internal/config.go +++ b/backup/internal/config.go @@ -29,7 +29,10 @@ type Config struct { } func (cfg Config) String() string { - out, _ := yaml.Marshal(cfg) + out, err := yaml.Marshal(cfg) + if err != nil { + return fmt.Sprintf("error marshaling config: %v", err) + } return string(out) } From 39beee04feba1799974a416b16cb42adbfbc5ae9 Mon Sep 17 00:00:00 2001 From: Jaap de Haan <261428+jdehaan@users.noreply.github.com> Date: Fri, 20 Mar 2026 12:05:58 +0000 Subject: [PATCH 23/25] feat: Add structured exit codes and summary --- backup/cmd/list.go | 3 +-- backup/cmd/run.go | 4 +--- backup/cmd/simulate.go | 4 +--- backup/internal/config.go | 24 +++++++++++++++++++++++- backup/internal/test/config_test.go | 9 +++++++-- 5 files changed, 33 insertions(+), 11 deletions(-) diff --git a/backup/cmd/list.go b/backup/cmd/list.go index 14bab48..341105a 100644 --- a/backup/cmd/list.go +++ b/backup/cmd/list.go @@ -26,9 +26,8 @@ func buildListCommand(shell internal.Exec) *cobra.Command { command := internal.NewListCommand(rsyncPath, shell, out) logger := log.New(io.Discard, "", 0) - cfg.Apply(command, logger, out) - return nil + return cfg.Apply(command, logger, out) }, } } diff --git a/backup/cmd/run.go b/backup/cmd/run.go index ba0f96d..6d1ec21 100644 --- a/backup/cmd/run.go +++ b/backup/cmd/run.go @@ -31,9 +31,7 @@ func buildRunCommand(shell internal.Exec) *cobra.Command { out := cmd.OutOrStdout() command := internal.NewSyncCommand(rsyncPath, logPath, shell, out) - cfg.Apply(command, logger, out) - - return nil + return cfg.Apply(command, logger, out) }, } } diff --git a/backup/cmd/simulate.go b/backup/cmd/simulate.go index 0a5d923..f63bfd4 100644 --- a/backup/cmd/simulate.go +++ b/backup/cmd/simulate.go @@ -31,9 +31,7 @@ func buildSimulateCommand(shell internal.Exec) *cobra.Command { out := cmd.OutOrStdout() command := internal.NewSimulateCommand(rsyncPath, logPath, shell, out) - cfg.Apply(command, logger, out) - - return nil + return cfg.Apply(command, logger, out) }, } } diff --git a/backup/internal/config.go b/backup/internal/config.go index 916cf58..6324c39 100644 --- a/backup/internal/config.go +++ b/backup/internal/config.go @@ -18,6 +18,7 @@ var ( ErrInvalidPath = errors.New("invalid path") ErrPathValidation = errors.New("path validation failed") ErrOverlappingPath = errors.New("overlapping path detected") + ErrJobFailure = errors.New("one or more jobs failed") ) // Config represents the overall backup configuration. @@ -37,7 +38,7 @@ func (cfg Config) String() string { return string(out) } -func (cfg Config) Apply(rsync JobCommand, logger *log.Logger, output io.Writer) { +func (cfg Config) Apply(rsync JobCommand, logger *log.Logger, output io.Writer) error { versionInfo, fullpath, err := rsync.GetVersionInfo() if err != nil { logger.Printf("Failed to fetch rsync version: %v", err) @@ -46,11 +47,32 @@ func (cfg Config) Apply(rsync JobCommand, logger *log.Logger, output io.Writer) logger.Printf("Rsync Version Info: %s", versionInfo) } + var succeeded, failed, skipped int + for _, job := range cfg.Jobs { status := job.Apply(rsync) logger.Printf("STATUS [%s]: %s", job.Name, status) fmt.Fprintf(output, "Status [%s]: %s\n", job.Name, status) + + switch status { + case Success: + succeeded++ + case Failure: + failed++ + case Skipped: + skipped++ + } + } + + summary := fmt.Sprintf("Summary: %d succeeded, %d failed, %d skipped", succeeded, failed, skipped) + logger.Print(summary) + fmt.Fprintln(output, summary) + + if failed > 0 { + return fmt.Errorf("%w: %d of %d jobs", ErrJobFailure, failed, len(cfg.Jobs)) } + + return nil } func LoadConfig(reader io.Reader) (Config, error) { diff --git a/backup/internal/test/config_test.go b/backup/internal/test/config_test.go index a0f5cd0..52b2821 100644 --- a/backup/internal/test/config_test.go +++ b/backup/internal/test/config_test.go @@ -492,14 +492,16 @@ func TestConfigApply_VersionInfoSuccess(t *testing.T) { mockCmd.EXPECT().GetVersionInfo().Return("rsync version 3.2.3", "/usr/bin/rsync", nil).Once() mockCmd.EXPECT().Run(mock.AnythingOfType("internal.Job")).Return(Success).Once() - cfg.Apply(mockCmd, logger, &output) + err := cfg.Apply(mockCmd, logger, &output) + require.NoError(t, err) assert.Contains(t, logBuf.String(), "Rsync Binary Path: /usr/bin/rsync") assert.Contains(t, logBuf.String(), "Rsync Version Info: rsync version 3.2.3") assert.Contains(t, logBuf.String(), "STATUS [job1]: SUCCESS") assert.Contains(t, logBuf.String(), "STATUS [job2]: SKIPPED") assert.Contains(t, output.String(), "Status [job1]: SUCCESS") assert.Contains(t, output.String(), "Status [job2]: SKIPPED") + assert.Contains(t, output.String(), "Summary: 1 succeeded, 0 failed, 1 skipped") } func TestConfigApply_VersionInfoError(t *testing.T) { @@ -520,10 +522,13 @@ func TestConfigApply_VersionInfoError(t *testing.T) { mockCmd.EXPECT().GetVersionInfo().Return("", "", errCommandNotFound).Once() mockCmd.EXPECT().Run(mock.AnythingOfType("internal.Job")).Return(Failure).Once() - cfg.Apply(mockCmd, logger, &output) + err := cfg.Apply(mockCmd, logger, &output) + require.Error(t, err) + require.ErrorIs(t, err, ErrJobFailure) assert.Contains(t, logBuf.String(), "Failed to fetch rsync version: command not found") assert.NotContains(t, logBuf.String(), "Rsync Binary Path") assert.Contains(t, logBuf.String(), "STATUS [backup]: FAILURE") assert.Contains(t, output.String(), "Status [backup]: FAILURE") + assert.Contains(t, output.String(), "Summary: 0 succeeded, 1 failed, 0 skipped") } From 6064cd35414c61b509356603ea1fca939a555fd7 Mon Sep 17 00:00:00 2001 From: Jaap de Haan <261428+jdehaan@users.noreply.github.com> Date: Fri, 20 Mar 2026 12:13:27 +0000 Subject: [PATCH 24/25] docs: Update test documentation and add godoc comments --- MOCKERY_INTEGRATION.md | 149 ++++++++------------ TESTING_GUIDE.md | 219 +++++++++++++++++------------- backup/cmd/root.go | 3 + backup/internal/exec.go | 1 + backup/internal/job_command.go | 5 + backup/internal/rsync.go | 3 + backup/internal/rsync_list.go | 2 + backup/internal/rsync_simulate.go | 2 + backup/internal/rsync_sync.go | 2 + 9 files changed, 199 insertions(+), 187 deletions(-) diff --git a/MOCKERY_INTEGRATION.md b/MOCKERY_INTEGRATION.md index 8e6e21b..2ef7cd9 100644 --- a/MOCKERY_INTEGRATION.md +++ b/MOCKERY_INTEGRATION.md @@ -1,18 +1,16 @@ # Mockery Integration Guide -This document explains how mockery has been integrated into this Go project for generating mocks from interfaces. +This document explains how mockery is integrated for generating mocks from interfaces. ## Installation -Mockery v3.6.1 has been installed in the project: - ```bash -go install github.com/vektra/mockery/v3@v3.6.1 +go install github.com/vektra/mockery/v3@latest ``` ## Configuration -The project uses a `.mockery.yml` configuration file to control mock generation: +The project uses `.mockery.yml` to control mock generation: ```yaml all: false @@ -34,116 +32,83 @@ packages: JobCommand: ``` -Key configuration points: -- **Filename pattern**: `mock_{{.InterfaceName | lower}}_test.go` for standard naming -- **Struct naming**: `Mock{{.InterfaceName}}` for standard mock naming -- **Package**: `internal_test` to match the test package -- **Template**: Uses the `testify` template for rich mock functionality +Key points: +- **Output directory**: `/test/` (alongside other test files) +- **Filename**: `mock__test.go` +- **Struct naming**: `Mock` (e.g., `MockExec`, `MockJobCommand`) +- **Package**: `internal_test` (external test package) +- **Template**: `testify` for expectation-based mocking ## Generated Mocks -Running `mockery` generates the following mock files: - -- `backup/internal/test/mock_exec_test.go` - Mock for the `Exec` interface -- `backup/internal/test/mock_jobcommand_test.go` - Mock for the `JobCommand` interface - -These mocks provide: -- **Type-safe mocking** with compile-time verification -- **Expectation-based testing** with automatic verification -- **Fluent interface** for setting up complex expectations -- **Automatic cleanup** via `t.Cleanup()` +| Mock | Source Interface | File | +|---|---|---| +| `MockExec` | `Exec` | `backup/internal/test/mock_exec_test.go` | +| `MockJobCommand` | `JobCommand` | `backup/internal/test/mock_jobcommand_test.go` | ## Usage Examples -### Basic Usage +### MockJobCommand — Testing Config.Apply ```go -func TestJobApply_WithMockeryJobCommand_Success(t *testing.T) { - // Create mock using mockery's generated constructor - mockJobCommand := NewMockJobCommand(t) - - // Create an enabled job - enabledJob := NewJob( - WithName("success_job"), - WithSource("/home/success/"), - WithTarget("/mnt/backup1/success/"), - WithEnabled(true), - ) - - // Set expectation that Run will be called and return Success - mockJobCommand.EXPECT().Run(mock.MatchedBy(func(job Job) bool { - return job.Name == "success_job" && - job.Source == "/home/success/" && - job.Target == "/mnt/backup1/success/" && - job.Enabled == true - })).Return(Success).Once() - - // Apply the job - status := enabledJob.Apply(mockJobCommand) - - // Assert that the status is Success - assert.Equal(t, Success, status) - - // mockery automatically verifies expectations via t.Cleanup() +func TestConfigApply_VersionInfoSuccess(t *testing.T) { + mockCmd := NewMockJobCommand(t) + var output bytes.Buffer + logger := log.New(&bytes.Buffer{}, "", 0) + + cfg := Config{ + Jobs: []Job{ + {Name: "job1", Source: "/src/", Target: "/dst/", Enabled: true}, + {Name: "job2", Source: "/src2/", Target: "/dst2/", Enabled: false}, + }, + } + + mockCmd.EXPECT().GetVersionInfo().Return("rsync version 3.2.3", "/usr/bin/rsync", nil).Once() + mockCmd.EXPECT().Run(mock.AnythingOfType("internal.Job")).Return(Success).Once() + + err := cfg.Apply(mockCmd, logger, &output) + require.NoError(t, err) + assert.Contains(t, output.String(), "Status [job1]: SUCCESS") } ``` -### Testing Disabled Jobs +### MockExec — Testing Command Execution ```go -func TestJobApply_WithMockeryJobCommand_DisabledJob(t *testing.T) { - // Create mock using mockery's generated constructor - mockJobCommand := NewMockJobCommand(t) - - // Create a disabled job - disabledJob := NewJob( - WithName("disabled_job"), - WithEnabled(false), - ) - - // No expectations set - the Run method should NOT be called for disabled jobs - - // Apply the job - status := disabledJob.Apply(mockJobCommand) - - // Assert that the status is Skipped - assert.Equal(t, Skipped, status) - - // The mock will automatically verify that Run was NOT called -} -``` +func TestSyncCommand_Run_Success(t *testing.T) { + mockExec := NewMockExec(t) + var output bytes.Buffer -## Replacement of Simple Mocks + cmd := NewSyncCommand("/usr/bin/rsync", "/tmp/logs", mockExec, &output) + job := Job{Name: "docs", Source: "/src/", Target: "/dst/", Enabled: true, Delete: true} -The project has fully migrated from simple mocks to mockery-generated mocks: + mockExec.EXPECT().Execute("/usr/bin/rsync", mock.Anything).Return([]byte("done"), nil).Once() -- **Old simple mocks**: Removed (used field-based APIs like `CapturedCommands`, `Output`, `Error`) -- **New mockery mocks** (`MockExec`, `MockJobCommand`): Used for all testing scenarios with rich expectations and automatic verification + status := cmd.Run(job) + assert.Equal(t, Success, status) +} +``` -## Benefits of Mockery Integration +### Testing Disabled Jobs (no mock expectations needed) -1. **Type Safety**: Compile-time verification of mock method signatures -2. **Rich Expectations**: Support for complex argument matching and call counting -3. **Automatic Verification**: Expectations are automatically verified at test completion -4. **Fluent Interface**: Easy-to-read test setup with method chaining -5. **Maintenance**: Mocks stay in sync with interface changes automatically +```go +func TestJobApply_DisabledJob(t *testing.T) { + mockCmd := NewMockJobCommand(t) + disabledJob := Job{Name: "skip_me", Enabled: false} + + // No expectations set — Run should NOT be called + status := disabledJob.Apply(mockCmd) + assert.Equal(t, Skipped, status) + // MockJobCommand automatically verifies Run was not called +} +``` ## Regenerating Mocks -When interfaces change, regenerate mocks with: +When interfaces change, regenerate with: ```bash mockery ``` -This will update all configured mocks according to the `.mockery.yml` configuration. - -## Dependencies - -The project now includes the testify mock dependency: - -```go -github.com/stretchr/testify/mock v1.11.1 -``` - -This enables the full mockery feature set including expectations, matchers, and automatic verification. \ No newline at end of file +This updates all mocks according to `.mockery.yml`. Generated files are committed to the repository. \ No newline at end of file diff --git a/TESTING_GUIDE.md b/TESTING_GUIDE.md index f1388be..1758263 100644 --- a/TESTING_GUIDE.md +++ b/TESTING_GUIDE.md @@ -1,132 +1,161 @@ # Testing Guide -## Better Go Testing Practices +## Overview -Instead of using `init()` functions to set up mocks, this codebase now uses dependency injection with interfaces for better testability. +All tests use dependency injection — no global state mutation. Key patterns: -### The Old Way (Problematic) +- **`testify`** for assertions (`require` for fatal checks, `assert` for non-fatal) +- **`mockery`** for generated mocks (`MockExec`, `MockJobCommand`) +- **`afero`** for in-memory filesystem abstraction +- **`bytes.Buffer`** for capturing output +- **Table-driven tests** for multiple input scenarios +- Test files live in `/test/` subdirectories -```go -// DON'T DO THIS - Global state mutation in init() -var mockExecCommand = func(name string, args ...string) *exec.Cmd { - // mock implementation -} +## Test Architecture -func init() { - internal.ExecCommand = mockExecCommand // Mutates global state -} +``` +backup/ + cmd/test/ + commands_test.go # CLI integration tests (all commands) + root_test.go # Root command help output + internal/test/ + check_test.go # CoverageChecker tests (afero-based) + config_test.go # Config loading, validation, Apply + helper_test.go # NormalizePath, CreateMainLogger + job_test.go # Job.Apply with MockJobCommand + rsync_test.go # Command constructors, Run methods, GetVersionInfo + mock_exec_test.go # Generated mock for Exec interface + mock_jobcommand_test.go # Generated mock for JobCommand interface ``` -**Problems with this approach:** -- Global state mutation affects all tests -- Tests can interfere with each other -- Difficult to reset between tests -- Not explicit about what's being mocked -- Makes parallel testing unsafe +## Dependency Injection Points -### The New Way (Recommended) +| Dependency | Interface/Type | Real | Test | +|---|---|---|---| +| Command execution | `internal.Exec` | `OsExec` | `MockExec` or `stubExec` | +| Job runner | `internal.JobCommand` | `ListCommand`, `SyncCommand`, `SimulateCommand` | `MockJobCommand` | +| Filesystem | `afero.Fs` | `afero.NewOsFs()` | `afero.NewMemMapFs()` | +| Output | `io.Writer` | `os.Stdout` / `cmd.OutOrStdout()` | `bytes.Buffer` | +| Logging | `*log.Logger` | File-backed logger | `log.New(&buf, "", 0)` | +| Time | `time.Time` | `time.Now()` | Fixed `time.Date(...)` | + +## Command-Level Tests (cmd/test/) + +Commands are tested through cobra's `Execute()` with captured stdout: ```go -// Define interface for testability -type JobRunner interface { - Execute(name string, args ...string) ([]byte, error) +// Stub for Exec interface — lightweight alternative to MockExec for cmd tests +type stubExec struct { + output []byte + err error } -// Real implementation -type RealSync struct{} -func (r *RealSync) Execute(name string, args ...string) ([]byte, error) { - cmd := exec.Command(name, args...) - return cmd.CombinedOutput() +func (s *stubExec) Execute(_ string, _ ...string) ([]byte, error) { + return s.output, s.err } -// Test implementation -type MockCommandExecutor struct { - CapturedCommands []MockCommand +// Helper: run a command with full dependency injection +func executeCommandWithDeps(t *testing.T, fs afero.Fs, shell internal.Exec, args ...string) (string, error) { + t.Helper() + rootCmd := cmd.BuildRootCommandWithDeps(fs, shell) + var stdout bytes.Buffer + rootCmd.SetOut(&stdout) + rootCmd.SetErr(&bytes.Buffer{}) + rootCmd.SetArgs(args) + err := rootCmd.Execute() + return stdout.String(), err } -func (m *MockCommandExecutor) Execute(name string, args ...string) ([]byte, error) { - // Mock logic here +``` + +Usage: + +```go +func TestRun_ValidConfig(t *testing.T) { + cfgPath := writeConfigFile(t, `...yaml...`) + shell := &stubExec{output: []byte("rsync version 3.2.7 protocol version 31\n")} + + stdout, err := executeCommandWithDeps(t, afero.NewMemMapFs(), shell, "run", "--config", cfgPath) + + require.NoError(t, err) + assert.Contains(t, stdout, "Job: docs") + assert.Contains(t, stdout, "Status [docs]: SUCCESS") } ``` -### Testing Examples +Three builder levels available: +- `BuildRootCommand()` — production defaults (real OS filesystem, real exec) +- `BuildRootCommandWithFs(fs)` — custom filesystem, real exec +- `BuildRootCommandWithDeps(fs, shell)` — full control for testing + +## Internal Tests — Mockery Mocks -#### 1. Simple Test with Mock +Generated mocks use the expectation pattern: ```go -func TestExecuteJob(t *testing.T) { - // Create mock executor for this test only - mockExecutor := &MockCommandExecutor{} - - job := internal.Job{ - Name: "test_job", - Source: "/home/test/", - Target: "/mnt/backup1/test/", - Enabled: true, +func TestConfigApply_VersionInfoSuccess(t *testing.T) { + mockCmd := NewMockJobCommand(t) + var output bytes.Buffer + logger := log.New(&bytes.Buffer{}, "", 0) + + cfg := Config{ + Jobs: []Job{ + {Name: "job1", Source: "/src/", Target: "/dst/", Enabled: true}, + }, } - - // Use the executor-aware function - status := internal.ExecuteJobWithExecutor(job, true, false, "", mockExecutor) - - if status != "SUCCESS" { - t.Errorf("Expected SUCCESS, got %s", status) - } - - // Verify the mock was called correctly - if len(mockExecutor.CapturedCommands) == 0 { - t.Error("Expected command to be executed") + + mockCmd.EXPECT().GetVersionInfo().Return("rsync version 3.2.3", "/usr/bin/rsync", nil).Once() + mockCmd.EXPECT().Run(mock.AnythingOfType("internal.Job")).Return(Success).Once() + + err := cfg.Apply(mockCmd, logger, &output) + require.NoError(t, err) +} +``` + +## CoverageChecker Tests (afero) + +The `CoverageChecker` uses `afero.Fs` so tests never hit the real filesystem: + +```go +func newTestChecker() (*CoverageChecker, *bytes.Buffer) { + var buf bytes.Buffer + checker := &CoverageChecker{ + Logger: log.New(&buf, "", 0), + Fs: afero.NewMemMapFs(), } + return checker, &buf } ``` -#### 2. Test Setup with t.Cleanup() (Alternative Pattern) +## Deterministic Time + +`CreateMainLogger` accepts `time.Time` for predictable log paths: ```go -func setupMockExecutor(t *testing.T) *MockCommandExecutor { - t.Helper() - - // Store original to restore later - originalExecutor := internal.DefaultExecutor - - // Create mock - mockExecutor := &MockCommandExecutor{} - - // Set mock globally - internal.DefaultExecutor = mockExecutor - - // Restore original after test - t.Cleanup(func() { - internal.DefaultExecutor = originalExecutor - }) - - return mockExecutor +func fixedTime() time.Time { + return time.Date(2025, 6, 15, 14, 30, 45, 0, time.UTC) } -func TestWithSetup(t *testing.T) { - mock := setupMockExecutor(t) - - // Use regular ExecuteJob function - it uses DefaultExecutor - status := internal.ExecuteJob(job, true, false, "") - - // Verify mock was used - assert.Equal(t, 1, len(mock.CapturedCommands)) +func TestCreateMainLogger_DeterministicLogPath(t *testing.T) { + _, logPath, cleanup, err := CreateMainLogger("backup.yaml", true, fixedTime()) + require.NoError(t, err) + defer cleanup() + assert.Equal(t, "logs/sync-2025-06-15T14-30-45-backup-sim", logPath) } ``` -### Benefits of the New Approach - -1. **Test Isolation**: Each test gets its own mock instance -2. **Explicit Dependencies**: Clear what's being mocked in each test -3. **Better Assertions**: Can inspect captured calls, arguments, etc. -4. **Parallel Safe**: Tests don't interfere with each other -5. **Easier Debugging**: Clearer test failures and state -6. **Production Safety**: Real code unchanged, only test behavior modified +## Running Tests -### Key Principles +```sh +make test # go test -race ./... -v +make check-coverage # Fail if coverage < 90% +make report-coverage # Generate HTML coverage report +``` -1. **Use interfaces for external dependencies** (file system, network, exec, etc.) -2. **Inject dependencies rather than using globals** -3. **Keep mocks scoped to individual tests** -4. **Use `t.Cleanup()` for setup/teardown patterns** -5. **Make test intentions explicit in the test code** +## Key Principles -This approach follows Go testing best practices and makes the codebase more maintainable and reliable. \ No newline at end of file +1. **Inject, don't hardcode** — all external dependencies go through interfaces +2. **Never hit the real filesystem** in unit tests — use `afero.NewMemMapFs()` +3. **Use `require` for errors, `assert` for values** — `require` stops the test on failure +4. **Table-driven tests** for multiple input/output scenarios +5. **Scope mocks to individual tests** — each test creates its own mock instance +6. **Defer cleanup** — `CreateMainLogger` returns a cleanup function; always `defer` it \ No newline at end of file diff --git a/backup/cmd/root.go b/backup/cmd/root.go index 18d5b90..3bb1dcb 100644 --- a/backup/cmd/root.go +++ b/backup/cmd/root.go @@ -7,14 +7,17 @@ import ( "github.com/spf13/cobra" ) +// BuildRootCommand creates the root cobra command with production defaults. func BuildRootCommand() *cobra.Command { return BuildRootCommandWithDeps(afero.NewOsFs(), &internal.OsExec{}) } +// BuildRootCommandWithFs creates the root command with a custom filesystem. func BuildRootCommandWithFs(fs afero.Fs) *cobra.Command { return BuildRootCommandWithDeps(fs, &internal.OsExec{}) } +// BuildRootCommandWithDeps creates the root command with full dependency injection. func BuildRootCommandWithDeps(fs afero.Fs, shell internal.Exec) *cobra.Command { rootCmd := &cobra.Command{ Use: "backup", diff --git a/backup/internal/exec.go b/backup/internal/exec.go index e72c984..3f94f50 100644 --- a/backup/internal/exec.go +++ b/backup/internal/exec.go @@ -7,6 +7,7 @@ import ( "strings" ) +// Exec abstracts command execution for testability. type Exec interface { Execute(name string, args ...string) ([]byte, error) } diff --git a/backup/internal/job_command.go b/backup/internal/job_command.go index f7c772f..80e7be7 100644 --- a/backup/internal/job_command.go +++ b/backup/internal/job_command.go @@ -1,13 +1,18 @@ package internal +// JobStatus represents the outcome of a job execution. type JobStatus string const ( + // Success indicates the job completed successfully. Success JobStatus = "SUCCESS" + // Failure indicates the job failed. Failure JobStatus = "FAILURE" + // Skipped indicates the job was skipped (e.g., disabled). Skipped JobStatus = "SKIPPED" ) +// JobCommand defines the interface for running backup jobs. type JobCommand interface { Run(job Job) JobStatus GetVersionInfo() (string, string, error) diff --git a/backup/internal/rsync.go b/backup/internal/rsync.go index 39298a2..73d3b43 100644 --- a/backup/internal/rsync.go +++ b/backup/internal/rsync.go @@ -14,6 +14,7 @@ var ErrInvalidRsyncPath = errors.New("rsync path must be an absolute path") const RsyncVersionFlag = "--version" +// SharedCommand holds common state for all rsync command types. type SharedCommand struct { BinPath string BaseLogPath string @@ -22,6 +23,7 @@ type SharedCommand struct { Output io.Writer } +// NewSharedCommand creates a SharedCommand with the given dependencies. func NewSharedCommand(binPath string, logPath string, shell Exec, output io.Writer) SharedCommand { return SharedCommand{ BinPath: binPath, @@ -93,6 +95,7 @@ func (c SharedCommand) GetVersionInfo() (string, string, error) { return string(output), rsyncPath, nil } +// ArgumentsForJob builds the rsync argument list for a given job. func ArgumentsForJob(job Job, logPath string, simulate bool) []string { args := []string{"-aiv", "--stats"} diff --git a/backup/internal/rsync_list.go b/backup/internal/rsync_list.go index 018e2f8..23b39fe 100644 --- a/backup/internal/rsync_list.go +++ b/backup/internal/rsync_list.go @@ -2,10 +2,12 @@ package internal import "io" +// ListCommand prints the rsync commands that would be executed without running them. type ListCommand struct { SharedCommand } +// NewListCommand creates a ListCommand with the given dependencies. func NewListCommand(binPath string, shell Exec, output io.Writer) ListCommand { return ListCommand{ SharedCommand: NewSharedCommand(binPath, "", shell, output), diff --git a/backup/internal/rsync_simulate.go b/backup/internal/rsync_simulate.go index 1c5a8be..aa9319d 100644 --- a/backup/internal/rsync_simulate.go +++ b/backup/internal/rsync_simulate.go @@ -2,10 +2,12 @@ package internal import "io" +// SimulateCommand runs rsync in dry-run mode and captures output. type SimulateCommand struct { SharedCommand } +// NewSimulateCommand creates a SimulateCommand with the given dependencies. func NewSimulateCommand(binPath string, logPath string, shell Exec, output io.Writer) SimulateCommand { return SimulateCommand{ SharedCommand: NewSharedCommand(binPath, logPath, shell, output), diff --git a/backup/internal/rsync_sync.go b/backup/internal/rsync_sync.go index 2f32b7a..fb1b1f5 100644 --- a/backup/internal/rsync_sync.go +++ b/backup/internal/rsync_sync.go @@ -2,10 +2,12 @@ package internal import "io" +// SyncCommand runs rsync to perform the actual backup. type SyncCommand struct { SharedCommand } +// NewSyncCommand creates a SyncCommand with the given dependencies. func NewSyncCommand(binPath string, logPath string, shell Exec, output io.Writer) SyncCommand { return SyncCommand{ SharedCommand: NewSharedCommand(binPath, logPath, shell, output), From 80fae252c8a3b2beacbd408f852b2921945beeed Mon Sep 17 00:00:00 2001 From: Jaap de Haan <261428+jdehaan@users.noreply.github.com> Date: Fri, 20 Mar 2026 12:35:08 +0000 Subject: [PATCH 25/25] fix: Increase test coverage --- Makefile | 2 +- backup/internal/test/check_test.go | 54 +++++++++++++++++++++++++++++ backup/internal/test/config_test.go | 44 +++++++++++++++++++++++ backup/internal/test/helper_test.go | 34 ++++++++++++++++++ backup/internal/test/job_test.go | 17 +++++++++ 5 files changed, 150 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 2cab072..b4b88c2 100644 --- a/Makefile +++ b/Makefile @@ -3,7 +3,7 @@ # Build command with common flags BUILD_CMD = CGO_ENABLED=0 go build -trimpath -ldflags="-s -w" -tags=prod PACKAGE = ./backup/main.go -COVERAGE_THRESHOLD = 90 +COVERAGE_THRESHOLD = 98 .PHONY: build clean test lint tidy checksums release sanity-check check-mod-tidy lint-config-check lint-fix format check-clean check-coverage diff --git a/backup/internal/test/check_test.go b/backup/internal/test/check_test.go index db49cae..236cee6 100644 --- a/backup/internal/test/check_test.go +++ b/backup/internal/test/check_test.go @@ -281,3 +281,57 @@ func TestListUncoveredPaths_UnreadableDirectory(t *testing.T) { assert.Equal(t, []string{"/data"}, result) assert.Contains(t, logBuf.String(), "ADD: Path '/data' is uncovered") } + +// Test that a child path matching a job exclusion is marked as excluded +// (covers isExcluded true + isCoveredByJob excluded log). +func TestListUncoveredPaths_ChildPathExcludedByJob(t *testing.T) { + fs := afero.NewMemMapFs() + _ = fs.MkdirAll("/data/stuff/docs", 0755) + _ = fs.MkdirAll("/data/stuff/cache", 0755) + + var logBuf bytes.Buffer + + checker := newTestChecker(fs, &logBuf) + + cfg := Config{ + Sources: []Path{ + {Path: "/data/stuff"}, + }, + Jobs: []Job{ + // Source "/data" with exclusion "stuff/cache" so exclusionPath = "/data/stuff/cache" + {Name: "data-backup", Source: "/data", Exclusions: []string{"stuff/cache"}}, + // Covers the /data/stuff/docs child directly + {Name: "docs", Source: "/data/stuff/docs"}, + }, + } + + result := checker.ListUncoveredPaths(cfg) + + assert.Empty(t, result) + assert.Contains(t, logBuf.String(), "EXCLUDED: Path '/data/stuff/cache' is excluded by job 'data-backup'") +} + +// Test that a source path that is globally excluded is skipped in checkPath. +func TestListUncoveredPaths_GloballyExcludedSourceSkipped(t *testing.T) { + fs := afero.NewMemMapFs() + _ = fs.MkdirAll("/data/cache", 0755) + + var logBuf bytes.Buffer + + checker := newTestChecker(fs, &logBuf) + + cfg := Config{ + Sources: []Path{ + {Path: "/data", Exclusions: []string{"cache"}}, + {Path: "/data/cache"}, + }, + Jobs: []Job{ + {Name: "backup", Source: "/data"}, + }, + } + + result := checker.ListUncoveredPaths(cfg) + + assert.Empty(t, result) + assert.Contains(t, logBuf.String(), "SKIP: Path '/data/cache' is globally excluded") +} diff --git a/backup/internal/test/config_test.go b/backup/internal/test/config_test.go index 52b2821..8556a96 100644 --- a/backup/internal/test/config_test.go +++ b/backup/internal/test/config_test.go @@ -452,6 +452,50 @@ jobs: assert.Contains(t, err.Error(), "job source path validation failed") } +func TestLoadResolvedConfig_OverlappingSourcePathsAllowedByExclusion(t *testing.T) { + yaml := ` +sources: + - path: "/home" +targets: + - path: "/backup" +jobs: + - name: "parent" + source: "/home/user" + target: "/backup/user" + exclusions: + - "docs" + - name: "child" + source: "/home/user/docs" + target: "/backup/docs" +` + path := writeTestConfig(t, yaml) + + cfg, err := LoadResolvedConfig(path) + require.NoError(t, err) + assert.Len(t, cfg.Jobs, 2) +} + +func TestLoadResolvedConfig_OverlappingTargetPaths(t *testing.T) { + yaml := ` +sources: + - path: "/home" +targets: + - path: "/backup" +jobs: + - name: "job1" + source: "/home/docs" + target: "/backup/all" + - name: "job2" + source: "/home/photos" + target: "/backup/all/photos" +` + path := writeTestConfig(t, yaml) + + _, err := LoadResolvedConfig(path) + require.Error(t, err) + assert.Contains(t, err.Error(), "job target path validation failed") +} + func TestLoadResolvedConfig_ValidConfig(t *testing.T) { yaml := ` sources: diff --git a/backup/internal/test/helper_test.go b/backup/internal/test/helper_test.go index d450003..67dd58b 100644 --- a/backup/internal/test/helper_test.go +++ b/backup/internal/test/helper_test.go @@ -1,6 +1,7 @@ package internal_test import ( + "os" "testing" "time" @@ -78,3 +79,36 @@ func TestCreateMainLogger_DeterministicLogPath_NoSimulate(t *testing.T) { assert.Equal(t, "logs/sync-2025-06-15T14-30-45-sync", logPath) } + +func TestCreateMainLogger_MkdirError(t *testing.T) { + // Use t.Chdir to a temp dir so we control the filesystem + tmpDir := t.TempDir() + t.Chdir(tmpDir) + + // Create "logs" as a regular file to block MkdirAll + err := os.WriteFile("logs", []byte("block"), 0600) + require.NoError(t, err) + + _, _, cleanup, err := CreateMainLogger("test.yaml", false, fixedTime()) + _ = cleanup + + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to create log directory") +} + +func TestCreateMainLogger_OpenFileError(t *testing.T) { + tmpDir := t.TempDir() + t.Chdir(tmpDir) + + // Pre-create the log path directory and make summary.log a directory to block OpenFile + logDir := "logs/sync-2025-06-15T14-30-45-test" + + err := os.MkdirAll(logDir+"/summary.log", 0750) + require.NoError(t, err) + + _, _, cleanup, err := CreateMainLogger("test.yaml", false, fixedTime()) + _ = cleanup + + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to open overall log file") +} diff --git a/backup/internal/test/job_test.go b/backup/internal/test/job_test.go index fe0b759..3186f81 100644 --- a/backup/internal/test/job_test.go +++ b/backup/internal/test/job_test.go @@ -5,6 +5,8 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gopkg.in/yaml.v3" ) func newJob() Job { @@ -56,3 +58,18 @@ func TestApply_JobSucceeds_RunIsCalledAndReturnsSuccess(t *testing.T) { assert.Equal(t, Success, status) } + +func TestUnmarshalYAML_InvalidNode(t *testing.T) { + // A scalar node cannot be decoded into the JobYAML struct + node := &yaml.Node{ + Kind: yaml.ScalarNode, + Value: "not a mapping", + } + + var job Job + + err := node.Decode(&job) + + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to decode YAML node") +}