[testify-expert] Improve Test Quality: pkg/parser/import_cache_test.go

[github-actions]

The test file pkg/parser/import_cache_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 using testify best practices.

Current State

• Test File: pkg/parser/import_cache_test.go
• Source File: pkg/parser/import_cache.go
• Test Functions: 4 test functions (TestImportCache, TestImportCacheDirectory, TestImportCacheMissingFile, TestImportCacheEmptyCache)
• Lines of Code: 161 lines (test), 175 lines (source)
• Exported Functions Tested: NewImportCache, Get, Set, GetCacheDir
• Internal Functions NOT Tested: sanitizePath, validatePathComponents, ensureGitAttributes

---

Strengths ✅

1. Good structure: Each test function focuses on a distinct scenario (set/get, directory creation, missing file, empty cache).
2. Proper cleanup: All tests use defer os.RemoveAll(tempDir) to clean up temp directories.
3. Build tag present: File correctly includes //go:build !integration at the top.

---

Areas for Improvement 🎯

1. Replace Raw Go Testing with Testify Assertions

The entire file uses raw t.Fatalf, t.Errorf, and t.Error instead of the testify library that is standard across this codebase.

Current anti-patterns throughout the file:

// ❌ CURRENT (anti-pattern) - raw error handling
if err != nil {
    t.Fatalf("Failed to create temp dir: %v", err)
}
if _, err := os.Stat(cachedPath); os.IsNotExist(err) {
    t.Errorf("Cache file was not created: %s", cachedPath)
}
if string(content) != string(testContent) {
    t.Errorf("Content mismatch. Expected %q, got %q", testContent, content)
}
if !found {
    t.Error("Cache entry not found after Set")
}

Recommended replacement:

// ✅ IMPROVED - testify assertions
require.NoError(t, err, "creating temp dir should succeed")
require.FileExists(t, cachedPath, "cache file should be created at expected path")
assert.Equal(t, string(testContent), string(content), "cached content should match original")
assert.True(t, found, "cache entry should be found after Set")

Why this matters: Testify provides clearer error messages, better test output, and is the standard used throughout this codebase (see scratchpad/testing.md).

Required import:

import (
    "testing"
    
    "github.com/stretchr/testify/assert"
    "github.com/stretchr/testify/require"
)

---

2. Table-Driven Tests for validatePathComponents and sanitizePath

Both sanitizePath and validatePathComponents are internal functions in the same package and are directly testable, yet have zero test coverage. These are security-critical functions (path traversal prevention) that deserve thorough testing.

Recommended: Add table-driven tests for sanitizePath:

func TestSanitizePath(t *testing.T) {
    tests := []struct {
        name     string
        input    string
        expected string
    }{
        {
            name:     "simple path",
            input:    "workflows/test.md",
            expected: "workflows_test.md",
        },
        {
            name:     "nested path",
            input:    "a/b/c/file.md",
            expected: "a_b_c_file.md",
        },
        {
            name:     "already flat",
            input:    "file.md",
            expected: "file.md",
        },
        {
            name:     "path with dots cleaned",
            input:    "a/./b/file.md",
            expected: "a_b_file.md",
        },
    }

    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            result := sanitizePath(tt.input)
            assert.Equal(t, tt.expected, result, "sanitizePath(%q) should return expected value", tt.input)
        })
    }
}

Recommended: Add table-driven tests for validatePathComponents:

func TestValidatePathComponents(t *testing.T) {
    tests := []struct {
        name      string
        owner     string
        repo      string
        path      string
        sha       string
        shouldErr bool
        errMsg    string
    }{
        {
            name:      "valid components",
            owner:     "testowner",
            repo:      "testrepo",
            path:      "workflows/test.md",
            sha:       "abc123",
            shouldErr: false,
        },
        {
            name:      "empty owner",
            owner:     "",
            repo:      "testrepo",
            path:      "test.md",
            sha:       "abc123",
            shouldErr: true,
            errMsg:    "empty component",
        },
        {
            name:      "path traversal in owner",
            owner:     "../etc",
            repo:      "testrepo",
            path:      "test.md",
            sha:       "abc123",
            shouldErr: true,
            errMsg:    "..",
        },
        {
            name:      "absolute path in sha",
            owner:     "testowner",
            repo:      "testrepo",
            path:      "test.md",
            sha:       "/absolute/path",
            shouldErr: true,
            errMsg:    "absolute path",
        },
        {
            name:      "path traversal in path",
            owner:     "testowner",
            repo:      "testrepo",
            path:      "../../etc/passwd",
            sha:       "abc123",
            shouldErr: true,
            errMsg:    "..",
        },
        {
            name:      "empty sha",
            owner:     "testowner",
            repo:      "testrepo",
            path:      "test.md",
            sha:       "",
            shouldErr: true,
            errMsg:    "empty component",
        },
    }

    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            err := validatePathComponents(tt.owner, tt.repo, tt.path, tt.sha)
            if tt.shouldErr {
                require.Error(t, err, "should return error for: %s", tt.name)
                assert.Contains(t, err.Error(), tt.errMsg, "error message should mention: %s", tt.errMsg)
            } else {
                assert.NoError(t, err, "should not return error for valid components")
            }
        })
    }
}

---

3. Missing Tests for Security-Critical Set Error Cases

The Set function has validation logic for file size and path traversal that is completely untested. These are critical security controls.

Missing test cases for Set:

func TestImportCacheSet_Validation(t *testing.T) {
    tempDir := t.TempDir() // use t.TempDir() instead of manual setup

    tests := []struct {
        name      string
        owner     string
        repo      string
        path      string
        sha       string
        content   []byte
        shouldErr bool
        errMsg    string
    }{
        {
            name:      "oversized content rejected",
            owner:     "owner",
            repo:      "repo",
            path:      "test.md",
            sha:       "abc123",
            content:   make([]byte, 10*1024*1024+1), // 10MB + 1 byte
            shouldErr: true,
            errMsg:    "exceeds maximum",
        },
        {
            name:      "path traversal in owner rejected",
            owner:     "../etc",
            repo:      "repo",
            path:      "test.md",
            sha:       "abc123",
            content:   []byte("content"),
            shouldErr: true,
            errMsg:    "invalid path components",
        },
        {
            name:      "path traversal in path rejected",
            owner:     "owner",
            repo:      "repo",
            path:      "../../etc/passwd",
            sha:       "abc123",
            content:   []byte("content"),
            shouldErr: true,
            errMsg:    "invalid path components",
        },
        {
            name:      "empty owner rejected",
            owner:     "",
            repo:      "repo",
            path:      "test.md",
            sha:       "abc123",
            content:   []byte("content"),
            shouldErr: true,
            errMsg:    "invalid path components",
        },
    }

    cache := NewImportCache(tempDir)
    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            _, err := cache.Set(tt.owner, tt.repo, tt.path, tt.sha, tt.content)
            if tt.shouldErr {
                require.Error(t, err, "Set should reject: %s", tt.name)
                assert.Contains(t, err.Error(), tt.errMsg, "error message should contain %q", tt.errMsg)
            } else {
                assert.NoError(t, err, "Set should succeed for: %s", tt.name)
            }
        })
    }
}

---

4. Use t.TempDir() Instead of Manual Temp Directory Management

Go 1.15+ provides t.TempDir() which automatically cleans up after tests and is the modern idiomatic approach.

// ❌ CURRENT - manual temp dir management
tempDir, err := os.MkdirTemp("", "import-cache-test-*")
if err != nil {
    t.Fatalf("Failed to create temp dir: %v", err)
}
defer os.RemoveAll(tempDir)

// ✅ IMPROVED - automatic cleanup with t.TempDir()
tempDir := t.TempDir()

Why this matters: t.TempDir() always cleans up (even on panic), requires no error handling, and is the idiomatic modern Go pattern.

---

5. Missing Assertion Messages

Many assertions lack context messages that would help diagnose failures.

// ❌ CURRENT - no context
if retrievedPath != cachedPath {
    t.Errorf("Path mismatch. Expected %s, got %s", cachedPath, retrievedPath)
}

// ✅ IMPROVED - testify with descriptive messages
assert.Equal(t, cachedPath, retrievedPath, 
    "retrieved path from Get should match path returned by Set")

---

Implementation Guidelines

Priority Order

1. High: Add TestValidatePathComponents and TestSanitizePath – untested security-critical functions
2. High: Add TestImportCacheSet_Validation – untested error paths in Set
3. High: Replace all t.Fatalf/t.Errorf/t.Error with testify assertions
4. Medium: Replace os.MkdirTemp + defer os.RemoveAll with t.TempDir()
5. Low: Add descriptive assertion messages throughout

Best Practices from scratchpad/testing.md

• ✅ Use require.* for critical setup that should stop the test on failure
• ✅ Use assert.* for test validations that can continue on failure
• ✅ Write table-driven tests with t.Run() and descriptive names
• ✅ No mocks or test suites - test real component interactions
• ✅ Always include helpful assertion messages

Testing Commands

# Run tests for this file
go test -v -run "TestImportCache" ./pkg/parser/

# Run all parser tests
go test -v ./pkg/parser/

# Run full unit tests
make test-unit

---

Acceptance Criteria

• All t.Fatalf/t.Errorf/t.Error replaced with require.*/assert.* testify assertions
• TestSanitizePath added as table-driven test covering normal and edge cases
• TestValidatePathComponents added as table-driven test covering all validation paths (empty, traversal, absolute)
• TestImportCacheSet_Validation added covering oversized content and invalid path components
• os.MkdirTemp + defer os.RemoveAll replaced with t.TempDir()
• All assertions include descriptive messages
• Tests pass: go test -v -run "TestImportCache|TestSanitizePath|TestValidatePathComponents" ./pkg/parser/
• Code follows patterns in scratchpad/testing.md
• Build tag //go:build !integration remains at top of file

---

Additional Context

• Repository Testing Guidelines: See scratchpad/testing.md for comprehensive testing patterns
• Example Tests: Look at pkg/parser/frontmatter_utils_test.go or pkg/workflow/*_test.go for testify examples
• Security Note: sanitizePath and validatePathComponents prevent path traversal attacks – thorough testing of these functions is especially important

---

Priority: Medium
Effort: Small (3-4 hours)
Expected Impact: Better test coverage for security-critical path validation, clearer failure messages, modern Go test patterns

Files Involved:

• Test file: pkg/parser/import_cache_test.go
• Source file: pkg/parser/import_cache.go

References:

• §22279476334

Generated by Daily Testify Uber Super Expert

• expires on Feb 24, 2026, 3:05 PM UTC