Skip to content

Commit 852ba38

Browse files
eleanorjboydeleanorjboyd
committed
simplify
1 parent 7901869 commit 852ba38

File tree

2 files changed

+12
-225
lines changed

2 files changed

+12
-225
lines changed

‎src/client/pythonEnvironments/base/locators/common/nativePythonFinder.ts‎

Lines changed: 2 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -135,18 +135,10 @@ class NativePythonFinderImpl extends DisposableBase implements NativePythonFinde
135135
}
136136

137137
public async resolve(executable: string): Promise<NativeEnvInfo> {
138-
this.outputChannel.info(
139-
`[test-failure-log] resolve() called: executable=${executable}, connectionClosed=${this._connectionClosed}, isDisposed=${this.isDisposed}`,
140-
);
141138
if (this._connectionClosed || this.isDisposed) {
142-
const error = new Error(
143-
`[test-failure-log] Cannot resolve: connection is ${this._connectionClosed ? 'closed' : 'disposed'}`,
144-
);
145-
this.outputChannel.error(error.message);
146-
throw error;
139+
throw new Error(`Cannot resolve: connection is ${this._connectionClosed ? 'closed' : 'disposed'}`);
147140
}
148141
await this.configure();
149-
this.outputChannel.info(`[test-failure-log] resolve() sending request for ${executable}`);
150142
const environment = await this.connection.sendRequest<NativeEnvInfo>('resolve', {
151143
executable,
152144
});
@@ -156,29 +148,17 @@ class NativePythonFinderImpl extends DisposableBase implements NativePythonFinde
156148
}
157149

158150
async *refresh(options?: NativePythonEnvironmentKind | Uri[]): AsyncIterable<NativeEnvInfo> {
159-
this.outputChannel.info(
160-
`[test-failure-log] refresh() called: firstRefreshResults=${!!this.firstRefreshResults}, connectionClosed=${
161-
this._connectionClosed
162-
}, isDisposed=${this.isDisposed}`,
163-
);
164151
if (this._connectionClosed || this.isDisposed) {
165-
this.outputChannel.error(
166-
`[test-failure-log] refresh() called but connection is ${
167-
this._connectionClosed ? 'closed' : 'disposed'
168-
}`,
169-
);
170152
return;
171153
}
172154
if (this.firstRefreshResults) {
173155
// If this is the first time we are refreshing,
174156
// Then get the results from the first refresh.
175157
// Those would have started earlier and cached in memory.
176-
this.outputChannel.info('[test-failure-log] Using firstRefreshResults');
177158
const results = this.firstRefreshResults();
178159
this.firstRefreshResults = undefined;
179160
yield* results;
180161
} else {
181-
this.outputChannel.info('[test-failure-log] Calling doRefresh');
182162
const result = this.doRefresh(options);
183163
let completed = false;
184164
void result.completed.finally(() => {
@@ -334,7 +314,6 @@ class NativePythonFinderImpl extends DisposableBase implements NativePythonFinde
334314
sendNativeTelemetry(data, this.initialRefreshMetrics),
335315
),
336316
connection.onClose(() => {
337-
this.outputChannel.info('[test-failure-log] JSON-RPC connection closed, marking connection as closed');
338317
this._connectionClosed = true;
339318
disposables.forEach((d) => d.dispose());
340319
}),
@@ -348,15 +327,7 @@ class NativePythonFinderImpl extends DisposableBase implements NativePythonFinde
348327
private doRefresh(
349328
options?: NativePythonEnvironmentKind | Uri[],
350329
): { completed: Promise<void>; discovered: Event<NativeEnvInfo | NativeEnvManagerInfo> } {
351-
this.outputChannel.info(
352-
`[test-failure-log] doRefresh() called: connectionClosed=${this._connectionClosed}, isDisposed=${this.isDisposed}`,
353-
);
354330
if (this._connectionClosed || this.isDisposed) {
355-
this.outputChannel.error(
356-
`[test-failure-log] doRefresh() called but connection is ${
357-
this._connectionClosed ? 'closed' : 'disposed'
358-
}`,
359-
);
360331
const emptyEmitter = new EventEmitter<NativeEnvInfo | NativeEnvManagerInfo>();
361332
return {
362333
completed: Promise.resolve(),
@@ -553,17 +524,7 @@ function getPythonSettingAndUntildify<T>(name: string, scope?: Uri): T | undefin
553524
return value;
554525
}
555526

556-
type NativePythonFinderFactory = (cacheDirectory?: Uri, context?: IExtensionContext) => NativePythonFinder;
557-
558527
let _finder: NativePythonFinder | undefined;
559-
let _finderFactory: NativePythonFinderFactory | undefined;
560-
561-
// For tests to inject a stable finder implementation.
562-
export function setNativePythonFinderFactory(factory?: NativePythonFinderFactory): void {
563-
_finderFactory = factory;
564-
clearNativePythonFinder();
565-
}
566-
567528
export function getNativePythonFinder(context?: IExtensionContext): NativePythonFinder {
568529
if (!isTrusted()) {
569530
return {
@@ -584,32 +545,16 @@ export function getNativePythonFinder(context?: IExtensionContext): NativePython
584545
},
585546
};
586547
}
587-
if (_finder && isFinderDisposed(_finder)) {
588-
_finder = undefined;
589-
}
590548
if (!_finder) {
591549
const cacheDirectory = context ? getCacheDirectory(context) : undefined;
592-
const factory = _finderFactory ?? ((cacheDir, ctx) => new NativePythonFinderImpl(cacheDir, ctx));
593-
_finder = factory(cacheDirectory, context);
550+
_finder = new NativePythonFinderImpl(cacheDirectory, context);
594551
if (context) {
595552
context.subscriptions.push(_finder);
596553
}
597554
}
598555
return _finder;
599556
}
600557

601-
function isFinderDisposed(finder: NativePythonFinder): boolean {
602-
const finderImpl = finder as { isDisposed?: boolean; isConnectionClosed?: boolean };
603-
const disposed = Boolean(finderImpl.isDisposed);
604-
const connectionClosed = Boolean(finderImpl.isConnectionClosed);
605-
if (disposed || connectionClosed) {
606-
traceError(
607-
`[test-failure-log] [NativePythonFinder] Finder needs recreation: isDisposed=${disposed}, isConnectionClosed=${connectionClosed}`,
608-
);
609-
}
610-
return disposed || connectionClosed;
611-
}
612-
613558
export function getCacheDirectory(context: IExtensionContext): Uri {
614559
return Uri.joinPath(context.globalStorageUri, 'pythonLocator');
615560
}
@@ -618,14 +563,3 @@ export async function clearCacheDirectory(context: IExtensionContext): Promise<v
618563
const cacheDirectory = getCacheDirectory(context);
619564
await fs.emptyDir(cacheDirectory.fsPath).catch(noop);
620565
}
621-
622-
/**
623-
* Clears the singleton finder instance. For testing purposes only.
624-
* @internal
625-
*/
626-
export function clearNativePythonFinder(): void {
627-
if (_finder) {
628-
_finder.dispose();
629-
_finder = undefined;
630-
}
631-
}

‎src/test/pythonEnvironments/nativePythonFinder.unit.test.ts‎

Lines changed: 10 additions & 157 deletions
Original file line numberDiff line numberDiff line change
@@ -6,79 +6,25 @@ import * as sinon from 'sinon';
66
import * as typemoq from 'typemoq';
77
import { WorkspaceConfiguration } from 'vscode';
88
import {
9-
clearNativePythonFinder,
109
getNativePythonFinder,
1110
isNativeEnvInfo,
12-
NativeCondaInfo,
1311
NativeEnvInfo,
1412
NativePythonFinder,
15-
setNativePythonFinderFactory,
1613
} from '../../client/pythonEnvironments/base/locators/common/nativePythonFinder';
1714
import * as windowsApis from '../../client/common/vscodeApis/windowApis';
1815
import { MockOutputChannel } from '../mockClasses';
1916
import * as workspaceApis from '../../client/common/vscodeApis/workspaceApis';
2017

21-
class FakeNativePythonFinder implements NativePythonFinder {
22-
private readonly versionsByPath = new Map<string, string>();
23-
24-
constructor(private readonly envs: NativeEnvInfo[]) {
25-
for (const env of envs) {
26-
const envPath = env.executable ?? env.prefix;
27-
if (envPath && env.version) {
28-
this.versionsByPath.set(envPath, env.version);
29-
}
30-
}
31-
}
32-
33-
async *refresh(): AsyncIterable<NativeEnvInfo> {
34-
for (const env of this.envs) {
35-
yield env;
36-
}
37-
}
38-
39-
async resolve(executable: string): Promise<NativeEnvInfo> {
40-
const env = this.envs.find((item) => item.executable === executable || item.prefix === executable);
41-
const version = this.versionsByPath.get(executable) ?? '3.11.9';
42-
return {
43-
...env,
44-
executable: env?.executable ?? executable,
45-
prefix: env?.prefix,
46-
version,
47-
};
48-
}
49-
50-
async getCondaInfo(): Promise<NativeCondaInfo> {
51-
return {
52-
canSpawnConda: false,
53-
condaRcs: [],
54-
envDirs: [],
55-
environmentsFromTxt: [],
56-
};
57-
}
58-
59-
dispose(): void {
60-
// no-op for fake finder
61-
}
62-
}
63-
6418
suite('Native Python Finder', () => {
6519
let finder: NativePythonFinder;
6620
let createLogOutputChannelStub: sinon.SinonStub;
6721
let getConfigurationStub: sinon.SinonStub;
6822
let configMock: typemoq.IMock<WorkspaceConfiguration>;
6923
let getWorkspaceFolderPathsStub: sinon.SinonStub;
70-
let outputChannel: MockOutputChannel;
71-
let useRealFinder: boolean;
7224

7325
setup(() => {
74-
// eslint-disable-next-line no-console
75-
console.log('[test-failure-log] setup() starting');
76-
// Clear singleton before each test to ensure fresh state
77-
clearNativePythonFinder();
78-
7926
createLogOutputChannelStub = sinon.stub(windowsApis, 'createLogOutputChannel');
80-
outputChannel = new MockOutputChannel('locator');
81-
createLogOutputChannelStub.returns(outputChannel);
27+
createLogOutputChannelStub.returns(new MockOutputChannel('locator'));
8228

8329
getWorkspaceFolderPathsStub = sinon.stub(workspaceApis, 'getWorkspaceFolderPaths');
8430
getWorkspaceFolderPathsStub.returns([]);
@@ -91,114 +37,44 @@ suite('Native Python Finder', () => {
9137
configMock.setup((c) => c.get<string>('poetryPath')).returns(() => '');
9238
getConfigurationStub.returns(configMock.object);
9339

94-
useRealFinder = process.env.VSC_PYTHON_NATIVE_FINDER_INTEGRATION === '1';
95-
if (!useRealFinder) {
96-
const envs: NativeEnvInfo[] = [
97-
{
98-
displayName: 'Python 3.11',
99-
executable: '/usr/bin/python3',
100-
prefix: '/usr',
101-
version: '3.11.9',
102-
},
103-
];
104-
setNativePythonFinderFactory(() => new FakeNativePythonFinder(envs));
105-
} else {
106-
setNativePythonFinderFactory(undefined);
107-
}
108-
10940
finder = getNativePythonFinder();
110-
// eslint-disable-next-line no-console
111-
console.log('[test-failure-log] setup() completed, finder created');
11241
});
11342

11443
teardown(() => {
115-
// eslint-disable-next-line no-console
116-
console.log('[test-failure-log] teardown() starting');
117-
// Clean up finder before restoring stubs to avoid issues with mock references
118-
clearNativePythonFinder();
119-
setNativePythonFinderFactory(undefined);
12044
sinon.restore();
121-
// eslint-disable-next-line no-console
122-
console.log('[test-failure-log] teardown() completed');
12345
});
12446

12547
suiteTeardown(() => {
126-
// Final cleanup (finder may already be disposed by teardown)
127-
clearNativePythonFinder();
128-
setNativePythonFinderFactory(undefined);
48+
finder.dispose();
12949
});
13050

13151
test('Refresh should return python environments', async () => {
132-
// eslint-disable-next-line no-console
133-
console.log('[test-failure-log] Starting: Refresh should return python environments');
134-
// eslint-disable-next-line no-console
135-
console.log(`[test-failure-log] useRealFinder=${useRealFinder}`);
136-
13752
const envs = [];
138-
// eslint-disable-next-line no-console
139-
console.log('[test-failure-log] About to call finder.refresh()');
14053
for await (const env of finder.refresh()) {
14154
envs.push(env);
14255
}
143-
// eslint-disable-next-line no-console
144-
console.log(`[test-failure-log] refresh() completed, found ${envs.length} environments`);
145-
146-
if (!envs.length) {
147-
// eslint-disable-next-line no-console
148-
console.error('[test-failure-log] Native finder produced no environments. Output channel:');
149-
// eslint-disable-next-line no-console
150-
console.error(outputChannel.output || '<empty>');
151-
// eslint-disable-next-line no-console
152-
console.error(`[test-failure-log] PATH=${process.env.PATH ?? ''}`);
153-
}
15456

15557
// typically all test envs should have at least one environment
15658
assert.isNotEmpty(envs);
15759
});
15860

15961
test('Resolve should return python environments with version', async () => {
160-
// eslint-disable-next-line no-console
161-
console.log('[test-failure-log] Starting: Resolve should return python environments with version');
162-
// eslint-disable-next-line no-console
163-
console.log(`[test-failure-log] useRealFinder=${useRealFinder}`);
164-
16562
const envs = [];
166-
// eslint-disable-next-line no-console
167-
console.log('[test-failure-log] About to call finder.refresh()');
16863
for await (const env of finder.refresh()) {
16964
envs.push(env);
17065
}
171-
// eslint-disable-next-line no-console
172-
console.log(`[test-failure-log] refresh() completed, found ${envs.length} environments`);
173-
174-
if (!envs.length) {
175-
// eslint-disable-next-line no-console
176-
console.error('[test-failure-log] Native finder produced no environments. Output channel:');
177-
// eslint-disable-next-line no-console
178-
console.error(outputChannel.output || '<empty>');
179-
// eslint-disable-next-line no-console
180-
console.error(`[test-failure-log] PATH=${process.env.PATH ?? ''}`);
181-
}
18266

18367
// typically all test envs should have at least one environment
18468
assert.isNotEmpty(envs);
18569

186-
// Check if finder is still usable (connection not closed)
187-
const finderImpl = finder as { isConnectionClosed?: boolean; isDisposed?: boolean };
188-
// eslint-disable-next-line no-console
189-
console.log(
190-
`[test-failure-log] Finder state: isConnectionClosed=${finderImpl.isConnectionClosed}, isDisposed=${finderImpl.isDisposed}`,
191-
);
192-
if (finderImpl.isConnectionClosed || finderImpl.isDisposed) {
193-
// eslint-disable-next-line no-console
194-
console.error('[test-failure-log] Finder connection closed prematurely, skipping resolve test');
195-
// eslint-disable-next-line no-console
196-
console.error(`[test-failure-log] Output channel: ${outputChannel.output || '<empty>'}`);
197-
// Skip the test gracefully if the connection is closed - this is the flaky condition
70+
// Check if finder connection is still open (can close due to race condition)
71+
const finderImpl = finder as { isConnectionClosed?: boolean };
72+
if (finderImpl.isConnectionClosed) {
73+
// Skip gracefully - this is a known race condition in CI
19874
return;
19975
}
20076

201-
// pick and env without version
77+
// pick an env with version
20278
const env: NativeEnvInfo | undefined = envs
20379
.filter((e) => isNativeEnvInfo(e))
20480
.find((e) => e.version && e.version.length > 0 && (e.executable || (e as NativeEnvInfo).prefix));
@@ -210,33 +86,10 @@ suite('Native Python Finder', () => {
21086
}
21187

21288
const envPath = env.executable ?? env.prefix;
213-
// eslint-disable-next-line no-console
214-
console.log(`[test-failure-log] About to call finder.resolve() for: ${envPath}`);
21589
if (envPath) {
216-
try {
217-
const resolved = await finder.resolve(envPath);
218-
// eslint-disable-next-line no-console
219-
console.log(`[test-failure-log] resolve() completed successfully: version=${resolved.version}`);
220-
assert.isString(resolved.version, 'Version must be a string');
221-
assert.isTrue((resolved?.version?.length ?? 0) > 0, 'Version must not be empty');
222-
} catch (error) {
223-
// eslint-disable-next-line no-console
224-
console.error(`[test-failure-log] resolve() failed with error: ${error}`);
225-
// eslint-disable-next-line no-console
226-
console.error(`[test-failure-log] Output channel: ${outputChannel.output || '<empty>'}`);
227-
228-
// Check if this is the known flaky connection disposed error
229-
const errorMessage = error instanceof Error ? error.message : String(error);
230-
if (errorMessage.includes('Connection is disposed') || errorMessage.includes('connection is closed')) {
231-
// eslint-disable-next-line no-console
232-
console.error('[test-failure-log] Known flaky error: connection disposed prematurely');
233-
// Re-throw a more informative error
234-
throw new Error(
235-
`[test-failure-log] Connection disposed during resolve - this is the known flaky issue. Original: ${errorMessage}`,
236-
);
237-
}
238-
throw error;
239-
}
90+
const resolved = await finder.resolve(envPath);
91+
assert.isString(resolved.version, 'Version must be a string');
92+
assert.isTrue((resolved?.version?.length ?? 0) > 0, 'Version must not be empty');
24093
} else {
24194
assert.fail('Expected either executable or prefix to be defined');
24295
}

0 commit comments

Comments
 (0)