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

[github-actions]

Overview

The test file pkg/parser/import_cache_test.go has been selected for quality improvement by the Testify Uber Super Expert. This file tests the import cache subsystem and is generally well-written, but has several specific areas where testify best practices can be strengthened and test coverage can be improved.

Current State

• Test File: pkg/parser/import_cache_test.go
• Source File: pkg/parser/import_cache.go
• Test Functions: 9 test functions
• Lines of Code: 380 lines
• Exported Functions in Source: NewImportCache, (*ImportCache).Get, (*ImportCache).Set, (*ImportCache).GetCacheDir, (*ImportCache).ensureGitAttributes

Test Quality Analysis

Strengths ✅

1. Good require/assert discipline – Critical setup steps use require.NoError and require.FileExists correctly; non-fatal validations use assert.*.
2. Table-driven tests used where appropriate – TestSanitizePath, TestValidatePathComponents, and TestImportCacheSet_Validation all use the recommended table-driven pattern with t.Run().
3. Meaningful assertion messages – Nearly every assertion includes a descriptive message, following the project's testing guidelines.

🎯 Areas for Improvement

1. Ignored Errors in Test Setup

Current Issues:

In TestImportCache, several t.Run subtests call cache.Set(...) and discard the error with _, treating a failed Set as non-fatal:

t.Run("Get returns cached path after Set", func(t *testing.T) {
    cachedPath, _ := cache.Set(owner, repo, path, sha, testContent) // ❌ error silently ignored
    retrievedPath, found := cache.Get(owner, repo, path, sha)
    ...
})

t.Run("New cache instance finds existing entry", func(t *testing.T) {
    cachedPath, _ := cache.Set(owner, repo, path, sha, testContent) // ❌ error silently ignored
    ...
})

Recommended Changes:

t.Run("Get returns cached path after Set", func(t *testing.T) {
    cachedPath, err := cache.Set(owner, repo, path, sha, testContent)
    require.NoError(t, err, "Set should succeed before testing Get")   // ✅
    retrievedPath, found := cache.Get(owner, repo, path, sha)
    ...
})

Why this matters: Silently discarded errors mean a test can proceed on a false premise (e.g., cachedPath is an empty string) and produce a misleading failure rather than pinpointing the root cause.

---

2. TestImportCache Subtests Repeat Setup Independently

Current Issues:

Each subtest inside TestImportCache independently calls cache.Set(...) with the same arguments. Because all subtests share the same cache instance and key, the first subtest's Set side-effects pollute later subtests. The subtests are also non-independent (they rely on execution order to have the directory pre-created).

Recommended Changes:

Refactor into a table-driven test or use t.Cleanup / a nested helper to give each subtest a clean cache:

func TestImportCache_Operations(t *testing.T) {
    const (
        owner = "testowner"
        repo  = "testrepo"
        path  = "workflows/test.md"
        sha   = "abc123"
    )
    testContent := []byte("# Test Workflow\n\nTest content")

    newCache := func(t *testing.T) *ImportCache {
        t.Helper()
        return NewImportCache(t.TempDir())
    }

    t.Run("Set creates file and returns path", func(t *testing.T) {
        cache := newCache(t)
        cachedPath, err := cache.Set(owner, repo, path, sha, testContent)
        require.NoError(t, err, "Set should succeed for valid inputs")
        require.FileExists(t, cachedPath, "cache file should be created")
    })

    t.Run("Get returns cached path after Set", func(t *testing.T) {
        cache := newCache(t)
        cachedPath, err := cache.Set(owner, repo, path, sha, testContent)
        require.NoError(t, err, "Set should succeed before testing Get")
        retrievedPath, found := cache.Get(owner, repo, path, sha)
        assert.True(t, found, "cache entry should be found after Set")
        assert.Equal(t, cachedPath, retrievedPath, "retrieved path should match Set path")
    })
    // ...
}

Why this matters: Isolated subtests can run in any order (including with t.Parallel()) and failures in one don't confuse others.

---

3. Missing Direct Test for ensureGitAttributes Error Path

Current Issues:

ensureGitAttributes is called by Set and its happy path is tested indirectly via TestImportCacheDirectory. However, there is no test for the error path: what happens when the cache directory exists but is not writable (e.g., permissions deny file creation)?

Recommended Test:

func TestImportCache_EnsureGitAttributes_ReadOnlyDir(t *testing.T) {
    if os.Getuid() == 0 {
        t.Skip("skipping permission test as root")
    }
    tempDir := t.TempDir()
    cache := NewImportCache(tempDir)

    // Pre-create the cache directory as read-only
    cacheDir := cache.GetCacheDir()
    require.NoError(t, os.MkdirAll(cacheDir, 0555), "setup: create read-only dir")
    t.Cleanup(func() { _ = os.Chmod(cacheDir, 0755) })

    // Set should succeed (ensureGitAttributes failure is non-fatal)
    _, err := cache.Set("owner", "repo", "test.md", "sha1", []byte("content"))
    // The current implementation treats ensureGitAttributes failures as non-fatal.
    // Verify Set either succeeds or returns a clear error.
    _ = err // document the behavior explicitly
}

Why this matters: The ensureGitAttributes failure is explicitly documented as non-fatal in the source (// Non-fatal error - continue with caching). A test validates and documents this contract.

---

4. Missing Test: GetCacheDir Is Only Tested as a Side Effect

Current Issues:

GetCacheDir is called inside TestImportCacheDirectory but its behavior is not directly tested in isolation.

Recommended Test:

func TestImportCache_GetCacheDir(t *testing.T) {
    tests := []struct {
        name     string
        repoRoot string
        expected string
    }{
        {
            name:     "standard repo root",
            repoRoot: "/home/user/project",
            expected: "/home/user/project/" + ImportCacheDir,
        },
        {
            name:     "relative root",
            repoRoot: ".",
            expected: filepath.Join(".", ImportCacheDir),
        },
    }

    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            cache := NewImportCache(tt.repoRoot)
            assert.Equal(t, tt.expected, cache.GetCacheDir(),
                "GetCacheDir should return %q for root %q", tt.expected, tt.repoRoot)
        })
    }
}

---

5. TestSanitizePath Missing Edge Cases

Current Issues:

The table-driven TestSanitizePath covers common paths but is missing several edge cases relevant to security (path injection via special characters):

Missing Test Cases:

{
    name:     "path with spaces",
    input:    "a/b c/file.md",
    expected: "a_b c_file.md",
},
{
    name:     "path with null bytes (security)",
    input:    "a\x00b/file.md",
    expected: "a\x00b_file.md", // documents current behavior (no null stripping)
},
{
    name:     "windows-style separator",
    input:    "a\\b\\file.md",
    expected: "a\\b\\file.md", // backslash is not treated as separator on Unix
},
{
    name:     "double slash",
    input:    "a//b/file.md",
    expected: "a_b_file.md", // filepath.Clean collapses double slashes
},

Why this matters: Security-sensitive functions like sanitizePath benefit from exhaustive edge-case documentation in tests.

📋 Implementation Guidelines

Priority Order

1. High: Fix silently-discarded errors in TestImportCache subtests (1–2 min change)
2. Medium: Refactor TestImportCache subtests to use isolated caches
3. Medium: Add direct GetCacheDir table-driven test
4. Low: Add ensureGitAttributes error path test
5. Low: Extend TestSanitizePath with edge cases

Testing Commands

# Run only this file's tests
go test -v -run 'TestImportCache' ./pkg/parser/

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

# Full unit test suite
make test-unit

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
• ✅ Always include helpful assertion messages
• ✅ Use t.TempDir() for isolated filesystem state per subtest

Acceptance Criteria

• All cache.Set(...) calls in TestImportCache subtests have their errors checked with require.NoError
• TestImportCache subtests use isolated caches (one t.TempDir() per subtest or a helper function)
• Direct table-driven test for GetCacheDir added
• TestSanitizePath extended with at least 2 additional edge cases
• Tests pass: go test -race ./pkg/parser/ and make test-unit

Additional Context

• Repository Testing Guidelines: See scratchpad/testing.md for comprehensive testing patterns
• Testify Documentation: https://github.com/stretchr/testify

---

Priority: Medium
Effort: Small
Expected Impact: Improved test isolation, clearer failure messages, better security edge-case documentation

Files Involved:

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

References:

• Workflow run: §25335675290

Generated by Daily Testify Uber Super Expert · ● 1.2M · ◷

• expires on May 6, 2026, 6:27 PM UTC

[github-actions]

🍪 Issue Monster selected this for Copilot

I've identified this issue as a good candidate for automated resolution and requested assignment to the Copilot coding agent.

If assignment succeeds, the Copilot coding agent will analyze the issue and create a pull request with the fix.

Om nom nom! 🍪

🍪 Om nom nom by Issue Monster · ● 23.6K · ◷