[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 (TestImportCache, TestImportCacheDirectory, TestImportCacheMissingFile, TestImportCacheEmptyCache)
• Lines of Code: 161 lines
• Testify Usage: ❌ None — all assertions use raw t.Fatalf / t.Errorf / t.Error

Test Quality Analysis

Strengths ✅

• Build tag //go:build !integration is correctly placed at the top
• Tests use os.MkdirTemp + defer os.RemoveAll for proper temp-dir lifecycle management
• The four test scenarios cover fundamentally distinct behaviors (happy path, directory creation, missing file, empty cache)

Areas for Improvement 🎯

1. Replace all plain Go assertions with testify

Every assertion in the file uses t.Fatalf / t.Errorf / t.Error directly. The project standard (see scratchpad/testing.md) is:

• require.* — critical setup that must succeed before the test can continue
• assert.* — validations that allow the test to keep running and collect all failures

Current (anti-pattern):

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

cachedPath, err := cache.Set(owner, repo, path, sha, testContent)
if err != nil {
    t.Fatalf("Failed to set cache entry: %v", err)
}

if string(content) != string(testContent) {
    t.Errorf("Content mismatch. Expected %q, got %q", testContent, content)
}

Improved (testify):

tempDir, err := os.MkdirTemp("", "import-cache-test-*")
require.NoError(t, err, "should create temp dir for test")
defer os.RemoveAll(tempDir)

cachedPath, err := cache.Set(owner, repo, path, sha, testContent)
require.NoError(t, err, "Set should succeed for valid inputs")

content, err := os.ReadFile(cachedPath)
require.NoError(t, err, "should be able to read the cached file")
assert.Equal(t, string(testContent), string(content), "cached content should match original")

Why it matters: Testify assertions produce structured diffs on failure, are consistent with all other test files in the codebase, and require.* stops execution immediately so a failing setup doesn't produce confusing downstream failures.

2. Add tests for untested functions: sanitizePath and validatePathComponents

The source file import_cache.go exports or uses two functions that have zero test coverage:

• sanitizePath(path string) string — converts a path to a safe flat filename
• validatePathComponents(owner, repo, path, sha string) error — guards against path traversal

These contain security-sensitive logic and deserve their own table-driven tests.

Recommended tests:

func TestSanitizePath(t *testing.T) {
    tests := []struct {
        name     string
        input    string
        expected string
    }{
        {
            name:     "simple filename",
            input:    "workflow.md",
            expected: "workflow.md",
        },
        {
            name:     "nested path is flattened",
            input:    "workflows/sub/test.md",
            expected: "workflows_sub_test.md",
        },
        {
            name:     "path with leading dot-dot is cleaned",
            input:    "../escape/test.md",
            expected: "escape_test.md",
        },
    }

    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            got := sanitizePath(tt.input)
            assert.Equal(t, tt.expected, got, "sanitizePath(%q) should produce safe flat filename", tt.input)
        })
    }
}

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

    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            err := validatePathComponents(tt.owner, tt.repo, tt.path, tt.sha)
            if tt.wantErr {
                require.Error(t, err, "should reject invalid path components")
                assert.Contains(t, err.Error(), tt.errContains, "error message should describe the problem")
            } else {
                assert.NoError(t, err, "should accept valid path components")
            }
        })
    }
}

3. Add missing edge-case tests for Set

Set validates file size and path components but neither is tested. Two critical paths are entirely uncovered:

Scenario	Current Coverage
File exceeds 10 MB size limit	❌ Not tested
Path traversal attempt via owner/repo/sha	❌ Not tested
Very long path or edge-case sha value	❌ Not tested

Recommended additions:

func TestImportCacheSetSizeLimit(t *testing.T) {
    tempDir, err := os.MkdirTemp("", "import-cache-size-*")
    require.NoError(t, err, "should create temp dir")
    defer os.RemoveAll(tempDir)

    cache := NewImportCache(tempDir)

    // Build content just over 10 MB
    oversized := make([]byte, 10*1024*1024+1)
    _, err = cache.Set("owner", "repo", "test.md", "sha1", oversized)
    require.Error(t, err, "Set should reject content exceeding 10 MB")
    assert.Contains(t, err.Error(), "exceeds maximum", "error should mention size limit")
}

func TestImportCacheSetPathTraversal(t *testing.T) {
    tempDir, err := os.MkdirTemp("", "import-cache-traversal-*")
    require.NoError(t, err, "should create temp dir")
    defer os.RemoveAll(tempDir)

    cache := NewImportCache(tempDir)

    _, err = cache.Set("owner", "repo", "../../escape.md", "sha1", []byte("content"))
    require.Error(t, err, "Set should reject path traversal attempts")
    assert.Contains(t, err.Error(), "'..'", "error should indicate path traversal")
}

4. Improve assertion messages throughout

Several assertions lack messages that would help diagnose failures in CI:

Current:

if !found {
    t.Error("Cache entry not found after Set")
}
if retrievedPath != cachedPath {
    t.Errorf("Path mismatch. Expected %s, got %s", cachedPath, retrievedPath)
}

Improved (testify with messages):

assert.True(t, found, "cache.Get should find entry immediately after cache.Set")
assert.Equal(t, cachedPath, retrievedPath, "path returned by Get should equal path returned by Set")

5. Extract shared test setup into a helper

All four test functions repeat the same os.MkdirTemp / defer os.RemoveAll / NewImportCache pattern. A small helper eliminates the repetition and reduces noise:

// newTestCache creates a temporary import cache for testing.
// The caller should not defer cleanup; the helper registers it via t.Cleanup.
func newTestCache(t *testing.T) *ImportCache {
    t.Helper()
    tempDir, err := os.MkdirTemp("", "import-cache-*")
    require.NoError(t, err, "should create temp dir for test cache")
    t.Cleanup(func() { os.RemoveAll(tempDir) })
    return NewImportCache(tempDir)
}

Then each test simplifies to:

func TestImportCacheEmptyCache(t *testing.T) {
    cache := newTestCache(t)
    _, found := cache.Get("owner", "repo", "test.md", "nonexistent-sha")
    assert.False(t, found, "empty cache should return not-found for any key")
}

Implementation Guidelines

Priority Order

1. High — Replace all t.Fatalf/t.Errorf/t.Error with require.*/assert.* (touches every test function)
2. High — Add TestValidatePathComponents table-driven test (security-sensitive code)
3. High — Add TestSanitizePath table-driven test (security-sensitive code)
4. Medium — Add TestImportCacheSetSizeLimit and TestImportCacheSetPathTraversal
5. Low — Extract newTestCache helper to reduce setup duplication

Best Practices from scratchpad/testing.md

• ✅ Use require.* for critical setup (stops test on failure)
• ✅ Use assert.* for test validations (continues checking)
• ✅ Write table-driven tests with t.Run() and descriptive names
• ✅ No mocks — test real component interactions (already satisfied)
• ✅ Always include helpful assertion messages

Testing Commands

# Run just this package's tests
go test -v ./pkg/parser/ -run "TestImportCache"

# Run with race detector
go test -race ./pkg/parser/ -run "TestImportCache"

# Run full unit suite before committing
make test-unit

Acceptance Criteria

• All t.Fatalf / t.Errorf / t.Error replaced with require.* / assert.*
• TestSanitizePath table-driven test added
• TestValidatePathComponents table-driven test added (covers empty, path-traversal, absolute path)
• TestImportCacheSetSizeLimit and TestImportCacheSetPathTraversal added
• Optional: newTestCache helper extracted
• All assertions include descriptive messages
• make test-unit passes

Additional Context

• Repository Testing Guidelines: scratchpad/testing.md
• Testify Documentation: https://github.com/stretchr/testify
• Security note: sanitizePath and validatePathComponents are security-sensitive functions that prevent path traversal; having explicit tests for them is especially important

---

Priority: Medium
Effort: Small (mostly mechanical assertion rewrites + a few new test functions)
Expected Impact: Better CI failure messages, security-sensitive code coverage, consistent style with the rest of the codebase

Files Involved:

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

Generated by Daily Testify Uber Super Expert

• expires on Feb 21, 2026, 3:23 PM UTC