From 2ff2b04434c252895d6d04865c162af0fbe73101 Mon Sep 17 00:00:00 2001 From: Daniel Riccio Date: Wed, 23 Jul 2025 12:23:54 -0500 Subject: [PATCH] fix: remove error state check from search service and improve tests - Remove error state check from search-service.ts as state checking is now handled in codebaseSearchTool - Add comprehensive test coverage for search service - Add translations for error messages - Consolidate duplicate test assertions for better readability --- .../__tests__/codebaseSearchTool.spec.ts | 51 +++--- src/core/tools/codebaseSearchTool.ts | 5 +- src/i18n/locales/en/tools.json | 6 +- .../__tests__/search-service.spec.ts | 155 ++++++++++++++++++ src/services/code-index/search-service.ts | 4 - 5 files changed, 191 insertions(+), 30 deletions(-) create mode 100644 src/services/code-index/__tests__/search-service.spec.ts diff --git a/src/core/tools/__tests__/codebaseSearchTool.spec.ts b/src/core/tools/__tests__/codebaseSearchTool.spec.ts index b6cd5092d8..5a20a81dfc 100644 --- a/src/core/tools/__tests__/codebaseSearchTool.spec.ts +++ b/src/core/tools/__tests__/codebaseSearchTool.spec.ts @@ -19,6 +19,16 @@ vi.mock("vscode", () => ({ asRelativePath: vi.fn((path: string) => path.replace("/test/workspace/", "")), }, })) +vi.mock("../../../i18n", () => ({ + t: vi.fn((key: string) => { + const translations: Record = { + "tools.codebaseSearch.errors.disabled": "Code Indexing is disabled in the settings.", + "tools.codebaseSearch.errors.notConfigured": + "Code Indexing is not configured (Missing OpenAI Key or Qdrant URL).", + } + return translations[key] || key + }), +})) describe("codebaseSearchTool", () => { let mockTask: Task @@ -92,13 +102,11 @@ describe("codebaseSearchTool", () => { mockRemoveClosingTag, ) - expect(mockPushToolResult).toHaveBeenCalledWith( - expect.stringContaining("Semantic search is not available yet (currently Standby)"), - ) - expect(mockPushToolResult).toHaveBeenCalledWith( - expect.stringContaining("Code indexing has not started yet"), - ) - expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("Please use file reading tools")) + // Verify the complete message was pushed + const pushedMessage = mockPushToolResult.mock.calls[0][0] + expect(pushedMessage).toContain("Semantic search is not available yet (currently Standby)") + expect(pushedMessage).toContain("Code indexing has not started yet") + expect(pushedMessage).toContain("Please use file reading tools") expect(mockCodeIndexManager.searchIndex).not.toHaveBeenCalled() }) @@ -121,12 +129,10 @@ describe("codebaseSearchTool", () => { mockRemoveClosingTag, ) - expect(mockPushToolResult).toHaveBeenCalledWith( - expect.stringContaining("Semantic search is not available yet (currently Indexing)"), - ) - expect(mockPushToolResult).toHaveBeenCalledWith( - expect.stringContaining("Code indexing is currently in progress"), - ) + // Verify the complete message was pushed + const pushedMessage = mockPushToolResult.mock.calls[0][0] + expect(pushedMessage).toContain("Semantic search is not available yet (currently Indexing)") + expect(pushedMessage).toContain("Code indexing is currently in progress") expect(mockCodeIndexManager.searchIndex).not.toHaveBeenCalled() }) @@ -149,12 +155,10 @@ describe("codebaseSearchTool", () => { mockRemoveClosingTag, ) - expect(mockPushToolResult).toHaveBeenCalledWith( - expect.stringContaining("Semantic search is not available yet (currently Error)"), - ) - expect(mockPushToolResult).toHaveBeenCalledWith( - expect.stringContaining("Code indexing encountered an error"), - ) + // Verify the complete message was pushed + const pushedMessage = mockPushToolResult.mock.calls[0][0] + expect(pushedMessage).toContain("Semantic search is not available yet (currently Error)") + expect(pushedMessage).toContain("Code indexing encountered an error") expect(mockCodeIndexManager.searchIndex).not.toHaveBeenCalled() }) @@ -181,8 +185,9 @@ describe("codebaseSearchTool", () => { // Check that say was called with the search results expect(mockTask.say).toHaveBeenCalledWith("codebase_search_result", expect.stringContaining("test code")) // Check that pushToolResult was called with the formatted output - expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("Query: test query")) - expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("test code")) + const pushedResult = mockPushToolResult.mock.calls[0][0] + expect(pushedResult).toContain("Query: test query") + expect(pushedResult).toContain("test code") expect(mockPushToolResult).not.toHaveBeenCalledWith( expect.stringContaining("Semantic search is not available"), ) @@ -212,7 +217,7 @@ describe("codebaseSearchTool", () => { expect(mockHandleError).toHaveBeenCalledWith( "codebase_search", expect.objectContaining({ - message: "Code Indexing is disabled in the settings.", + message: expect.stringContaining("Code Indexing is disabled"), }), ) }) @@ -239,7 +244,7 @@ describe("codebaseSearchTool", () => { expect(mockHandleError).toHaveBeenCalledWith( "codebase_search", expect.objectContaining({ - message: "Code Indexing is not configured (Missing OpenAI Key or Qdrant URL).", + message: expect.stringContaining("Code Indexing is not configured"), }), ) }) diff --git a/src/core/tools/codebaseSearchTool.ts b/src/core/tools/codebaseSearchTool.ts index a5e2baaaea..c0b0877dc4 100644 --- a/src/core/tools/codebaseSearchTool.ts +++ b/src/core/tools/codebaseSearchTool.ts @@ -7,6 +7,7 @@ import { formatResponse } from "../prompts/responses" import { VectorStoreSearchResult } from "../../services/code-index/interfaces" import { AskApproval, HandleError, PushToolResult, RemoveClosingTag, ToolUse } from "../../shared/tools" import path from "path" +import { t } from "../../i18n" export async function codebaseSearchTool( cline: Task, @@ -76,10 +77,10 @@ export async function codebaseSearchTool( } if (!manager.isFeatureEnabled) { - throw new Error("Code Indexing is disabled in the settings.") + throw new Error(t("tools.codebaseSearch.errors.disabled")) } if (!manager.isFeatureConfigured) { - throw new Error("Code Indexing is not configured (Missing OpenAI Key or Qdrant URL).") + throw new Error(t("tools.codebaseSearch.errors.notConfigured")) } // Check indexing state at runtime diff --git a/src/i18n/locales/en/tools.json b/src/i18n/locales/en/tools.json index 0265a84398..5bf546d576 100644 --- a/src/i18n/locales/en/tools.json +++ b/src/i18n/locales/en/tools.json @@ -6,7 +6,11 @@ }, "toolRepetitionLimitReached": "Roo appears to be stuck in a loop, attempting the same action ({{toolName}}) repeatedly. This might indicate a problem with its current strategy. Consider rephrasing the task, providing more specific instructions, or guiding it towards a different approach.", "codebaseSearch": { - "approval": "Searching for '{{query}}' in codebase..." + "approval": "Searching for '{{query}}' in codebase...", + "errors": { + "disabled": "Code Indexing is disabled in the settings.", + "notConfigured": "Code Indexing is not configured (Missing OpenAI Key or Qdrant URL)." + } }, "newTask": { "errors": { diff --git a/src/services/code-index/__tests__/search-service.spec.ts b/src/services/code-index/__tests__/search-service.spec.ts new file mode 100644 index 0000000000..10439f9fa9 --- /dev/null +++ b/src/services/code-index/__tests__/search-service.spec.ts @@ -0,0 +1,155 @@ +import { describe, it, expect, vi, beforeEach } from "vitest" +import { CodeIndexSearchService } from "../search-service" +import { CodeIndexConfigManager } from "../config-manager" +import { CodeIndexStateManager } from "../state-manager" +import { IEmbedder } from "../interfaces/embedder" +import { IVectorStore } from "../interfaces/vector-store" +import { TelemetryService } from "@roo-code/telemetry" +import { TelemetryEventName } from "@roo-code/types" + +// Mock dependencies +vi.mock("@roo-code/telemetry") + +describe("CodeIndexSearchService", () => { + let searchService: CodeIndexSearchService + let mockConfigManager: any + let mockStateManager: any + let mockEmbedder: any + let mockVectorStore: any + + beforeEach(() => { + vi.clearAllMocks() + + // Setup mock config manager + mockConfigManager = { + isFeatureEnabled: true, + isFeatureConfigured: true, + currentSearchMinScore: 0.5, + currentSearchMaxResults: 10, + } + + // Setup mock state manager + mockStateManager = { + getCurrentStatus: vi.fn(() => ({ systemStatus: "Indexed" })), + setSystemState: vi.fn(), + } + + // Setup mock embedder + mockEmbedder = { + createEmbeddings: vi.fn().mockResolvedValue({ + embeddings: [[0.1, 0.2, 0.3]], + }), + } + + // Setup mock vector store + mockVectorStore = { + search: vi.fn().mockResolvedValue([ + { + score: 0.9, + payload: { + filePath: "/test/file.ts", + startLine: 1, + endLine: 10, + codeChunk: "test code", + }, + }, + ]), + } + + // Setup mock telemetry + const mockTelemetryInstance = { + captureEvent: vi.fn(), + } + vi.spyOn(TelemetryService, "instance", "get").mockReturnValue(mockTelemetryInstance as any) + + searchService = new CodeIndexSearchService( + mockConfigManager as CodeIndexConfigManager, + mockStateManager as CodeIndexStateManager, + mockEmbedder as IEmbedder, + mockVectorStore as IVectorStore, + ) + }) + + describe("searchIndex", () => { + it("should throw error when feature is disabled", async () => { + mockConfigManager.isFeatureEnabled = false + + await expect(searchService.searchIndex("test query")).rejects.toThrow( + "Code index feature is disabled or not configured.", + ) + }) + + it("should throw error when feature is not configured", async () => { + mockConfigManager.isFeatureConfigured = false + + await expect(searchService.searchIndex("test query")).rejects.toThrow( + "Code index feature is disabled or not configured.", + ) + }) + + it("should perform search successfully when in Indexed state", async () => { + const query = "test query" + const results = await searchService.searchIndex(query) + + expect(mockEmbedder.createEmbeddings).toHaveBeenCalledWith([query]) + expect(mockVectorStore.search).toHaveBeenCalledWith([0.1, 0.2, 0.3], undefined, 0.5, 10) + expect(results).toHaveLength(1) + expect(results[0].score).toBe(0.9) + }) + + it("should handle directory prefix correctly", async () => { + const query = "test query" + const directoryPrefix = "src/components" + + await searchService.searchIndex(query, directoryPrefix) + + expect(mockVectorStore.search).toHaveBeenCalledWith([0.1, 0.2, 0.3], "src/components", 0.5, 10) + }) + + it("should NOT throw error when in Error state (state checking moved to tool)", async () => { + mockStateManager.getCurrentStatus.mockReturnValue({ systemStatus: "Error" }) + + // Should not throw, as state checking is now handled in the tool + const results = await searchService.searchIndex("test query") + expect(results).toHaveLength(1) + }) + + it("should handle embedding generation failure", async () => { + mockEmbedder.createEmbeddings.mockResolvedValue({ embeddings: [] }) + + await expect(searchService.searchIndex("test query")).rejects.toThrow( + "Failed to generate embedding for query.", + ) + }) + + it("should capture telemetry and set error state on search failure", async () => { + const error = new Error("Vector store error") + mockVectorStore.search.mockRejectedValue(error) + + await expect(searchService.searchIndex("test query")).rejects.toThrow("Vector store error") + + expect(mockStateManager.setSystemState).toHaveBeenCalledWith("Error", "Search failed: Vector store error") + expect(TelemetryService.instance.captureEvent).toHaveBeenCalledWith(TelemetryEventName.CODE_INDEX_ERROR, { + error: "Vector store error", + stack: expect.any(String), + location: "searchIndex", + }) + }) + + it("should work correctly when in Indexing state", async () => { + mockStateManager.getCurrentStatus.mockReturnValue({ systemStatus: "Indexing" }) + + // Should not throw, as state checking is now handled in the tool + const results = await searchService.searchIndex("test query") + expect(results).toHaveLength(1) + }) + + it("should work correctly when in Standby state", async () => { + mockStateManager.getCurrentStatus.mockReturnValue({ systemStatus: "Standby" }) + + // Should not throw, as state checking is now handled in the tool + const results = await searchService.searchIndex("test query") + expect(results).toHaveLength(1) + }) + }) +}) diff --git a/src/services/code-index/search-service.ts b/src/services/code-index/search-service.ts index a8361587db..b2e41dd9d4 100644 --- a/src/services/code-index/search-service.ts +++ b/src/services/code-index/search-service.ts @@ -36,10 +36,6 @@ export class CodeIndexSearchService { // Note: State checking is now handled in the codebaseSearchTool // This allows the tool to provide more user-friendly feedback - const currentState = this.stateManager.getCurrentStatus().systemStatus - if (currentState === "Error") { - throw new Error(`Code index is in error state. Please check your configuration.`) - } try { // Generate embedding for query