Skip to content

Commit a06de4b

Browse files
eleanorjboydeleanorjboyd
authored
Fix smoke tests and enhance testing workflow with PET (#1226)
Introduce smoke tests for functional checks and improve the testing workflow by adding error handling for async initialization, removing retries to reduce expecting flakiness, and enhancing assertions for environment managers.
1 parent 6a01851 commit a06de4b

File tree

9 files changed

+313
-87
lines changed

9 files changed

+313
-87
lines changed

‎.github/instructions/testing-workflow.instructions.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -602,3 +602,7 @@ envConfig.inspect
602602
- Never create "documentation tests" that just `assert.ok(true)` — if mocking limitations prevent testing, either test a different layer that IS mockable, or skip the test entirely with a clear explanation (1)
603603
- When stubbing vscode APIs in tests via wrapper modules (e.g., `workspaceApis`), the production code must also use those wrappers — sinon cannot stub properties directly on the vscode namespace like `workspace.workspaceFolders`, so both production and test code must reference the same stubbable wrapper functions (4)
604604
- **Before writing tests**, check if the function under test calls VS Code APIs directly (e.g., `commands.executeCommand`, `window.createTreeView`, `workspace.getConfiguration`). If so, FIRST update the production code to use wrapper functions from `src/common/*.apis.ts` (create the wrapper if it doesn't exist), THEN write tests that stub those wrappers. This prevents CI failures where sinon cannot stub the vscode namespace (4)
605+
- **Async initialization in `setImmediate` must have error handling**: When extension activation uses `setImmediate(async () => {...})` for async manager registration, wrap it in try-catch with proper error logging. Unhandled errors cause silent failures where managers never register, making smoke/E2E tests hang forever (1)
606+
- **Never skip tests to hide infrastructure problems**: If tests require native binaries (like `pet`), the CI workflow must build/download them. Skipping tests when infrastructure is missing gives false confidence. Build from source (like vscode-python does) rather than skipping. Tests should fail clearly when something is wrong (2)
607+
- **No retries for masking flakiness**: Mocha `retries` should not be used to mask test flakiness. If a test is flaky, fix the root cause. Retries hide real issues and slow down CI (1)
608+
- **pet binary is required for environment manager registration**: The smoke/E2E/integration tests require the `pet` binary from `microsoft/python-environment-tools` to be built and placed in `python-env-tools/bin/`. Without it, `waitForApiReady()` will timeout because managers never register. CI must build pet from source using `cargo build --release --package pet` (2)

‎.github/workflows/pr-check.yml‎

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,49 @@ jobs:
8888
- name: Checkout
8989
uses: actions/checkout@v4
9090

91+
- name: Checkout Python Environment Tools
92+
uses: actions/checkout@v4
93+
with:
94+
repository: 'microsoft/python-environment-tools'
95+
path: 'python-env-tools-src'
96+
sparse-checkout: |
97+
crates
98+
Cargo.toml
99+
Cargo.lock
100+
sparse-checkout-cone-mode: false
101+
102+
- name: Install Rust Toolchain
103+
uses: dtolnay/rust-toolchain@stable
104+
105+
- name: Cache Rust build
106+
uses: actions/cache@v4
107+
with:
108+
path: |
109+
~/.cargo/registry
110+
~/.cargo/git
111+
python-env-tools-src/target
112+
key: ${{ runner.os }}-cargo-pet-${{ hashFiles('python-env-tools-src/Cargo.lock') }}
113+
restore-keys: |
114+
${{ runner.os }}-cargo-pet-
115+
116+
- name: Build Python Environment Tools
117+
run: cargo build --release --package pet
118+
working-directory: python-env-tools-src
119+
120+
- name: Copy pet binary (Unix)
121+
if: runner.os != 'Windows'
122+
run: |
123+
mkdir -p python-env-tools/bin
124+
cp python-env-tools-src/target/release/pet python-env-tools/bin/
125+
chmod +x python-env-tools/bin/pet
126+
127+
- name: Copy pet binary (Windows)
128+
if: runner.os == 'Windows'
129+
run: |
130+
mkdir -p python-env-tools/bin
131+
cp python-env-tools-src/target/release/pet.exe python-env-tools/bin/
132+
shell: bash
133+
91134
- name: Install Node
92135
uses: actions/setup-node@v4
93136
with:
@@ -138,6 +181,49 @@ jobs:
138181
- name: Checkout
139182
uses: actions/checkout@v4
140183

184+
- name: Checkout Python Environment Tools
185+
uses: actions/checkout@v4
186+
with:
187+
repository: 'microsoft/python-environment-tools'
188+
path: 'python-env-tools-src'
189+
sparse-checkout: |
190+
crates
191+
Cargo.toml
192+
Cargo.lock
193+
sparse-checkout-cone-mode: false
194+
195+
- name: Install Rust Toolchain
196+
uses: dtolnay/rust-toolchain@stable
197+
198+
- name: Cache Rust build
199+
uses: actions/cache@v4
200+
with:
201+
path: |
202+
~/.cargo/registry
203+
~/.cargo/git
204+
python-env-tools-src/target
205+
key: ${{ runner.os }}-cargo-pet-${{ hashFiles('python-env-tools-src/Cargo.lock') }}
206+
restore-keys: |
207+
${{ runner.os }}-cargo-pet-
208+
209+
- name: Build Python Environment Tools
210+
run: cargo build --release --package pet
211+
working-directory: python-env-tools-src
212+
213+
- name: Copy pet binary (Unix)
214+
if: runner.os != 'Windows'
215+
run: |
216+
mkdir -p python-env-tools/bin
217+
cp python-env-tools-src/target/release/pet python-env-tools/bin/
218+
chmod +x python-env-tools/bin/pet
219+
220+
- name: Copy pet binary (Windows)
221+
if: runner.os == 'Windows'
222+
run: |
223+
mkdir -p python-env-tools/bin
224+
cp python-env-tools-src/target/release/pet.exe python-env-tools/bin/
225+
shell: bash
226+
141227
- name: Install Node
142228
uses: actions/setup-node@v4
143229
with:
@@ -188,6 +274,49 @@ jobs:
188274
- name: Checkout
189275
uses: actions/checkout@v4
190276

277+
- name: Checkout Python Environment Tools
278+
uses: actions/checkout@v4
279+
with:
280+
repository: 'microsoft/python-environment-tools'
281+
path: 'python-env-tools-src'
282+
sparse-checkout: |
283+
crates
284+
Cargo.toml
285+
Cargo.lock
286+
sparse-checkout-cone-mode: false
287+
288+
- name: Install Rust Toolchain
289+
uses: dtolnay/rust-toolchain@stable
290+
291+
- name: Cache Rust build
292+
uses: actions/cache@v4
293+
with:
294+
path: |
295+
~/.cargo/registry
296+
~/.cargo/git
297+
python-env-tools-src/target
298+
key: ${{ runner.os }}-cargo-pet-${{ hashFiles('python-env-tools-src/Cargo.lock') }}
299+
restore-keys: |
300+
${{ runner.os }}-cargo-pet-
301+
302+
- name: Build Python Environment Tools
303+
run: cargo build --release --package pet
304+
working-directory: python-env-tools-src
305+
306+
- name: Copy pet binary (Unix)
307+
if: runner.os != 'Windows'
308+
run: |
309+
mkdir -p python-env-tools/bin
310+
cp python-env-tools-src/target/release/pet python-env-tools/bin/
311+
chmod +x python-env-tools/bin/pet
312+
313+
- name: Copy pet binary (Windows)
314+
if: runner.os == 'Windows'
315+
run: |
316+
mkdir -p python-env-tools/bin
317+
cp python-env-tools-src/target/release/pet.exe python-env-tools/bin/
318+
shell: bash
319+
191320
- name: Install Node
192321
uses: actions/setup-node@v4
193322
with:

‎.vscode-test.mjs‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ export default defineConfig([
1111
mocha: {
1212
ui: 'tdd',
1313
timeout: 120000,
14-
retries: 1,
1514
},
1615
env: {
1716
VSC_PYTHON_SMOKE_TEST: '1',
@@ -32,7 +31,6 @@ export default defineConfig([
3231
mocha: {
3332
ui: 'tdd',
3433
timeout: 180000,
35-
retries: 1,
3634
},
3735
env: {
3836
VSC_PYTHON_E2E_TEST: '1',
@@ -51,7 +49,6 @@ export default defineConfig([
5149
mocha: {
5250
ui: 'tdd',
5351
timeout: 60000,
54-
retries: 1,
5552
},
5653
env: {
5754
VSC_PYTHON_INTEGRATION_TEST: '1',

‎src/test/common/testUtils.unit.test.ts‎

Lines changed: 174 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,174 @@
1+
// Copyright (c) Microsoft Corporation. All rights reserved.
2+
// Licensed under the MIT License.
3+
4+
import * as assert from 'assert';
5+
import { retryUntilSuccess, sleep, waitForApiReady, waitForCondition } from '../testUtils';
6+
7+
suite('Test Utilities', () => {
8+
suite('sleep', () => {
9+
test('should resolve after specified time', async () => {
10+
const start = Date.now();
11+
await sleep(50);
12+
const elapsed = Date.now() - start;
13+
assert.ok(elapsed >= 45, `Expected at least 45ms, got ${elapsed}ms`);
14+
});
15+
});
16+
17+
suite('waitForCondition', () => {
18+
test('should resolve immediately when condition is true', async () => {
19+
await waitForCondition(() => true, 100, 'Should not fail');
20+
});
21+
22+
test('should resolve when condition becomes true', async () => {
23+
let counter = 0;
24+
await waitForCondition(
25+
() => {
26+
counter++;
27+
return counter >= 3;
28+
},
29+
1000,
30+
'Condition did not become true',
31+
10,
32+
);
33+
assert.ok(counter >= 3, 'Condition should have been checked multiple times');
34+
});
35+
36+
test('should reject when timeout is reached', async () => {
37+
await assert.rejects(
38+
() => waitForCondition(() => false, 100, 'Custom error message', 10),
39+
/Custom error message \(waited 100ms\)/,
40+
);
41+
});
42+
43+
test('should handle async conditions', async () => {
44+
let counter = 0;
45+
await waitForCondition(
46+
async () => {
47+
counter++;
48+
await sleep(5);
49+
return counter >= 2;
50+
},
51+
1000,
52+
'Async condition failed',
53+
10,
54+
);
55+
assert.ok(counter >= 2);
56+
});
57+
58+
test('should continue polling when condition throws', async () => {
59+
let counter = 0;
60+
await waitForCondition(
61+
() => {
62+
counter++;
63+
if (counter < 3) {
64+
throw new Error('Not ready yet');
65+
}
66+
return true;
67+
},
68+
1000,
69+
'Should eventually succeed',
70+
10,
71+
);
72+
assert.ok(counter >= 3);
73+
});
74+
});
75+
76+
suite('retryUntilSuccess', () => {
77+
test('should return result when function succeeds immediately', async () => {
78+
const result = await retryUntilSuccess(
79+
() => 42,
80+
() => true,
81+
1000,
82+
'Should not fail',
83+
);
84+
assert.strictEqual(result, 42);
85+
});
86+
87+
test('should return result when validation passes', async () => {
88+
let counter = 0;
89+
const result = await retryUntilSuccess(
90+
() => {
91+
counter++;
92+
return counter;
93+
},
94+
(val) => val >= 3,
95+
1000,
96+
'Validation failed',
97+
);
98+
assert.ok(result >= 3);
99+
});
100+
101+
test('should reject when timeout reached', async () => {
102+
await assert.rejects(
103+
() =>
104+
retryUntilSuccess(
105+
() => 1,
106+
(val) => val > 10,
107+
100,
108+
'Custom timeout error',
109+
),
110+
/Custom timeout error: validation failed/,
111+
);
112+
});
113+
114+
test('should include last error message in rejection', async () => {
115+
await assert.rejects(
116+
() =>
117+
retryUntilSuccess(
118+
() => {
119+
throw new Error('Specific failure');
120+
},
121+
() => true,
122+
100,
123+
'Operation failed',
124+
),
125+
/Operation failed: Specific failure/,
126+
);
127+
});
128+
129+
test('should handle async functions', async () => {
130+
let counter = 0;
131+
const result = await retryUntilSuccess(
132+
async () => {
133+
counter++;
134+
await sleep(5);
135+
return counter;
136+
},
137+
(val) => val >= 2,
138+
1000,
139+
'Async retry failed',
140+
);
141+
assert.ok(result >= 2);
142+
});
143+
});
144+
145+
suite('waitForApiReady', () => {
146+
test('should return ready:true when getEnvironments succeeds', async () => {
147+
const mockApi = {
148+
getEnvironments: async () => [],
149+
};
150+
const result = await waitForApiReady(mockApi, 1000);
151+
assert.deepStrictEqual(result, { ready: true });
152+
});
153+
154+
test('should return ready:false with error when timeout reached', async () => {
155+
const mockApi = {
156+
getEnvironments: (): Promise<unknown[]> => new Promise(() => {}), // Never resolves
157+
};
158+
const result = await waitForApiReady(mockApi, 100);
159+
assert.strictEqual(result.ready, false);
160+
assert.ok(result.error?.includes('API not ready within 100ms'));
161+
});
162+
163+
test('should return ready:false when getEnvironments throws', async () => {
164+
const mockApi = {
165+
getEnvironments: async () => {
166+
throw new Error('Manager not registered');
167+
},
168+
};
169+
const result = await waitForApiReady(mockApi, 1000);
170+
assert.strictEqual(result.ready, false);
171+
assert.ok(result.error?.includes('Manager not registered'));
172+
});
173+
});
174+
});

‎src/test/e2e/index.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ export async function run(): Promise<void> {
2020
ui: 'tdd',
2121
color: true,
2222
timeout: 180_000, // 3 minutes - E2E workflows can be slow
23-
retries: 1, // Retry once on failure
2423
slow: 30_000, // Mark tests as slow if they take > 30s
2524
});
2625

‎src/test/integration/index.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ export async function run(): Promise<void> {
1717
ui: 'tdd',
1818
color: true,
1919
timeout: 120_000, // 2 minutes
20-
retries: 1,
2120
slow: 15_000, // Mark as slow if > 15s
2221
});
2322

0 commit comments

Comments
 (0)