[testify-expert] Improve Test Quality: pkg/parser/github_urls_test.go

[github-actions]

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

Current State

• Test File: pkg/parser/github_urls_test.go
• Source File: pkg/parser/github_urls.go
• Test Functions: 10 test functions
• Lines of Code: 930 lines
• Exported Functions Covered: 5/5 (ParseGitHubURL, ParseRunURLExtended, ParsePRURL, ParseRepoFileURL, IsValidGitHubIdentifier)

Test Quality Analysis

Strengths ✅

1. All exported functions are tested — Good coverage of ParseGitHubURL, ParseRunURLExtended, ParsePRURL, ParseRepoFileURL, and IsValidGitHubIdentifier
2. Table-driven tests with t.Run() — All tests use the table-driven pattern with subtests, which is the recommended pattern in scratchpad/testing.md
3. Variety of edge cases — Tests include enterprise URLs, short URLs, various path formats, and invalid inputs

Areas for Improvement 🎯

1. No testify assertions used anywhere (High Priority)

The entire 930-line file uses plain Go testing patterns with manual if checks and t.Errorf() calls. The codebase standard (per scratchpad/testing.md) is to use testify assertions throughout.

Current pattern (anti-pattern, repeated ~80 times):

if tt.wantErr {
    if err == nil {
        t.Errorf("ParseGitHubURL() expected error but got none")
    }
    return
}

if err != nil {
    t.Errorf("ParseGitHubURL() unexpected error: %v", err)
    return
}

if components.Owner != tt.wantOwner {
    t.Errorf("ParseGitHubURL() owner = %v, want %v", components.Owner, tt.wantOwner)
}

Recommended (testify pattern):

if tt.wantErr {
    require.Error(t, err, "ParseGitHubURL(%q) should return an error", tt.url)
    return
}

require.NoError(t, err, "ParseGitHubURL(%q) should not return an error", tt.url)
assert.Equal(t, tt.wantOwner, components.Owner, "owner should match")
assert.Equal(t, tt.wantRepo,  components.Repo,  "repo should match")
assert.Equal(t, tt.wantHost,  components.Host,  "host should match")
assert.Equal(t, tt.wantRunID, components.Number, "run ID should match")
assert.Equal(t, URLTypeRun,   components.Type,   "URL type should be run")

Why this matters: require.NoError stops the test immediately on failure, preventing confusing nil-dereference panics. assert.Equal produces better diffs and readable messages.

2. Manual error-contains checks instead of assert.ErrorContains (Medium Priority)

TestParseGitHubURL_Errors manually calls strings.Contains on the error string, requiring an extra import and verbose code:

// ❌ CURRENT
if !strings.Contains(err.Error(), tt.errContains) {
    t.Errorf("ParseGitHubURL() error = %v, want error containing %v", err, tt.errContains)
}

// ✅ IMPROVED
require.Error(t, err, "should return an error for %q", tt.url)
assert.ErrorContains(t, err, tt.errContains, "error message should contain expected substring")

This also lets the strings import be removed from the test file.

3. Complex routing logic in TestParseRunURL hides intent (Medium Priority)

TestParseRunURL uses strconv.ParseInt to decide which function to call, mixing test routing logic with test assertions:

// ❌ CURRENT - confusing dual-function test
var components *GitHubURLComponents
var err error

if _, numErr := strconv.ParseInt(tt.input, 10, 64); numErr == nil {
    components, err = ParseRunURLExtended(tt.input)
} else {
    components, err = ParseGitHubURL(tt.input)
}

Recommendation: Split into two focused tests:

• TestParseRunURLExtended_NumericID — tests ParseRunURLExtended with numeric strings
• TestParseRunURLExtended_URLs — tests ParseRunURLExtended with full URLs

This also removes the strconv import dependency from the test file.

4. Missing assertion messages throughout (Low Priority)

None of the t.Errorf calls (and there would be none after migration) include contextual information about the test case that failed. After migrating to testify, ensure all assertions include messages:

// ❌ CURRENT
assert.Equal(t, tt.wantOwner, components.Owner)

// ✅ IMPROVED
assert.Equal(t, tt.wantOwner, components.Owner,
    "owner should match for URL %q", tt.url)

5. Thin coverage for issue URLs (Low Priority)

TestParseGitHubURL_IssueURLs has only 2 test cases: one valid URL and one invalid number. Compare to other URL type tests which have 4-6 cases. Consider adding:

• Issue URL with comment anchor (e.g., issues/123#issuecomment-456)
• Issue URL with enterprise host
• Issue URL with large number (max int64)

{
    name:      "Enterprise issue URL",
    url:       "(github.example.com/redacted),
    wantOwner: "owner",
    wantRepo:  "repo",
    wantIssue: 99,
    wantErr:   false,
},

6. IsValidGitHubIdentifier missing dot-notation test case (Low Priority)

The tests don't cover identifiers with dots (.), which can appear in GitHub usernames and some repository names:

{
    name:  "Identifier with dot",
    input: "user.name",
    want:  true,  // or false — depends on actual validation rules
},

Verify the behavior and add a test case to document it.

Implementation Guidelines

Priority Order

1. High: Migrate all t.Errorf/t.Fatalf patterns to require.*/assert.*
2. Medium: Replace manual strings.Contains with assert.ErrorContains
3. Medium: Split TestParseRunURL into two focused tests
4. Low: Add descriptive assertion messages to all assertions
5. Low: Add missing edge cases for issue URLs and dot identifiers

Before/After Example (Complete Function)

// ❌ BEFORE — TestParseGitHubURL_IssueURLs (current)
func TestParseGitHubURL_IssueURLs(t *testing.T) {
    tests := []struct {
        name      string
        url       string
        wantOwner string
        wantRepo  string
        wantIssue int64
        wantErr   bool
    }{
        {
            name:      "Valid issue URL",
            url:       "https://github.com/owner/repo/issues/123",
            wantOwner: "owner",
            wantRepo:  "repo",
            wantIssue: 123,
            wantErr:   false,
        },
        {
            name:    "Invalid issue number",
            url:     "https://github.com/owner/repo/issues/abc",
            wantErr: true,
        },
    }

    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            components, err := ParseGitHubURL(tt.url)

            if tt.wantErr {
                if err == nil {
                    t.Errorf("ParseGitHubURL() expected error but got none")
                }
                return
            }

            if err != nil {
                t.Errorf("ParseGitHubURL() unexpected error: %v", err)
                return
            }

            if components.Type != URLTypeIssue {
                t.Errorf("ParseGitHubURL() type = %v, want %v", components.Type, URLTypeIssue)
            }

            if components.Number != tt.wantIssue {
                t.Errorf("ParseGitHubURL() issueNumber = %v, want %v", components.Number, tt.wantIssue)
            }
        })
    }
}

// ✅ AFTER — using testify
func TestParseGitHubURL_IssueURLs(t *testing.T) {
    tests := []struct {
        name      string
        url       string
        wantOwner string
        wantRepo  string
        wantIssue int64
        wantErr   bool
    }{
        {
            name:      "Valid issue URL",
            url:       "https://github.com/owner/repo/issues/123",
            wantOwner: "owner",
            wantRepo:  "repo",
            wantIssue: 123,
        },
        {
            name:    "Invalid issue number",
            url:     "https://github.com/owner/repo/issues/abc",
            wantErr: true,
        },
        {
            name:      "Enterprise issue URL",
            url:       "(github.example.com/redacted),
            wantOwner: "owner",
            wantRepo:  "repo",
            wantIssue: 99,
        },
    }

    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            components, err := ParseGitHubURL(tt.url)

            if tt.wantErr {
                require.Error(t, err, "ParseGitHubURL(%q) should return an error", tt.url)
                return
            }

            require.NoError(t, err, "ParseGitHubURL(%q) should not return an error", tt.url)
            assert.Equal(t, URLTypeIssue, components.Type, "URL type should be issue")
            assert.Equal(t, tt.wantOwner, components.Owner, "owner should match")
            assert.Equal(t, tt.wantIssue, components.Number, "issue number should match")
        })
    }
}

Acceptance Criteria

• All t.Errorf/t.Fatalf replaced with require.*/assert.* testify calls
• Manual strings.Contains(err.Error(), ...) replaced with assert.ErrorContains
• TestParseRunURL split into two focused tests (numeric ID vs URL)
• All assertions include descriptive messages referencing the URL input
• strings and strconv imports removed (no longer needed after migration)
• Additional test cases added for issue URLs and dot-notation identifiers
• Tests pass: go test -v ./pkg/parser/ -run "TestParseGitHubURL\|TestParsePR\|TestParseRepo\|TestIsValid\|TestParseRun"
• Code follows patterns in scratchpad/testing.md

Additional Context

• Repository Testing Guidelines: See scratchpad/testing.md for comprehensive testing patterns
• Testify import: "github.com/stretchr/testify/assert" and "github.com/stretchr/testify/require"
• Example test for reference: pkg/parser/frontmatter_extraction_test.go for testify usage patterns

---

Priority: Medium
Effort: Medium (systematic migration of ~80 assertion patterns across 930 lines)
Expected Impact: Better test output on failure, clearer failure messages, aligned with codebase standards

Files Involved:

• Test file: pkg/parser/github_urls_test.go
• Source file: pkg/parser/github_urls.go

References: §22959539433

Generated by Daily Testify Uber Super Expert · ◷

• expires on Mar 13, 2026, 3:21 PM UTC