[Code Quality] Improve test assertions to use assert.Error() instead of assert.NotNil()

[github-actions]

Description

Many test files use assert.NotNil(t, err) for error checking instead of the more idiomatic assert.Error(t, err). This should be updated for better test readability and consistency with Go testing best practices.

Current Pattern (Anti-pattern)

// ❌ BAD: Using NotNil for error checking
if err != nil {
    assert.NotNil(t, err, "Expected error")
}

// or
assert.NotNil(t, err, "Expected error")

Recommended Pattern

// ✅ GOOD: Using Error/NoError for error checking
assert.Error(t, err, "Expected error when...")
assert.NoError(t, err, "Should not error when...")

Why This Matters

1. Semantic clarity: Error() explicitly checks for errors, not just non-nil values
2. Better error messages: Error() includes the actual error message in test output
3. Linter compliance: Aligns with testifylint best practices
4. Consistency: Matches the pattern used in AGENTS.md guidelines (lines 90-95)

Affected Areas

Tests throughout the codebase that check for errors using:

• assert.NotNil(t, err)
• assert.Nil(t, err) (should be assert.NoError(t, err))
• require.NotNil(t, err) (should be require.Error(t, err))
• require.Nil(t, err) (should be require.NoError(t, err))

Suggested Changes

Automated Refactoring

Use a script or manual find-replace to update:

# Find files with the anti-pattern
grep -r "assert\.NotNil(t, err" pkg/ cmd/ --include="*_test.go"
grep -r "assert\.Nil(t, err" pkg/ cmd/ --include="*_test.go"
grep -r "require\.NotNil(t, err" pkg/ cmd/ --include="*_test.go"
grep -r "require\.Nil(t, err" pkg/ cmd/ --include="*_test.go"

Replacement Patterns

// Pattern 1: assert.NotNil(t, err, msg) → assert.Error(t, err, msg)
assert.NotNil(t, err, "Expected error") 
// becomes:
assert.Error(t, err, "Expected error")

// Pattern 2: assert.Nil(t, err, msg) → assert.NoError(t, err, msg)
assert.Nil(t, err, "Should not error")
// becomes:
assert.NoError(t, err, "Should not error")

// Pattern 3: Same for require.*
require.NotNil(t, err, "Setup must fail")
// becomes:
require.Error(t, err, "Setup must fail")

Success Criteria

• ✅ All error assertions use Error() or NoError()
• ✅ No usage of NotNil() or Nil() for error checking
• ✅ All tests pass (make test-unit)
• ✅ Linter passes (make lint)
• ✅ Test error messages are descriptive

Benefits

• Better test output: Error messages show actual error content
• Code clarity: Intent is immediately clear
• Linter compliance: Eliminates testifylint warnings
• Consistency: Aligns with project guidelines in AGENTS.md
• Best practices: Follows Go testing community standards

Implementation Strategy

1. Phase 1: Update pkg/constants/ tests (smallest scope)
2. Phase 2: Update pkg/cli/ tests
3. Phase 3: Update pkg/workflow/ tests
4. Phase 4: Update remaining packages

Run make agent-finish after each phase to ensure no regressions.

Reference

• AGENTS.md lines 90-95: Testifylint assertion best practices
• Testify documentation

Source

Extracted from AGENTS.md testing guidelines and common linting patterns

Priority

Low-Medium - Code quality improvement that enhances test readability

AI generated by Discussion Task Miner - Code Quality Improvement Agent

• expires on Feb 10, 2026, 5:12 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 agent.

The Copilot agent will analyze the issue and create a pull request with the fix.

Om nom nom! 🍪

AI generated by Issue Monster