Skip to content

Consolidate MockChatService#300472

Merged
mjbvz merged 1 commit intomicrosoft:mainfrom
mjbvz:dev/mjbvz/stable-lark
Mar 10, 2026
Merged

Consolidate MockChatService#300472
mjbvz merged 1 commit intomicrosoft:mainfrom
mjbvz:dev/mjbvz/stable-lark

Conversation

@mjbvz
Copy link
Copy Markdown
Collaborator

@mjbvz mjbvz commented Mar 10, 2026

We have two MockChatService classes. This makes updating the interface slow since you have to update both locations. The two also behave slightly differently. After a quick code review I think they can be consolidated

We have two `MockChatService` classes. This makes updating the interface slow since you have to update both locations. The two also behave slightly differently
@mjbvz mjbvz self-assigned this Mar 10, 2026
Copilot AI review requested due to automatic review settings March 10, 2026 16:03
@mjbvz mjbvz enabled auto-merge March 10, 2026 16:04
@vs-code-engineering vs-code-engineering bot added this to the 1.112.0 milestone Mar 10, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR consolidates duplicated MockChatService implementations used in chat-related tests, aiming to reduce maintenance burden and keep test behavior consistent with the IChatService interface.

Changes:

  • Updated the shared MockChatService implementation to cover the behavior previously embedded in a browser test.
  • Removed the inline MockChatService from localAgentSessionsController.test.ts and switched the test to import the shared mock.
  • Adjusted test calls to match the shared mock’s API (e.g., addSession(mockModel)).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/vs/workbench/contrib/chat/test/common/chatService/mockChatService.ts Refactors the shared mock service implementation to support more test scenarios and centralize behavior.
src/vs/workbench/contrib/chat/test/browser/agentSessions/localAgentSessionsController.test.ts Removes the local mock and consumes the shared mock service instead.
Comments suppressed due to low confidence (3)

src/vs/workbench/contrib/chat/test/common/chatService/mockChatService.ts:136

  • IChatService.addCompleteRequest is defined with five parameters (sessionResource/message/variableData/attempt/response). The mock currently declares addCompleteRequest() with no parameters, which removes compile-time pressure to keep the mock aligned with the interface. Please update the method signature to match IChatService.addCompleteRequest (unused params can be _-prefixed).
	addCompleteRequest(): void { }

src/vs/workbench/contrib/chat/test/common/chatService/mockChatService.ts:152

  • notifyQuestionCarouselAnswer should use the service’s IChatQuestionAnswers | undefined type for its answers parameter (per IChatService) rather than Record<string, unknown>. Using the real type keeps this mock in sync with interface changes and prevents accidentally accepting/producing shapes that production code can’t handle.
	notifyQuestionCarouselAnswer(_requestId: string, _resolveId: string, _answers: Record<string, unknown> | undefined): void { }

src/vs/workbench/contrib/chat/test/common/chatService/mockChatService.ts:156

  • IChatService.transferChatSession and IChatService.setChatSessionTitle both take (sessionResource: URI, ...) parameters. In this mock they are currently declared with no parameters, which again weakens compile-time alignment with the interface and can hide breaking signature changes. Please update both method signatures to match IChatService (parameters can be unused).
	async transferChatSession(): Promise<void> { }

	setChatSessionTitle(): void { }

@mjbvz mjbvz merged commit 3f2a0f6 into microsoft:main Mar 10, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants