[testify-expert] Improve Test Quality: pkg/cli/actionlint_test.go

[github-actions]

The test file pkg/cli/actionlint_test.go has been selected for quality improvement by the Testify Uber Super Expert. This issue provides specific, actionable recommendations to enhance test quality using testify best practices.

Current State

• Test File: pkg/cli/actionlint_test.go
• Source File: pkg/cli/actionlint.go
• Test Functions: 5 test functions (TestParseAndDisplayActionlintOutput, TestGetActionlintVersion, TestParseAndDisplayActionlintOutputMultiFile, TestDisplayActionlintSummary, TestInitActionlintStats, TestGetActionlintDocsURL)
• Lines of Code: 427 lines
• Critical Issue: Zero testify usage despite testify being available and used throughout the codebase

Test Quality Analysis

Strengths ✅

• Uses table-driven tests with t.Run() subtests throughout
• Good test case coverage with distinct scenarios (errors, multi-file, no errors)
• Tests include both success and failure paths

Areas for Improvement 🎯

1. No Testify Assertions — Zero testify usage

The file does not import testify at all, relying entirely on manual Go testing patterns (t.Errorf, t.Error, t.Fatal). This is inconsistent with the rest of the codebase.

Current Issues:

// ❌ CURRENT (anti-pattern) — manual error checks
if tt.expectError && err == nil {
    t.Errorf("Expected error but got none")
}
if !tt.expectError && err != nil {
    t.Errorf("Unexpected error: %v", err)
}

// ❌ CURRENT — manual equality check
if count != tt.expectedCount {
    t.Errorf("Expected count %d, got %d", tt.expectedCount, count)
}

// ❌ CURRENT — manual nil check
if actionlintStats == nil {
    t.Fatal("actionlintStats should not be nil after initialization")
}

// ❌ CURRENT — manual string comparison
if result != tt.expected {
    t.Errorf("getActionlintDocsURL(%q) = %q, want %q", tt.kind, result, tt.expected)
}

Recommended Changes:

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

// Error checks
if tt.expectError {
    require.Error(t, err, "should return error for invalid input")
} else {
    require.NoError(t, err, "should not return error for valid input")
}

// Equality checks
assert.Equal(t, tt.expectedCount, count, "error count should match expected")

// Nil checks
require.NotNil(t, actionlintStats, "actionlintStats should be initialized")

// String comparisons
assert.Equal(t, tt.expected, result, "docs URL should match expected for kind %q", tt.kind)

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

2. Duplicated Test Body in TestParseAndDisplayActionlintOutputMultiFile

TestParseAndDisplayActionlintOutputMultiFile is a separate test function that duplicates the exact same assertion logic as TestParseAndDisplayActionlintOutput. They test the same function with different data but have identical assertion boilerplate.

Recommended Change: Merge into a single TestParseAndDisplayActionlintOutput with additional table-driven test cases:

func TestParseAndDisplayActionlintOutput(t *testing.T) {
    tests := []struct {
        name          string
        stdout        string
        verbose       bool
        expectedParts []string
        expectError   bool
        expectedCount int
        expectedKinds map[string]int
    }{
        // ... existing single-file cases ...
        {
            name: "multiple errors from multiple files",
            // ... multi-file case data ...
        },
        {
            name: "errors from three files",
            // ... three-file case data ...
        },
    }
    // shared assertion loop...
}

3. Duplicated Stderr Capture Setup

The stderr capture setup is copy-pasted verbatim in TestParseAndDisplayActionlintOutput, TestParseAndDisplayActionlintOutputMultiFile, and TestDisplayActionlintSummary. This is brittle and hard to maintain.

Recommended Change: Extract a helper:

// captureStderr runs fn and returns what was written to stderr
func captureStderr(t *testing.T, fn func()) string {
    t.Helper()
    r, w, err := os.Pipe()
    require.NoError(t, err, "should create pipe for stderr capture")

    origStderr := os.Stderr
    os.Stderr = w
    t.Cleanup(func() { os.Stderr = origStderr })

    fn()

    w.Close()
    var buf bytes.Buffer
    _, err = buf.ReadFrom(r)
    require.NoError(t, err, "should read stderr output")
    return buf.String()
}

Usage:

output := captureStderr(t, func() {
    count, kinds, err = parseAndDisplayActionlintOutput(tt.stdout, tt.verbose)
})

4. TestInitActionlintStats — No Testify, Verbose Manual Checks

This function uses 5 separate if checks with t.Fatal / t.Errorf to validate a newly-initialized struct:

// ❌ CURRENT
if actionlintStats == nil {
    t.Fatal("actionlintStats should not be nil after initialization")
}
if actionlintStats.TotalWorkflows != 0 {
    t.Errorf("TotalWorkflows should be 0, got %d", actionlintStats.TotalWorkflows)
}
// ... 3 more manual checks

Recommended Change:

// ✅ IMPROVED
initActionlintStats()
require.NotNil(t, actionlintStats, "actionlintStats should not be nil after initialization")
assert.Zero(t, actionlintStats.TotalWorkflows, "TotalWorkflows should start at 0")
assert.Zero(t, actionlintStats.TotalErrors, "TotalErrors should start at 0")
assert.Zero(t, actionlintStats.TotalWarnings, "TotalWarnings should start at 0")
assert.NotNil(t, actionlintStats.ErrorsByKind, "ErrorsByKind map should be initialized")
assert.Empty(t, actionlintStats.ErrorsByKind, "ErrorsByKind should start empty")

5. Coverage Gap — runActionlintOnFile Not Tested

The source function runActionlintOnFile has no test coverage. While it requires actionlint to be installed, the error path (when actionlint is not available) is testable:

func TestRunActionlintOnFileErrors(t *testing.T) {
    t.Run("empty lock files list", func(t *testing.T) {
        err := runActionlintOnFile([]string{}, false, false)
        // Depending on behavior - no files should succeed or be a no-op
        assert.NoError(t, err, "empty file list should not error")
    })

    t.Run("non-existent file path", func(t *testing.T) {
        err := runActionlintOnFile([]string{"/nonexistent/path.yml"}, false, false)
        // Document expected behavior for missing files
    })
}

6. TestGetActionlintVersion — Incomplete Test

The "first call fetches version" test case does nothing when tt.expectCached is false:

// ❌ CURRENT — skips the actual test for the uncached case
if tt.expectCached {
    version, err := getActionlintVersion()
    // ... assertions
}
// When expectCached=false: no assertions at all!

Recommended Change: Either test both cases properly, or simplify to a single test for the cached version case:

func TestGetActionlintVersionCached(t *testing.T) {
    original := actionlintVersion
    defer func() { actionlintVersion = original }()

    actionlintVersion = "1.7.9"
    version, err := getActionlintVersion()
    require.NoError(t, err, "should not error when version is cached")
    assert.Equal(t, "1.7.9", version, "should return cached version")
}

View Complete Recommended Imports

import (
    "bytes"
    "os"
    "strings"
    "testing"

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

Implementation Guidelines

Priority Order

1. High: Add testify imports and convert all manual assertions
2. High: Extract captureStderr helper to reduce duplication
3. Medium: Merge TestParseAndDisplayActionlintOutputMultiFile into the main table
4. Medium: Fix TestGetActionlintVersion incomplete test case
5. Low: Add runActionlintOnFile error path tests

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 or test suites — test real component interactions
• ✅ Always include helpful assertion messages

Testing Commands

# Run tests for this file
go test -v ./pkg/cli/ -run "TestParseAndDisplayActionlintOutput|TestDisplayActionlintSummary|TestInitActionlintStats|TestGetActionlintDocsURL|TestGetActionlintVersion"

# Run all CLI unit tests
make test-unit

# Validate after changes
make agent-finish

Acceptance Criteria

• testify/assert and testify/require imported and used throughout
• All t.Errorf / t.Error / t.Fatal replaced with testify assertions
• captureStderr helper extracted to eliminate code duplication
• TestParseAndDisplayActionlintOutputMultiFile merged into main table-driven test
• TestGetActionlintVersion covers the cached version case properly
• All assertions include descriptive messages
• Tests pass: make test-unit
• Code follows patterns in scratchpad/testing.md

Additional Context

• Repository Testing Guidelines: See scratchpad/testing.md for comprehensive testing patterns
• Example Tests: See pkg/cli/access_log_test.go for a well-structured test file in the same package that correctly uses testify
• Testify Documentation: https://github.com/stretchr/testify

---

Priority: Medium
Effort: Small (mostly mechanical conversion + minor refactoring)
Expected Impact: Cleaner test failures, consistent style with rest of codebase, easier maintenance

Files Involved:

• Test file: pkg/cli/actionlint_test.go
• Source file: pkg/cli/actionlint.go

References:

• §22312026941

Generated by Daily Testify Uber Super Expert

• expires on Feb 25, 2026, 3:20 PM UTC