From 05b399b63e1272c6a9a71258fabb095db54a98b9 Mon Sep 17 00:00:00 2001 From: Elliott de Launay Date: Fri, 24 Apr 2026 10:13:18 -0400 Subject: [PATCH] test: feedback --- apps/vscode-e2e/src/suite/mcp-oauth.test.ts | 20 ++++++---- .../__tests__/McpOAuthClientProvider.spec.ts | 7 +++- .../__tests__/SecretStorageService.spec.ts | 40 +++++++++---------- 3 files changed, 38 insertions(+), 29 deletions(-) diff --git a/apps/vscode-e2e/src/suite/mcp-oauth.test.ts b/apps/vscode-e2e/src/suite/mcp-oauth.test.ts index 0a1219ae88..40792f03d5 100644 --- a/apps/vscode-e2e/src/suite/mcp-oauth.test.ts +++ b/apps/vscode-e2e/src/suite/mcp-oauth.test.ts @@ -238,11 +238,15 @@ suite("Roo Code MCP OAuth", function () { } } + // Only remove .roo/mcp.json if it's inside the ephemeral tempDir — never + // touch a real workspace's config. const workspaceDir = vscode.workspace.workspaceFolders?.[0]?.uri.fsPath || tempDir - try { - await fs.rm(path.join(workspaceDir, ".roo"), { recursive: true, force: true }) - } catch { - // ignore + if (workspaceDir === tempDir || workspaceDir.startsWith(tempDir + path.sep)) { + try { + await fs.unlink(path.join(workspaceDir, ".roo", "mcp.json")) + } catch { + // ignore + } } await fs.rm(tempDir, { recursive: true, force: true }) @@ -325,11 +329,11 @@ suite("Roo Code MCP OAuth", function () { }) test("Should reuse stored token on reconnect without re-running the full OAuth flow", async function () { - // This test runs after the previous one, so a token is already stored in SecretStorage. - // Trigger another reconnect — the SDK should inject the cached token directly and skip the - // browser-based auth flow (no new register or token endpoints should be hit). + // Ensure a token is in SecretStorage before testing reuse — this makes the + // test self-contained regardless of execution order. + await waitFor(() => endpointsHit.has("token"), { timeout: 30_000 }) - // Clear only mcp-related hit tracking (token endpoint should NOT be re-hit) + // Clear hit tracking so we can assert the token endpoint is NOT re-hit. endpointsHit.clear() const workspaceDir = vscode.workspace.workspaceFolders?.[0]?.uri.fsPath || tempDir diff --git a/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts b/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts index 3232eb10e2..19ce121ebe 100644 --- a/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts +++ b/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, vi, beforeEach } from "vitest" +import { describe, it, expect, vi, beforeEach, afterAll } from "vitest" // Mock vscode vi.mock("vscode", () => ({ @@ -21,6 +21,7 @@ vi.mock("../utils/callbackServer", () => ({ // Mock fetch for auth discovery so tests don't make real network calls const mockFetch = vi.fn() +const originalFetch = global.fetch global.fetch = mockFetch // Mock SDK auth discovery functions @@ -86,6 +87,10 @@ describe("McpOAuthClientProvider", () => { McpOAuthClientProvider.clearNonOAuthCache() }) + afterAll(() => { + global.fetch = originalFetch + }) + describe("static negative cache", () => { it("isKnownNonOAuth returns false for unknown servers", () => { expect(McpOAuthClientProvider.isKnownNonOAuth("https://unknown.com/mcp")).toBe(false) diff --git a/src/services/mcp/__tests__/SecretStorageService.spec.ts b/src/services/mcp/__tests__/SecretStorageService.spec.ts index a0d4adfebb..9a6833c4ab 100644 --- a/src/services/mcp/__tests__/SecretStorageService.spec.ts +++ b/src/services/mcp/__tests__/SecretStorageService.spec.ts @@ -131,35 +131,35 @@ describe("SecretStorageService", () => { const result = await service.getOAuthData("https://example.com/mcp") expect(result).toBeUndefined() }) + }) - describe("onDidChange", () => { - it("should call the callback when the key for the given URL changes", () => { - const cb = vi.fn() - service.onDidChange("https://example.com/mcp", cb) + describe("onDidChange", () => { + it("should call the callback when the key for the given URL changes", () => { + const cb = vi.fn() + service.onDidChange("https://example.com/mcp", cb) - context.secrets._emit("mcp.oauth.example.com.L21jcA.data") + context.secrets._emit("mcp.oauth.example.com.L21jcA.data") - expect(cb).toHaveBeenCalledTimes(1) - }) + expect(cb).toHaveBeenCalledTimes(1) + }) - it("should not call the callback for a different URL's key", () => { - const cb = vi.fn() - service.onDidChange("https://example.com/mcp", cb) + it("should not call the callback for a different URL's key", () => { + const cb = vi.fn() + service.onDidChange("https://example.com/mcp", cb) - context.secrets._emit("mcp.oauth.other.com.L21jcA.data") + context.secrets._emit("mcp.oauth.other.com.L21jcA.data") - expect(cb).not.toHaveBeenCalled() - }) + expect(cb).not.toHaveBeenCalled() + }) - it("should stop calling the callback after the returned dispose function is called", () => { - const cb = vi.fn() - const unsubscribe = service.onDidChange("https://example.com/mcp", cb) + it("should stop calling the callback after the returned dispose function is called", () => { + const cb = vi.fn() + const unsubscribe = service.onDidChange("https://example.com/mcp", cb) - unsubscribe() - context.secrets._emit("mcp.oauth.example.com.L21jcA.data") + unsubscribe() + context.secrets._emit("mcp.oauth.example.com.L21jcA.data") - expect(cb).not.toHaveBeenCalled() - }) + expect(cb).not.toHaveBeenCalled() }) })