[testify-expert] Improve Test Quality: scripts/lint_error_messages_test.go

The test file `scripts/lint_error_messages_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**: `scripts/lint_error_messages_test.go`
- **Source File**: `scripts/lint_error_messages.go`
- **Test Functions**: 4 (`TestCheckErrorQuality`, `TestShouldSkipQualityCheck`, `TestSuggestImprovement`, `TestPatternMatching`)
- **Lines of Code**: 239 lines
- **Testify Usage**: ❌ None — the file has **no testify imports**

### Test Quality Analysis

### Strengths ✅

1. **Well-structured table-driven tests** — All 4 test functions use `tests := []struct{...}` with `t.Run()` and descriptive names.
2. **Good test case coverage** — `TestCheckErrorQuality` covers 11 distinct scenarios including edge cases (wrapped errors, doc links, short messages).
3. **Descriptive test case naming** — Names clearly communicate intent (e.g., `"bad validation error without example"`, `"wrapped error should pass"`).

<details>
<summary><b>🎯 Areas for Improvement</b></summary>

#### 1. No Testify Assertions — Entire File Uses Manual Comparisons

The entire file uses manual `if result != expected { t.Errorf(...) }` patterns instead of testify. This is the highest-priority improvement.

**Current anti-patterns throughout the file:**
```go
// ❌ CURRENT — Manual boolean comparison (ci_test.go pattern, same issue here)
if result != tt.shouldSkip {
    t.Errorf("shouldSkipQualityCheck(%q) = %v, want %v", tt.message, result, tt.shouldSkip)
}

// ❌ CURRENT — Manual nil check with complex logic
issue := checkErrorQuality(tt.message, 1)
passed := (issue == nil)
if passed != tt.shouldPass {
    if tt.shouldPass {
        t.Errorf("Expected message to pass quality check but failed: %s\nMessage: %q\nIssue: %v",
            tt.description, tt.message, issue)
    } else {
        t.Errorf("Expected message to fail quality check but passed: %s\nMessage: %q",
            tt.description, tt.message)
    }
}

// ❌ CURRENT — Manual empty string check
if result == "" {
    t.Errorf("suggestImprovement(%q) returned empty string", tt.message)
}
```

**Recommended changes:**
```go
import (
    "testing"
    "github.com/stretchr/testify/assert"
    "github.com/stretchr/testify/require"
)

// ✅ IMPROVED — TestShouldSkipQualityCheck
for _, tt := range tests {
    t.Run(tt.name, func(t *testing.T) {
        result := shouldSkipQualityCheck(tt.message)
        assert.Equal(t, tt.shouldSkip, result, "shouldSkipQualityCheck(%q) should return %v", tt.message, tt.shouldSkip)
    })
}

// ✅ IMPROVED — TestCheckErrorQuality (replaces the complex if/else logic)
for _, tt := range tests {
    t.Run(tt.name, func(t *testing.T) {
        issue := checkErrorQuality(tt.message, 1)
        if tt.shouldPass {
            assert.Nil(t, issue, "message should pass quality check: %s\nMessage: %q", tt.description, tt.message)
        } else {
            assert.NotNil(t, issue, "message should fail quality check: %s\nMessage: %q", tt.description, tt.message)
        }
    })
}

// ✅ IMPROVED — TestSuggestImprovement
for _, tt := range tests {
    t.Run(tt.name, func(t *testing.T) {
        result := suggestImprovement(tt.message)
        require.NotEmpty(t, result, "suggestImprovement(%q) should return a non-empty suggestion", tt.message)
        assert.Contains(t, result, tt.wantContain, "suggestion should reference the relevant concept")
    })
}
```

**Why this matters**: Testify provides significantly clearer failure output, is the standard used throughout the rest of the codebase, and reduces boilerplate. The complex if/else in `TestCheckErrorQuality` in particular is error-prone and harder to read.

#### 2. `TestSuggestImprovement` — Doesn't Verify Suggestion Content

The test currently checks that `result != ""` but ignores `tt.wantContain` entirely. The comment `// Just verify it returns something, specific suggestions may vary` means the `wantContain` field in the test struct is **defined but never used**, which is misleading.

**Current code:**
```go
for _, tt := range tests {
    t.Run(tt.name, func(t *testing.T) {
        result := suggestImprovement(tt.message)
        if result == "" {
            t.Errorf("suggestImprovement(%q) returned empty string", tt.message)
        }
        // Just verify it returns something, specific suggestions may vary
    })
}
```

**Recommended fix:**
```go
// ✅ Either use wantContain or remove the field
// Option A: Use the field (preferred)
for _, tt := range tests {
    t.Run(tt.name, func(t *testing.T) {
        result := suggestImprovement(tt.message)
        require.NotEmpty(t, result, "suggestImprovement(%q) should return a non-empty suggestion", tt.message)
        assert.Contains(t, strings.ToLower(result), tt.wantContain,
            "suggestion for %q should reference %q", tt.message, tt.wantContain)
    })
}

// Option B: Remove the unused field if content verification is intentionally skipped
tests := []struct {
    name    string
    message string
    // wantContain removed since we only verify non-empty
}{...}
```

**Why this matters**: Having a struct field that is never read is misleading — it implies the test validates content it doesn't actually verify. Fix it one way or the other.

#### 3. `TestPatternMatching` — Incomplete Pattern Coverage

The test only tests 4 of the 7 package-level regex patterns. The following are defined in `lint_error_messages.go` but have **zero test coverage** in `TestPatternMatching`:

- `isFormatError` — `(?i)\bformat\b`
- `isTypeError` — `(?i)\b(must be|got %T|expected type)\b`
- `isEnumError` — `(?i)\b(valid (options|values|engines|modes|levels)|one of)\b`
- `isWrappedError` — `%w`
- `hasDocLink` — `https?://`

Additionally, the current implementation uses a `switch` on `tt.pattern` with string literals (`"example"`, `"expected"`, `"validation"`), which is brittle — a typo in a test case string would silently skip the assertion entirely.

**Recommended additions:**
```go
func TestPatternMatching(t *testing.T) {
    tests := []struct {
        name    string
        message string
        // Test each pattern directly
        patternName string
        want        bool
    }{
        // Existing tests...
        {
            name:        "isFormatError matches format keyword",
            message:     "invalid time format",
            patternName: "isFormatError",
            want:        true,
        },
        {
            name:        "isTypeError matches must be",
            message:     "value must be a string",
            patternName: "isTypeError",
            want:        true,
        },
        {
            name:        "isTypeError matches got %T",
            message:     "expected string, got %T",
            patternName: "isTypeError",
            want:        true,
        },
        {
            name:        "isEnumError matches valid options",
            message:     "valid options are: a, b, c",
            patternName: "isEnumError",
            want:        true,
        },
        {
            name:        "isWrappedError matches %w",
            message:     "failed to parse: %w",
            patternName: "isWrappedError",
            want:        true,
        },
        {
            name:        "hasDocLink matches https URL",
            message:     "see (docs.example.com/redacted)",
            patternName: "hasDocLink",
            want:        true,
        },
        // Negative cases
        {
            name:        "isFormatError does not match unrelated message",
            message:     "something went wrong",
            patternName: "isFormatError",
            want:        false,
        },
    }

    patternMap := map[string]*regexp.Regexp{
        "example":     hasExample,
        "expected":    hasExpected,
        "validation":  isValidationError,
        "isFormatError": isFormatError,
        "isTypeError":   isTypeError,
        "isEnumError":   isEnumError,
        "isWrappedError": isWrappedError,
        "hasDocLink":    hasDocLink,
    }

    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            pattern, ok := patternMap[tt.patternName]
            require.True(t, ok, "unknown pattern name %q", tt.patternName)
            result := pattern.MatchString(tt.message)
            assert.Equal(t, tt.want, result, "pattern %q on %q", tt.patternName, tt.message)
        })
    }
}
```

#### 4. Missing Tests for `analyzeFile` Function

The core function `analyzeFile(path string) *FileStats` has **no unit tests**. This is the most important function in the linter — it parses Go source files and identifies quality issues. Testing it would catch regressions in the AST traversal logic.

**Recommended tests:**
```go
func TestAnalyzeFile(t *testing.T) {
    t.Run("file with compliant error messages", func(t *testing.T) {
        // Write a temp .go file with valid fmt.Errorf calls
        content := `package test
import "fmt"
func good() error {
    return fmt.Errorf("invalid engine: %s. Example: engine: copilot", "bad")
}`
        path := writeTempGoFile(t, content)
        stats := analyzeFile(path)
        require.NotNil(t, stats, "should return stats")
        assert.Equal(t, 1, stats.Total, "should count 1 error message")
        assert.Equal(t, 1, stats.Compliant, "should mark compliant message as compliant")
        assert.Empty(t, stats.Issues, "should have no issues")
    })

    t.Run("file with non-compliant error messages", func(t *testing.T) {
        content := `package test
import "fmt"
func bad() error {
    return fmt.Errorf("invalid configuration")
}`
        path := writeTempGoFile(t, content)
        stats := analyzeFile(path)
        require.NotNil(t, stats)
        assert.Equal(t, 1, stats.Total)
        assert.Equal(t, 0, stats.Compliant)
        assert.Len(t, stats.Issues, 1, "should report 1 issue")
    })

    t.Run("non-existent file returns empty stats", func(t *testing.T) {
        stats := analyzeFile("/nonexistent/path.go")
        require.NotNil(t, stats)
        assert.Equal(t, 0, stats.Total, "should have 0 messages for unreadable file")
    })
}

// Helper to write a temp Go file for testing
func writeTempGoFile(t *testing.T, content string) string {
    t.Helper()
    dir := t.TempDir()
    path := filepath.Join(dir, "test.go")
    require.NoError(t, os.WriteFile(path, []byte(content), 0644))
    return path
}
```

</details>

<details>
<summary><b>📋 Implementation Guidelines</b></summary>

#### Priority Order
1. **High**: Add `testify` import and replace all manual `if result != expected { t.Errorf(...) }` patterns
2. **High**: Fix the unused `wantContain` field in `TestSuggestImprovement` — either use it or remove it
3. **Medium**: Expand `TestPatternMatching` to cover all 7 regex patterns
4. **Medium**: Add `TestAnalyzeFile` for the core analysis function
5. **Low**: Add negative test cases to `TestPatternMatching`

#### Best Practices from `scratchpad/testing.md`
- ✅ Use `require.*` for critical setup (stops test on failure)
- ✅ Use `assert.*` for test validations (continues checking)
- ✅ 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
```bash
# Run tests for this file
go test -v ./scripts/ -run TestCheckErrorQuality
go test -v ./scripts/ -run TestShouldSkipQualityCheck
go test -v ./scripts/ -run TestSuggestImprovement
go test -v ./scripts/ -run TestPatternMatching

# Run all tests in the package
go test -v ./scripts/

# Run all unit tests
make test-unit
```

</details>

### Acceptance Criteria

- [ ] `testify` imported (`assert`, `require`) and used throughout
- [ ] All `if result != expected { t.Errorf(...) }` patterns replaced with `assert.Equal(t, ...)` or `assert.True/False`
- [ ] Complex `if passed != tt.shouldPass { if ... else ... }` in `TestCheckErrorQuality` replaced with `assert.Nil` / `assert.NotNil`
- [ ] `wantContain` field in `TestSuggestImprovement` either verified with `assert.Contains` or removed
- [ ] `TestPatternMatching` expanded to cover all 7 regex patterns: `isFormatError`, `isTypeError`, `isEnumError`, `isWrappedError`, `hasDocLink`
- [ ] `TestAnalyzeFile` added for the core `analyzeFile` function
- [ ] All assertions include helpful messages
- [ ] Tests pass: `make test-unit`
- [ ] Code follows patterns in `scratchpad/testing.md`

### Additional Context

- **Repository Testing Guidelines**: See `scratchpad/testing.md` for comprehensive testing patterns
- **Example Tests**: Look at `pkg/cli/access_log_test.go` for a well-structured example using testify
- **Testify Documentation**: https://github.com/stretchr/testify

---

**Priority**: Medium  
**Effort**: Small (primary changes are mechanical testify replacements + adding ~40 lines for new tests)  
**Expected Impact**: Improved failure diagnostics, cleaner assertion code, better coverage of regex patterns and `analyzeFile` function

**Files Involved:**
- Test file: `scripts/lint_error_messages_test.go`
- Source file: `scripts/lint_error_messages.go`

**References:**
- [§23795202270](https://github.com/github/gh-aw/actions/runs/23795202270)







> Generated by [Daily Testify Uber Super Expert](https://github.com/github/gh-aw/actions/runs/23795202270/agentic_workflow) · [◷](https://github.com/search?q=repo%3Agithub%2Fgh-aw+is%3Aissue+%22gh-aw-workflow-call-id%3A+github%2Fgh-aw%2Fdaily-testify-uber-super-expert%22&type=issues)
> - [x] expires  on Apr 2, 2026, 11:48 AM UTC

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[testify-expert] Improve Test Quality: scripts/lint_error_messages_test.go #23679

Current State

Test Quality Analysis

Strengths ✅

1. No Testify Assertions — Entire File Uses Manual Comparisons

2. `TestSuggestImprovement` — Doesn't Verify Suggestion Content

3. `TestPatternMatching` — Incomplete Pattern Coverage

4. Missing Tests for `analyzeFile` Function

Priority Order

Best Practices from `scratchpad/testing.md`

Testing Commands

Acceptance Criteria

Additional Context

Metadata

Assignees

Labels

Type

Fields

Projects

Milestone

Relationships

Development

[testify-expert] Improve Test Quality: scripts/lint_error_messages_test.go #23679

Description

Current State

Test Quality Analysis

Strengths ✅

1. No Testify Assertions — Entire File Uses Manual Comparisons

2. TestSuggestImprovement — Doesn't Verify Suggestion Content

3. TestPatternMatching — Incomplete Pattern Coverage

4. Missing Tests for analyzeFile Function

Priority Order

Best Practices from scratchpad/testing.md

Testing Commands

Acceptance Criteria

Additional Context

Metadata

Metadata

Assignees

Labels

Type

Fields

Projects

Milestone

Relationships

Development

Issue actions

2. `TestSuggestImprovement` — Doesn't Verify Suggestion Content

3. `TestPatternMatching` — Incomplete Pattern Coverage

4. Missing Tests for `analyzeFile` Function

Best Practices from `scratchpad/testing.md`