From 9ed38f1383d97c402f35e93fda8bb181619be90b Mon Sep 17 00:00:00 2001 From: Tomasz Janiszewski Date: Thu, 11 Dec 2025 14:27:40 +0100 Subject: [PATCH 01/13] Add unit tests for internal/testutil and internal/toolsets/mock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add comprehensive unit tests with high coverage: - internal/testutil: 95.8% coverage (ports, server, config) - internal/toolsets/mock: 100% coverage (tool, toolset) Tests cover normal operation, edge cases, error handling, and interface compliance verification. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 --- internal/testutil/config_test.go | 172 ++++++++++++ internal/testutil/ports_test.go | 76 +++++ internal/testutil/server_test.go | 170 +++++++++++ internal/toolsets/mock/tool_test.go | 373 +++++++++++++++++++++++++ internal/toolsets/mock/toolset_test.go | 259 +++++++++++++++++ 5 files changed, 1050 insertions(+) create mode 100644 internal/testutil/config_test.go create mode 100644 internal/testutil/ports_test.go create mode 100644 internal/testutil/server_test.go create mode 100644 internal/toolsets/mock/tool_test.go create mode 100644 internal/toolsets/mock/toolset_test.go diff --git a/internal/testutil/config_test.go b/internal/testutil/config_test.go new file mode 100644 index 0000000..05d27c2 --- /dev/null +++ b/internal/testutil/config_test.go @@ -0,0 +1,172 @@ +package testutil + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func TestWriteYAMLFile_CreatesFile(t *testing.T) { + content := "key: value\nfoo: bar" + + filePath := WriteYAMLFile(t, content) + + data, err := os.ReadFile(filePath) + if err != nil { + t.Fatalf("Failed to read created file: %v", err) + } + + if string(data) != content { + t.Errorf("Expected content %q, got %q", content, string(data)) + } +} + +func TestWriteYAMLFile_ReturnsAbsolutePath(t *testing.T) { + filePath := WriteYAMLFile(t, "test: content") + + if !filepath.IsAbs(filePath) { + t.Errorf("Expected absolute path, got: %s", filePath) + } +} + +func TestWriteYAMLFile_HasYamlExtension(t *testing.T) { + filePath := WriteYAMLFile(t, "test: content") + + if !strings.HasSuffix(filePath, ".yaml") { + t.Errorf("Expected .yaml extension, got: %s", filePath) + } +} + +func TestWriteYAMLFile_FileIsReadable(t *testing.T) { + content := "readable: true\ntest: data" + filePath := WriteYAMLFile(t, content) + + data, err := os.ReadFile(filePath) + if err != nil { + t.Fatalf("File should be readable: %v", err) + } + + if len(data) == 0 { + t.Error("File should not be empty") + } +} + +func TestWriteYAMLFile_EmptyContent(t *testing.T) { + filePath := WriteYAMLFile(t, "") + + data, err := os.ReadFile(filePath) + if err != nil { + t.Fatalf("Should create file even with empty content: %v", err) + } + + if len(data) != 0 { + t.Errorf("Expected empty file, got %d bytes", len(data)) + } +} + +func TestWriteYAMLFile_MultiLineContent(t *testing.T) { + content := `server: + host: localhost + port: 8080 +database: + name: testdb + user: admin` + + filePath := WriteYAMLFile(t, content) + + data, err := os.ReadFile(filePath) + if err != nil { + t.Fatalf("Failed to read file: %v", err) + } + + if string(data) != content { + t.Errorf("Multi-line content mismatch.\nExpected:\n%s\n\nGot:\n%s", content, string(data)) + } +} + +func TestWriteYAMLFile_SpecialCharacters(t *testing.T) { + content := `special: "chars: -, |, >, &, *, #, @" +quote: 'single and "double"' +unicode: "测试"` + + filePath := WriteYAMLFile(t, content) + + data, err := os.ReadFile(filePath) + if err != nil { + t.Fatalf("Failed to read file: %v", err) + } + + if string(data) != content { + t.Errorf("Special characters not preserved.\nExpected:\n%s\n\nGot:\n%s", content, string(data)) + } +} + +func TestWriteYAMLFile_PathIncludesTestName(t *testing.T) { + filePath := WriteYAMLFile(t, "test: value") + + fileName := filepath.Base(filePath) + // The filename should contain part of the test name + // but sanitized (slashes replaced or handled) + if !strings.HasPrefix(fileName, "config-") { + t.Errorf("Expected filename to start with 'config-', got: %s", fileName) + } +} + +func TestWriteYAMLFile_FileExists(t *testing.T) { + filePath := WriteYAMLFile(t, "exists: true") + + if _, err := os.Stat(filePath); os.IsNotExist(err) { + t.Errorf("File should exist at path: %s", filePath) + } +} + +func TestWriteYAMLFile_InTempDirectory(t *testing.T) { + filePath := WriteYAMLFile(t, "test: content") + + // The file should be in a temp directory managed by t.TempDir() + // which will be automatically cleaned up + dir := filepath.Dir(filePath) + if dir == "" || dir == "." { + t.Errorf("File should be in a proper directory, got: %s", dir) + } +} + +func TestWriteYAMLFile_LargeContent(t *testing.T) { + // Test with larger YAML content + var sb strings.Builder + for i := 0; i < 100; i++ { + sb.WriteString("key") + sb.WriteString(string(rune('0' + i%10))) + sb.WriteString(": value") + sb.WriteString(string(rune('0' + i%10))) + sb.WriteString("\n") + } + content := sb.String() + + filePath := WriteYAMLFile(t, content) + + data, err := os.ReadFile(filePath) + if err != nil { + t.Fatalf("Failed to read file with large content: %v", err) + } + + if string(data) != content { + t.Error("Large content not preserved correctly") + } +} + +func ExampleWriteYAMLFile() { + t := &testing.T{} + + content := `name: example +version: 1.0` + + path := WriteYAMLFile(t, content) + data, _ := os.ReadFile(path) + + println(string(data)) + // Output will contain: + // name: example + // version: 1.0 +} diff --git a/internal/testutil/ports_test.go b/internal/testutil/ports_test.go new file mode 100644 index 0000000..0cd363a --- /dev/null +++ b/internal/testutil/ports_test.go @@ -0,0 +1,76 @@ +package testutil + +import ( + "testing" +) + +func TestGetPortForTest(t *testing.T) { + t.Run("returns port in valid range", func(t *testing.T) { + port := GetPortForTest(t) + + if port < minPort || port >= maxPort { + t.Errorf("Port %d is outside valid range [%d, %d)", port, minPort, maxPort) + } + }) + + t.Run("returns deterministic port for same test name", func(t *testing.T) { + port1 := GetPortForTest(t) + port2 := GetPortForTest(t) + + if port1 != port2 { + t.Errorf("Expected same port for same test name, got %d and %d", port1, port2) + } + }) + + t.Run("different test names get different ports", func(t *testing.T) { + ports := make(map[int]bool) + testCount := 0 + + // Create subtests with different names + for i := 0; i < 10; i++ { + t.Run("subtest", func(t *testing.T) { + port := GetPortForTest(t) + ports[port] = true + testCount++ + }) + } + + // We should have different ports for different test paths + if len(ports) < 2 { + t.Errorf("Expected multiple different ports, got %d unique ports from %d tests", len(ports), testCount) + } + }) + + t.Run("port is within safe range avoiding privileged ports", func(t *testing.T) { + port := GetPortForTest(t) + + if port < 1024 { + t.Errorf("Port %d is in privileged range (< 1024)", port) + } + + if port >= 65536 { + t.Errorf("Port %d exceeds maximum valid port number", port) + } + }) +} + +func TestPortRangeConstants(t *testing.T) { + t.Run("minPort is above privileged range", func(t *testing.T) { + if minPort < 1024 { + t.Errorf("minPort %d should be above 1024 to avoid privileged ports", minPort) + } + }) + + t.Run("maxPort is below max valid port", func(t *testing.T) { + if maxPort > 65536 { + t.Errorf("maxPort %d should be below 65536", maxPort) + } + }) + + t.Run("port range is reasonable", func(t *testing.T) { + portRange := maxPort - minPort + if portRange < 1000 { + t.Errorf("Port range %d is too small, may cause collisions", portRange) + } + }) +} diff --git a/internal/testutil/server_test.go b/internal/testutil/server_test.go new file mode 100644 index 0000000..de33795 --- /dev/null +++ b/internal/testutil/server_test.go @@ -0,0 +1,170 @@ +package testutil + +import ( + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" +) + +func TestWaitForServerReady(t *testing.T) { + t.Run("returns nil when server is ready immediately", func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + err := WaitForServerReady(server.URL, 1*time.Second) + if err != nil { + t.Errorf("Expected no error, got: %v", err) + } + }) + + t.Run("returns nil when server becomes ready after a delay", func(t *testing.T) { + ready := false + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if ready { + w.WriteHeader(http.StatusOK) + } else { + w.WriteHeader(http.StatusServiceUnavailable) + } + })) + defer server.Close() + + // Make server ready after a short delay + go func() { + time.Sleep(200 * time.Millisecond) + ready = true + }() + + err := WaitForServerReady(server.URL, 2*time.Second) + if err != nil { + t.Errorf("Expected no error, got: %v", err) + } + }) + + t.Run("returns error when server never becomes ready", func(t *testing.T) { + // Use an address that won't have a server + err := WaitForServerReady("http://localhost:59999", 300*time.Millisecond) + if err == nil { + t.Error("Expected error when server is not ready, got nil") + } + + expectedSubstring := "did not become ready" + if !strings.Contains(err.Error(), expectedSubstring) { + t.Errorf("Expected error to contain %q, got: %v", expectedSubstring, err) + } + }) + + t.Run("respects timeout parameter", func(t *testing.T) { + start := time.Now() + timeout := 300 * time.Millisecond + + err := WaitForServerReady("http://localhost:59998", timeout) + + elapsed := time.Since(start) + + if err == nil { + t.Error("Expected error when server is not ready") + } + + // Allow some margin for timing (timeout + 200ms for final attempt) + maxExpected := timeout + 300*time.Millisecond + if elapsed > maxExpected { + t.Errorf("Expected to wait approximately %v, but waited %v", timeout, elapsed) + } + + if elapsed < timeout { + t.Errorf("Expected to wait at least %v, but only waited %v", timeout, elapsed) + } + }) + + t.Run("error message includes timeout duration", func(t *testing.T) { + timeout := 250 * time.Millisecond + err := WaitForServerReady("http://localhost:59997", timeout) + + if err == nil { + t.Fatal("Expected error, got nil") + } + + if !strings.Contains(err.Error(), "250ms") { + t.Errorf("Expected error message to include timeout duration, got: %v", err) + } + }) + + t.Run("works with different HTTP status codes", func(t *testing.T) { + tests := []struct { + name string + statusCode int + }{ + {"200 OK", http.StatusOK}, + {"201 Created", http.StatusCreated}, + {"404 Not Found", http.StatusNotFound}, + {"500 Internal Server Error", http.StatusInternalServerError}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(tt.statusCode) + })) + defer server.Close() + + err := WaitForServerReady(server.URL, 1*time.Second) + if err != nil { + t.Errorf("Expected no error for status %d, got: %v", tt.statusCode, err) + } + }) + } + }) + + t.Run("handles invalid URL gracefully", func(t *testing.T) { + err := WaitForServerReady("http://invalid-host-that-does-not-exist.local:12345", 200*time.Millisecond) + if err == nil { + t.Error("Expected error for invalid URL, got nil") + } + }) +} + +func TestWaitForServerReadyIntegration(t *testing.T) { + t.Run("simulates real server startup scenario", func(t *testing.T) { + var server *httptest.Server + serverReady := make(chan struct{}) + + // Simulate server starting up in background + go func() { + time.Sleep(150 * time.Millisecond) + server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + close(serverReady) + }() + + // Wait for server to be created + <-serverReady + defer server.Close() + + err := WaitForServerReady(server.URL, 2*time.Second) + if err != nil { + t.Errorf("Expected server to become ready, got error: %v", err) + } + }) +} + +func ExampleWaitForServerReady() { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + err := WaitForServerReady(server.URL, 5*time.Second) + if err != nil { + fmt.Printf("Server not ready: %v\n", err) + return + } + + fmt.Println("Server is ready!") + // Output: Server is ready! +} diff --git a/internal/toolsets/mock/tool_test.go b/internal/toolsets/mock/tool_test.go new file mode 100644 index 0000000..7020fb9 --- /dev/null +++ b/internal/toolsets/mock/tool_test.go @@ -0,0 +1,373 @@ +package mock + +import ( + "testing" + + "github.com/stackrox/stackrox-mcp/internal/toolsets" +) + +func TestNewTool(t *testing.T) { + t.Run("creates tool with provided values", func(t *testing.T) { + name := "test-tool" + readOnly := true + + tool := NewTool(name, readOnly) + + if tool.NameValue != name { + t.Errorf("Expected NameValue %q, got %q", name, tool.NameValue) + } + + if tool.ReadOnlyValue != readOnly { + t.Errorf("Expected ReadOnlyValue %v, got %v", readOnly, tool.ReadOnlyValue) + } + + if tool.RegisterCalled { + t.Error("Expected RegisterCalled to be false initially") + } + }) + + t.Run("creates read-only tool", func(t *testing.T) { + tool := NewTool("readonly", true) + + if !tool.ReadOnlyValue { + t.Error("Expected tool to be read-only") + } + }) + + t.Run("creates writable tool", func(t *testing.T) { + tool := NewTool("writable", false) + + if tool.ReadOnlyValue { + t.Error("Expected tool to be writable") + } + }) + + t.Run("creates tool with empty name", func(t *testing.T) { + tool := NewTool("", true) + + if tool.NameValue != "" { + t.Errorf("Expected empty name, got %q", tool.NameValue) + } + }) +} + +func TestTool_GetName(t *testing.T) { + t.Run("returns configured name", func(t *testing.T) { + name := "my-tool" + tool := NewTool(name, true) + + if tool.GetName() != name { + t.Errorf("Expected name %q, got %q", name, tool.GetName()) + } + }) + + t.Run("returns empty string if configured", func(t *testing.T) { + tool := NewTool("", false) + + if tool.GetName() != "" { + t.Errorf("Expected empty name, got %q", tool.GetName()) + } + }) + + t.Run("name with special characters", func(t *testing.T) { + name := "tool-name_123!@#" + tool := NewTool(name, true) + + if tool.GetName() != name { + t.Errorf("Expected name %q, got %q", name, tool.GetName()) + } + }) +} + +func TestTool_IsReadOnly(t *testing.T) { + t.Run("returns true when read-only", func(t *testing.T) { + tool := NewTool("readonly", true) + + if !tool.IsReadOnly() { + t.Error("Expected tool to be read-only") + } + }) + + t.Run("returns false when writable", func(t *testing.T) { + tool := NewTool("writable", false) + + if tool.IsReadOnly() { + t.Error("Expected tool to be writable") + } + }) + + t.Run("can toggle read-only state", func(t *testing.T) { + tool := NewTool("toggle", true) + + if !tool.IsReadOnly() { + t.Error("Expected initially read-only") + } + + tool.ReadOnlyValue = false + + if tool.IsReadOnly() { + t.Error("Expected writable after toggle") + } + }) +} + +func TestTool_GetTool(t *testing.T) { + t.Run("returns MCP tool definition", func(t *testing.T) { + name := "test-tool" + tool := NewTool(name, true) + + mcpTool := tool.GetTool() + + if mcpTool == nil { + t.Fatal("Expected non-nil MCP tool") + } + + if mcpTool.Name != name { + t.Errorf("Expected MCP tool name %q, got %q", name, mcpTool.Name) + } + + if mcpTool.Description == "" { + t.Error("Expected non-empty description") + } + + expectedDesc := "Mock tool for testing" + if mcpTool.Description != expectedDesc { + t.Errorf("Expected description %q, got %q", expectedDesc, mcpTool.Description) + } + }) + + t.Run("returns new tool instance each time", func(t *testing.T) { + tool := NewTool("test", true) + + mcpTool1 := tool.GetTool() + mcpTool2 := tool.GetTool() + + // Should be different instances + if mcpTool1 == mcpTool2 { + t.Error("Expected different instances, got same pointer") + } + + // But with same values + if mcpTool1.Name != mcpTool2.Name { + t.Error("Expected same name in both instances") + } + }) + + t.Run("MCP tool has correct structure", func(t *testing.T) { + tool := NewTool("structured-tool", false) + + mcpTool := tool.GetTool() + + if mcpTool.Name == "" { + t.Error("MCP tool should have a name") + } + + if mcpTool.Description == "" { + t.Error("MCP tool should have a description") + } + }) +} + +func TestTool_RegisterWith(t *testing.T) { + t.Run("sets RegisterCalled flag", func(t *testing.T) { + tool := NewTool("register-test", true) + + if tool.RegisterCalled { + t.Error("Expected RegisterCalled to be false initially") + } + + tool.RegisterWith(nil) + + if !tool.RegisterCalled { + t.Error("Expected RegisterCalled to be true after calling RegisterWith") + } + }) + + t.Run("can be called multiple times", func(t *testing.T) { + tool := NewTool("multi-register", false) + + tool.RegisterWith(nil) + if !tool.RegisterCalled { + t.Error("Expected RegisterCalled to be true after first call") + } + + tool.RegisterWith(nil) + if !tool.RegisterCalled { + t.Error("Expected RegisterCalled to remain true after second call") + } + }) + + t.Run("accepts nil server", func(t *testing.T) { + tool := NewTool("nil-server", true) + + // Should not panic + tool.RegisterWith(nil) + + if !tool.RegisterCalled { + t.Error("Expected RegisterCalled to be true") + } + }) + + t.Run("can track registration state", func(t *testing.T) { + tool := NewTool("track-registration", true) + + // Reset the flag + tool.RegisterCalled = false + + if tool.RegisterCalled { + t.Error("Expected RegisterCalled to be false after reset") + } + + tool.RegisterWith(nil) + + if !tool.RegisterCalled { + t.Error("Expected RegisterCalled to be true after registration") + } + }) +} + +func TestTool_InterfaceCompliance(t *testing.T) { + t.Run("implements toolsets.Tool interface", func(t *testing.T) { + var _ toolsets.Tool = (*Tool)(nil) + }) + + t.Run("can be used as toolsets.Tool", func(t *testing.T) { + var tool toolsets.Tool = NewTool("interface-test", true) + + if tool.GetName() != "interface-test" { + t.Errorf("Expected name 'interface-test', got %q", tool.GetName()) + } + + if !tool.IsReadOnly() { + t.Error("Expected tool to be read-only") + } + + mcpTool := tool.GetTool() + if mcpTool == nil { + t.Error("Expected non-nil MCP tool") + } + + tool.RegisterWith(nil) + // Can't check RegisterCalled through interface, but shouldn't panic + }) +} + +func TestTool_EdgeCases(t *testing.T) { + t.Run("tool with very long name", func(t *testing.T) { + longName := "very-long-tool-name-that-might-be-used-in-some-edge-case-scenario-for-testing-purposes" + tool := NewTool(longName, true) + + if tool.GetName() != longName { + t.Errorf("Expected long name to be preserved") + } + + mcpTool := tool.GetTool() + if mcpTool.Name != longName { + t.Error("Expected MCP tool to have long name") + } + }) + + t.Run("tool state is mutable", func(t *testing.T) { + tool := NewTool("mutable", true) + + // Change name + tool.NameValue = "new-name" + if tool.GetName() != "new-name" { + t.Error("Expected name to be mutable") + } + + // Change read-only + tool.ReadOnlyValue = false + if tool.IsReadOnly() { + t.Error("Expected read-only to be mutable") + } + + // Change register flag + tool.RegisterCalled = true + if !tool.RegisterCalled { + t.Error("Expected register flag to be mutable") + } + }) + + t.Run("multiple tools with same name", func(t *testing.T) { + tool1 := NewTool("same-name", true) + tool2 := NewTool("same-name", false) + + if tool1.GetName() != tool2.GetName() { + t.Error("Expected both tools to have same name") + } + + if tool1 == tool2 { + t.Error("Expected different tool instances") + } + + if tool1.IsReadOnly() == tool2.IsReadOnly() { + t.Error("Expected different read-only values") + } + }) +} + +func TestTool_UsageScenarios(t *testing.T) { + t.Run("typical read-only tool workflow", func(t *testing.T) { + tool := NewTool("read-tool", true) + + // Check initial state + if !tool.IsReadOnly() { + t.Error("Expected read-only tool") + } + + if tool.RegisterCalled { + t.Error("Should not be registered initially") + } + + // Get MCP definition + mcpTool := tool.GetTool() + if mcpTool.Name != "read-tool" { + t.Error("MCP tool should have correct name") + } + + // Register with server + tool.RegisterWith(nil) + if !tool.RegisterCalled { + t.Error("Should be registered after RegisterWith call") + } + }) + + t.Run("typical writable tool workflow", func(t *testing.T) { + tool := NewTool("write-tool", false) + + // Check initial state + if tool.IsReadOnly() { + t.Error("Expected writable tool") + } + + // Get tool definition and register + _ = tool.GetTool() + tool.RegisterWith(nil) + + if !tool.RegisterCalled { + t.Error("Should be registered") + } + }) + + t.Run("tool in toolset context", func(t *testing.T) { + tool1 := NewTool("tool1", true) + tool2 := NewTool("tool2", false) + + tools := []toolsets.Tool{tool1, tool2} + + for _, tool := range tools { + if tool.GetName() == "" { + t.Error("Tool in toolset should have name") + } + + _ = tool.GetTool() + tool.RegisterWith(nil) + } + + // Check that both were registered + if !tool1.RegisterCalled || !tool2.RegisterCalled { + t.Error("All tools should be registered") + } + }) +} diff --git a/internal/toolsets/mock/toolset_test.go b/internal/toolsets/mock/toolset_test.go new file mode 100644 index 0000000..d62cb05 --- /dev/null +++ b/internal/toolsets/mock/toolset_test.go @@ -0,0 +1,259 @@ +package mock + +import ( + "testing" + + "github.com/stackrox/stackrox-mcp/internal/toolsets" +) + +func TestNewToolset(t *testing.T) { + t.Run("creates toolset with provided values", func(t *testing.T) { + name := "test-toolset" + enabled := true + tools := []toolsets.Tool{ + NewTool("tool1", true), + NewTool("tool2", false), + } + + ts := NewToolset(name, enabled, tools) + + if ts.NameValue != name { + t.Errorf("Expected NameValue %q, got %q", name, ts.NameValue) + } + + if ts.EnabledValue != enabled { + t.Errorf("Expected EnabledValue %v, got %v", enabled, ts.EnabledValue) + } + + if len(ts.ToolsValue) != len(tools) { + t.Errorf("Expected %d tools, got %d", len(tools), len(ts.ToolsValue)) + } + }) + + t.Run("creates disabled toolset", func(t *testing.T) { + ts := NewToolset("disabled-toolset", false, nil) + + if ts.EnabledValue { + t.Error("Expected toolset to be disabled") + } + }) + + t.Run("creates toolset with empty tools", func(t *testing.T) { + ts := NewToolset("empty-toolset", true, []toolsets.Tool{}) + + if ts.ToolsValue == nil { + t.Error("Expected non-nil ToolsValue, got nil") + } + + if len(ts.ToolsValue) != 0 { + t.Errorf("Expected 0 tools, got %d", len(ts.ToolsValue)) + } + }) + + t.Run("creates toolset with nil tools", func(t *testing.T) { + ts := NewToolset("nil-tools", true, nil) + + if ts.ToolsValue != nil { + t.Errorf("Expected nil ToolsValue, got %v", ts.ToolsValue) + } + }) +} + +func TestToolset_GetName(t *testing.T) { + t.Run("returns configured name", func(t *testing.T) { + name := "my-toolset" + ts := NewToolset(name, true, nil) + + if ts.GetName() != name { + t.Errorf("Expected name %q, got %q", name, ts.GetName()) + } + }) + + t.Run("returns empty string if configured", func(t *testing.T) { + ts := NewToolset("", true, nil) + + if ts.GetName() != "" { + t.Errorf("Expected empty name, got %q", ts.GetName()) + } + }) +} + +func TestToolset_IsEnabled(t *testing.T) { + t.Run("returns true when enabled", func(t *testing.T) { + ts := NewToolset("enabled", true, nil) + + if !ts.IsEnabled() { + t.Error("Expected toolset to be enabled") + } + }) + + t.Run("returns false when disabled", func(t *testing.T) { + ts := NewToolset("disabled", false, nil) + + if ts.IsEnabled() { + t.Error("Expected toolset to be disabled") + } + }) + + t.Run("can toggle enabled state", func(t *testing.T) { + ts := NewToolset("toggle", true, nil) + + if !ts.IsEnabled() { + t.Error("Expected initially enabled") + } + + ts.EnabledValue = false + + if ts.IsEnabled() { + t.Error("Expected disabled after toggle") + } + }) +} + +func TestToolset_GetTools(t *testing.T) { + t.Run("returns tools when enabled", func(t *testing.T) { + tools := []toolsets.Tool{ + NewTool("tool1", true), + NewTool("tool2", false), + NewTool("tool3", true), + } + ts := NewToolset("enabled-toolset", true, tools) + + result := ts.GetTools() + + if len(result) != len(tools) { + t.Errorf("Expected %d tools, got %d", len(tools), len(result)) + } + + for i, tool := range result { + if tool != tools[i] { + t.Errorf("Tool at index %d doesn't match", i) + } + } + }) + + t.Run("returns empty slice when disabled", func(t *testing.T) { + tools := []toolsets.Tool{ + NewTool("tool1", true), + NewTool("tool2", false), + } + ts := NewToolset("disabled-toolset", false, tools) + + result := ts.GetTools() + + if result == nil { + t.Error("Expected non-nil slice, got nil") + } + + if len(result) != 0 { + t.Errorf("Expected empty slice when disabled, got %d tools", len(result)) + } + }) + + t.Run("returns empty slice when enabled with no tools", func(t *testing.T) { + ts := NewToolset("no-tools", true, []toolsets.Tool{}) + + result := ts.GetTools() + + if result == nil { + t.Error("Expected non-nil slice, got nil") + } + + if len(result) != 0 { + t.Errorf("Expected 0 tools, got %d", len(result)) + } + }) + + t.Run("returns nil when enabled with nil tools", func(t *testing.T) { + ts := NewToolset("nil-tools", true, nil) + + result := ts.GetTools() + + if result != nil { + t.Errorf("Expected nil, got %v", result) + } + }) + + t.Run("changing enabled state changes returned tools", func(t *testing.T) { + tools := []toolsets.Tool{NewTool("tool1", true)} + ts := NewToolset("toggle-toolset", true, tools) + + // Initially enabled - should return tools + result1 := ts.GetTools() + if len(result1) != 1 { + t.Errorf("Expected 1 tool when enabled, got %d", len(result1)) + } + + // Disable - should return empty slice + ts.EnabledValue = false + result2 := ts.GetTools() + if len(result2) != 0 { + t.Errorf("Expected 0 tools when disabled, got %d", len(result2)) + } + + // Re-enable - should return tools again + ts.EnabledValue = true + result3 := ts.GetTools() + if len(result3) != 1 { + t.Errorf("Expected 1 tool when re-enabled, got %d", len(result3)) + } + }) +} + +func TestToolset_InterfaceCompliance(t *testing.T) { + t.Run("implements toolsets.Toolset interface", func(t *testing.T) { + var _ toolsets.Toolset = (*Toolset)(nil) + }) + + t.Run("can be used as toolsets.Toolset", func(t *testing.T) { + var ts toolsets.Toolset = NewToolset("interface-test", true, nil) + + if ts.GetName() != "interface-test" { + t.Errorf("Expected name 'interface-test', got %q", ts.GetName()) + } + + if !ts.IsEnabled() { + t.Error("Expected toolset to be enabled") + } + + tools := ts.GetTools() + if tools != nil { + t.Errorf("Expected nil tools, got %v", tools) + } + }) +} + +func TestToolset_EdgeCases(t *testing.T) { + t.Run("toolset with single tool", func(t *testing.T) { + tools := []toolsets.Tool{NewTool("only-tool", true)} + ts := NewToolset("single", true, tools) + + result := ts.GetTools() + if len(result) != 1 { + t.Errorf("Expected 1 tool, got %d", len(result)) + } + }) + + t.Run("toolset with many tools", func(t *testing.T) { + tools := make([]toolsets.Tool, 100) + for i := 0; i < 100; i++ { + tools[i] = NewTool("tool", i%2 == 0) + } + + ts := NewToolset("many-tools", true, tools) + + result := ts.GetTools() + if len(result) != 100 { + t.Errorf("Expected 100 tools, got %d", len(result)) + } + }) + + t.Run("toolset name with special characters", func(t *testing.T) { + name := "tool-set_123!@#" + ts := NewToolset(name, true, nil) + + if ts.GetName() != name { + t.Errorf("Expected name %q, got %q", name, ts.GetName()) + } + }) +} From c2d98466feda4cd445d96adc5fb41ba3080000b2 Mon Sep 17 00:00:00 2001 From: Tomasz Janiszewski Date: Thu, 11 Dec 2025 14:48:36 +0100 Subject: [PATCH 02/13] fix Signed-off-by: Tomasz Janiszewski --- Makefile | 2 +- internal/testutil/config_test.go | 39 ++-- internal/testutil/ports_test.go | 2 +- internal/testutil/server_test.go | 205 +++++++++++---------- internal/toolsets/mock/tool_test.go | 124 ++++++------- internal/toolsets/mock/toolset_test.go | 242 ++++++++++++------------- 6 files changed, 303 insertions(+), 311 deletions(-) diff --git a/Makefile b/Makefile index db172a3..ff52fed 100644 --- a/Makefile +++ b/Makefile @@ -56,7 +56,7 @@ test: ## Run unit tests .PHONY: test-coverage-and-junit test-coverage-and-junit: ## Run unit tests with coverage and junit output go install github.com/jstemmer/go-junit-report/v2@v2.1.0 - $(GOTEST) -v -cover -race -coverprofile=$(COVERAGE_OUT) ./... -json 2>&1 | go-junit-report -parser gojson > $(JUNIT_OUT) + $(GOTEST) -v -cover -race -coverprofile=$(COVERAGE_OUT) ./... -json 2>&1 | go-junit-report -set-exit-code -parser gojson -iocopy -out $(JUNIT_OUT) .PHONY: coverage-html coverage-html: test ## Generate and open HTML coverage report diff --git a/internal/testutil/config_test.go b/internal/testutil/config_test.go index 05d27c2..9803857 100644 --- a/internal/testutil/config_test.go +++ b/internal/testutil/config_test.go @@ -12,6 +12,7 @@ func TestWriteYAMLFile_CreatesFile(t *testing.T) { filePath := WriteYAMLFile(t, content) + //nolint:gosec // Test code reading from known test file data, err := os.ReadFile(filePath) if err != nil { t.Fatalf("Failed to read created file: %v", err) @@ -42,6 +43,7 @@ func TestWriteYAMLFile_FileIsReadable(t *testing.T) { content := "readable: true\ntest: data" filePath := WriteYAMLFile(t, content) + //nolint:gosec // Test code reading from known test file data, err := os.ReadFile(filePath) if err != nil { t.Fatalf("File should be readable: %v", err) @@ -55,6 +57,7 @@ func TestWriteYAMLFile_FileIsReadable(t *testing.T) { func TestWriteYAMLFile_EmptyContent(t *testing.T) { filePath := WriteYAMLFile(t, "") + //nolint:gosec // Test code reading from known test file data, err := os.ReadFile(filePath) if err != nil { t.Fatalf("Should create file even with empty content: %v", err) @@ -75,6 +78,7 @@ database: filePath := WriteYAMLFile(t, content) + //nolint:gosec // Test code reading from known test file data, err := os.ReadFile(filePath) if err != nil { t.Fatalf("Failed to read file: %v", err) @@ -86,12 +90,14 @@ database: } func TestWriteYAMLFile_SpecialCharacters(t *testing.T) { + //nolint:gosmopolitan // Testing unicode support content := `special: "chars: -, |, >, &, *, #, @" quote: 'single and "double"' unicode: "测试"` filePath := WriteYAMLFile(t, content) + //nolint:gosec // Test code reading from known test file data, err := os.ReadFile(filePath) if err != nil { t.Fatalf("Failed to read file: %v", err) @@ -134,18 +140,20 @@ func TestWriteYAMLFile_InTempDirectory(t *testing.T) { func TestWriteYAMLFile_LargeContent(t *testing.T) { // Test with larger YAML content - var sb strings.Builder - for i := 0; i < 100; i++ { - sb.WriteString("key") - sb.WriteString(string(rune('0' + i%10))) - sb.WriteString(": value") - sb.WriteString(string(rune('0' + i%10))) - sb.WriteString("\n") + var builder strings.Builder + for i := range 100 { + builder.WriteString("key") + builder.WriteRune(rune('0' + i%10)) + builder.WriteString(": value") + builder.WriteRune(rune('0' + i%10)) + builder.WriteString("\n") } - content := sb.String() + + content := builder.String() filePath := WriteYAMLFile(t, content) + //nolint:gosec // Test code reading from known test file data, err := os.ReadFile(filePath) if err != nil { t.Fatalf("Failed to read file with large content: %v", err) @@ -155,18 +163,3 @@ func TestWriteYAMLFile_LargeContent(t *testing.T) { t.Error("Large content not preserved correctly") } } - -func ExampleWriteYAMLFile() { - t := &testing.T{} - - content := `name: example -version: 1.0` - - path := WriteYAMLFile(t, content) - data, _ := os.ReadFile(path) - - println(string(data)) - // Output will contain: - // name: example - // version: 1.0 -} diff --git a/internal/testutil/ports_test.go b/internal/testutil/ports_test.go index 0cd363a..2563db8 100644 --- a/internal/testutil/ports_test.go +++ b/internal/testutil/ports_test.go @@ -27,7 +27,7 @@ func TestGetPortForTest(t *testing.T) { testCount := 0 // Create subtests with different names - for i := 0; i < 10; i++ { + for range 10 { t.Run("subtest", func(t *testing.T) { port := GetPortForTest(t) ports[port] = true diff --git a/internal/testutil/server_test.go b/internal/testutil/server_test.go index de33795..ead68ac 100644 --- a/internal/testutil/server_test.go +++ b/internal/testutil/server_test.go @@ -9,141 +9,145 @@ import ( "time" ) -func TestWaitForServerReady(t *testing.T) { - t.Run("returns nil when server is ready immediately", func(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusOK) - })) - defer server.Close() +func TestWaitForServerReady_Immediate(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + })) + defer server.Close() - err := WaitForServerReady(server.URL, 1*time.Second) - if err != nil { - t.Errorf("Expected no error, got: %v", err) + err := WaitForServerReady(server.URL, 1*time.Second) + if err != nil { + t.Errorf("Expected no error, got: %v", err) + } +} + +func TestWaitForServerReady_AfterDelay(t *testing.T) { + ready := false + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + if ready { + w.WriteHeader(http.StatusOK) + } else { + w.WriteHeader(http.StatusServiceUnavailable) } - }) + })) + defer server.Close() - t.Run("returns nil when server becomes ready after a delay", func(t *testing.T) { - ready := false - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if ready { - w.WriteHeader(http.StatusOK) - } else { - w.WriteHeader(http.StatusServiceUnavailable) - } - })) - defer server.Close() + // Make server ready after a short delay + go func() { + time.Sleep(200 * time.Millisecond) - // Make server ready after a short delay - go func() { - time.Sleep(200 * time.Millisecond) - ready = true - }() + ready = true + }() - err := WaitForServerReady(server.URL, 2*time.Second) - if err != nil { - t.Errorf("Expected no error, got: %v", err) - } - }) + err := WaitForServerReady(server.URL, 2*time.Second) + if err != nil { + t.Errorf("Expected no error, got: %v", err) + } +} - t.Run("returns error when server never becomes ready", func(t *testing.T) { - // Use an address that won't have a server - err := WaitForServerReady("http://localhost:59999", 300*time.Millisecond) - if err == nil { - t.Error("Expected error when server is not ready, got nil") - } +func TestWaitForServerReady_NeverReady(t *testing.T) { + // Use an address that won't have a server + err := WaitForServerReady("http://localhost:59999", 300*time.Millisecond) + if err == nil { + t.Error("Expected error when server is not ready, got nil") + } - expectedSubstring := "did not become ready" - if !strings.Contains(err.Error(), expectedSubstring) { - t.Errorf("Expected error to contain %q, got: %v", expectedSubstring, err) - } - }) + expectedSubstring := "did not become ready" + if !strings.Contains(err.Error(), expectedSubstring) { + t.Errorf("Expected error to contain %q, got: %v", expectedSubstring, err) + } +} - t.Run("respects timeout parameter", func(t *testing.T) { - start := time.Now() - timeout := 300 * time.Millisecond +func TestWaitForServerReady_RespectsTimeout(t *testing.T) { + start := time.Now() + timeout := 300 * time.Millisecond - err := WaitForServerReady("http://localhost:59998", timeout) + err := WaitForServerReady("http://localhost:59998", timeout) - elapsed := time.Since(start) + elapsed := time.Since(start) - if err == nil { - t.Error("Expected error when server is not ready") - } + if err == nil { + t.Error("Expected error when server is not ready") + } - // Allow some margin for timing (timeout + 200ms for final attempt) - maxExpected := timeout + 300*time.Millisecond - if elapsed > maxExpected { - t.Errorf("Expected to wait approximately %v, but waited %v", timeout, elapsed) - } + // Allow some margin for timing (timeout + 200ms for final attempt) + maxExpected := timeout + 300*time.Millisecond + if elapsed > maxExpected { + t.Errorf("Expected to wait approximately %v, but waited %v", timeout, elapsed) + } - if elapsed < timeout { - t.Errorf("Expected to wait at least %v, but only waited %v", timeout, elapsed) - } - }) + if elapsed < timeout { + t.Errorf("Expected to wait at least %v, but only waited %v", timeout, elapsed) + } +} - t.Run("error message includes timeout duration", func(t *testing.T) { - timeout := 250 * time.Millisecond - err := WaitForServerReady("http://localhost:59997", timeout) +func TestWaitForServerReady_ErrorMessage(t *testing.T) { + timeout := 250 * time.Millisecond - if err == nil { - t.Fatal("Expected error, got nil") - } + err := WaitForServerReady("http://localhost:59997", timeout) + if err == nil { + t.Fatal("Expected error, got nil") + } - if !strings.Contains(err.Error(), "250ms") { - t.Errorf("Expected error message to include timeout duration, got: %v", err) - } - }) + if !strings.Contains(err.Error(), "250ms") { + t.Errorf("Expected error message to include timeout duration, got: %v", err) + } +} - t.Run("works with different HTTP status codes", func(t *testing.T) { - tests := []struct { - name string - statusCode int - }{ - {"200 OK", http.StatusOK}, - {"201 Created", http.StatusCreated}, - {"404 Not Found", http.StatusNotFound}, - {"500 Internal Server Error", http.StatusInternalServerError}, - } +func TestWaitForServerReady_StatusCodes(t *testing.T) { + tests := []struct { + name string + statusCode int + }{ + {"200 OK", http.StatusOK}, + {"201 Created", http.StatusCreated}, + {"404 Not Found", http.StatusNotFound}, + {"500 Internal Server Error", http.StatusInternalServerError}, + } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(tt.statusCode) - })) - defer server.Close() - - err := WaitForServerReady(server.URL, 1*time.Second) - if err != nil { - t.Errorf("Expected no error for status %d, got: %v", tt.statusCode, err) - } - }) - } - }) + for _, testCase := range tests { + t.Run(testCase.name, func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(testCase.statusCode) + })) + defer server.Close() - t.Run("handles invalid URL gracefully", func(t *testing.T) { - err := WaitForServerReady("http://invalid-host-that-does-not-exist.local:12345", 200*time.Millisecond) - if err == nil { - t.Error("Expected error for invalid URL, got nil") - } - }) + err := WaitForServerReady(server.URL, 1*time.Second) + if err != nil { + t.Errorf("Expected no error for status %d, got: %v", testCase.statusCode, err) + } + }) + } +} + +func TestWaitForServerReady_InvalidURL(t *testing.T) { + err := WaitForServerReady("http://invalid-host-that-does-not-exist.local:12345", 200*time.Millisecond) + if err == nil { + t.Error("Expected error for invalid URL, got nil") + } } func TestWaitForServerReadyIntegration(t *testing.T) { t.Run("simulates real server startup scenario", func(t *testing.T) { var server *httptest.Server + serverReady := make(chan struct{}) // Simulate server starting up in background go func() { time.Sleep(150 * time.Millisecond) - server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + + server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) })) + close(serverReady) }() // Wait for server to be created <-serverReady + defer server.Close() err := WaitForServerReady(server.URL, 2*time.Second) @@ -154,7 +158,7 @@ func TestWaitForServerReadyIntegration(t *testing.T) { } func ExampleWaitForServerReady() { - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) })) defer server.Close() @@ -162,6 +166,7 @@ func ExampleWaitForServerReady() { err := WaitForServerReady(server.URL, 5*time.Second) if err != nil { fmt.Printf("Server not ready: %v\n", err) + return } diff --git a/internal/toolsets/mock/tool_test.go b/internal/toolsets/mock/tool_test.go index 7020fb9..5b8d522 100644 --- a/internal/toolsets/mock/tool_test.go +++ b/internal/toolsets/mock/tool_test.go @@ -187,11 +187,13 @@ func TestTool_RegisterWith(t *testing.T) { tool := NewTool("multi-register", false) tool.RegisterWith(nil) + if !tool.RegisterCalled { t.Error("Expected RegisterCalled to be true after first call") } tool.RegisterWith(nil) + if !tool.RegisterCalled { t.Error("Expected RegisterCalled to remain true after second call") } @@ -227,29 +229,28 @@ func TestTool_RegisterWith(t *testing.T) { } func TestTool_InterfaceCompliance(t *testing.T) { - t.Run("implements toolsets.Tool interface", func(t *testing.T) { + t.Run("implements toolsets.Tool interface", func(*testing.T) { var _ toolsets.Tool = (*Tool)(nil) }) +} - t.Run("can be used as toolsets.Tool", func(t *testing.T) { - var tool toolsets.Tool = NewTool("interface-test", true) +func TestTool_AsInterface(t *testing.T) { + var toolInstance toolsets.Tool = NewTool("interface-test", true) - if tool.GetName() != "interface-test" { - t.Errorf("Expected name 'interface-test', got %q", tool.GetName()) - } + if toolInstance.GetName() != "interface-test" { + t.Errorf("Expected name 'interface-test', got %q", toolInstance.GetName()) + } - if !tool.IsReadOnly() { - t.Error("Expected tool to be read-only") - } + if !toolInstance.IsReadOnly() { + t.Error("Expected tool to be read-only") + } - mcpTool := tool.GetTool() - if mcpTool == nil { - t.Error("Expected non-nil MCP tool") - } + mcpTool := toolInstance.GetTool() + if mcpTool == nil { + t.Error("Expected non-nil MCP tool") + } - tool.RegisterWith(nil) - // Can't check RegisterCalled through interface, but shouldn't panic - }) + toolInstance.RegisterWith(nil) } func TestTool_EdgeCases(t *testing.T) { @@ -307,67 +308,60 @@ func TestTool_EdgeCases(t *testing.T) { }) } -func TestTool_UsageScenarios(t *testing.T) { - t.Run("typical read-only tool workflow", func(t *testing.T) { - tool := NewTool("read-tool", true) +func TestTool_ReadOnlyWorkflow(t *testing.T) { + tool := NewTool("read-tool", true) - // Check initial state - if !tool.IsReadOnly() { - t.Error("Expected read-only tool") - } + if !tool.IsReadOnly() { + t.Error("Expected read-only tool") + } - if tool.RegisterCalled { - t.Error("Should not be registered initially") - } + if tool.RegisterCalled { + t.Error("Should not be registered initially") + } - // Get MCP definition - mcpTool := tool.GetTool() - if mcpTool.Name != "read-tool" { - t.Error("MCP tool should have correct name") - } + mcpTool := tool.GetTool() + if mcpTool.Name != "read-tool" { + t.Error("MCP tool should have correct name") + } - // Register with server - tool.RegisterWith(nil) - if !tool.RegisterCalled { - t.Error("Should be registered after RegisterWith call") - } - }) + tool.RegisterWith(nil) - t.Run("typical writable tool workflow", func(t *testing.T) { - tool := NewTool("write-tool", false) + if !tool.RegisterCalled { + t.Error("Should be registered after RegisterWith call") + } +} - // Check initial state - if tool.IsReadOnly() { - t.Error("Expected writable tool") - } +func TestTool_WritableWorkflow(t *testing.T) { + tool := NewTool("write-tool", false) - // Get tool definition and register - _ = tool.GetTool() - tool.RegisterWith(nil) + if tool.IsReadOnly() { + t.Error("Expected writable tool") + } - if !tool.RegisterCalled { - t.Error("Should be registered") - } - }) + _ = tool.GetTool() + tool.RegisterWith(nil) - t.Run("tool in toolset context", func(t *testing.T) { - tool1 := NewTool("tool1", true) - tool2 := NewTool("tool2", false) + if !tool.RegisterCalled { + t.Error("Should be registered") + } +} - tools := []toolsets.Tool{tool1, tool2} +func TestTool_InToolset(t *testing.T) { + tool1 := NewTool("tool1", true) + tool2 := NewTool("tool2", false) - for _, tool := range tools { - if tool.GetName() == "" { - t.Error("Tool in toolset should have name") - } + tools := []toolsets.Tool{tool1, tool2} - _ = tool.GetTool() - tool.RegisterWith(nil) + for _, toolInstance := range tools { + if toolInstance.GetName() == "" { + t.Error("Tool in toolset should have name") } - // Check that both were registered - if !tool1.RegisterCalled || !tool2.RegisterCalled { - t.Error("All tools should be registered") - } - }) + _ = toolInstance.GetTool() + toolInstance.RegisterWith(nil) + } + + if !tool1.RegisterCalled || !tool2.RegisterCalled { + t.Error("All tools should be registered") + } } diff --git a/internal/toolsets/mock/toolset_test.go b/internal/toolsets/mock/toolset_test.go index d62cb05..658e21c 100644 --- a/internal/toolsets/mock/toolset_test.go +++ b/internal/toolsets/mock/toolset_test.go @@ -15,46 +15,46 @@ func TestNewToolset(t *testing.T) { NewTool("tool2", false), } - ts := NewToolset(name, enabled, tools) + toolset := NewToolset(name, enabled, tools) - if ts.NameValue != name { - t.Errorf("Expected NameValue %q, got %q", name, ts.NameValue) + if toolset.NameValue != name { + t.Errorf("Expected NameValue %q, got %q", name, toolset.NameValue) } - if ts.EnabledValue != enabled { - t.Errorf("Expected EnabledValue %v, got %v", enabled, ts.EnabledValue) + if toolset.EnabledValue != enabled { + t.Errorf("Expected EnabledValue %v, got %v", enabled, toolset.EnabledValue) } - if len(ts.ToolsValue) != len(tools) { - t.Errorf("Expected %d tools, got %d", len(tools), len(ts.ToolsValue)) + if len(toolset.ToolsValue) != len(tools) { + t.Errorf("Expected %d tools, got %d", len(tools), len(toolset.ToolsValue)) } }) t.Run("creates disabled toolset", func(t *testing.T) { - ts := NewToolset("disabled-toolset", false, nil) + toolset := NewToolset("disabled-toolset", false, nil) - if ts.EnabledValue { + if toolset.EnabledValue { t.Error("Expected toolset to be disabled") } }) t.Run("creates toolset with empty tools", func(t *testing.T) { - ts := NewToolset("empty-toolset", true, []toolsets.Tool{}) + toolset := NewToolset("empty-toolset", true, []toolsets.Tool{}) - if ts.ToolsValue == nil { + if toolset.ToolsValue == nil { t.Error("Expected non-nil ToolsValue, got nil") } - if len(ts.ToolsValue) != 0 { - t.Errorf("Expected 0 tools, got %d", len(ts.ToolsValue)) + if len(toolset.ToolsValue) != 0 { + t.Errorf("Expected 0 tools, got %d", len(toolset.ToolsValue)) } }) t.Run("creates toolset with nil tools", func(t *testing.T) { - ts := NewToolset("nil-tools", true, nil) + toolset := NewToolset("nil-tools", true, nil) - if ts.ToolsValue != nil { - t.Errorf("Expected nil ToolsValue, got %v", ts.ToolsValue) + if toolset.ToolsValue != nil { + t.Errorf("Expected nil ToolsValue, got %v", toolset.ToolsValue) } }) } @@ -62,173 +62,173 @@ func TestNewToolset(t *testing.T) { func TestToolset_GetName(t *testing.T) { t.Run("returns configured name", func(t *testing.T) { name := "my-toolset" - ts := NewToolset(name, true, nil) + toolset := NewToolset(name, true, nil) - if ts.GetName() != name { - t.Errorf("Expected name %q, got %q", name, ts.GetName()) + if toolset.GetName() != name { + t.Errorf("Expected name %q, got %q", name, toolset.GetName()) } }) t.Run("returns empty string if configured", func(t *testing.T) { - ts := NewToolset("", true, nil) + toolset := NewToolset("", true, nil) - if ts.GetName() != "" { - t.Errorf("Expected empty name, got %q", ts.GetName()) + if toolset.GetName() != "" { + t.Errorf("Expected empty name, got %q", toolset.GetName()) } }) } func TestToolset_IsEnabled(t *testing.T) { t.Run("returns true when enabled", func(t *testing.T) { - ts := NewToolset("enabled", true, nil) + toolset := NewToolset("enabled", true, nil) - if !ts.IsEnabled() { + if !toolset.IsEnabled() { t.Error("Expected toolset to be enabled") } }) t.Run("returns false when disabled", func(t *testing.T) { - ts := NewToolset("disabled", false, nil) + toolset := NewToolset("disabled", false, nil) - if ts.IsEnabled() { + if toolset.IsEnabled() { t.Error("Expected toolset to be disabled") } }) t.Run("can toggle enabled state", func(t *testing.T) { - ts := NewToolset("toggle", true, nil) + toolset := NewToolset("toggle", true, nil) - if !ts.IsEnabled() { + if !toolset.IsEnabled() { t.Error("Expected initially enabled") } - ts.EnabledValue = false + toolset.EnabledValue = false - if ts.IsEnabled() { + if toolset.IsEnabled() { t.Error("Expected disabled after toggle") } }) } -func TestToolset_GetTools(t *testing.T) { - t.Run("returns tools when enabled", func(t *testing.T) { - tools := []toolsets.Tool{ - NewTool("tool1", true), - NewTool("tool2", false), - NewTool("tool3", true), - } - ts := NewToolset("enabled-toolset", true, tools) +func TestToolset_GetTools_Enabled(t *testing.T) { + tools := []toolsets.Tool{ + NewTool("tool1", true), + NewTool("tool2", false), + NewTool("tool3", true), + } + toolset := NewToolset("enabled-toolset", true, tools) - result := ts.GetTools() + result := toolset.GetTools() - if len(result) != len(tools) { - t.Errorf("Expected %d tools, got %d", len(tools), len(result)) - } + if len(result) != len(tools) { + t.Errorf("Expected %d tools, got %d", len(tools), len(result)) + } - for i, tool := range result { - if tool != tools[i] { - t.Errorf("Tool at index %d doesn't match", i) - } + for i, tool := range result { + if tool != tools[i] { + t.Errorf("Tool at index %d doesn't match", i) } - }) + } +} - t.Run("returns empty slice when disabled", func(t *testing.T) { - tools := []toolsets.Tool{ - NewTool("tool1", true), - NewTool("tool2", false), - } - ts := NewToolset("disabled-toolset", false, tools) +func TestToolset_GetTools_Disabled(t *testing.T) { + tools := []toolsets.Tool{ + NewTool("tool1", true), + NewTool("tool2", false), + } + toolset := NewToolset("disabled-toolset", false, tools) - result := ts.GetTools() + result := toolset.GetTools() - if result == nil { - t.Error("Expected non-nil slice, got nil") - } + if result == nil { + t.Error("Expected non-nil slice, got nil") + } - if len(result) != 0 { - t.Errorf("Expected empty slice when disabled, got %d tools", len(result)) - } - }) + if len(result) != 0 { + t.Errorf("Expected empty slice when disabled, got %d tools", len(result)) + } +} - t.Run("returns empty slice when enabled with no tools", func(t *testing.T) { - ts := NewToolset("no-tools", true, []toolsets.Tool{}) +func TestToolset_GetTools_EmptyList(t *testing.T) { + toolset := NewToolset("no-tools", true, []toolsets.Tool{}) - result := ts.GetTools() + result := toolset.GetTools() - if result == nil { - t.Error("Expected non-nil slice, got nil") - } + if result == nil { + t.Error("Expected non-nil slice, got nil") + } - if len(result) != 0 { - t.Errorf("Expected 0 tools, got %d", len(result)) - } - }) + if len(result) != 0 { + t.Errorf("Expected 0 tools, got %d", len(result)) + } +} - t.Run("returns nil when enabled with nil tools", func(t *testing.T) { - ts := NewToolset("nil-tools", true, nil) +func TestToolset_GetTools_NilTools(t *testing.T) { + toolset := NewToolset("nil-tools", true, nil) - result := ts.GetTools() + result := toolset.GetTools() - if result != nil { - t.Errorf("Expected nil, got %v", result) - } - }) + if result != nil { + t.Errorf("Expected nil, got %v", result) + } +} - t.Run("changing enabled state changes returned tools", func(t *testing.T) { - tools := []toolsets.Tool{NewTool("tool1", true)} - ts := NewToolset("toggle-toolset", true, tools) +func TestToolset_GetTools_ToggleState(t *testing.T) { + tools := []toolsets.Tool{NewTool("tool1", true)} + toolset := NewToolset("toggle-toolset", true, tools) - // Initially enabled - should return tools - result1 := ts.GetTools() - if len(result1) != 1 { - t.Errorf("Expected 1 tool when enabled, got %d", len(result1)) - } + // Initially enabled - should return tools + result1 := toolset.GetTools() + if len(result1) != 1 { + t.Errorf("Expected 1 tool when enabled, got %d", len(result1)) + } - // Disable - should return empty slice - ts.EnabledValue = false - result2 := ts.GetTools() - if len(result2) != 0 { - t.Errorf("Expected 0 tools when disabled, got %d", len(result2)) - } + // Disable - should return empty slice + toolset.EnabledValue = false - // Re-enable - should return tools again - ts.EnabledValue = true - result3 := ts.GetTools() - if len(result3) != 1 { - t.Errorf("Expected 1 tool when re-enabled, got %d", len(result3)) - } - }) + result2 := toolset.GetTools() + if len(result2) != 0 { + t.Errorf("Expected 0 tools when disabled, got %d", len(result2)) + } + + // Re-enable - should return tools again + toolset.EnabledValue = true + + result3 := toolset.GetTools() + if len(result3) != 1 { + t.Errorf("Expected 1 tool when re-enabled, got %d", len(result3)) + } } func TestToolset_InterfaceCompliance(t *testing.T) { - t.Run("implements toolsets.Toolset interface", func(t *testing.T) { + t.Run("implements toolsets.Toolset interface", func(*testing.T) { var _ toolsets.Toolset = (*Toolset)(nil) }) +} - t.Run("can be used as toolsets.Toolset", func(t *testing.T) { - var ts toolsets.Toolset = NewToolset("interface-test", true, nil) +func TestToolset_AsInterface(t *testing.T) { + var toolsetInstance toolsets.Toolset = NewToolset("interface-test", true, nil) - if ts.GetName() != "interface-test" { - t.Errorf("Expected name 'interface-test', got %q", ts.GetName()) - } + if toolsetInstance.GetName() != "interface-test" { + t.Errorf("Expected name 'interface-test', got %q", toolsetInstance.GetName()) + } - if !ts.IsEnabled() { - t.Error("Expected toolset to be enabled") - } + if !toolsetInstance.IsEnabled() { + t.Error("Expected toolset to be enabled") + } - tools := ts.GetTools() - if tools != nil { - t.Errorf("Expected nil tools, got %v", tools) - } - }) + tools := toolsetInstance.GetTools() + if tools != nil { + t.Errorf("Expected nil tools, got %v", tools) + } } func TestToolset_EdgeCases(t *testing.T) { t.Run("toolset with single tool", func(t *testing.T) { tools := []toolsets.Tool{NewTool("only-tool", true)} - ts := NewToolset("single", true, tools) + toolset := NewToolset("single", true, tools) - result := ts.GetTools() + result := toolset.GetTools() if len(result) != 1 { t.Errorf("Expected 1 tool, got %d", len(result)) } @@ -236,13 +236,13 @@ func TestToolset_EdgeCases(t *testing.T) { t.Run("toolset with many tools", func(t *testing.T) { tools := make([]toolsets.Tool, 100) - for i := 0; i < 100; i++ { + for i := range 100 { tools[i] = NewTool("tool", i%2 == 0) } - ts := NewToolset("many-tools", true, tools) + toolset := NewToolset("many-tools", true, tools) - result := ts.GetTools() + result := toolset.GetTools() if len(result) != 100 { t.Errorf("Expected 100 tools, got %d", len(result)) } @@ -250,10 +250,10 @@ func TestToolset_EdgeCases(t *testing.T) { t.Run("toolset name with special characters", func(t *testing.T) { name := "tool-set_123!@#" - ts := NewToolset(name, true, nil) + toolset := NewToolset(name, true, nil) - if ts.GetName() != name { - t.Errorf("Expected name %q, got %q", name, ts.GetName()) + if toolset.GetName() != name { + t.Errorf("Expected name %q, got %q", name, toolset.GetName()) } }) } From 27d5659620b366e7cd50520fa30f9d32c90a7c0f Mon Sep 17 00:00:00 2001 From: Tomasz Janiszewski Date: Thu, 11 Dec 2025 15:01:53 +0100 Subject: [PATCH 03/13] fix Signed-off-by: Tomasz Janiszewski --- Makefile | 2 +- internal/testutil/server_test.go | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Makefile b/Makefile index ff52fed..816cbee 100644 --- a/Makefile +++ b/Makefile @@ -56,7 +56,7 @@ test: ## Run unit tests .PHONY: test-coverage-and-junit test-coverage-and-junit: ## Run unit tests with coverage and junit output go install github.com/jstemmer/go-junit-report/v2@v2.1.0 - $(GOTEST) -v -cover -race -coverprofile=$(COVERAGE_OUT) ./... -json 2>&1 | go-junit-report -set-exit-code -parser gojson -iocopy -out $(JUNIT_OUT) + $(GOTEST) -v -cover -race -coverprofile=$(COVERAGE_OUT) ./... 2>&1 | go-junit-report -set-exit-code -iocopy -out $(JUNIT_OUT) .PHONY: coverage-html coverage-html: test ## Generate and open HTML coverage report diff --git a/internal/testutil/server_test.go b/internal/testutil/server_test.go index ead68ac..074103f 100644 --- a/internal/testutil/server_test.go +++ b/internal/testutil/server_test.go @@ -5,6 +5,7 @@ import ( "net/http" "net/http/httptest" "strings" + "sync/atomic" "testing" "time" ) @@ -22,10 +23,10 @@ func TestWaitForServerReady_Immediate(t *testing.T) { } func TestWaitForServerReady_AfterDelay(t *testing.T) { - ready := false + var ready atomic.Bool server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - if ready { + if ready.Load() { w.WriteHeader(http.StatusOK) } else { w.WriteHeader(http.StatusServiceUnavailable) @@ -36,8 +37,7 @@ func TestWaitForServerReady_AfterDelay(t *testing.T) { // Make server ready after a short delay go func() { time.Sleep(200 * time.Millisecond) - - ready = true + ready.Store(true) }() err := WaitForServerReady(server.URL, 2*time.Second) From 3855f7a85d32f8e838ee946ccd048c140a88108e Mon Sep 17 00:00:00 2001 From: Tomasz Janiszewski Date: Fri, 12 Dec 2025 12:14:50 +0100 Subject: [PATCH 04/13] Update internal/testutil/config_test.go Co-authored-by: Mladen Todorovic --- internal/testutil/config_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/testutil/config_test.go b/internal/testutil/config_test.go index 9803857..b98cc90 100644 --- a/internal/testutil/config_test.go +++ b/internal/testutil/config_test.go @@ -7,7 +7,7 @@ import ( "testing" ) -func TestWriteYAMLFile_CreatesFile(t *testing.T) { +func TestWriteYAMLFile(t *testing.T) { content := "key: value\nfoo: bar" filePath := WriteYAMLFile(t, content) From 8a78ad6ad74d2ee10cdbc171fef939af5c91887c Mon Sep 17 00:00:00 2001 From: Tomasz Janiszewski Date: Fri, 12 Dec 2025 12:15:30 +0100 Subject: [PATCH 05/13] Update internal/testutil/config_test.go Co-authored-by: Mladen Todorovic --- internal/testutil/config_test.go | 69 -------------------------------- 1 file changed, 69 deletions(-) diff --git a/internal/testutil/config_test.go b/internal/testutil/config_test.go index b98cc90..aabbf59 100644 --- a/internal/testutil/config_test.go +++ b/internal/testutil/config_test.go @@ -39,75 +39,6 @@ func TestWriteYAMLFile_HasYamlExtension(t *testing.T) { } } -func TestWriteYAMLFile_FileIsReadable(t *testing.T) { - content := "readable: true\ntest: data" - filePath := WriteYAMLFile(t, content) - - //nolint:gosec // Test code reading from known test file - data, err := os.ReadFile(filePath) - if err != nil { - t.Fatalf("File should be readable: %v", err) - } - - if len(data) == 0 { - t.Error("File should not be empty") - } -} - -func TestWriteYAMLFile_EmptyContent(t *testing.T) { - filePath := WriteYAMLFile(t, "") - - //nolint:gosec // Test code reading from known test file - data, err := os.ReadFile(filePath) - if err != nil { - t.Fatalf("Should create file even with empty content: %v", err) - } - - if len(data) != 0 { - t.Errorf("Expected empty file, got %d bytes", len(data)) - } -} - -func TestWriteYAMLFile_MultiLineContent(t *testing.T) { - content := `server: - host: localhost - port: 8080 -database: - name: testdb - user: admin` - - filePath := WriteYAMLFile(t, content) - - //nolint:gosec // Test code reading from known test file - data, err := os.ReadFile(filePath) - if err != nil { - t.Fatalf("Failed to read file: %v", err) - } - - if string(data) != content { - t.Errorf("Multi-line content mismatch.\nExpected:\n%s\n\nGot:\n%s", content, string(data)) - } -} - -func TestWriteYAMLFile_SpecialCharacters(t *testing.T) { - //nolint:gosmopolitan // Testing unicode support - content := `special: "chars: -, |, >, &, *, #, @" -quote: 'single and "double"' -unicode: "测试"` - - filePath := WriteYAMLFile(t, content) - - //nolint:gosec // Test code reading from known test file - data, err := os.ReadFile(filePath) - if err != nil { - t.Fatalf("Failed to read file: %v", err) - } - - if string(data) != content { - t.Errorf("Special characters not preserved.\nExpected:\n%s\n\nGot:\n%s", content, string(data)) - } -} - func TestWriteYAMLFile_PathIncludesTestName(t *testing.T) { filePath := WriteYAMLFile(t, "test: value") From fb5c3e06fa845358f21e1777c80c065e15dd704f Mon Sep 17 00:00:00 2001 From: Tomasz Janiszewski Date: Fri, 12 Dec 2025 12:15:39 +0100 Subject: [PATCH 06/13] Update internal/testutil/ports_test.go Co-authored-by: Mladen Todorovic --- internal/testutil/ports_test.go | 21 +++------------------ 1 file changed, 3 insertions(+), 18 deletions(-) diff --git a/internal/testutil/ports_test.go b/internal/testutil/ports_test.go index 2563db8..14ef3d5 100644 --- a/internal/testutil/ports_test.go +++ b/internal/testutil/ports_test.go @@ -55,22 +55,7 @@ func TestGetPortForTest(t *testing.T) { } func TestPortRangeConstants(t *testing.T) { - t.Run("minPort is above privileged range", func(t *testing.T) { - if minPort < 1024 { - t.Errorf("minPort %d should be above 1024 to avoid privileged ports", minPort) - } - }) - - t.Run("maxPort is below max valid port", func(t *testing.T) { - if maxPort > 65536 { - t.Errorf("maxPort %d should be below 65536", maxPort) - } - }) - - t.Run("port range is reasonable", func(t *testing.T) { - portRange := maxPort - minPort - if portRange < 1000 { - t.Errorf("Port range %d is too small, may cause collisions", portRange) - } - }) + assert.Greater(t, 1024, minPort) + assert.Less(t, 65536, minPort) + assert.Equal(t, 10000, maxPort-minPort) } From 03f4028780043174119824019d9ec21037af808f Mon Sep 17 00:00:00 2001 From: Tomasz Janiszewski Date: Fri, 12 Dec 2025 12:15:50 +0100 Subject: [PATCH 07/13] Update internal/testutil/ports_test.go Co-authored-by: Mladen Todorovic --- internal/testutil/ports_test.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/internal/testutil/ports_test.go b/internal/testutil/ports_test.go index 14ef3d5..5c07bec 100644 --- a/internal/testutil/ports_test.go +++ b/internal/testutil/ports_test.go @@ -35,10 +35,7 @@ func TestGetPortForTest(t *testing.T) { }) } - // We should have different ports for different test paths - if len(ports) < 2 { - t.Errorf("Expected multiple different ports, got %d unique ports from %d tests", len(ports), testCount) - } + assert.Len(t, 10, ports) }) t.Run("port is within safe range avoiding privileged ports", func(t *testing.T) { From 3b8cedc174f08f723e78b7f465c600786574a421 Mon Sep 17 00:00:00 2001 From: Tomasz Janiszewski Date: Fri, 12 Dec 2025 12:15:58 +0100 Subject: [PATCH 08/13] Update internal/testutil/config_test.go Co-authored-by: Mladen Todorovic --- internal/testutil/config_test.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/internal/testutil/config_test.go b/internal/testutil/config_test.go index aabbf59..8c760ad 100644 --- a/internal/testutil/config_test.go +++ b/internal/testutil/config_test.go @@ -18,9 +18,7 @@ func TestWriteYAMLFile(t *testing.T) { t.Fatalf("Failed to read created file: %v", err) } - if string(data) != content { - t.Errorf("Expected content %q, got %q", content, string(data)) - } + assert.Equal(t, content, string(data)) } func TestWriteYAMLFile_ReturnsAbsolutePath(t *testing.T) { From 16d77f0fe40881ba112c4c3f7f07cc82479b250e Mon Sep 17 00:00:00 2001 From: Tomasz Janiszewski Date: Fri, 12 Dec 2025 12:43:39 +0100 Subject: [PATCH 09/13] Address PR review comments for test files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Simplified config_test.go to single comprehensive test - Removed redundant test cases as suggested - Replaced manual error checking with assert package - Fixed and simplified ports_test.go assertions - Removed redundant "port is within safe range" test Addresses feedback from PR #17 review. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 --- internal/testutil/config_test.go | 76 +------------------------------- internal/testutil/ports_test.go | 33 ++++---------- 2 files changed, 11 insertions(+), 98 deletions(-) diff --git a/internal/testutil/config_test.go b/internal/testutil/config_test.go index 8c760ad..8d410b5 100644 --- a/internal/testutil/config_test.go +++ b/internal/testutil/config_test.go @@ -2,9 +2,9 @@ package testutil import ( "os" - "path/filepath" - "strings" "testing" + + "github.com/stretchr/testify/assert" ) func TestWriteYAMLFile(t *testing.T) { @@ -20,75 +20,3 @@ func TestWriteYAMLFile(t *testing.T) { assert.Equal(t, content, string(data)) } - -func TestWriteYAMLFile_ReturnsAbsolutePath(t *testing.T) { - filePath := WriteYAMLFile(t, "test: content") - - if !filepath.IsAbs(filePath) { - t.Errorf("Expected absolute path, got: %s", filePath) - } -} - -func TestWriteYAMLFile_HasYamlExtension(t *testing.T) { - filePath := WriteYAMLFile(t, "test: content") - - if !strings.HasSuffix(filePath, ".yaml") { - t.Errorf("Expected .yaml extension, got: %s", filePath) - } -} - -func TestWriteYAMLFile_PathIncludesTestName(t *testing.T) { - filePath := WriteYAMLFile(t, "test: value") - - fileName := filepath.Base(filePath) - // The filename should contain part of the test name - // but sanitized (slashes replaced or handled) - if !strings.HasPrefix(fileName, "config-") { - t.Errorf("Expected filename to start with 'config-', got: %s", fileName) - } -} - -func TestWriteYAMLFile_FileExists(t *testing.T) { - filePath := WriteYAMLFile(t, "exists: true") - - if _, err := os.Stat(filePath); os.IsNotExist(err) { - t.Errorf("File should exist at path: %s", filePath) - } -} - -func TestWriteYAMLFile_InTempDirectory(t *testing.T) { - filePath := WriteYAMLFile(t, "test: content") - - // The file should be in a temp directory managed by t.TempDir() - // which will be automatically cleaned up - dir := filepath.Dir(filePath) - if dir == "" || dir == "." { - t.Errorf("File should be in a proper directory, got: %s", dir) - } -} - -func TestWriteYAMLFile_LargeContent(t *testing.T) { - // Test with larger YAML content - var builder strings.Builder - for i := range 100 { - builder.WriteString("key") - builder.WriteRune(rune('0' + i%10)) - builder.WriteString(": value") - builder.WriteRune(rune('0' + i%10)) - builder.WriteString("\n") - } - - content := builder.String() - - filePath := WriteYAMLFile(t, content) - - //nolint:gosec // Test code reading from known test file - data, err := os.ReadFile(filePath) - if err != nil { - t.Fatalf("Failed to read file with large content: %v", err) - } - - if string(data) != content { - t.Error("Large content not preserved correctly") - } -} diff --git a/internal/testutil/ports_test.go b/internal/testutil/ports_test.go index 5c07bec..4e4e8ce 100644 --- a/internal/testutil/ports_test.go +++ b/internal/testutil/ports_test.go @@ -2,57 +2,42 @@ package testutil import ( "testing" + + "github.com/stretchr/testify/assert" ) func TestGetPortForTest(t *testing.T) { t.Run("returns port in valid range", func(t *testing.T) { port := GetPortForTest(t) - if port < minPort || port >= maxPort { - t.Errorf("Port %d is outside valid range [%d, %d)", port, minPort, maxPort) - } + assert.GreaterOrEqual(t, port, minPort) + assert.Less(t, port, maxPort) }) t.Run("returns deterministic port for same test name", func(t *testing.T) { port1 := GetPortForTest(t) port2 := GetPortForTest(t) - if port1 != port2 { - t.Errorf("Expected same port for same test name, got %d and %d", port1, port2) - } + assert.Equal(t, port1, port2) }) t.Run("different test names get different ports", func(t *testing.T) { ports := make(map[int]bool) - testCount := 0 // Create subtests with different names for range 10 { t.Run("subtest", func(t *testing.T) { port := GetPortForTest(t) ports[port] = true - testCount++ }) } - assert.Len(t, 10, ports) - }) - - t.Run("port is within safe range avoiding privileged ports", func(t *testing.T) { - port := GetPortForTest(t) - - if port < 1024 { - t.Errorf("Port %d is in privileged range (< 1024)", port) - } - - if port >= 65536 { - t.Errorf("Port %d exceeds maximum valid port number", port) - } + assert.Len(t, ports, 10) }) } func TestPortRangeConstants(t *testing.T) { - assert.Greater(t, 1024, minPort) - assert.Less(t, 65536, minPort) - assert.Equal(t, 10000, maxPort-minPort) + assert.Greater(t, minPort, 1024) + assert.LessOrEqual(t, maxPort, 65536) + assert.Equal(t, maxPort-minPort, 50000) } From 6e256290e9871aef4c92d93bde87f1835e0c2571 Mon Sep 17 00:00:00 2001 From: Tomasz Janiszewski Date: Fri, 12 Dec 2025 13:12:59 +0100 Subject: [PATCH 10/13] Refactor subtests into separate test functions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Split TestGetPortForTest subtests into independent test functions as suggested in PR review: - TestGetPortForTest_ReturnsPortInValidRange - TestGetPortForTest_ReturnsDeterministicPort - TestGetPortForTest_DifferentTestNamesGetDifferentPorts Keeps the nested subtest structure only where needed (for generating different test names). Addresses remaining feedback from PR #17 review. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 --- internal/testutil/ports_test.go | 54 ++++++++++++++++----------------- 1 file changed, 26 insertions(+), 28 deletions(-) diff --git a/internal/testutil/ports_test.go b/internal/testutil/ports_test.go index 4e4e8ce..55b410b 100644 --- a/internal/testutil/ports_test.go +++ b/internal/testutil/ports_test.go @@ -6,34 +6,32 @@ import ( "github.com/stretchr/testify/assert" ) -func TestGetPortForTest(t *testing.T) { - t.Run("returns port in valid range", func(t *testing.T) { - port := GetPortForTest(t) - - assert.GreaterOrEqual(t, port, minPort) - assert.Less(t, port, maxPort) - }) - - t.Run("returns deterministic port for same test name", func(t *testing.T) { - port1 := GetPortForTest(t) - port2 := GetPortForTest(t) - - assert.Equal(t, port1, port2) - }) - - t.Run("different test names get different ports", func(t *testing.T) { - ports := make(map[int]bool) - - // Create subtests with different names - for range 10 { - t.Run("subtest", func(t *testing.T) { - port := GetPortForTest(t) - ports[port] = true - }) - } - - assert.Len(t, ports, 10) - }) +func TestGetPortForTest_ReturnsPortInValidRange(t *testing.T) { + port := GetPortForTest(t) + + assert.GreaterOrEqual(t, port, minPort) + assert.Less(t, port, maxPort) +} + +func TestGetPortForTest_ReturnsDeterministicPort(t *testing.T) { + port1 := GetPortForTest(t) + port2 := GetPortForTest(t) + + assert.Equal(t, port1, port2) +} + +func TestGetPortForTest_DifferentTestNamesGetDifferentPorts(t *testing.T) { + ports := make(map[int]bool) + + // Create subtests with different names + for range 10 { + t.Run("subtest", func(t *testing.T) { + port := GetPortForTest(t) + ports[port] = true + }) + } + + assert.Len(t, ports, 10) } func TestPortRangeConstants(t *testing.T) { From 6162e0bd7a7f180b1e9664f1522cf69a9a82888f Mon Sep 17 00:00:00 2001 From: Tomasz Janiszewski Date: Fri, 12 Dec 2025 14:30:50 +0100 Subject: [PATCH 11/13] Replace manual error checking with assert package MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaced all t.Errorf and t.Error calls with appropriate assert functions from github.com/stretchr/testify/assert: - internal/testutil/server_test.go: Use assert.NoError, assert.Error, assert.Contains, assert.LessOrEqual, assert.GreaterOrEqual - internal/toolsets/mock/tool_test.go: Use assert.Equal, assert.True, assert.False, assert.NotNil, assert.NotEmpty, assert.NotSame - internal/toolsets/mock/toolset_test.go: Use assert.Equal, assert.Len, assert.True, assert.False, assert.Nil, assert.NotNil, assert.Same This makes tests more readable and provides better error messages when assertions fail. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 --- internal/testutil/server_test.go | 56 ++----- internal/toolsets/mock/tool_test.go | 200 +++++++------------------ internal/toolsets/mock/toolset_test.go | 123 ++++----------- 3 files changed, 95 insertions(+), 284 deletions(-) diff --git a/internal/testutil/server_test.go b/internal/testutil/server_test.go index 074103f..3217e61 100644 --- a/internal/testutil/server_test.go +++ b/internal/testutil/server_test.go @@ -4,10 +4,12 @@ import ( "fmt" "net/http" "net/http/httptest" - "strings" "sync/atomic" "testing" "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestWaitForServerReady_Immediate(t *testing.T) { @@ -17,9 +19,7 @@ func TestWaitForServerReady_Immediate(t *testing.T) { defer server.Close() err := WaitForServerReady(server.URL, 1*time.Second) - if err != nil { - t.Errorf("Expected no error, got: %v", err) - } + assert.NoError(t, err) } func TestWaitForServerReady_AfterDelay(t *testing.T) { @@ -41,22 +41,14 @@ func TestWaitForServerReady_AfterDelay(t *testing.T) { }() err := WaitForServerReady(server.URL, 2*time.Second) - if err != nil { - t.Errorf("Expected no error, got: %v", err) - } + assert.NoError(t, err) } func TestWaitForServerReady_NeverReady(t *testing.T) { // Use an address that won't have a server err := WaitForServerReady("http://localhost:59999", 300*time.Millisecond) - if err == nil { - t.Error("Expected error when server is not ready, got nil") - } - - expectedSubstring := "did not become ready" - if !strings.Contains(err.Error(), expectedSubstring) { - t.Errorf("Expected error to contain %q, got: %v", expectedSubstring, err) - } + require.Error(t, err) + assert.Contains(t, err.Error(), "did not become ready") } func TestWaitForServerReady_RespectsTimeout(t *testing.T) { @@ -67,32 +59,20 @@ func TestWaitForServerReady_RespectsTimeout(t *testing.T) { elapsed := time.Since(start) - if err == nil { - t.Error("Expected error when server is not ready") - } + require.Error(t, err) // Allow some margin for timing (timeout + 200ms for final attempt) maxExpected := timeout + 300*time.Millisecond - if elapsed > maxExpected { - t.Errorf("Expected to wait approximately %v, but waited %v", timeout, elapsed) - } - - if elapsed < timeout { - t.Errorf("Expected to wait at least %v, but only waited %v", timeout, elapsed) - } + assert.LessOrEqual(t, elapsed, maxExpected) + assert.GreaterOrEqual(t, elapsed, timeout) } func TestWaitForServerReady_ErrorMessage(t *testing.T) { timeout := 250 * time.Millisecond err := WaitForServerReady("http://localhost:59997", timeout) - if err == nil { - t.Fatal("Expected error, got nil") - } - - if !strings.Contains(err.Error(), "250ms") { - t.Errorf("Expected error message to include timeout duration, got: %v", err) - } + require.Error(t, err) + assert.Contains(t, err.Error(), "250ms") } func TestWaitForServerReady_StatusCodes(t *testing.T) { @@ -114,18 +94,14 @@ func TestWaitForServerReady_StatusCodes(t *testing.T) { defer server.Close() err := WaitForServerReady(server.URL, 1*time.Second) - if err != nil { - t.Errorf("Expected no error for status %d, got: %v", testCase.statusCode, err) - } + assert.NoError(t, err) }) } } func TestWaitForServerReady_InvalidURL(t *testing.T) { err := WaitForServerReady("http://invalid-host-that-does-not-exist.local:12345", 200*time.Millisecond) - if err == nil { - t.Error("Expected error for invalid URL, got nil") - } + assert.Error(t, err) } func TestWaitForServerReadyIntegration(t *testing.T) { @@ -151,9 +127,7 @@ func TestWaitForServerReadyIntegration(t *testing.T) { defer server.Close() err := WaitForServerReady(server.URL, 2*time.Second) - if err != nil { - t.Errorf("Expected server to become ready, got error: %v", err) - } + assert.NoError(t, err) }) } diff --git a/internal/toolsets/mock/tool_test.go b/internal/toolsets/mock/tool_test.go index 5b8d522..735d6e5 100644 --- a/internal/toolsets/mock/tool_test.go +++ b/internal/toolsets/mock/tool_test.go @@ -4,6 +4,8 @@ import ( "testing" "github.com/stackrox/stackrox-mcp/internal/toolsets" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestNewTool(t *testing.T) { @@ -13,41 +15,27 @@ func TestNewTool(t *testing.T) { tool := NewTool(name, readOnly) - if tool.NameValue != name { - t.Errorf("Expected NameValue %q, got %q", name, tool.NameValue) - } - - if tool.ReadOnlyValue != readOnly { - t.Errorf("Expected ReadOnlyValue %v, got %v", readOnly, tool.ReadOnlyValue) - } - - if tool.RegisterCalled { - t.Error("Expected RegisterCalled to be false initially") - } + assert.Equal(t, name, tool.NameValue) + assert.Equal(t, readOnly, tool.ReadOnlyValue) + assert.False(t, tool.RegisterCalled) }) t.Run("creates read-only tool", func(t *testing.T) { tool := NewTool("readonly", true) - if !tool.ReadOnlyValue { - t.Error("Expected tool to be read-only") - } + assert.True(t, tool.ReadOnlyValue) }) t.Run("creates writable tool", func(t *testing.T) { tool := NewTool("writable", false) - if tool.ReadOnlyValue { - t.Error("Expected tool to be writable") - } + assert.False(t, tool.ReadOnlyValue) }) t.Run("creates tool with empty name", func(t *testing.T) { tool := NewTool("", true) - if tool.NameValue != "" { - t.Errorf("Expected empty name, got %q", tool.NameValue) - } + assert.Equal(t, "", tool.NameValue) }) } @@ -56,26 +44,20 @@ func TestTool_GetName(t *testing.T) { name := "my-tool" tool := NewTool(name, true) - if tool.GetName() != name { - t.Errorf("Expected name %q, got %q", name, tool.GetName()) - } + assert.Equal(t, name, tool.GetName()) }) t.Run("returns empty string if configured", func(t *testing.T) { tool := NewTool("", false) - if tool.GetName() != "" { - t.Errorf("Expected empty name, got %q", tool.GetName()) - } + assert.Equal(t, "", tool.GetName()) }) t.Run("name with special characters", func(t *testing.T) { name := "tool-name_123!@#" tool := NewTool(name, true) - if tool.GetName() != name { - t.Errorf("Expected name %q, got %q", name, tool.GetName()) - } + assert.Equal(t, name, tool.GetName()) }) } @@ -83,31 +65,23 @@ func TestTool_IsReadOnly(t *testing.T) { t.Run("returns true when read-only", func(t *testing.T) { tool := NewTool("readonly", true) - if !tool.IsReadOnly() { - t.Error("Expected tool to be read-only") - } + assert.True(t, tool.IsReadOnly()) }) t.Run("returns false when writable", func(t *testing.T) { tool := NewTool("writable", false) - if tool.IsReadOnly() { - t.Error("Expected tool to be writable") - } + assert.False(t, tool.IsReadOnly()) }) t.Run("can toggle read-only state", func(t *testing.T) { tool := NewTool("toggle", true) - if !tool.IsReadOnly() { - t.Error("Expected initially read-only") - } + assert.True(t, tool.IsReadOnly()) tool.ReadOnlyValue = false - if tool.IsReadOnly() { - t.Error("Expected writable after toggle") - } + assert.False(t, tool.IsReadOnly()) }) } @@ -118,22 +92,12 @@ func TestTool_GetTool(t *testing.T) { mcpTool := tool.GetTool() - if mcpTool == nil { - t.Fatal("Expected non-nil MCP tool") - } - - if mcpTool.Name != name { - t.Errorf("Expected MCP tool name %q, got %q", name, mcpTool.Name) - } - - if mcpTool.Description == "" { - t.Error("Expected non-empty description") - } + require.NotNil(t, mcpTool) + assert.Equal(t, name, mcpTool.Name) + assert.NotEmpty(t, mcpTool.Description) expectedDesc := "Mock tool for testing" - if mcpTool.Description != expectedDesc { - t.Errorf("Expected description %q, got %q", expectedDesc, mcpTool.Description) - } + assert.Equal(t, expectedDesc, mcpTool.Description) }) t.Run("returns new tool instance each time", func(t *testing.T) { @@ -143,14 +107,10 @@ func TestTool_GetTool(t *testing.T) { mcpTool2 := tool.GetTool() // Should be different instances - if mcpTool1 == mcpTool2 { - t.Error("Expected different instances, got same pointer") - } + assert.NotSame(t, mcpTool1, mcpTool2) // But with same values - if mcpTool1.Name != mcpTool2.Name { - t.Error("Expected same name in both instances") - } + assert.Equal(t, mcpTool1.Name, mcpTool2.Name) }) t.Run("MCP tool has correct structure", func(t *testing.T) { @@ -158,13 +118,8 @@ func TestTool_GetTool(t *testing.T) { mcpTool := tool.GetTool() - if mcpTool.Name == "" { - t.Error("MCP tool should have a name") - } - - if mcpTool.Description == "" { - t.Error("MCP tool should have a description") - } + assert.NotEmpty(t, mcpTool.Name) + assert.NotEmpty(t, mcpTool.Description) }) } @@ -172,15 +127,11 @@ func TestTool_RegisterWith(t *testing.T) { t.Run("sets RegisterCalled flag", func(t *testing.T) { tool := NewTool("register-test", true) - if tool.RegisterCalled { - t.Error("Expected RegisterCalled to be false initially") - } + assert.False(t, tool.RegisterCalled) tool.RegisterWith(nil) - if !tool.RegisterCalled { - t.Error("Expected RegisterCalled to be true after calling RegisterWith") - } + assert.True(t, tool.RegisterCalled) }) t.Run("can be called multiple times", func(t *testing.T) { @@ -188,15 +139,11 @@ func TestTool_RegisterWith(t *testing.T) { tool.RegisterWith(nil) - if !tool.RegisterCalled { - t.Error("Expected RegisterCalled to be true after first call") - } + assert.True(t, tool.RegisterCalled) tool.RegisterWith(nil) - if !tool.RegisterCalled { - t.Error("Expected RegisterCalled to remain true after second call") - } + assert.True(t, tool.RegisterCalled) }) t.Run("accepts nil server", func(t *testing.T) { @@ -205,9 +152,7 @@ func TestTool_RegisterWith(t *testing.T) { // Should not panic tool.RegisterWith(nil) - if !tool.RegisterCalled { - t.Error("Expected RegisterCalled to be true") - } + assert.True(t, tool.RegisterCalled) }) t.Run("can track registration state", func(t *testing.T) { @@ -216,15 +161,11 @@ func TestTool_RegisterWith(t *testing.T) { // Reset the flag tool.RegisterCalled = false - if tool.RegisterCalled { - t.Error("Expected RegisterCalled to be false after reset") - } + assert.False(t, tool.RegisterCalled) tool.RegisterWith(nil) - if !tool.RegisterCalled { - t.Error("Expected RegisterCalled to be true after registration") - } + assert.True(t, tool.RegisterCalled) }) } @@ -237,18 +178,11 @@ func TestTool_InterfaceCompliance(t *testing.T) { func TestTool_AsInterface(t *testing.T) { var toolInstance toolsets.Tool = NewTool("interface-test", true) - if toolInstance.GetName() != "interface-test" { - t.Errorf("Expected name 'interface-test', got %q", toolInstance.GetName()) - } - - if !toolInstance.IsReadOnly() { - t.Error("Expected tool to be read-only") - } + assert.Equal(t, "interface-test", toolInstance.GetName()) + assert.True(t, toolInstance.IsReadOnly()) mcpTool := toolInstance.GetTool() - if mcpTool == nil { - t.Error("Expected non-nil MCP tool") - } + assert.NotNil(t, mcpTool) toolInstance.RegisterWith(nil) } @@ -258,14 +192,10 @@ func TestTool_EdgeCases(t *testing.T) { longName := "very-long-tool-name-that-might-be-used-in-some-edge-case-scenario-for-testing-purposes" tool := NewTool(longName, true) - if tool.GetName() != longName { - t.Errorf("Expected long name to be preserved") - } + assert.Equal(t, longName, tool.GetName()) mcpTool := tool.GetTool() - if mcpTool.Name != longName { - t.Error("Expected MCP tool to have long name") - } + assert.Equal(t, longName, mcpTool.Name) }) t.Run("tool state is mutable", func(t *testing.T) { @@ -273,77 +203,50 @@ func TestTool_EdgeCases(t *testing.T) { // Change name tool.NameValue = "new-name" - if tool.GetName() != "new-name" { - t.Error("Expected name to be mutable") - } + assert.Equal(t, "new-name", tool.GetName()) // Change read-only tool.ReadOnlyValue = false - if tool.IsReadOnly() { - t.Error("Expected read-only to be mutable") - } + assert.False(t, tool.IsReadOnly()) // Change register flag tool.RegisterCalled = true - if !tool.RegisterCalled { - t.Error("Expected register flag to be mutable") - } + assert.True(t, tool.RegisterCalled) }) t.Run("multiple tools with same name", func(t *testing.T) { tool1 := NewTool("same-name", true) tool2 := NewTool("same-name", false) - if tool1.GetName() != tool2.GetName() { - t.Error("Expected both tools to have same name") - } - - if tool1 == tool2 { - t.Error("Expected different tool instances") - } - - if tool1.IsReadOnly() == tool2.IsReadOnly() { - t.Error("Expected different read-only values") - } + assert.Equal(t, tool1.GetName(), tool2.GetName()) + assert.NotSame(t, tool1, tool2) + assert.NotEqual(t, tool1.IsReadOnly(), tool2.IsReadOnly()) }) } func TestTool_ReadOnlyWorkflow(t *testing.T) { tool := NewTool("read-tool", true) - if !tool.IsReadOnly() { - t.Error("Expected read-only tool") - } - - if tool.RegisterCalled { - t.Error("Should not be registered initially") - } + assert.True(t, tool.IsReadOnly()) + assert.False(t, tool.RegisterCalled) mcpTool := tool.GetTool() - if mcpTool.Name != "read-tool" { - t.Error("MCP tool should have correct name") - } + assert.Equal(t, "read-tool", mcpTool.Name) tool.RegisterWith(nil) - if !tool.RegisterCalled { - t.Error("Should be registered after RegisterWith call") - } + assert.True(t, tool.RegisterCalled) } func TestTool_WritableWorkflow(t *testing.T) { tool := NewTool("write-tool", false) - if tool.IsReadOnly() { - t.Error("Expected writable tool") - } + assert.False(t, tool.IsReadOnly()) _ = tool.GetTool() tool.RegisterWith(nil) - if !tool.RegisterCalled { - t.Error("Should be registered") - } + assert.True(t, tool.RegisterCalled) } func TestTool_InToolset(t *testing.T) { @@ -353,15 +256,12 @@ func TestTool_InToolset(t *testing.T) { tools := []toolsets.Tool{tool1, tool2} for _, toolInstance := range tools { - if toolInstance.GetName() == "" { - t.Error("Tool in toolset should have name") - } + assert.NotEmpty(t, toolInstance.GetName()) _ = toolInstance.GetTool() toolInstance.RegisterWith(nil) } - if !tool1.RegisterCalled || !tool2.RegisterCalled { - t.Error("All tools should be registered") - } + assert.True(t, tool1.RegisterCalled) + assert.True(t, tool2.RegisterCalled) } diff --git a/internal/toolsets/mock/toolset_test.go b/internal/toolsets/mock/toolset_test.go index 658e21c..3237d89 100644 --- a/internal/toolsets/mock/toolset_test.go +++ b/internal/toolsets/mock/toolset_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/stackrox/stackrox-mcp/internal/toolsets" + "github.com/stretchr/testify/assert" ) func TestNewToolset(t *testing.T) { @@ -17,45 +18,28 @@ func TestNewToolset(t *testing.T) { toolset := NewToolset(name, enabled, tools) - if toolset.NameValue != name { - t.Errorf("Expected NameValue %q, got %q", name, toolset.NameValue) - } - - if toolset.EnabledValue != enabled { - t.Errorf("Expected EnabledValue %v, got %v", enabled, toolset.EnabledValue) - } - - if len(toolset.ToolsValue) != len(tools) { - t.Errorf("Expected %d tools, got %d", len(tools), len(toolset.ToolsValue)) - } + assert.Equal(t, name, toolset.NameValue) + assert.Equal(t, enabled, toolset.EnabledValue) + assert.Len(t, toolset.ToolsValue, len(tools)) }) t.Run("creates disabled toolset", func(t *testing.T) { toolset := NewToolset("disabled-toolset", false, nil) - if toolset.EnabledValue { - t.Error("Expected toolset to be disabled") - } + assert.False(t, toolset.EnabledValue) }) t.Run("creates toolset with empty tools", func(t *testing.T) { toolset := NewToolset("empty-toolset", true, []toolsets.Tool{}) - if toolset.ToolsValue == nil { - t.Error("Expected non-nil ToolsValue, got nil") - } - - if len(toolset.ToolsValue) != 0 { - t.Errorf("Expected 0 tools, got %d", len(toolset.ToolsValue)) - } + assert.NotNil(t, toolset.ToolsValue) + assert.Len(t, toolset.ToolsValue, 0) }) t.Run("creates toolset with nil tools", func(t *testing.T) { toolset := NewToolset("nil-tools", true, nil) - if toolset.ToolsValue != nil { - t.Errorf("Expected nil ToolsValue, got %v", toolset.ToolsValue) - } + assert.Nil(t, toolset.ToolsValue) }) } @@ -64,17 +48,13 @@ func TestToolset_GetName(t *testing.T) { name := "my-toolset" toolset := NewToolset(name, true, nil) - if toolset.GetName() != name { - t.Errorf("Expected name %q, got %q", name, toolset.GetName()) - } + assert.Equal(t, name, toolset.GetName()) }) t.Run("returns empty string if configured", func(t *testing.T) { toolset := NewToolset("", true, nil) - if toolset.GetName() != "" { - t.Errorf("Expected empty name, got %q", toolset.GetName()) - } + assert.Equal(t, "", toolset.GetName()) }) } @@ -82,31 +62,23 @@ func TestToolset_IsEnabled(t *testing.T) { t.Run("returns true when enabled", func(t *testing.T) { toolset := NewToolset("enabled", true, nil) - if !toolset.IsEnabled() { - t.Error("Expected toolset to be enabled") - } + assert.True(t, toolset.IsEnabled()) }) t.Run("returns false when disabled", func(t *testing.T) { toolset := NewToolset("disabled", false, nil) - if toolset.IsEnabled() { - t.Error("Expected toolset to be disabled") - } + assert.False(t, toolset.IsEnabled()) }) t.Run("can toggle enabled state", func(t *testing.T) { toolset := NewToolset("toggle", true, nil) - if !toolset.IsEnabled() { - t.Error("Expected initially enabled") - } + assert.True(t, toolset.IsEnabled()) toolset.EnabledValue = false - if toolset.IsEnabled() { - t.Error("Expected disabled after toggle") - } + assert.False(t, toolset.IsEnabled()) }) } @@ -120,14 +92,10 @@ func TestToolset_GetTools_Enabled(t *testing.T) { result := toolset.GetTools() - if len(result) != len(tools) { - t.Errorf("Expected %d tools, got %d", len(tools), len(result)) - } + assert.Len(t, result, len(tools)) for i, tool := range result { - if tool != tools[i] { - t.Errorf("Tool at index %d doesn't match", i) - } + assert.Same(t, tools[i], tool) } } @@ -140,13 +108,8 @@ func TestToolset_GetTools_Disabled(t *testing.T) { result := toolset.GetTools() - if result == nil { - t.Error("Expected non-nil slice, got nil") - } - - if len(result) != 0 { - t.Errorf("Expected empty slice when disabled, got %d tools", len(result)) - } + assert.NotNil(t, result) + assert.Len(t, result, 0) } func TestToolset_GetTools_EmptyList(t *testing.T) { @@ -154,13 +117,8 @@ func TestToolset_GetTools_EmptyList(t *testing.T) { result := toolset.GetTools() - if result == nil { - t.Error("Expected non-nil slice, got nil") - } - - if len(result) != 0 { - t.Errorf("Expected 0 tools, got %d", len(result)) - } + assert.NotNil(t, result) + assert.Len(t, result, 0) } func TestToolset_GetTools_NilTools(t *testing.T) { @@ -168,9 +126,7 @@ func TestToolset_GetTools_NilTools(t *testing.T) { result := toolset.GetTools() - if result != nil { - t.Errorf("Expected nil, got %v", result) - } + assert.Nil(t, result) } func TestToolset_GetTools_ToggleState(t *testing.T) { @@ -179,25 +135,19 @@ func TestToolset_GetTools_ToggleState(t *testing.T) { // Initially enabled - should return tools result1 := toolset.GetTools() - if len(result1) != 1 { - t.Errorf("Expected 1 tool when enabled, got %d", len(result1)) - } + assert.Len(t, result1, 1) // Disable - should return empty slice toolset.EnabledValue = false result2 := toolset.GetTools() - if len(result2) != 0 { - t.Errorf("Expected 0 tools when disabled, got %d", len(result2)) - } + assert.Len(t, result2, 0) // Re-enable - should return tools again toolset.EnabledValue = true result3 := toolset.GetTools() - if len(result3) != 1 { - t.Errorf("Expected 1 tool when re-enabled, got %d", len(result3)) - } + assert.Len(t, result3, 1) } func TestToolset_InterfaceCompliance(t *testing.T) { @@ -209,18 +159,11 @@ func TestToolset_InterfaceCompliance(t *testing.T) { func TestToolset_AsInterface(t *testing.T) { var toolsetInstance toolsets.Toolset = NewToolset("interface-test", true, nil) - if toolsetInstance.GetName() != "interface-test" { - t.Errorf("Expected name 'interface-test', got %q", toolsetInstance.GetName()) - } - - if !toolsetInstance.IsEnabled() { - t.Error("Expected toolset to be enabled") - } + assert.Equal(t, "interface-test", toolsetInstance.GetName()) + assert.True(t, toolsetInstance.IsEnabled()) tools := toolsetInstance.GetTools() - if tools != nil { - t.Errorf("Expected nil tools, got %v", tools) - } + assert.Nil(t, tools) } func TestToolset_EdgeCases(t *testing.T) { @@ -229,9 +172,7 @@ func TestToolset_EdgeCases(t *testing.T) { toolset := NewToolset("single", true, tools) result := toolset.GetTools() - if len(result) != 1 { - t.Errorf("Expected 1 tool, got %d", len(result)) - } + assert.Len(t, result, 1) }) t.Run("toolset with many tools", func(t *testing.T) { @@ -243,17 +184,13 @@ func TestToolset_EdgeCases(t *testing.T) { toolset := NewToolset("many-tools", true, tools) result := toolset.GetTools() - if len(result) != 100 { - t.Errorf("Expected 100 tools, got %d", len(result)) - } + assert.Len(t, result, 100) }) t.Run("toolset name with special characters", func(t *testing.T) { name := "tool-set_123!@#" toolset := NewToolset(name, true, nil) - if toolset.GetName() != name { - t.Errorf("Expected name %q, got %q", name, toolset.GetName()) - } + assert.Equal(t, name, toolset.GetName()) }) } From 8191126e51e8a23615b12f67632c2053287df550 Mon Sep 17 00:00:00 2001 From: Tomasz Janiszewski Date: Fri, 12 Dec 2025 15:55:04 +0100 Subject: [PATCH 12/13] Fix testifylint issues: use assert.Empty for empty values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace assert.Equal(t, "", ...) with assert.Empty for empty strings and assert.Len(t, ..., 0) with assert.Empty for empty slices as recommended by testifylint. Changes: - tool_test.go: Use assert.Empty for empty name checks (2 instances) - toolset_test.go: Use assert.Empty for empty tools/name (5 instances) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 --- internal/toolsets/mock/tool_test.go | 4 ++-- internal/toolsets/mock/toolset_test.go | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/internal/toolsets/mock/tool_test.go b/internal/toolsets/mock/tool_test.go index 735d6e5..3e1759e 100644 --- a/internal/toolsets/mock/tool_test.go +++ b/internal/toolsets/mock/tool_test.go @@ -35,7 +35,7 @@ func TestNewTool(t *testing.T) { t.Run("creates tool with empty name", func(t *testing.T) { tool := NewTool("", true) - assert.Equal(t, "", tool.NameValue) + assert.Empty(t, tool.NameValue) }) } @@ -50,7 +50,7 @@ func TestTool_GetName(t *testing.T) { t.Run("returns empty string if configured", func(t *testing.T) { tool := NewTool("", false) - assert.Equal(t, "", tool.GetName()) + assert.Empty(t, tool.GetName()) }) t.Run("name with special characters", func(t *testing.T) { diff --git a/internal/toolsets/mock/toolset_test.go b/internal/toolsets/mock/toolset_test.go index 3237d89..a67bbab 100644 --- a/internal/toolsets/mock/toolset_test.go +++ b/internal/toolsets/mock/toolset_test.go @@ -33,7 +33,7 @@ func TestNewToolset(t *testing.T) { toolset := NewToolset("empty-toolset", true, []toolsets.Tool{}) assert.NotNil(t, toolset.ToolsValue) - assert.Len(t, toolset.ToolsValue, 0) + assert.Empty(t, toolset.ToolsValue) }) t.Run("creates toolset with nil tools", func(t *testing.T) { @@ -54,7 +54,7 @@ func TestToolset_GetName(t *testing.T) { t.Run("returns empty string if configured", func(t *testing.T) { toolset := NewToolset("", true, nil) - assert.Equal(t, "", toolset.GetName()) + assert.Empty(t, toolset.GetName()) }) } @@ -109,7 +109,7 @@ func TestToolset_GetTools_Disabled(t *testing.T) { result := toolset.GetTools() assert.NotNil(t, result) - assert.Len(t, result, 0) + assert.Empty(t, result) } func TestToolset_GetTools_EmptyList(t *testing.T) { @@ -118,7 +118,7 @@ func TestToolset_GetTools_EmptyList(t *testing.T) { result := toolset.GetTools() assert.NotNil(t, result) - assert.Len(t, result, 0) + assert.Empty(t, result) } func TestToolset_GetTools_NilTools(t *testing.T) { @@ -141,7 +141,7 @@ func TestToolset_GetTools_ToggleState(t *testing.T) { toolset.EnabledValue = false result2 := toolset.GetTools() - assert.Len(t, result2, 0) + assert.Empty(t, result2) // Re-enable - should return tools again toolset.EnabledValue = true From d49666a909ad273321a24cf24a8a82a5f694ba9c Mon Sep 17 00:00:00 2001 From: Tomasz Janiszewski Date: Mon, 15 Dec 2025 17:02:24 +0100 Subject: [PATCH 13/13] Remove redundant tests and add filename validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add filepath import and test name validation in config_test.go - Remove redundant subtests in tool_test.go (empty name, read-only/writable variants, GetName tests, edge cases) - Consolidate RegisterWith tests into single subtest - Remove redundant subtests in toolset_test.go (empty name, nil tools, toggle state, edge cases) These changes address PR review comments to reduce test redundancy while maintaining adequate coverage. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 --- internal/testutil/config_test.go | 4 + internal/toolsets/mock/tool_test.go | 127 +------------------------ internal/toolsets/mock/toolset_test.go | 64 ------------- 3 files changed, 5 insertions(+), 190 deletions(-) diff --git a/internal/testutil/config_test.go b/internal/testutil/config_test.go index 8d410b5..40861fa 100644 --- a/internal/testutil/config_test.go +++ b/internal/testutil/config_test.go @@ -2,6 +2,7 @@ package testutil import ( "os" + "path/filepath" "testing" "github.com/stretchr/testify/assert" @@ -12,6 +13,9 @@ func TestWriteYAMLFile(t *testing.T) { filePath := WriteYAMLFile(t, content) + fileName := filepath.Base(filePath) + assert.Contains(t, fileName, t.Name(), "File name should contain test name") + //nolint:gosec // Test code reading from known test file data, err := os.ReadFile(filePath) if err != nil { diff --git a/internal/toolsets/mock/tool_test.go b/internal/toolsets/mock/tool_test.go index 3e1759e..18b02e0 100644 --- a/internal/toolsets/mock/tool_test.go +++ b/internal/toolsets/mock/tool_test.go @@ -19,46 +19,6 @@ func TestNewTool(t *testing.T) { assert.Equal(t, readOnly, tool.ReadOnlyValue) assert.False(t, tool.RegisterCalled) }) - - t.Run("creates read-only tool", func(t *testing.T) { - tool := NewTool("readonly", true) - - assert.True(t, tool.ReadOnlyValue) - }) - - t.Run("creates writable tool", func(t *testing.T) { - tool := NewTool("writable", false) - - assert.False(t, tool.ReadOnlyValue) - }) - - t.Run("creates tool with empty name", func(t *testing.T) { - tool := NewTool("", true) - - assert.Empty(t, tool.NameValue) - }) -} - -func TestTool_GetName(t *testing.T) { - t.Run("returns configured name", func(t *testing.T) { - name := "my-tool" - tool := NewTool(name, true) - - assert.Equal(t, name, tool.GetName()) - }) - - t.Run("returns empty string if configured", func(t *testing.T) { - tool := NewTool("", false) - - assert.Empty(t, tool.GetName()) - }) - - t.Run("name with special characters", func(t *testing.T) { - name := "tool-name_123!@#" - tool := NewTool(name, true) - - assert.Equal(t, name, tool.GetName()) - }) } func TestTool_IsReadOnly(t *testing.T) { @@ -73,16 +33,6 @@ func TestTool_IsReadOnly(t *testing.T) { assert.False(t, tool.IsReadOnly()) }) - - t.Run("can toggle read-only state", func(t *testing.T) { - tool := NewTool("toggle", true) - - assert.True(t, tool.IsReadOnly()) - - tool.ReadOnlyValue = false - - assert.False(t, tool.IsReadOnly()) - }) } func TestTool_GetTool(t *testing.T) { @@ -112,15 +62,6 @@ func TestTool_GetTool(t *testing.T) { // But with same values assert.Equal(t, mcpTool1.Name, mcpTool2.Name) }) - - t.Run("MCP tool has correct structure", func(t *testing.T) { - tool := NewTool("structured-tool", false) - - mcpTool := tool.GetTool() - - assert.NotEmpty(t, mcpTool.Name) - assert.NotEmpty(t, mcpTool.Description) - }) } func TestTool_RegisterWith(t *testing.T) { @@ -132,37 +73,8 @@ func TestTool_RegisterWith(t *testing.T) { tool.RegisterWith(nil) assert.True(t, tool.RegisterCalled) - }) - - t.Run("can be called multiple times", func(t *testing.T) { - tool := NewTool("multi-register", false) - - tool.RegisterWith(nil) - - assert.True(t, tool.RegisterCalled) - - tool.RegisterWith(nil) - - assert.True(t, tool.RegisterCalled) - }) - - t.Run("accepts nil server", func(t *testing.T) { - tool := NewTool("nil-server", true) - - // Should not panic - tool.RegisterWith(nil) - - assert.True(t, tool.RegisterCalled) - }) - - t.Run("can track registration state", func(t *testing.T) { - tool := NewTool("track-registration", true) - - // Reset the flag - tool.RegisterCalled = false - - assert.False(t, tool.RegisterCalled) + // Can be called multiple times tool.RegisterWith(nil) assert.True(t, tool.RegisterCalled) @@ -187,43 +99,6 @@ func TestTool_AsInterface(t *testing.T) { toolInstance.RegisterWith(nil) } -func TestTool_EdgeCases(t *testing.T) { - t.Run("tool with very long name", func(t *testing.T) { - longName := "very-long-tool-name-that-might-be-used-in-some-edge-case-scenario-for-testing-purposes" - tool := NewTool(longName, true) - - assert.Equal(t, longName, tool.GetName()) - - mcpTool := tool.GetTool() - assert.Equal(t, longName, mcpTool.Name) - }) - - t.Run("tool state is mutable", func(t *testing.T) { - tool := NewTool("mutable", true) - - // Change name - tool.NameValue = "new-name" - assert.Equal(t, "new-name", tool.GetName()) - - // Change read-only - tool.ReadOnlyValue = false - assert.False(t, tool.IsReadOnly()) - - // Change register flag - tool.RegisterCalled = true - assert.True(t, tool.RegisterCalled) - }) - - t.Run("multiple tools with same name", func(t *testing.T) { - tool1 := NewTool("same-name", true) - tool2 := NewTool("same-name", false) - - assert.Equal(t, tool1.GetName(), tool2.GetName()) - assert.NotSame(t, tool1, tool2) - assert.NotEqual(t, tool1.IsReadOnly(), tool2.IsReadOnly()) - }) -} - func TestTool_ReadOnlyWorkflow(t *testing.T) { tool := NewTool("read-tool", true) diff --git a/internal/toolsets/mock/toolset_test.go b/internal/toolsets/mock/toolset_test.go index a67bbab..e749797 100644 --- a/internal/toolsets/mock/toolset_test.go +++ b/internal/toolsets/mock/toolset_test.go @@ -50,12 +50,6 @@ func TestToolset_GetName(t *testing.T) { assert.Equal(t, name, toolset.GetName()) }) - - t.Run("returns empty string if configured", func(t *testing.T) { - toolset := NewToolset("", true, nil) - - assert.Empty(t, toolset.GetName()) - }) } func TestToolset_IsEnabled(t *testing.T) { @@ -121,35 +115,6 @@ func TestToolset_GetTools_EmptyList(t *testing.T) { assert.Empty(t, result) } -func TestToolset_GetTools_NilTools(t *testing.T) { - toolset := NewToolset("nil-tools", true, nil) - - result := toolset.GetTools() - - assert.Nil(t, result) -} - -func TestToolset_GetTools_ToggleState(t *testing.T) { - tools := []toolsets.Tool{NewTool("tool1", true)} - toolset := NewToolset("toggle-toolset", true, tools) - - // Initially enabled - should return tools - result1 := toolset.GetTools() - assert.Len(t, result1, 1) - - // Disable - should return empty slice - toolset.EnabledValue = false - - result2 := toolset.GetTools() - assert.Empty(t, result2) - - // Re-enable - should return tools again - toolset.EnabledValue = true - - result3 := toolset.GetTools() - assert.Len(t, result3, 1) -} - func TestToolset_InterfaceCompliance(t *testing.T) { t.Run("implements toolsets.Toolset interface", func(*testing.T) { var _ toolsets.Toolset = (*Toolset)(nil) @@ -165,32 +130,3 @@ func TestToolset_AsInterface(t *testing.T) { tools := toolsetInstance.GetTools() assert.Nil(t, tools) } - -func TestToolset_EdgeCases(t *testing.T) { - t.Run("toolset with single tool", func(t *testing.T) { - tools := []toolsets.Tool{NewTool("only-tool", true)} - toolset := NewToolset("single", true, tools) - - result := toolset.GetTools() - assert.Len(t, result, 1) - }) - - t.Run("toolset with many tools", func(t *testing.T) { - tools := make([]toolsets.Tool, 100) - for i := range 100 { - tools[i] = NewTool("tool", i%2 == 0) - } - - toolset := NewToolset("many-tools", true, tools) - - result := toolset.GetTools() - assert.Len(t, result, 100) - }) - - t.Run("toolset name with special characters", func(t *testing.T) { - name := "tool-set_123!@#" - toolset := NewToolset(name, true, nil) - - assert.Equal(t, name, toolset.GetName()) - }) -}