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

[github-actions]

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

Current State

• Test File: pkg/cli/awinfo_steps_test.go
• Source File: pkg/cli/logs_models.go (contains AwInfo, AwInfoSteps, GetFirewallVersion)
• Test Functions: 2 test functions (TestAwInfoStepsFieldParsing, TestAwInfoStepsMarshaling)
• Lines of Code: 132 lines
• Testify imports: ❌ None — file does not import testify at all

Test Quality Analysis

Strengths ✅

• Uses table-driven tests with t.Run() in TestAwInfoStepsFieldParsing — good structure
• Covers backward-compatibility scenario (missing steps field)
• Clear test case names that describe the scenario

🎯 Areas for Improvement

1. Missing Testify Imports — No Assertions Used

The file imports only encoding/json and testing. Testify is not used at all, despite being the standard assertion library throughout this codebase.

Current (anti-pattern):

import (
    "encoding/json"
    "testing"
)

// ...
if err != nil {
    t.Fatalf("Failed to unmarshal JSON: %v", err)
}
if info.Steps.Firewall != tt.expectedSteps.Firewall {
    t.Errorf("%s\nExpected firewall: '%s', got: '%s'",
        tt.description, tt.expectedSteps.Firewall, info.Steps.Firewall)
}

Recommended:

import (
    "encoding/json"
    "testing"

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

// ...
err := json.Unmarshal([]byte(tt.jsonContent), &info)
require.NoError(t, err, "should unmarshal JSON without error")
assert.Equal(t, tt.expectedSteps.Firewall, info.Steps.Firewall, tt.description)

Why this matters: Testify provides clearer diffs on failure, is the codebase standard (see scratchpad/testing.md), and reduces boilerplate.

---

2. Manual Type Assertions in TestAwInfoStepsMarshaling

TestAwInfoStepsMarshaling uses raw Go map assertions and manual if !ok checks instead of testify.

Current (verbose and fragile):

steps, ok := result["steps"].(map[string]any)
if !ok {
    t.Fatal("Expected 'steps' field in marshaled JSON")
}
firewall, ok := steps["firewall"].(string)
if !ok {
    t.Fatal("Expected 'firewall' field in steps object")
}
if firewall != "squid" {
    t.Errorf("Expected firewall to be 'squid', got: '%s'", firewall)
}

Recommended — unmarshal back into typed struct:

func TestAwInfoStepsMarshaling(t *testing.T) {
    info := AwInfo{
        EngineID:     "copilot",
        EngineName:   "Copilot",
        Model:        "gpt-4",
        Version:      "1.0",
        WorkflowName: "test-workflow",
        Steps:        AwInfoSteps{Firewall: "squid"},
        CreatedAt:    "2025-01-27T15:00:00Z",
    }

    jsonData, err := json.Marshal(info)
    require.NoError(t, err, "should marshal AwInfo without error")

    var roundTripped AwInfo
    err = json.Unmarshal(jsonData, &roundTripped)
    require.NoError(t, err, "should unmarshal marshaled JSON")

    assert.Equal(t, "squid", roundTripped.Steps.Firewall, "firewall should survive JSON round-trip")
}

---

3. Redundant description Field in Table Test Struct

TestAwInfoStepsFieldParsing has both a name field (used for t.Run) and a description field (used in error messages). These carry almost identical information, creating maintenance overhead.

Current:

tests := []struct {
    name          string
    jsonContent   string
    expectedSteps AwInfoSteps
    description   string  // ← redundant
}

Recommended — use name as assertion message:

tests := []struct {
    name          string
    jsonContent   string
    expectedSteps AwInfoSteps
}

// In the test body:
assert.Equal(t, tt.expectedSteps.Firewall, info.Steps.Firewall, tt.name)

---

4. Test Coverage Gaps

GetFirewallVersion() in logs_models.go is untested. It has backward-compatibility logic that merits dedicated tests:

// GetFirewallVersion returns the AWF firewall version, preferring awf_version
// but falling back to firewall_version for backward compatibility.
func (a *AwInfo) GetFirewallVersion() string {
    if a.AwfVersion != "" {
        return a.AwfVersion
    }
    return a.FirewallVersion
}

Missing tests:

func TestGetFirewallVersion(t *testing.T) {
    tests := []struct {
        name     string
        info     AwInfo
        expected string
    }{
        {
            name:     "prefers awf_version when both set",
            info:     AwInfo{AwfVersion: "1.2.3", FirewallVersion: "1.0.0"},
            expected: "1.2.3",
        },
        {
            name:     "falls back to firewall_version when awf_version empty",
            info:     AwInfo{FirewallVersion: "1.0.0"},
            expected: "1.0.0",
        },
        {
            name:     "returns empty when both unset",
            info:     AwInfo{},
            expected: "",
        },
        {
            name:     "uses only awf_version when firewall_version absent",
            info:     AwInfo{AwfVersion: "2.0.0"},
            expected: "2.0.0",
        },
    }

    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            result := tt.info.GetFirewallVersion()
            assert.Equal(t, tt.expected, result, "GetFirewallVersion should return correct version")
        })
    }
}

Additionally, the json:"steps,omitzero" tag is unusual (typically omitempty is used). A test verifying that a zero-value AwInfoSteps{} is omitted from JSON output would help document and guard this behavior.

func TestAwInfoStepsOmitZero(t *testing.T) {
    info := AwInfo{
        EngineID:  "copilot",
        CreatedAt: "2025-01-27T15:00:00Z",
        // Steps deliberately left zero-value
    }

    jsonData, err := json.Marshal(info)
    require.NoError(t, err, "should marshal without error")

    var raw map[string]any
    require.NoError(t, json.Unmarshal(jsonData, &raw), "should unmarshal raw")

    // Verify steps field behavior with zero value
    _, hasSteps := raw["steps"]
    // Document the actual behavior of omitzero
    t.Logf("steps field present with zero value: %v", hasSteps)
}

---

5. Spurious t.Logf("✓ ...") Calls

The t.Logf("✓ %s", tt.description) calls at the end of each subtest add no value — testify assertion messages already provide this context on failure. They add visual noise and encourage the false impression that "success" needs to be explicitly logged.

Remove:

t.Logf("✓ %s", tt.description)  // ← remove
t.Log("✓ AwInfo marshals correctly with steps field")  // ← remove

📋 Implementation Guidelines

Priority Order

1. High: Add require/assert testify imports and replace all t.Fatalf/t.Errorf calls
2. High: Add missing TestGetFirewallVersion table-driven test
3. Medium: Simplify TestAwInfoStepsMarshaling to use typed round-trip instead of raw map assertions
4. Medium: Remove redundant description field, pass tt.name as assertion message
5. Low: Remove spurious t.Logf("✓ ...") success logs

Best Practices from scratchpad/testing.md

• ✅ Use require.* for setup assertions that must pass (JSON parse/marshal)
• ✅ Use assert.* for value validations (field comparisons)
• ✅ 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 "TestAwInfoSteps|TestGetFirewallVersion"

# Run all unit tests
make test-unit

Acceptance Criteria

• All t.Fatalf/t.Errorf replaced with require.NoError/assert.Equal (testify)
• TestAwInfoStepsMarshaling refactored to use typed JSON round-trip
• TestGetFirewallVersion table-driven test added (4 cases: prefer new, fallback, empty both, only new)
• Redundant description struct field removed; tt.name used as assertion message
• Spurious t.Logf("✓ ...") calls removed
• Tests pass: go test -v ./pkg/cli/ -run "TestAwInfoSteps|TestGetFirewallVersion"
• Code follows patterns in scratchpad/testing.md

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 quality, better failure messages, documented GetFirewallVersion behavior

Files Involved:

• Test file: pkg/cli/awinfo_steps_test.go
• Source file: pkg/cli/logs_models.go

References: §23944688499

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

• expires on Apr 5, 2026, 11:38 AM UTC

[github-actions]

🍪 Issue Monster has assigned this to Copilot!

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

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 · ● 191.4K · ◷

[copilot-swe-agent]

The agent encountered an error and was unable to start working on this issue: This may be caused by a repository ruleset violation. See granting bypass permissions for the agent, or please contact support if the issue persists. (Request id: 1B40:1FDBEF:3053399:BF55AA1:69CFA9BC)

[pelikhan]

/q optimize token usage

[github-actions]

🔧 Technical difficulties! Q failed. Even Q Branch has bad days...