fix: await setImmediate in setEnvironment to eliminate race conditions#1575
Merged
Conversation
The `setEnvironment persists selection` and `Project selection is independent of global` tests had a race condition where `getEnvironment` was called immediately after `setEnvironment` before the async config write had propagated. On slower CI runners, `getEnvironment` would fall back to auto-discovery and return `/usr/bin/python` instead of the newly-set environment. Fix: add `waitForCondition` (already imported) to poll until `getEnvironment` reflects the expected value before asserting, with a 15s timeout matching other tests in the file.
Copilot
AI
changed the title
[WIP] Fix flaky integration test due to race condition
Fix race condition in interpreter selection integration tests
Jun 10, 2026
The initial setEnvironment(undefined, oldEnv) fires an async event that could be captured by the TestEventHandler registered immediately after, causing the handler to see the wrong event. Add waitForCondition to ensure the initial environment is fully set before registering the handler.
Copilot
AI
changed the title
Fix race condition in interpreter selection integration tests
Fix race conditions in integration tests for interpreter selection
Jun 10, 2026
…uplicate getEnvironment calls
Copilot
AI
changed the title
Fix race conditions in integration tests for interpreter selection
Eliminate duplicate getEnvironment calls in integration tests
Jun 10, 2026
Copilot
AI
changed the title
Eliminate duplicate getEnvironment calls in integration tests
Fix flaky integration test and clean up test patterns
Jun 10, 2026
Fix two flaky tests in interpreterSelection.integration.test.ts that fail consistently on CI (all Python versions: 3.10, 3.12, 3.14): 1. 'Change event includes old and new values': setEnvironment fires events via setImmediate(), so the handler created immediately after the first setEnvironment call caught the stale event instead of the intended one. Added settle time and now searches for the specific matching event instead of using handler.last. 2. 'Setting same environment is idempotent': The underlying manager's onDidChangeEnvironment can trigger refreshEnvironment(), firing events even on same-value sets. Changed assertion to verify that if events fire, they don't indicate an actual change (old === new), rather than asserting no events fire at all. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The idempotent test was asserting that events fired on a second setEnvironment call have matching old/new envIds. However, refreshEnvironment() (triggered by manager-level callbacks) can replace the _activeSelection cache between calls, making the second set non-idempotent from the cache perspective even though the same env object is passed. Instead, test functional idempotency: verify getEnvironment returns the correct environment after setting the same one twice. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
edvilme
approved these changes
Jun 11, 2026
eleanorjboyd
requested changes
Jun 11, 2026
eleanorjboyd
left a comment
eleanorjboyd
left a comment
Member
There was a problem hiding this comment.
any way we could keep this event driven so we don't arbitrarily increase the time of our CI runs?
Replace waitForCondition polling loops with event-driven drain handlers to avoid increasing CI run times. Instead of polling getEnvironment() until it returns the expected value, subscribe a temporary TestEventHandler before setEnvironment, wait for its event, then dispose it. This resolves in ~1 setImmediate tick rather than multiple 100ms poll cycles. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Consistently use TestEventHandler drain pattern for all tests that call setEnvironment, not just the two that were failing. This eliminates waitForCondition polling (100ms intervals) in favor of event-driven resolution (~1 setImmediate tick). Remaining waitForCondition uses are appropriate: - extension.isActive: no event API available, must poll - handler.all.some(): already event-driven (searching collected events) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
setEnvironment only fires onDidChangeActiveEnvironment when the new env differs from the _activeSelection cache. When teardown restores the original env, the next test's setEnvironment is idempotent — no event fires — and drain handlers wait forever. Fix: persistence tests (setEnvironment persists, project independent, idempotent) don't need event waits at all — getEnvironment calls manager.get() directly, returning correct values immediately. The event test uses a setImmediate-based drain that tolerates silence when the first set is idempotent — just enough to let any pending setImmediate callbacks fire without blocking on a 15s timeout. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Subscribe the handler BEFORE both setEnvironment calls instead of trying to drain async events between them. The handler captures all events from both sets and any refreshEnvironment side-effects across all setImmediate ticks. We then filter for the specific event matching newEnv.envId.id, making the test race-free without any timeouts or drain mechanisms. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…Wait Add setEnvironmentAndWait() helper that subscribes to onDidChangeEnvironment before calling setEnvironment, then drains the setImmediate event chain (up to 5 event loop ticks) to ensure all async side-effects settle before proceeding. All setEnvironment calls (including teardown) now use this helper, eliminating race conditions where stale events from a previous call leak into subsequent test steps. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Always drain all 5 setImmediate ticks instead of breaking early on first event — prevents secondary events from refreshEnvironment leaking into subsequent test steps. Replace waitForCondition polling in Change event test with the same setImmediate drain pattern for consistency and speed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…l ticks setEnvironmentAndWait now races an event promise against the drain fallback — resolves immediately when the event fires, falls back to tick draining for idempotent sets. Returns captured events so callers can inspect payloads directly. Simplifies the Change event test to use the returned events instead of creating a separate TestEventHandler. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Instead of draining setImmediate ticks, the helper now checks whether the environment will actually change by comparing current vs target envId. If a change is expected, it subscribes to onDidChangeEnvironment and awaits the event. If no change is expected (idempotent set), it calls setEnvironment and returns immediately — no ticks, no polling. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Extract onceEvent<T> utility that converts a VS Code Event<T> into a one-shot Promise<T> — subscribes once, resolves on first fire, auto- disposes. setEnvironmentAndWait now returns a single event (or undefined for idempotent sets) instead of an array. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The root cause of flaky integration tests was that setEnvironment fired change events via setImmediate but returned before they fired. This made it impossible for callers to know when events were done. Now setEnvironment awaits the setImmediate callback, guaranteeing that onDidChangeActiveEnvironment has fired when the promise resolves. This eliminates all race conditions and removes the need for test-side workarounds (onceEvent, setEnvironmentAndWait, drain loops). Tests are simplified to just 'await api.setEnvironment(...)' — no helpers, no predictions, no timeouts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
edvilme
changed the title
- Wrap .fire() in try/catch so awaited setImmediate rejects on listener errors instead of hanging indefinitely - Skip setImmediate/await when events array is empty (no unnecessary tick delay) - Update idempotent test JSDoc to match what the test actually verifies Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
eleanorjboyd
approved these changes
Jun 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Integration tests for interpreter selection were flaky on CI. The root cause was in the main code: \setEnvironment\ fired change events via \setImmediate()\ but returned before they fired. This meant \�wait api.setEnvironment()\ resolved before \onDidChangeActiveEnvironment\ had fired, creating race conditions between sequential test steps.
Fix
Main code (\src/features/envManagers.ts)
\setEnvironment\ and \setEnvironments\ now \�wait\ their \setImmediate\ callbacks, guaranteeing that \onDidChangeActiveEnvironment\ has fired when the promise resolves. The \setImmediate\ is preserved to avoid reentrant event handlers, but callers can now rely on \�wait\ to know events are done.
Tests (\interpreterSelection.integration.test.ts)
With the main code fix, all test-side workarounds (\onceEvent, \setEnvironmentAndWait, drain loops, polling) are removed. Tests simply use \�wait api.setEnvironment(...)\ — no helpers, no predictions, no timeouts.