Test Coverage Assessment for PR #13

Summary

This refactoring PR has good overall test coverage. The existing tests adequately verify that the refactored code maintains the same behavior as before. However, I identified one notable gap that should be addressed.

What's Well Tested

Shared Client (shared/client/client.go)

• Client creation with default and custom options
• Trailing slash normalization
• GET, POST, PUT, DELETE operations
• Error handling for all common HTTP status codes (401, 403, 404, 400, 429, 500)
• Verbose output logging
• Context cancellation
• Path normalization (adding leading / when missing)

cfl API Client (tools/cfl/api/client.go)

• Client creation and delegation to shared client
• Auth header generation and verification
• Header propagation (Accept, Content-Type)
• Error response handling
• Context cancellation
• URL construction for relative paths
• Accessor methods (GetBaseURL(), GetAuthHeader()) are tested

jtk API Client (tools/jtk/api/client.go)

• Client validation (missing URL, email, API token)
• URL normalization (scheme addition, trailing slash removal)
• Jira-specific URL construction (BaseURL, AgileURL)
• HTTP operations with proper headers
• Verbose mode
• All consumer tests updated to new API patterns

Error Re-exports (tools/jtk/api/errors.go)

• All sentinel errors tested via sharederrors.ParseAPIError()
• IsNotFound(), IsUnauthorized(), IsForbidden() helpers tested
• Non-standard status codes verified

Config (tools/cfl/internal/config/config.go)

• Environment variable fallback (CFL_* → ATLASSIAN_*)
• Tests verify sharedconfig.GetEnvWithFallback() is called correctly

Gap Identified: Absolute URL Handling

Location: shared/client/client.go lines 75-77

// Check if path is an absolute URL
if strings.HasPrefix(path, "http://") || strings.HasPrefix(path, "https://") {
    url = path
}

This is new functionality added in this PR - the client can now accept absolute URLs that bypass BaseURL construction. However, there is no dedicated test for this behavior.

Why it matters: This feature is used by the Jira client which constructs full URLs like baseURL + "/rest/api/3" before calling the shared client. If this absolute URL detection breaks, requests would double up paths like https://example.com/rest/api/3/rest/api/3/issue.

Recommendation

Add a test case to shared/client/client_test.go:

t.Run("absolute URL bypasses BaseURL", func(t *testing.T) {
    server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        if r.URL.Path != "/external/api/test" {
            t.Errorf("Path = %v, want /external/api/test", r.URL.Path)
        }
        w.WriteHeader(http.StatusOK)
    }))
    defer server.Close()

    // Create client with a different base URL
    c := New("https://should-not-be-used.com", "user@example.com", "token", nil)
    
    // Pass absolute URL - should go directly to server, not prepend BaseURL
    _, err := c.Get(context.Background(), server.URL+"/external/api/test")
    if err != nil {
        t.Fatalf("Get() with absolute URL error = %v", err)
    }
})

Verdict

Coverage is sufficient for merging - this is a refactoring with minimal new behavior, and existing tests provide good confidence in backward compatibility. The missing test for absolute URL handling is a minor gap that can be addressed in a follow-up PR.

The test updates in this PR are well-structured:

• They verify the embedding pattern works correctly
• Consumer tests (fields_test.go, transitions_test.go, types_test.go) correctly use the new client construction
• Error handling tests verify shared error types work through re-exports