TDD Assessment: jtk attachments Command

Summary

The test coverage for the API layer is good but incomplete. The CLI command layer has no tests, which is consistent with the rest of the codebase but represents a coverage gap for this new feature.

What's Well-Tested

API Layer (api/attachments.go):

Coverage Gaps

1. AddAttachment is completely untested

This is the most significant gap. AddAttachment (lines 104-188) has complex multipart form handling, goroutine-based streaming, and error paths that are not exercised:

• File open failure
• Multipart writer errors
• HTTP request creation failure
• Goroutine error propagation via errChan
• Response parsing
• API error responses (4xx/5xx)

Testing this is admittedly harder due to the goroutine and multipart complexity, but it's also where bugs are most likely to hide.

2. No HTTP error response tests

The existing tests only cover happy paths and input validation. None test:

• 404 responses for non-existent issues/attachments
• 401/403 authentication/authorization errors
• 500 server errors
• Malformed JSON responses

Compare with transitions_test.go which includes error response cases.

3. CLI command layer is untested

internal/cmd/attachments/attachments.go has no tests. This is consistent with the codebase pattern (no *_test.go files exist in internal/cmd/**), so I'm not flagging this as a blocker. However, the command layer does have logic worth testing:

• att.Created[:10] slice operation could panic on malformed dates
• File existence checking in runAdd
• Path resolution logic in runGet

Verdict

Acceptable for merge with the following caveats:

1. The test quality matches or exceeds the existing codebase patterns
2. The AddAttachment gap is notable but not blocking - this is complex code that may be better validated through integration testing
3. Error path coverage could be improved in a follow-up

Recommendations (Non-blocking)

If you want to strengthen coverage before merge:

// Add to attachments_test.go

func TestAddAttachment_EmptyIssueKey(t *testing.T) {
    client, _ := New(ClientConfig{URL: "http://unused", Email: "test@example.com", APIToken: "token"})
    _, err := client.AddAttachment("", "/tmp/file.txt")
    assert.Error(t, err)
    assert.Contains(t, err.Error(), "issue key is required")
}

func TestAddAttachment_EmptyFilePath(t *testing.T) {
    client, _ := New(ClientConfig{URL: "http://unused", Email: "test@example.com", APIToken: "token"})
    _, err := client.AddAttachment("PROJ-123", "")
    assert.Error(t, err)
    assert.Contains(t, err.Error(), "file path is required")
}

func TestGetIssueAttachments_NotFound(t *testing.T) {
    server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        w.WriteHeader(http.StatusNotFound)
        w.Write([]byte(`{"errorMessages":["Issue does not exist"]}`))
    }))
    defer server.Close()
    // ... test 404 handling
}

These would take ~15 minutes to add and would catch real bugs (e.g., the input validation exists but isn't verified by tests).