[testify-expert] Improve Test Quality: pkg/constants/constants_test.go

[github-actions]

Overview

File: pkg/constants/constants_test.go
Source pair: pkg/constants/constants.go (585 LOC)
Test count: 22 test functions, ~709 LOC
Build tag: //go:build !integration

The test file validates constant values, semantic types, and helper functions in the constants package. Coverage is reasonably broad but relies entirely on raw testing package patterns with no testify usage. Several improvements would increase clarity and reduce repetitive boilerplate.

---

Strengths

• Good use of table-driven tests for repeated constant checks (TestConstantValues, TestTimeoutConstants, TestFeatureFlagConstants)
• Tests cover edge cases like empty env var override and duplicate detection
• Tests for type safety between semantic types (TestTypeSafetyBetweenSemanticTypes)

---

Prioritized Improvements

1. Missing / High-Value Tests

Several exported constants and functions defined in constants.go are not covered:

• AWFDefaultCommand, AWFProxyLogsDir, AWFAuditDir, PreAgentAuditFilePath, AWFConfigFilePath — path constants that are referenced widely in the codebase
• DefaultMaxAICredits, DefaultDetectionMaxAICredits, DefaultMaxDailyAICredits, DefaultMaxRuns, DefaultMaxTurnCacheMisses — numeric policy constants
• MCPSessionTimeoutMin, MCPToolTimeoutMin, MCPToolTimeoutMax — timeout bounds relationships are not asserted (e.g., MCPToolTimeoutMin < MCPToolTimeoutMax)
• GetEngineOption for engines beyond opencode/crush/pi (e.g., claude, copilot, gemini, codex) — only multi-provider engines are tested
• OTELSentryEndpointSecretName — used in observability configuration, never tested

Example: Add timeout bounds test

Before: no test for MCP timeout relationship

After:

func TestMCPTimeoutBounds(t *testing.T) {
    require.Greater(t, MCPToolTimeoutMax, MCPToolTimeoutMin,
        "MCPToolTimeoutMax must be greater than MCPToolTimeoutMin")
    require.Greater(t, MCPSessionTimeoutMin, MCPToolTimeoutMin,
        "MCPSessionTimeoutMin must be greater than MCPToolTimeoutMin")
}

2. Testify Assertion Upgrades

The entire file uses raw t.Error/t.Errorf/t.Fatalf with no testify. Switching to testify/assert and testify/require would:

• Produce more readable failure messages
• Use require for fatal checks (nil guards), assert for non-fatal checks
• Eliminate manual map-building for membership checks (use assert.Contains)

Before / After examples

Before:

func TestDefaultAllowedDomains(t *testing.T) {
    if len(DefaultAllowedDomains) == 0 {
        t.Error("DefaultAllowedDomains should not be empty")
    }
    expectedDomains := []string{"localhost", "localhost:*", "127.0.0.1", "127.0.0.1:*"}
    if len(DefaultAllowedDomains) != len(expectedDomains) {
        t.Errorf("DefaultAllowedDomains length = %d, want %d", len(DefaultAllowedDomains), len(expectedDomains))
    }
    for i, domain := range expectedDomains {
        if DefaultAllowedDomains[i] != domain {
            t.Errorf("DefaultAllowedDomains[%d] = %q, want %q", i, DefaultAllowedDomains[i], domain)
        }
    }
}

After:

func TestDefaultAllowedDomains(t *testing.T) {
    require.NotEmpty(t, DefaultAllowedDomains)
    assert.Equal(t, []string{"localhost", "localhost:*", "127.0.0.1", "127.0.0.1:*"}, DefaultAllowedDomains)
}

---

Before (AllowedExpressions):

expressionsMap := make(map[string]bool)
for _, expr := range AllowedExpressions {
    expressionsMap[expr] = true
}
for _, required := range requiredExpressions {
    if !expressionsMap[required] {
        t.Errorf("AllowedExpressions missing required expression: %q", required)
    }
}

After:

for _, required := range requiredExpressions {
    assert.Contains(t, AllowedExpressions, required)
}

3. Table-Driven Refactors

• TestHelperMethods duplicates the same String() / IsValid() pattern across Version, JobName, StepID, CommandPrefix subtests. Extract a shared helper or single table.
• TestSemanticTypeAliases repeats the same conversion pattern for each type (URL, ModelName, JobName, StepID, CommandPrefix, EngineName). A table with name string, convert func() string, expected string would halve the line count.

Example: table-driven SemanticTypeAliases

func TestSemanticTypeAliases(t *testing.T) {
    cases := []struct {
        name    string
        got     string
        want    string
    }{
        {"URL", string(URL("(example.com/redacted)")), "(example.com/redacted)"},
        {"ModelName", string(ModelName("test-model")), "test-model"},
        {"JobName", string(JobName("test-job")), "test-job"},
        {"StepID", string(StepID("test-step")), "test-step"},
        {"CommandPrefix", string(CommandPrefix("gh aw")), "gh aw"},
        {"EngineName", string(EngineName("copilot")), "copilot"},
        {"DefaultMCPRegistryURL", string(DefaultMCPRegistryURL), "https://api.mcp.github.com/v0.1"},
        {"AgentJobName", string(AgentJobName), "agent"},
        {"CopilotEngine", string(CopilotEngine), "copilot"},
    }
    for _, tc := range cases {
        t.Run(tc.name, func(t *testing.T) {
            assert.Equal(t, tc.want, tc.got)
        })
    }
}

This replaces ~120 lines with ~25.

4. Organization / Readability

• TestConstantValues and TestSemanticTypeAliases overlap heavily — TestConstantValues already checks string values for most constants. Consider consolidating into a single exhaustive table test.
• TestKnownBuiltInJobNamesContainsAllKnownJobs would benefit from require.Contains instead of manual map lookup.
• Add import block using testify/assert and testify/require and remove reflect import (currently used only in TestEngineOptions_MultiProviderAlternatives — replace with assert.Equal).

---

Acceptance Checklist

• Add testify imports (github.com/stretchr/testify/assert, github.com/stretchr/testify/require)
• Replace t.Error/t.Errorf membership loops with assert.Contains
• Replace manual equality checks with assert.Equal / require.Equal
• Refactor TestSemanticTypeAliases and TestHelperMethods into consolidated tables
• Add missing tests: MCP timeout bounds, path constants, GetEngineOption for all engines
• Remove reflect import (replace reflect.DeepEqual with assert.Equal)
• Verify: make test-unit passes with no regressions

Generated by 🧪 Daily Testify Uber Super Expert · 30.9 AIC · ⌖ 11.9 AIC · ⊞ 5.2K · ◷

• expires on Jul 14, 2026, 10:17 AM UTC-08:00