[testify-expert] Improve Test Quality: pkg/fileutil/fileutil_test.go

[github-actions]

The test file pkg/fileutil/fileutil_test.go has been selected for quality improvement by the Testify Uber Super Expert. This issue provides specific, actionable recommendations to enhance test quality, coverage, and maintainability.

Current State

• Test File: pkg/fileutil/fileutil_test.go
• Source Files: pkg/fileutil/fileutil.go, pkg/fileutil/tar.go
• Test Functions: 3 test functions
• Lines of Code: 178 lines
• Exported Functions in Source: 6 (ValidateAbsolutePath, FileExists, DirExists, IsDirEmpty, CopyFile, ExtractFileFromTar)

Test Quality Analysis

Strengths ✅

1. Good use of testify — Correctly uses require.* for critical assertions (error checks) and assert.* for validations, matching the codebase's established pattern.
2. Well-structured table-driven tests — TestValidateAbsolutePath and TestValidateAbsolutePath_Cleaning use t.Run() with descriptive names and meaningful test case structs.
3. Helpful assertion messages — Most assertions include context messages (e.g., "Should not error for valid absolute path: %s").
4. Security-focused testing — TestValidateAbsolutePath_SecurityScenarios covers multiple path traversal attack patterns.

🎯 Areas for Improvement

1. Missing Test Coverage — 5 of 6 Exported Functions Untested

The test file only tests ValidateAbsolutePath but the package exports 5 additional functions with zero test coverage.

Untested Functions:

Function	File	Priority
FileExists(path string) bool	fileutil.go	High
DirExists(path string) bool	fileutil.go	High
IsDirEmpty(path string) bool	fileutil.go	Medium
CopyFile(src, dst string) error	fileutil.go	High
ExtractFileFromTar(data []byte, path string) ([]byte, error)	tar.go	High

Recommended new tests:

func TestFileExists(t *testing.T) {
    t.Run("returns true for existing file", func(t *testing.T) {
        f, err := os.CreateTemp(t.TempDir(), "test-*.txt")
        require.NoError(t, err, "should create temp file")
        require.NoError(t, f.Close())

        assert.True(t, FileExists(f.Name()), "should return true for existing file")
    })

    t.Run("returns false for non-existent file", func(t *testing.T) {
        assert.False(t, FileExists("/nonexistent/path/file.txt"), "should return false for missing file")
    })

    t.Run("returns false for directory", func(t *testing.T) {
        dir := t.TempDir()
        assert.False(t, FileExists(dir), "should return false for directory path")
    })
}

func TestDirExists(t *testing.T) {
    t.Run("returns true for existing directory", func(t *testing.T) {
        dir := t.TempDir()
        assert.True(t, DirExists(dir), "should return true for existing directory")
    })

    t.Run("returns false for non-existent directory", func(t *testing.T) {
        assert.False(t, DirExists("/nonexistent/path/dir"), "should return false for missing directory")
    })

    t.Run("returns false for file", func(t *testing.T) {
        f, err := os.CreateTemp(t.TempDir(), "test-*.txt")
        require.NoError(t, err, "should create temp file")
        require.NoError(t, f.Close())

        assert.False(t, DirExists(f.Name()), "should return false for file path")
    })
}

func TestIsDirEmpty(t *testing.T) {
    t.Run("returns true for empty directory", func(t *testing.T) {
        dir := t.TempDir()
        assert.True(t, IsDirEmpty(dir), "should return true for empty directory")
    })

    t.Run("returns false for non-empty directory", func(t *testing.T) {
        dir := t.TempDir()
        f, err := os.CreateTemp(dir, "test-*.txt")
        require.NoError(t, err, "should create temp file")
        require.NoError(t, f.Close())

        assert.False(t, IsDirEmpty(dir), "should return false for non-empty directory")
    })

    t.Run("returns true for non-existent path", func(t *testing.T) {
        // Documents the current behavior: returns true (empty) when path can't be read
        assert.True(t, IsDirEmpty("/nonexistent/path"), "should return true for unreadable path")
    })
}

func TestCopyFile(t *testing.T) {
    t.Run("copies file content correctly", func(t *testing.T) {
        dir := t.TempDir()
        src := filepath.Join(dir, "source.txt")
        dst := filepath.Join(dir, "dest.txt")
        content := []byte("test file content")

        require.NoError(t, os.WriteFile(src, content, 0o600), "should write source file")

        err := CopyFile(src, dst)
        require.NoError(t, err, "should copy file without error")

        got, err := os.ReadFile(dst)
        require.NoError(t, err, "should read destination file")
        assert.Equal(t, content, got, "destination should have same content as source")
    })

    t.Run("fails when source does not exist", func(t *testing.T) {
        dir := t.TempDir()
        err := CopyFile(filepath.Join(dir, "nonexistent.txt"), filepath.Join(dir, "dst.txt"))
        require.Error(t, err, "should fail when source file does not exist")
    })

    t.Run("fails when destination directory does not exist", func(t *testing.T) {
        dir := t.TempDir()
        src := filepath.Join(dir, "source.txt")
        require.NoError(t, os.WriteFile(src, []byte("data"), 0o600))

        err := CopyFile(src, filepath.Join(dir, "nonexistent", "dst.txt"))
        require.Error(t, err, "should fail when destination directory does not exist")
    })
}

2. Missing Tests for ExtractFileFromTar

tar.go is completely untested. This function handles binary archive extraction and needs coverage of:

• Successful extraction of a known file
• File not found in archive
• Corrupted/invalid tar data

func TestExtractFileFromTar(t *testing.T) {
    // Helper to build a tar archive in memory
    makeTar := func(t *testing.T, files map[string]string) []byte {
        t.Helper()
        var buf bytes.Buffer
        tw := tar.NewWriter(&buf)
        for name, content := range files {
            hdr := &tar.Header{Name: name, Mode: 0o600, Size: int64(len(content))}
            require.NoError(t, tw.WriteHeader(hdr), "should write tar header")
            _, err := tw.Write([]byte(content))
            require.NoError(t, err, "should write tar entry content")
        }
        require.NoError(t, tw.Close(), "should close tar writer")
        return buf.Bytes()
    }

    tests := []struct {
        name        string
        targetPath  string
        expected    string
        shouldError bool
        errContains string
    }{
        {
            name:       "extracts existing file",
            targetPath: "config.yaml",
            expected:   "key: value",
        },
        {
            name:        "returns error for missing file",
            targetPath:  "missing.txt",
            shouldError: true,
            errContains: "not found",
        },
    }

    archive := makeTar(t, map[string]string{
        "config.yaml": "key: value",
        "README.md":   "# Hello",
    })

    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            got, err := ExtractFileFromTar(archive, tt.targetPath)

            if tt.shouldError {
                require.Error(t, err, "should return error for %q", tt.targetPath)
                if tt.errContains != "" {
                    assert.Contains(t, err.Error(), tt.errContains, "error message should contain expected text")
                }
            } else {
                require.NoError(t, err, "should extract file without error")
                assert.Equal(t, tt.expected, string(got), "extracted content should match")
            }
        })
    }

    t.Run("returns error for corrupted tar data", func(t *testing.T) {
        _, err := ExtractFileFromTar([]byte("not a tar archive"), "file.txt")
        require.Error(t, err, "should fail for invalid tar data")
    })
}

3. Minor: Windows-conditional tests in TestValidateAbsolutePath_Cleaning

The current test wraps the skip inside t.Run, meaning Windows test cases in the table still run but internally skip. This is slightly misleading — the table entry shows a run in output but does nothing on non-Windows. Prefer skipping at the test level:

// ✅ IMPROVED — Skip entire test on non-Windows systems, or use build tags
func TestValidateAbsolutePath_Cleaning_Unix(t *testing.T) {
    if runtime.GOOS == "windows" {
        t.Skip("Unix-specific path cleaning tests")
    }
    // ... test cases without the inner OS check ...
}

4. IsDirEmpty behavioral documentation

IsDirEmpty returns true when the path doesn't exist (because os.ReadDir fails). This is an implicit behavioral contract that isn't tested or documented. The recommended test above covers this, but consider also adding a godoc comment:

// IsDirEmpty checks if a directory is empty.
// Returns true if the directory is empty or if it cannot be read.
func IsDirEmpty(path string) bool {

📋 Implementation Guidelines

Priority Order

1. High: Add TestFileExists, TestDirExists, TestCopyFile — these test real filesystem functions critical to the codebase
2. High: Add TestExtractFileFromTar — tar extraction has zero coverage
3. Medium: Add TestIsDirEmpty including the "unreadable path" behavior documentation
4. Low: Refactor Windows-conditional test structure

Required Imports for New Tests

import (
    "archive/tar"
    "bytes"
    "os"
    "path/filepath"
    "runtime"
    "strings"
    "testing"

    "github.com/stretchr/testify/assert"
    "github.com/stretchr/testify/require"
)

Testing Commands

# Run tests for this package
go test -v ./pkg/fileutil/

# Run specific tests
go test -v -run "TestFileExists" ./pkg/fileutil/
go test -v -run "TestExtractFileFromTar" ./pkg/fileutil/

# Run all unit tests
make test-unit

Best Practices from scratchpad/testing.md

• ✅ Use t.TempDir() for temporary files — auto-cleaned after test
• ✅ Use require.* for setup (file creation, archive building)
• ✅ Use assert.* for behavioral validations
• ✅ Table-driven tests with t.Run() for function variants
• ✅ Always include helpful assertion messages

Acceptance Criteria

• FileExists has tests: existing file, non-existent path, directory path
• DirExists has tests: existing dir, non-existent path, file path
• IsDirEmpty has tests: empty dir, non-empty dir, unreadable path
• CopyFile has tests: successful copy, missing source, missing destination dir
• ExtractFileFromTar has tests: found file, file not found, corrupted archive
• All assertions include descriptive messages
• Tests use t.TempDir() for temporary filesystem operations
• Tests pass: go test -v ./pkg/fileutil/

Additional Context

• Repository Testing Guidelines: See scratchpad/testing.md for comprehensive testing patterns
• Example Tests: pkg/fileutil/fileutil_test.go is itself a good model for assertion style
• Testify Documentation: https://github.com/stretchr/testify

---

Priority: Medium
Effort: Small–Medium (adding ~150–200 lines of well-structured tests)
Expected Impact: Coverage jumps from ~17% (1/6 functions) to ~100% of exported functions

Files Involved:

• Test file: pkg/fileutil/fileutil_test.go
• Source files: pkg/fileutil/fileutil.go, pkg/fileutil/tar.go

References:

• §23151220193

Generated by Daily Testify Uber Super Expert · ◷

• expires on Mar 18, 2026, 3:30 PM UTC