Merge pull request #258 from githubnext/copilot/review-go-module-testify

Author: pelikhan

.golangci.yml

@@ -10,6 +10,7 @@ linters:
     - misspell
     - unconvert
     # testifylint disabled due to requiring extensive test refactoring
+    # Re-enable after addressing existing issues: golangci-lint run --enable=testifylint
     # gosec disabled in golangci-lint due to configuration complexity
     # Can be enabled separately with: gosec -exclude=G101,G104,G115,G301,G302,G304,G306 ./...
   disable:
@@ -20,6 +21,8 @@ linters:
     - gosec
 
 linters-settings:
+  testifylint:
+    enable-all: true
   # Configuration for disabled linters is kept for reference if they are re-enabled
   errcheck:
     exclude-functions:

AGENTS.md

@@ -87,6 +87,87 @@ args = ["run", "--rm", "-e", "GITHUB_PERSONAL_ACCESS_TOKEN", "-i", "ghcr.io/gith
 - Godoc comments for exports
 - Mock external dependencies (Docker, network)
 
+## Testing with Testify
+
+**ALWAYS use testify for test assertions** - The project uses [stretchr/testify](https://github.com/stretchr/testify) for all test assertions.
+
+### Assert vs Require
+
+- **`require`**: Use for critical checks - test stops on failure
+- **`assert`**: Use for non-critical checks - test continues on failure
+
+```go
+import (
+    "github.com/stretchr/testify/assert"
+    "github.com/stretchr/testify/require"
+)
+
+func TestExample(t *testing.T) {
+    result, err := DoSomething()
+    require.NoError(t, err)  // Stop if error - can't continue
+    assert.Equal(t, "expected", result.Field)  // Continue even if fails
+}
+```
+
+### Bound Asserters
+
+For tests with multiple assertions, use bound asserters to reduce repetition:
+
+```go
+func TestMultipleAssertions(t *testing.T) {
+    assert := assert.New(t)
+    require := require.New(t)
+    
+    result := GetResult()
+    require.NotNil(result)  // Stop if nil
+    
+    // Cleaner - no need to pass 't' repeatedly
+    assert.Equal("value1", result.Field1)
+    assert.Equal("value2", result.Field2)
+    assert.True(result.Active)
+}
+```
+
+### Specific Assertion Methods
+
+Use specific assertion methods instead of generic ones for better error messages:
+
+```go
+// ❌ Avoid generic assertions
+assert.True(t, len(items) == 0)
+assert.True(t, err == nil)
+assert.True(t, strings.Contains(msg, "error"))
+
+// ✅ Use specific assertions
+assert.Empty(t, items)
+assert.NoError(t, err)
+assert.Contains(t, msg, "error")
+```
+
+### Common Patterns
+
+```go
+// Length checking
+assert.Len(t, items, 5, "Expected 5 items")
+
+// Unordered slice comparison
+assert.ElementsMatch(t, expected, actual, "Slices should contain same elements")
+
+// Nil checking
+assert.NotNil(t, obj, "Object should not be nil")
+assert.Nil(t, err, "Error should be nil")
+
+// Error checking (prefer NoError over Nil for errors)
+assert.NoError(t, err, "Operation should succeed")
+assert.Error(t, err, "Operation should fail")
+
+// HTTP status codes
+assert.Equal(t, http.StatusOK, response.StatusCode, "Should return 200 OK")
+
+// JSON comparison (ignores formatting)
+assert.JSONEq(t, expectedJSON, actualJSON)
+```
+
 ## Linting
 
 **golangci-lint** is integrated and runs as part of `make lint`:
@@ -96,9 +177,17 @@ args = ["run", "--rm", "-e", "GITHUB_PERSONAL_ACCESS_TOKEN", "-i", "ghcr.io/gith
 - Install: `make install` (installs golangci-lint v2.8.0)
 - Run manually: `golangci-lint run --timeout=5m`
 
-**Note**: Some linters (gosec, testifylint) are disabled to minimize noise. Enable them for stricter checks:
+**testifylint**: Available but disabled due to requiring extensive test refactoring across the codebase.
+- Automatically catches common testify mistakes:
+  - Suggests `assert.Empty(t, x)` instead of `assert.True(t, len(x) == 0)`
+  - Suggests `assert.True(t, x)` instead of `assert.Equal(t, true, x)`
+  - Suggests `assert.NoError(t, err)` instead of `assert.Nil(t, err)`
+- To run on specific files: `golangci-lint run --enable=testifylint --timeout=5m <files>`
+- To run on entire codebase: `golangci-lint run --enable=testifylint --timeout=5m`
+
+**Note**: Some linters (gosec, testifylint, errcheck) are disabled to minimize noise. Enable them for stricter checks:
 ```bash
-golangci-lint run --enable=gosec,testifylint --timeout=5m
+golangci-lint run --enable=gosec,testifylint,errcheck --timeout=5m
 ```
 
 ## Test Structure

internal/server/routed_test.go

@@ -43,28 +43,19 @@ func TestCloseEndpoint_Success(t *testing.T) {
 	httpServer.Handler.ServeHTTP(w, req)
 
 	// Verify response
-	if w.Code != http.StatusOK {
-		t.Errorf("Expected status 200, got %d", w.Code)
-	}
+	assert.Equal(t, http.StatusOK, w.Code, "Close endpoint should return 200 OK")
 
 	var response map[string]interface{}
-	if err := json.NewDecoder(w.Body).Decode(&response); err != nil {
-		t.Fatalf("Failed to decode response: %v", err)
-	}
+	require.NoError(t, json.NewDecoder(w.Body).Decode(&response), "Failed to decode response")
 
 	// Check response fields
-	if status, ok := response["status"].(string); !ok || status != "closed" {
-		t.Errorf("Expected status 'closed', got %v", response["status"])
-	}
-
-	if msg, ok := response["message"].(string); !ok || msg != "Gateway shutdown initiated" {
-		t.Errorf("Expected message 'Gateway shutdown initiated', got %v", response["message"])
-	}
+	assert.Equal(t, "closed", response["status"], "Expected status 'closed'")
+	assert.Equal(t, "Gateway shutdown initiated", response["message"], "Expected shutdown message")
 
 	// Should report 2 servers terminated
-	if count, ok := response["serversTerminated"].(float64); !ok || count != 2 {
-		t.Errorf("Expected serversTerminated 2, got %v", response["serversTerminated"])
-	}
+	serversTerminated, ok := response["serversTerminated"].(float64)
+	require.True(t, ok, "serversTerminated should be a number")
+	assert.InDelta(t, 2.0, serversTerminated, 0.01, "Expected 2 servers terminated")
 
 	// Verify server is marked as shutdown
 	assert.True(t, us.IsShutdown(), "Expected server to be marked as shutdown")

internal/server/unified_test.go

@@ -24,16 +24,9 @@ func TestUnifiedServer_GetServerIDs(t *testing.T) {
 	defer us.Close()
 
 	serverIDs := us.GetServerIDs()
-	if len(serverIDs) != 2 {
-		t.Errorf("Expected 2 server IDs, got %d", len(serverIDs))
-	}
+	assert.Len(t, serverIDs, 2, "Expected 2 server IDs")
 
-	expectedIDs := map[string]bool{"github": true, "fetch": true}
-	for _, id := range serverIDs {
-		if !expectedIDs[id] {
-			t.Errorf("Unexpected server ID: %s", id)
-		}
-	}
+	assert.ElementsMatch(t, []string{"github", "fetch"}, serverIDs, "Server IDs should match expected values")
 }
 
 func TestUnifiedServer_SessionManagement(t *testing.T) {
@@ -59,17 +52,9 @@ func TestUnifiedServer_SessionManagement(t *testing.T) {
 	session, exists := us.sessions[sessionID]
 	us.sessionMu.RUnlock()
 
-	if !exists {
-		t.Error("Session not found after creation")
-	}
-
-	if session.Token != token {
-		t.Errorf("Expected token '%s', got '%s'", token, session.Token)
-	}
-
-	if session.SessionID != sessionID {
-		t.Errorf("Expected session ID '%s', got '%s'", sessionID, session.SessionID)
-	}
+	assert.True(t, exists, "Session should exist after creation")
+	assert.Equal(t, token, session.Token, "Session token should match")
+	assert.Equal(t, sessionID, session.SessionID, "Session ID should match")
 }
 
 func TestUnifiedServer_GetSessionKeys(t *testing.T) {
@@ -91,18 +76,9 @@ func TestUnifiedServer_GetSessionKeys(t *testing.T) {
 	}
 
 	keys := us.getSessionKeys()
-	assert.Equal(t, len(sessions), len(keys))
+	assert.Len(t, keys, len(sessions), "Number of session keys should match")
 
-	keyMap := make(map[string]bool)
-	for _, key := range keys {
-		keyMap[key] = true
-	}
-
-	for _, expected := range sessions {
-		if !keyMap[expected] {
-			t.Errorf("Session key '%s' not found", expected)
-		}
-	}
+	assert.ElementsMatch(t, sessions, keys, "Session keys should match expected sessions")
 }
 
 func TestUnifiedServer_GetToolsForBackend(t *testing.T) {