[testify-expert] Improve Test Quality: pkg/cli/access_log_test.go #60896

Current State

• Test file: pkg/cli/access_log_test.go (347 LOC, 7 top-level Test* functions, several t.Run subtests)
• Source pair: pkg/cli/access_log.go (230 LOC, squid access-log parsing/aggregation)
• Build tag !integration; all tests use t.Parallel() and a mix of assert/require from testify.

Strengths

• Good require vs assert discipline: fatal preconditions (parseSquidAccessLog errors, nil checks) use require, value checks use assert.
• Table-driven subtests already used for TestExtractDomainFromURL, TestParseSquidLogLine, and TestAddMetrics.
• TestDomainAnalysisJSONWireNames nicely locks in the legacy JSON wire format (allowed_count/blocked_count) with a round-trip check.
• Consistent t.Parallel() usage throughout, including nested t.Run subtests.

Prioritized Improvements

1. Missing / high-value tests

func TestIsAllowedSquidStatus(t *testing.T) {
	t.Parallel()
	tests := []struct {
		name   string
		status string
		want   bool
	}{
		{"TCP_HIT/200", "TCP_HIT/200", true},
		{"TCP_MISS/200", "TCP_MISS/200", true},
		{"TCP_REFRESH_MODIFIED/200", "TCP_REFRESH_MODIFIED/200", true},
		{"TCP_IMS_HIT/304", "TCP_IMS_HIT/304", true},
		{"generic /206 partial content", "TCP_MISS/206", true},
		{"TCP_DENIED/403", "TCP_DENIED/403", false},
		{"TCP_MISS/301 redirect not treated as allowed", "TCP_MISS/301", false},
		{"TCP_DENIED/407", "TCP_DENIED/407", false},
	}
	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			t.Parallel()
			assert.Equal(t, tt.want, isAllowedSquidStatus(tt.status))
		})
	}
}

func TestParseSquidAccessLogMissingFile(t *testing.T) {
	t.Parallel()
	_, err := parseSquidAccessLog(filepath.Join(testutil.TempDir(t, "test-*"), "missing.log"), false)
	require.Error(t, err, "should error when access log file does not exist")
}

func TestAccessLogDeduplicatesDomains(t *testing.T) {
	t.Parallel()
	tempDir := testutil.TempDir(t, "test-*")
	content := `1701234567.123 180 192.168.1.100 TCP_MISS/200 1234 GET (example.com/redacted) - HIER_DIRECT/93.184.216.34 text/html
1701234568.456 180 192.168.1.100 TCP_HIT/200 1234 GET (example.com/redacted) - HIER_DIRECT/93.184.216.34 text/html`
	path := filepath.Join(tempDir, "access.log")
	require.NoError(t, os.WriteFile(path, []byte(content), 0644))

	analysis, err := parseSquidAccessLog(path, false)
	require.NoError(t, err)
	assert.Equal(t, []string{"example.com"}, analysis.AllowedDomains, "duplicate domain should only appear once")
	assert.Equal(t, 2, analysis.AllowedRequests, "both requests should still be counted")
}

2. Testify assertion upgrades

expectedAllowed := []string{"api.github.com", "example.com"}
assert.Len(t, analysis.AllowedDomains, len(expectedAllowed), "should extract correct number of allowed domains")

expectedAllowed := []string{"api.github.com", "example.com"}
assert.ElementsMatch(t, expectedAllowed, analysis.AllowedDomains, "should extract correct allowed domains")

3. Table-driven refactor

4. Organization / readability

• TestAnalyzeAccessLogsDirectory already groups its three scenarios well with t.Run — consider applying the same subtest grouping to TestAccessLogParsing/TestMultipleAccessLogAnalysis so shared setup (temp dir, log content) is easier to extend with the new edge cases above.
• Consider extracting the repeated squid log line literals (e.g. "1701234567.123 180 192.168.1.100 TCP_MISS/200 ...") into small named fixture builders/constants to reduce visual noise and make the intent (allowed vs blocked request) clearer at a glance.

Acceptance Checklist

• Add TestIsAllowedSquidStatus covering all status branches (positive and negative cases)
• Add test for parseSquidAccessLog missing-file error path
• Add test verifying domain de-duplication behavior
• Add test for skipped lines (comments, blanks, empty-domain URLs)
• Add test for analyzeMultipleAccessLogs on an empty directory
• Replace assert.Len domain-count checks with assert.ElementsMatch where expected domain values are already computed
• Run make test-unit and confirm all pkg/cli access-log tests pass

network:
  allowed:
    - defaults
    - "proxy.golang.org"

Generated by 🧪 Daily Testify Uber Super Expert · copilot · auto · 35.8 AIC · ⌖ 7.72 AIC · ⊞ 7.8K · ◷

• expires on Sep 16, 2026, 10:05 AM UTC-08:00