test: improve test quality in access_log_test.go with testify patterns

[Copilot]

Converts pkg/cli/access_log_test.go from manual error checking to testify assertions, adds missing test coverage, and applies table-driven test patterns with t.Run() subtests.

Testify Migration

• Converted 13+ manual if err != nil { t.Fatalf() } → require.NoError(t, err, "msg")
• Converted 10+ manual if got != want { t.Errorf() } → assert.Equal(t, expected, actual, "msg")
• Applied testifylint rules: assert.Len for slice lengths, require.Error in conditionals

Test Organization

• Added t.Run() subtests to TestAnalyzeAccessLogsDirectory ("multiple access logs in subdirectory", "no access logs - returns nil")
• Converted TestExtractDomainFromURL to use t.Run() per test case

New Test Coverage

• TestParseSquidLogLine: 5 cases covering valid/invalid squid log parsing (valid lines, insufficient fields, empty input)
• TestAddMetrics: 3 cases for metrics aggregation (valid addition, zero values, empty base)

Before/After

// Before: Manual error checking
err := os.WriteFile(accessLogPath, []byte(testLogContent), 0644)
if err != nil {
    t.Fatalf("Failed to create test access.log: %v", err)
}

// After: Testify with clear intent
err := os.WriteFile(accessLogPath, []byte(testLogContent), 0644)
require.NoError(t, err, "should create test access log file")

Test count: 4 → 6 functions with organized subtests. Follows specs/testing.md conventions.

Original prompt

---

This section details on the original issue you should resolve

<issue_title>[testify-expert] Improve Test Quality: pkg/cli/access_log_test.go</issue_title>
<issue_description>### Overview

The test file pkg/cli/access_log_test.go has been selected for quality improvement by the Testify Uber Super Expert. This file contains 4 test functions (182 lines) testing access log parsing functionality. While the tests are functional, they rely heavily on manual error checking rather than testify assertions, lack descriptive test names for edge cases, and have opportunities for table-driven test patterns.

Current State

• Test File: pkg/cli/access_log_test.go
• Source File: pkg/cli/access_log.go
• Test Functions: 4 test functions
• Lines of Code: 182 lines
• Test Coverage: Tests cover the main parsing functions but use anti-patterns

Test Quality Analysis

Strengths ✅

1. Good Test Isolation: Uses testutil.TempDir(t, "test-*") for proper test isolation with temporary directories
2. Realistic Test Data: Uses actual squid log format data that matches real-world scenarios
3. Multiple Scenarios: Tests single log files, multiple log files, and edge cases (no logs)

Areas for Improvement 🎯

1. Testify Assertions

Current Issues:

• 13+ instances of manual error handling using if err != nil { t.Fatalf() }
• 10+ instances of manual comparisons using if got != want { t.Errorf() }
• Zero use of testify assert or require packages

Recommended Changes:

// ❌ CURRENT (lines 24-27, repeated pattern)
err := os.WriteFile(accessLogPath, []byte(testLogContent), 0644)
if err != nil {
    t.Fatalf("Failed to create test access.log: %v", err)
}

// ✅ IMPROVED (testify)
err := os.WriteFile(accessLogPath, []byte(testLogContent), 0644)
require.NoError(t, err, "should create test access.log")

// ❌ CURRENT (lines 36-38)
if analysis.TotalRequests != 4 {
    t.Errorf("Expected 4 total requests, got %d", analysis.TotalRequests)
}

// ✅ IMPROVED (testify)
assert.Equal(t, 4, analysis.TotalRequests, "should count all requests in log file")

// ❌ CURRENT (lines 30-33)
analysis, err := parseSquidAccessLog(accessLogPath, false)
if err != nil {
    t.Fatalf("Failed to parse access log: %v", err)
}

// ✅ IMPROVED (testify with clear intent)
analysis, err := parseSquidAccessLog(accessLogPath, false)
require.NoError(t, err, "parseSquidAccessLog should parse valid squid log")
require.NotNil(t, analysis, "should return analysis result")

Why this matters: Testify provides:

• Clearer error messages with context
• Consistent assertion style across the codebase
• Better test output readability
• Follows specs/testing.md conventions

Files to update: All 4 test functions need conversion

2. Table-Driven Tests

Current Issues:

• TestExtractDomainFromURL (lines 164-182) uses table-driven tests ✅ BUT doesn't use t.Run() for subtests
• The loop doesn't create named subtests, making failures harder to identify
• Missing use of testify assertions in the table test

Recommended Changes:

// ❌ CURRENT (lines 176-181)
for _, test := range tests {
    result := stringutil.ExtractDomainFromURL(test.url)
    if result != test.expected {
        t.Errorf("stringutil.ExtractDomainFromURL(%q) = %q, expected %q", 
            test.url, result, test.expected)
    }
}

// ✅ IMPROVED (with t.Run and testify)
for _, tt := range tests {
    t.Run(tt.url, func(t *testing.T) {
        result := stringutil.ExtractDomainFromURL(tt.url)
        assert.Equal(t, tt.expected, result, 
            "should extract correct domain from URL")
    })
}

Additional Opportunity: TestAccessLogParsing and TestMultipleAccessLogAnalysis test similar patterns (allowed/blocked counting) and could benefit from table-driven tests for different log content scenarios.

3. Test Coverage Gaps

Missing Tests:

Based on the source file, these exported functions lack dedicated tests:

1. AddMetrics (lines 41-47) - Method for aggregating domain analysis metrics
2. parseSquidLogLine (lines 136-154) - Low-level log line parsing with error cases
3. formatDomainWithEcosystem (line 193) - Domain formatting helper

Priority Functions to Test:

1. parseSquidLogLine - High Priority

   • Critical parsing logic that handles malformed input
   • Should test: valid lines, lines with <10 fields, empty lines, special characters
   • Current tests only indirectly test this through higher-level functions

2. AddMetrics - Medium Priority

   • Aggregation logic for combining multiple analysis results
   • Should test: adding metrics, handling non-DomainAnalysis types

Recommended Test Cases:

func TestParseSquidLogLine(t *testing.T) {
    tests := []struct {
        name      string
        line      string
        expected  *AccessLogEntry
        shouldErr bool
    }{
        {
            name: "va...

</details>



<!-- START COPILOT CODING AGENT SUFFIX -->

- Fixes githubnext/gh-aw#11746

<!-- START COPILOT CODING AGENT TIPS -->
---

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs.