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

[github-actions]

Current State

• File: pkg/cli/logs_firewall_parse_test.go (51 LOC, 2 test functions)
• Source pair: pkg/cli/logs_parsing_firewall.go (282 LOC) — exports parseFirewallLogs, findFirewallLogsDir, dirHasMatchingFiles (helper, not directly tested)
• Tests covered: TestParseFirewallLogsNoLogs, TestFindFirewallLogsDirSandboxFallbackToTopLevel

Strengths

• Uses testutil.TempDir for isolated fixture setup.
• Covers a real fallback-path scenario (sandbox squid-logs discovery).

Prioritized Improvements

1. Missing / high-value tests

findFirewallLogsDir checks 4 candidate directories in priority order, but only the first (sandbox/firewall/logs/squid-logs) is tested. Add cases for:

• sandbox/firewall/logs (non-squid-logs subfolder) fallback
• top-level squid-logs fallback
• workflow-logs/squid-logs fallback
• no directories present at all → expect ("", nil)
• directory exists but contains no *.log files → expect ("", nil), not an error

parseFirewallLogs only tests the "no logs" skip path. Consider (at minimum documenting as a gap, since full success path likely needs the JS parser/node runtime):

• verbose vs non-verbose output behavior when no logs found
• error propagation when findFirewallLogsDir fails (e.g., permission-denied directory)

2. Testify assertion upgrades

Both tests use raw if ... { t.Fatalf/t.Errorf } instead of testify. Convert to require/assert:

Before / after

// Before
err := parseFirewallLogs(tempDir, true)
if err != nil {
    t.Fatalf("parseFirewallLogs should not fail when no logs present: %v", err)
}
if _, err := os.Stat(firewallMdPath); !os.IsNotExist(err) {
    t.Errorf("firewall.md should not be created when no logs are present")
}

// After
err := parseFirewallLogs(tempDir, true)
require.NoError(t, err, "parseFirewallLogs should not fail when no logs present")
assert.NoFileExists(t, firewallMdPath, "firewall.md should not be created when no logs are present")

Use require for setup/fatal-style checks (os.MkdirAll, os.WriteFile), and assert for result checks so multiple assertions per test can all report failures.

3. Table-driven refactor

findFirewallLogsDir's 4 fallback branches are a natural table-driven case:

Suggested structure

func TestFindFirewallLogsDirFallbacks(t *testing.T) {
    t.Parallel()
    tests := []struct {
        name       string
        setupPath  func(root string) string // dir to create logs in
        wantSuffix string
    }{
        {"sandbox squid-logs", func(root string) string { return filepath.Join(root, "sandbox", "firewall", "logs", "squid-logs") }, "sandbox/firewall/logs/squid-logs"},
        {"sandbox logs (no squid-logs)", func(root string) string { return filepath.Join(root, "sandbox", "firewall", "logs") }, "sandbox/firewall/logs"},
        {"top-level squid-logs", func(root string) string { return filepath.Join(root, "squid-logs") }, "squid-logs"},
        {"workflow-logs squid-logs", func(root string) string { return filepath.Join(root, "workflow-logs", "squid-logs") }, "workflow-logs/squid-logs"},
    }
    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            t.Parallel()
            tempDir := testutil.TempDir(t, "test-firewall-*")
            logDir := tt.setupPath(tempDir)
            require.NoError(t, os.MkdirAll(logDir, 0755))
            require.NoError(t, os.WriteFile(filepath.Join(logDir, "access.log"), []byte("dummy\n"), 0644))

            got, err := findFirewallLogsDir(tempDir)
            require.NoError(t, err)
            assert.Equal(t, filepath.Join(tempDir, tt.wantSuffix), got)
        })
    }
}

Add a companion negative-case table for "no matching dir" / "empty dir" → ("", nil).

4. Organization / readability

• Rename TestFindFirewallLogsDirSandboxFallbackToTopLevel — current name is misleading (it tests the first/most-specific match, not a "fallback to top-level"). Suggest TestFindFirewallLogsDirSandboxSquidLogs.
• Add t.Parallel() to TestParseFirewallLogsNoLogs for consistency with the other test.
• Import github.com/stretchr/testify/assert and require alongside existing imports.

Acceptance Checklist

• Add missing fallback-path test cases for findFirewallLogsDir (table-driven)
• Add empty/no-log-file negative case
• Replace manual if err != nil { t.Fatalf } / if ... { t.Errorf } with require/assert
• Rename misleading test function name
• Add t.Parallel() where missing
• make test-unit passes for pkg/cli

Generated by 🧪 Daily Testify Uber Super Expert · copilot · auto · 18.3 AIC · ⌖ 5.07 AIC · ⊞ 7.6K · ◷

• expires on Aug 26, 2026, 10:10 AM UTC-08:00

[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 · codex · mai10 · 2.27 AIC · ⌖ 3.15 AIC · ⊞ 17.4K · ◷