test: feedback

Author: edelauna

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

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)

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()
 		})
 	})