From d80cff6d37c300aa0b518c3130ca88531e47693e Mon Sep 17 00:00:00 2001 From: Daniel Riccio Date: Thu, 19 Jun 2025 12:37:18 -0500 Subject: [PATCH] refactor(marketplace): Add marketplace disabled check to RemoteConfigLoader - Check marketplace enabled state before making API calls - Return empty array when marketplace is disabled - Add comprehensive tests for network timeout and disabled state - Consolidate test files into RemoteConfigLoader.spec.ts --- .../marketplace/RemoteConfigLoader.ts | 21 +- ...fix.test.ts => RemoteConfigLoader.spec.ts} | 35 +- .../__tests__/RemoteConfigLoader.test.ts | 346 ------------------ 3 files changed, 33 insertions(+), 369 deletions(-) rename src/services/marketplace/__tests__/{network-timeout-fix.test.ts => RemoteConfigLoader.spec.ts} (76%) delete mode 100644 src/services/marketplace/__tests__/RemoteConfigLoader.test.ts diff --git a/src/services/marketplace/RemoteConfigLoader.ts b/src/services/marketplace/RemoteConfigLoader.ts index 8500a3c4ef..75ecc138d2 100644 --- a/src/services/marketplace/RemoteConfigLoader.ts +++ b/src/services/marketplace/RemoteConfigLoader.ts @@ -101,10 +101,23 @@ export class RemoteConfigLoader { return response.data as T } catch (error) { lastError = error as Error - console.log( - `Marketplace: API request failed (attempt ${i + 1}/${maxRetries}):`, - error instanceof Error ? error.message : String(error), - ) + console.error(`Marketplace: API request failed (attempt ${i + 1}/${maxRetries})`, { + error: + error instanceof Error + ? { + name: error.name, + message: error.message, + stack: error.stack, + ...(axios.isAxiosError(error) && { + code: error.code, + status: error.response?.status, + statusText: error.response?.statusText, + data: error.response?.data, + headers: error.response?.headers, + }), + } + : error, + }) if (i < maxRetries - 1) { // Exponential backoff: 1s, 2s, 4s diff --git a/src/services/marketplace/__tests__/network-timeout-fix.test.ts b/src/services/marketplace/__tests__/RemoteConfigLoader.spec.ts similarity index 76% rename from src/services/marketplace/__tests__/network-timeout-fix.test.ts rename to src/services/marketplace/__tests__/RemoteConfigLoader.spec.ts index d4172c85f0..b21bf5d612 100644 --- a/src/services/marketplace/__tests__/network-timeout-fix.test.ts +++ b/src/services/marketplace/__tests__/RemoteConfigLoader.spec.ts @@ -1,32 +1,31 @@ import * as vscode from "vscode" +import { describe, it, expect, beforeEach, vi } from "vitest" import { RemoteConfigLoader } from "../RemoteConfigLoader" import { MarketplaceManager } from "../MarketplaceManager" // Mock vscode -jest.mock("vscode", () => ({ +vi.mock("vscode", () => ({ workspace: { - getConfiguration: jest.fn(), + getConfiguration: vi.fn(), }, })) // Mock axios to simulate network timeouts -jest.mock("axios") +vi.mock("axios") describe("Network Timeout Fix", () => { - let mockGetConfiguration: jest.MockedFunction + let mockGetConfiguration: ReturnType beforeEach(() => { - mockGetConfiguration = vscode.workspace.getConfiguration as jest.MockedFunction< - typeof vscode.workspace.getConfiguration - > - jest.clearAllMocks() + mockGetConfiguration = vscode.workspace.getConfiguration as ReturnType + vi.clearAllMocks() }) describe("RemoteConfigLoader", () => { it("should return empty array when marketplace is disabled via user setting", async () => { // Mock configuration to disable marketplace mockGetConfiguration.mockReturnValue({ - get: jest.fn((key: string, defaultValue?: any) => { + get: vi.fn((key: string, defaultValue?: any) => { if (key === "disableMarketplace") return true if (key === "marketplaceTimeout") return 10000 return defaultValue @@ -45,7 +44,7 @@ describe("Network Timeout Fix", () => { // Mock configuration with custom timeout mockGetConfiguration.mockReturnValue({ - get: jest.fn((key: string, defaultValue?: any) => { + get: vi.fn((key: string, defaultValue?: any) => { if (key === "disableMarketplace") return false if (key === "marketplaceTimeout") return customTimeout return defaultValue @@ -55,7 +54,7 @@ describe("Network Timeout Fix", () => { const loader = new RemoteConfigLoader() // Mock the private fetchWithRetry method to verify timeout is used - const fetchWithRetrySpy = jest.spyOn(loader as any, "fetchWithRetry") + const fetchWithRetrySpy = vi.spyOn(loader as any, "fetchWithRetry") fetchWithRetrySpy.mockRejectedValue(new Error("Network timeout")) try { @@ -68,11 +67,11 @@ describe("Network Timeout Fix", () => { }) it("should log appropriate messages when marketplace is disabled", async () => { - const consoleSpy = jest.spyOn(console, "log").mockImplementation() + const consoleSpy = vi.spyOn(console, "log").mockImplementation(() => {}) // Mock configuration to disable marketplace mockGetConfiguration.mockReturnValue({ - get: jest.fn((key: string, defaultValue?: any) => { + get: vi.fn((key: string, defaultValue?: any) => { if (key === "disableMarketplace") return true return defaultValue }), @@ -91,7 +90,7 @@ describe("Network Timeout Fix", () => { it("should handle network errors gracefully", async () => { // Mock configuration to enable marketplace but simulate network issues mockGetConfiguration.mockReturnValue({ - get: jest.fn((key: string, defaultValue?: any) => { + get: vi.fn((key: string, defaultValue?: any) => { if (key === "disableMarketplace") return false if (key === "marketplaceTimeout") return 1000 // Very short timeout return defaultValue @@ -100,8 +99,8 @@ describe("Network Timeout Fix", () => { const mockContext = { globalState: { - get: jest.fn(), - update: jest.fn(), + get: vi.fn(), + update: vi.fn(), }, extensionPath: "/mock/path", } as any @@ -109,9 +108,7 @@ describe("Network Timeout Fix", () => { const manager = new MarketplaceManager(mockContext) // Mock the config loader to throw a timeout error - jest.spyOn(manager["configLoader"], "loadAllItems").mockRejectedValue( - new Error("timeout of 1000ms exceeded"), - ) + vi.spyOn(manager["configLoader"], "loadAllItems").mockRejectedValue(new Error("timeout of 1000ms exceeded")) const result = await manager.getMarketplaceItems() diff --git a/src/services/marketplace/__tests__/RemoteConfigLoader.test.ts b/src/services/marketplace/__tests__/RemoteConfigLoader.test.ts deleted file mode 100644 index c0c48bc289..0000000000 --- a/src/services/marketplace/__tests__/RemoteConfigLoader.test.ts +++ /dev/null @@ -1,346 +0,0 @@ -import axios from "axios" -import { RemoteConfigLoader } from "../RemoteConfigLoader" -import type { MarketplaceItemType } from "@roo-code/types" - -// Mock axios -jest.mock("axios") -const mockedAxios = axios as jest.Mocked - -// Mock vscode -jest.mock("vscode", () => ({ - workspace: { - getConfiguration: jest.fn(() => ({ - get: jest.fn((key: string, defaultValue?: any) => { - if (key === "disableMarketplace") return false - if (key === "marketplaceTimeout") return 10000 - return defaultValue - }), - })), - }, -})) - -// Mock the cloud config -jest.mock("@roo-code/cloud", () => ({ - getRooCodeApiUrl: () => "https://test.api.com", -})) - -describe("RemoteConfigLoader", () => { - let loader: RemoteConfigLoader - - beforeEach(() => { - loader = new RemoteConfigLoader() - jest.clearAllMocks() - // Clear any existing cache - loader.clearCache() - }) - - describe("loadAllItems", () => { - it("should fetch and combine modes and MCPs from API", async () => { - const mockModesYaml = `items: - - id: "test-mode" - name: "Test Mode" - description: "A test mode" - content: "customModes:\\n - slug: test\\n name: Test"` - - const mockMcpsYaml = `items: - - id: "test-mcp" - name: "Test MCP" - description: "A test MCP" - url: "https://github.com/test/test-mcp" - content: '{"command": "test"}'` - - mockedAxios.get.mockImplementation((url) => { - if (url.includes("/modes")) { - return Promise.resolve({ data: mockModesYaml }) - } - if (url.includes("/mcps")) { - return Promise.resolve({ data: mockMcpsYaml }) - } - return Promise.reject(new Error("Unknown URL")) - }) - - const items = await loader.loadAllItems() - - expect(mockedAxios.get).toHaveBeenCalledTimes(2) - expect(mockedAxios.get).toHaveBeenCalledWith( - "https://test.api.com/api/marketplace/modes", - expect.objectContaining({ - timeout: 10000, - headers: { - Accept: "application/json", - "Content-Type": "application/json", - }, - }), - ) - expect(mockedAxios.get).toHaveBeenCalledWith( - "https://test.api.com/api/marketplace/mcps", - expect.objectContaining({ - timeout: 10000, - headers: { - Accept: "application/json", - "Content-Type": "application/json", - }, - }), - ) - - expect(items).toHaveLength(2) - expect(items[0]).toEqual({ - type: "mode", - id: "test-mode", - name: "Test Mode", - description: "A test mode", - content: "customModes:\n - slug: test\n name: Test", - }) - expect(items[1]).toEqual({ - type: "mcp", - id: "test-mcp", - name: "Test MCP", - description: "A test MCP", - url: "https://github.com/test/test-mcp", - content: '{"command": "test"}', - }) - }) - - it("should use cache on subsequent calls", async () => { - const mockModesYaml = `items: - - id: "test-mode" - name: "Test Mode" - description: "A test mode" - content: "test content"` - - const mockMcpsYaml = `items: - - id: "test-mcp" - name: "Test MCP" - description: "A test MCP" - url: "https://github.com/test/test-mcp" - content: "test content"` - - mockedAxios.get.mockImplementation((url) => { - if (url.includes("/modes")) { - return Promise.resolve({ data: mockModesYaml }) - } - if (url.includes("/mcps")) { - return Promise.resolve({ data: mockMcpsYaml }) - } - return Promise.reject(new Error("Unknown URL")) - }) - - // First call - should hit API - const items1 = await loader.loadAllItems() - expect(mockedAxios.get).toHaveBeenCalledTimes(2) - - // Second call - should use cache - const items2 = await loader.loadAllItems() - expect(mockedAxios.get).toHaveBeenCalledTimes(2) // Still 2, not 4 - - expect(items1).toEqual(items2) - }) - - it("should retry on network failures", async () => { - const mockModesYaml = `items: - - id: "test-mode" - name: "Test Mode" - description: "A test mode" - content: "test content"` - - const mockMcpsYaml = `items: []` - - // Mock modes endpoint to fail twice then succeed - let modesCallCount = 0 - mockedAxios.get.mockImplementation((url) => { - if (url.includes("/modes")) { - modesCallCount++ - if (modesCallCount <= 2) { - return Promise.reject(new Error("Network error")) - } - return Promise.resolve({ data: mockModesYaml }) - } - if (url.includes("/mcps")) { - return Promise.resolve({ data: mockMcpsYaml }) - } - return Promise.reject(new Error("Unknown URL")) - }) - - const items = await loader.loadAllItems() - - // Should have retried modes endpoint 3 times (2 failures + 1 success) - expect(modesCallCount).toBe(3) - expect(items).toHaveLength(1) - expect(items[0].type).toBe("mode") - }) - - it("should throw error after max retries", async () => { - mockedAxios.get.mockRejectedValue(new Error("Persistent network error")) - - await expect(loader.loadAllItems()).rejects.toThrow("Persistent network error") - - // Both endpoints will be called with retries since Promise.all starts both promises - // Each endpoint retries 3 times, but due to Promise.all behavior, one might fail faster - expect(mockedAxios.get).toHaveBeenCalledWith( - expect.stringContaining("/api/marketplace/"), - expect.any(Object), - ) - // Verify we got at least some retry attempts (should be at least 2 calls) - expect(mockedAxios.get.mock.calls.length).toBeGreaterThanOrEqual(2) - }) - - it("should handle invalid data gracefully", async () => { - const invalidModesYaml = `items: - - id: "invalid-mode" - # Missing required fields like name and description` - - const validMcpsYaml = `items: - - id: "valid-mcp" - name: "Valid MCP" - description: "A valid MCP" - url: "https://github.com/test/test-mcp" - content: "test content"` - - mockedAxios.get.mockImplementation((url) => { - if (url.includes("/modes")) { - return Promise.resolve({ data: invalidModesYaml }) - } - if (url.includes("/mcps")) { - return Promise.resolve({ data: validMcpsYaml }) - } - return Promise.reject(new Error("Unknown URL")) - }) - - // Should throw validation error for invalid modes - await expect(loader.loadAllItems()).rejects.toThrow() - }) - }) - - describe("getItem", () => { - it("should find specific item by id and type", async () => { - const mockModesYaml = `items: - - id: "target-mode" - name: "Target Mode" - description: "The mode we want" - content: "test content"` - - const mockMcpsYaml = `items: - - id: "target-mcp" - name: "Target MCP" - description: "The MCP we want" - url: "https://github.com/test/test-mcp" - content: "test content"` - - mockedAxios.get.mockImplementation((url) => { - if (url.includes("/modes")) { - return Promise.resolve({ data: mockModesYaml }) - } - if (url.includes("/mcps")) { - return Promise.resolve({ data: mockMcpsYaml }) - } - return Promise.reject(new Error("Unknown URL")) - }) - - const modeItem = await loader.getItem("target-mode", "mode" as MarketplaceItemType) - const mcpItem = await loader.getItem("target-mcp", "mcp" as MarketplaceItemType) - const notFound = await loader.getItem("nonexistent", "mode" as MarketplaceItemType) - - expect(modeItem).toEqual({ - type: "mode", - id: "target-mode", - name: "Target Mode", - description: "The mode we want", - content: "test content", - }) - - expect(mcpItem).toEqual({ - type: "mcp", - id: "target-mcp", - name: "Target MCP", - description: "The MCP we want", - url: "https://github.com/test/test-mcp", - content: "test content", - }) - - expect(notFound).toBeNull() - }) - }) - - describe("clearCache", () => { - it("should clear cache and force fresh API calls", async () => { - const mockModesYaml = `items: - - id: "test-mode" - name: "Test Mode" - description: "A test mode" - content: "test content"` - - const mockMcpsYaml = `items: []` - - mockedAxios.get.mockImplementation((url) => { - if (url.includes("/modes")) { - return Promise.resolve({ data: mockModesYaml }) - } - if (url.includes("/mcps")) { - return Promise.resolve({ data: mockMcpsYaml }) - } - return Promise.reject(new Error("Unknown URL")) - }) - - // First call - await loader.loadAllItems() - expect(mockedAxios.get).toHaveBeenCalledTimes(2) - - // Second call - should use cache - await loader.loadAllItems() - expect(mockedAxios.get).toHaveBeenCalledTimes(2) - - // Clear cache - loader.clearCache() - - // Third call - should hit API again - await loader.loadAllItems() - expect(mockedAxios.get).toHaveBeenCalledTimes(4) - }) - }) - - describe("cache expiration", () => { - it("should expire cache after 5 minutes", async () => { - const mockModesYaml = `items: - - id: "test-mode" - name: "Test Mode" - description: "A test mode" - content: "test content"` - - const mockMcpsYaml = `items: []` - - mockedAxios.get.mockImplementation((url) => { - if (url.includes("/modes")) { - return Promise.resolve({ data: mockModesYaml }) - } - if (url.includes("/mcps")) { - return Promise.resolve({ data: mockMcpsYaml }) - } - return Promise.reject(new Error("Unknown URL")) - }) - - // Mock Date.now to control time - const originalDateNow = Date.now - let currentTime = 1000000 - - Date.now = jest.fn(() => currentTime) - - // First call - await loader.loadAllItems() - expect(mockedAxios.get).toHaveBeenCalledTimes(2) - - // Second call immediately - should use cache - await loader.loadAllItems() - expect(mockedAxios.get).toHaveBeenCalledTimes(2) - - // Advance time by 6 minutes (360,000 ms) - currentTime += 6 * 60 * 1000 - - // Third call - cache should be expired - await loader.loadAllItems() - expect(mockedAxios.get).toHaveBeenCalledTimes(4) - - // Restore original Date.now - Date.now = originalDateNow - }) - }) -})