Skip to content

Dispose ConversationFeature in sanity test teardown#316719

Draft
benvillalobos wants to merge 1 commit into
microsoft:mainfrom
benvillalobos:bv/sanity-dispose
Draft

Dispose ConversationFeature in sanity test teardown#316719
benvillalobos wants to merge 1 commit into
microsoft:mainfrom
benvillalobos:bv/sanity-dispose

Conversation

@benvillalobos
Copy link
Copy Markdown
Member

Replace conversationFeature.activated = false; with conversationFeature.dispose(); in each per-test finally block of the Copilot Chat Sanity Test suite.

The previous teardown only flipped the activated setter, which clears _activatedDisposables (commands, terminal quick-fix providers, participant registrations, etc.) but leaves _disposables intact. _disposables holds the two onDidAuthenticationChange listeners that each test instance subscribes to. Across retries those listeners accumulate on the shared IAuthenticationService, and on an auth-token oscillation they can re-trigger registration of the global copilot-chat.fixWithCopilot terminal quick-fix provider, surfacing as a flaky "already registered" failure.

dispose() is a strict superset of activated = false — it clears both stores. Each ConversationFeature instance's disposables are per-listener / per-registration (verified against IAuthenticationService.onDidAuthenticationChange and vscode.window.registerTerminalQuickFixProvider), so disposing the test-owned instance does not affect the contribution-registered ConversationFeature from the extension activation path.

No test reuses conversationFeature after its finally block.

Copilot AI review requested due to automatic review settings May 15, 2026 23:12
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 fixes a teardown leak in the Copilot Chat sanity test suite by disposing per-test ConversationFeature instances rather than only deactivating them, preventing accumulation of authentication-change listeners across retries.

Changes:

  • Replace conversationFeature.activated = false; with conversationFeature.dispose(); in per-test finally blocks.
  • Ensure both _activatedDisposables and lifetime _disposables (e.g., auth listeners) are cleaned up for each test instance.
Show a summary per file
File Description
extensions/copilot/src/extension/test/vscode-node/sanity.sanity-test.ts Updates test teardown to dispose ConversationFeature instances to avoid listener/provider re-registration flakiness.

Copilot's findings

  • Files reviewed: 1/1 changed files
  • Comments generated: 0

@benvillalobos
Copy link
Copy Markdown
Member Author

Closing as superseded by #316732. That PR drops the per-test ConversationFeature construction entirely, so the four finally blocks this PR was patching no longer exist on main. The leak diagnosis here (activated = false doesn't dispose _disposables / the auth listener) is valid but moot once #316732 lands.

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.

2 participants