diff --git a/.github/instructions/testing-workflow.instructions.md b/.github/instructions/testing-workflow.instructions.md index 8dd6067b..b374c38d 100644 --- a/.github/instructions/testing-workflow.instructions.md +++ b/.github/instructions/testing-workflow.instructions.md @@ -602,3 +602,7 @@ envConfig.inspect - 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) - 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) - **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) +- **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) +- **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) +- **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) +- **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) diff --git a/.github/workflows/pr-check.yml b/.github/workflows/pr-check.yml index 03efdd0d..3e2e446d 100644 --- a/.github/workflows/pr-check.yml +++ b/.github/workflows/pr-check.yml @@ -88,6 +88,49 @@ jobs: - name: Checkout uses: actions/checkout@v4 + - name: Checkout Python Environment Tools + uses: actions/checkout@v4 + with: + repository: 'microsoft/python-environment-tools' + path: 'python-env-tools-src' + sparse-checkout: | + crates + Cargo.toml + Cargo.lock + sparse-checkout-cone-mode: false + + - name: Install Rust Toolchain + uses: dtolnay/rust-toolchain@stable + + - name: Cache Rust build + uses: actions/cache@v4 + with: + path: | + ~/.cargo/registry + ~/.cargo/git + python-env-tools-src/target + key: ${{ runner.os }}-cargo-pet-${{ hashFiles('python-env-tools-src/Cargo.lock') }} + restore-keys: | + ${{ runner.os }}-cargo-pet- + + - name: Build Python Environment Tools + run: cargo build --release --package pet + working-directory: python-env-tools-src + + - name: Copy pet binary (Unix) + if: runner.os != 'Windows' + run: | + mkdir -p python-env-tools/bin + cp python-env-tools-src/target/release/pet python-env-tools/bin/ + chmod +x python-env-tools/bin/pet + + - name: Copy pet binary (Windows) + if: runner.os == 'Windows' + run: | + mkdir -p python-env-tools/bin + cp python-env-tools-src/target/release/pet.exe python-env-tools/bin/ + shell: bash + - name: Install Node uses: actions/setup-node@v4 with: @@ -138,6 +181,49 @@ jobs: - name: Checkout uses: actions/checkout@v4 + - name: Checkout Python Environment Tools + uses: actions/checkout@v4 + with: + repository: 'microsoft/python-environment-tools' + path: 'python-env-tools-src' + sparse-checkout: | + crates + Cargo.toml + Cargo.lock + sparse-checkout-cone-mode: false + + - name: Install Rust Toolchain + uses: dtolnay/rust-toolchain@stable + + - name: Cache Rust build + uses: actions/cache@v4 + with: + path: | + ~/.cargo/registry + ~/.cargo/git + python-env-tools-src/target + key: ${{ runner.os }}-cargo-pet-${{ hashFiles('python-env-tools-src/Cargo.lock') }} + restore-keys: | + ${{ runner.os }}-cargo-pet- + + - name: Build Python Environment Tools + run: cargo build --release --package pet + working-directory: python-env-tools-src + + - name: Copy pet binary (Unix) + if: runner.os != 'Windows' + run: | + mkdir -p python-env-tools/bin + cp python-env-tools-src/target/release/pet python-env-tools/bin/ + chmod +x python-env-tools/bin/pet + + - name: Copy pet binary (Windows) + if: runner.os == 'Windows' + run: | + mkdir -p python-env-tools/bin + cp python-env-tools-src/target/release/pet.exe python-env-tools/bin/ + shell: bash + - name: Install Node uses: actions/setup-node@v4 with: @@ -188,6 +274,49 @@ jobs: - name: Checkout uses: actions/checkout@v4 + - name: Checkout Python Environment Tools + uses: actions/checkout@v4 + with: + repository: 'microsoft/python-environment-tools' + path: 'python-env-tools-src' + sparse-checkout: | + crates + Cargo.toml + Cargo.lock + sparse-checkout-cone-mode: false + + - name: Install Rust Toolchain + uses: dtolnay/rust-toolchain@stable + + - name: Cache Rust build + uses: actions/cache@v4 + with: + path: | + ~/.cargo/registry + ~/.cargo/git + python-env-tools-src/target + key: ${{ runner.os }}-cargo-pet-${{ hashFiles('python-env-tools-src/Cargo.lock') }} + restore-keys: | + ${{ runner.os }}-cargo-pet- + + - name: Build Python Environment Tools + run: cargo build --release --package pet + working-directory: python-env-tools-src + + - name: Copy pet binary (Unix) + if: runner.os != 'Windows' + run: | + mkdir -p python-env-tools/bin + cp python-env-tools-src/target/release/pet python-env-tools/bin/ + chmod +x python-env-tools/bin/pet + + - name: Copy pet binary (Windows) + if: runner.os == 'Windows' + run: | + mkdir -p python-env-tools/bin + cp python-env-tools-src/target/release/pet.exe python-env-tools/bin/ + shell: bash + - name: Install Node uses: actions/setup-node@v4 with: diff --git a/.vscode-test.mjs b/.vscode-test.mjs index f6a5edf7..6bd41d01 100644 --- a/.vscode-test.mjs +++ b/.vscode-test.mjs @@ -11,7 +11,6 @@ export default defineConfig([ mocha: { ui: 'tdd', timeout: 120000, - retries: 1, }, env: { VSC_PYTHON_SMOKE_TEST: '1', @@ -32,7 +31,6 @@ export default defineConfig([ mocha: { ui: 'tdd', timeout: 180000, - retries: 1, }, env: { VSC_PYTHON_E2E_TEST: '1', @@ -51,7 +49,6 @@ export default defineConfig([ mocha: { ui: 'tdd', timeout: 60000, - retries: 1, }, env: { VSC_PYTHON_INTEGRATION_TEST: '1', diff --git a/src/test/common/testUtils.unit.test.ts b/src/test/common/testUtils.unit.test.ts new file mode 100644 index 00000000..526367a6 --- /dev/null +++ b/src/test/common/testUtils.unit.test.ts @@ -0,0 +1,174 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import * as assert from 'assert'; +import { retryUntilSuccess, sleep, waitForApiReady, waitForCondition } from '../testUtils'; + +suite('Test Utilities', () => { + suite('sleep', () => { + test('should resolve after specified time', async () => { + const start = Date.now(); + await sleep(50); + const elapsed = Date.now() - start; + assert.ok(elapsed >= 45, `Expected at least 45ms, got ${elapsed}ms`); + }); + }); + + suite('waitForCondition', () => { + test('should resolve immediately when condition is true', async () => { + await waitForCondition(() => true, 100, 'Should not fail'); + }); + + test('should resolve when condition becomes true', async () => { + let counter = 0; + await waitForCondition( + () => { + counter++; + return counter >= 3; + }, + 1000, + 'Condition did not become true', + 10, + ); + assert.ok(counter >= 3, 'Condition should have been checked multiple times'); + }); + + test('should reject when timeout is reached', async () => { + await assert.rejects( + () => waitForCondition(() => false, 100, 'Custom error message', 10), + /Custom error message \(waited 100ms\)/, + ); + }); + + test('should handle async conditions', async () => { + let counter = 0; + await waitForCondition( + async () => { + counter++; + await sleep(5); + return counter >= 2; + }, + 1000, + 'Async condition failed', + 10, + ); + assert.ok(counter >= 2); + }); + + test('should continue polling when condition throws', async () => { + let counter = 0; + await waitForCondition( + () => { + counter++; + if (counter < 3) { + throw new Error('Not ready yet'); + } + return true; + }, + 1000, + 'Should eventually succeed', + 10, + ); + assert.ok(counter >= 3); + }); + }); + + suite('retryUntilSuccess', () => { + test('should return result when function succeeds immediately', async () => { + const result = await retryUntilSuccess( + () => 42, + () => true, + 1000, + 'Should not fail', + ); + assert.strictEqual(result, 42); + }); + + test('should return result when validation passes', async () => { + let counter = 0; + const result = await retryUntilSuccess( + () => { + counter++; + return counter; + }, + (val) => val >= 3, + 1000, + 'Validation failed', + ); + assert.ok(result >= 3); + }); + + test('should reject when timeout reached', async () => { + await assert.rejects( + () => + retryUntilSuccess( + () => 1, + (val) => val > 10, + 100, + 'Custom timeout error', + ), + /Custom timeout error: validation failed/, + ); + }); + + test('should include last error message in rejection', async () => { + await assert.rejects( + () => + retryUntilSuccess( + () => { + throw new Error('Specific failure'); + }, + () => true, + 100, + 'Operation failed', + ), + /Operation failed: Specific failure/, + ); + }); + + test('should handle async functions', async () => { + let counter = 0; + const result = await retryUntilSuccess( + async () => { + counter++; + await sleep(5); + return counter; + }, + (val) => val >= 2, + 1000, + 'Async retry failed', + ); + assert.ok(result >= 2); + }); + }); + + suite('waitForApiReady', () => { + test('should return ready:true when getEnvironments succeeds', async () => { + const mockApi = { + getEnvironments: async () => [], + }; + const result = await waitForApiReady(mockApi, 1000); + assert.deepStrictEqual(result, { ready: true }); + }); + + test('should return ready:false with error when timeout reached', async () => { + const mockApi = { + getEnvironments: (): Promise => new Promise(() => {}), // Never resolves + }; + const result = await waitForApiReady(mockApi, 100); + assert.strictEqual(result.ready, false); + assert.ok(result.error?.includes('API not ready within 100ms')); + }); + + test('should return ready:false when getEnvironments throws', async () => { + const mockApi = { + getEnvironments: async () => { + throw new Error('Manager not registered'); + }, + }; + const result = await waitForApiReady(mockApi, 1000); + assert.strictEqual(result.ready, false); + assert.ok(result.error?.includes('Manager not registered')); + }); + }); +}); diff --git a/src/test/e2e/index.ts b/src/test/e2e/index.ts index fa7822e8..25ef2564 100644 --- a/src/test/e2e/index.ts +++ b/src/test/e2e/index.ts @@ -20,7 +20,6 @@ export async function run(): Promise { ui: 'tdd', color: true, timeout: 180_000, // 3 minutes - E2E workflows can be slow - retries: 1, // Retry once on failure slow: 30_000, // Mark tests as slow if they take > 30s }); diff --git a/src/test/integration/index.ts b/src/test/integration/index.ts index 55454103..f805b5eb 100644 --- a/src/test/integration/index.ts +++ b/src/test/integration/index.ts @@ -17,7 +17,6 @@ export async function run(): Promise { ui: 'tdd', color: true, timeout: 120_000, // 2 minutes - retries: 1, slow: 15_000, // Mark as slow if > 15s }); diff --git a/src/test/smoke/functional.smoke.test.ts b/src/test/smoke/functional.smoke.test.ts index e4f5ae2e..2f2e8491 100644 --- a/src/test/smoke/functional.smoke.test.ts +++ b/src/test/smoke/functional.smoke.test.ts @@ -25,7 +25,6 @@ suite('Smoke: Functional Checks', function () { this.timeout(MAX_EXTENSION_ACTIVATION_TIME); let api: PythonEnvironmentApi; - let managersReady = false; suiteSetup(async function () { const extension = vscode.extensions.getExtension(ENVS_EXTENSION_ID); @@ -40,13 +39,9 @@ suite('Smoke: Functional Checks', function () { assert.ok(api, 'API not exported'); // Wait for environment managers to register (happens async in setImmediate) - // This may fail in CI if the pet binary is not available + // This will fail if the pet binary is not available - which is correct behavior const result = await waitForApiReady(api, 45_000); - managersReady = result.ready; - if (!result.ready) { - console.log(`[WARN] Managers not ready: ${result.error}`); - console.log('[WARN] Tests requiring managers will be skipped'); - } + assert.ok(result.ready, `Environment managers failed to initialize: ${result.error}`); }); // ========================================================================= @@ -54,12 +49,6 @@ suite('Smoke: Functional Checks', function () { // ========================================================================= test('getEnvironments returns an array', async function () { - // Skip if managers aren't ready (e.g., pet binary not available in CI) - if (!managersReady) { - this.skip(); - return; - } - // This test verifies discovery machinery works // Even if no Python is installed, it should return an empty array, not throw @@ -69,27 +58,10 @@ suite('Smoke: Functional Checks', function () { }); test('getEnvironments finds Python installations when available', async function () { - // Skip if managers aren't ready (e.g., pet binary not available in CI) - if (!managersReady) { - this.skip(); - return; - } - - // Skip this test if no Python is expected (CI without Python) - if (process.env.SKIP_PYTHON_TESTS) { - this.skip(); - return; - } - const environments = await api.getEnvironments('all'); - // On a typical dev machine, we expect at least one Python - // This test may need to be conditional based on CI environment - if (environments.length === 0) { - console.log('[WARN] No Python environments found - is Python installed?'); - // Don't fail - just warn. CI may not have Python. - return; - } + // CI should have Python installed, so we expect at least one environment + assert.ok(environments.length > 0, 'Expected at least one Python environment - is Python installed?'); // Verify environment structure const env = environments[0]; @@ -102,12 +74,6 @@ suite('Smoke: Functional Checks', function () { }); test('getEnvironments with scope "global" returns global interpreters', async function () { - // Skip if managers aren't ready (e.g., pet binary not available in CI) - if (!managersReady) { - this.skip(); - return; - } - const globalEnvs = await api.getEnvironments('global'); assert.ok(Array.isArray(globalEnvs), 'getEnvironments("global") should return an array'); @@ -119,12 +85,6 @@ suite('Smoke: Functional Checks', function () { }); test('refreshEnvironments completes without error', async function () { - // Skip if managers aren't ready (e.g., pet binary not available in CI) - if (!managersReady) { - this.skip(); - return; - } - // This should not throw await api.refreshEnvironments(undefined); @@ -167,12 +127,6 @@ suite('Smoke: Functional Checks', function () { // ========================================================================= test('getEnvironment returns undefined or a valid environment', async function () { - // Skip if managers aren't ready (e.g., pet binary not available in CI) - if (!managersReady) { - this.skip(); - return; - } - // With no explicit selection, may return undefined or auto-selected env const env = await api.getEnvironment(undefined); @@ -220,12 +174,6 @@ suite('Smoke: Functional Checks', function () { // ========================================================================= test('resolveEnvironment handles invalid path gracefully', async function () { - // Skip if managers aren't ready (e.g., pet binary not available in CI) - if (!managersReady) { - this.skip(); - return; - } - const fakeUri = vscode.Uri.file('/this/is/not/a/python/installation'); // Should return undefined, not throw @@ -234,12 +182,6 @@ suite('Smoke: Functional Checks', function () { }); test('resolveEnvironment returns full details for valid environment', async function () { - // Skip if managers aren't ready (e.g., pet binary not available in CI) - if (!managersReady) { - this.skip(); - return; - } - const environments = await api.getEnvironments('all'); if (environments.length === 0) { @@ -264,12 +206,6 @@ suite('Smoke: Functional Checks', function () { // ========================================================================= test('getPackages returns array or undefined for valid environment', async function () { - // Skip if managers aren't ready (e.g., pet binary not available in CI) - if (!managersReady) { - this.skip(); - return; - } - const environments = await api.getEnvironments('all'); if (environments.length === 0) { diff --git a/src/test/smoke/index.ts b/src/test/smoke/index.ts index dbdafafc..7957417f 100644 --- a/src/test/smoke/index.ts +++ b/src/test/smoke/index.ts @@ -23,7 +23,6 @@ export async function run(): Promise { ui: 'tdd', color: true, timeout: 120_000, // 2 minutes - smoke tests can be slow - retries: 1, // Retry once on failure to handle flakiness slow: 10_000, // Mark tests as slow if they take > 10s }); diff --git a/src/test/smoke/registration.smoke.test.ts b/src/test/smoke/registration.smoke.test.ts index cf297b05..bd8d469e 100644 --- a/src/test/smoke/registration.smoke.test.ts +++ b/src/test/smoke/registration.smoke.test.ts @@ -26,7 +26,6 @@ suite('Smoke: Registration Checks', function () { this.timeout(MAX_EXTENSION_ACTIVATION_TIME); let api: PythonEnvironmentApi; - let managersReady = false; suiteSetup(async function () { const extension = vscode.extensions.getExtension(ENVS_EXTENSION_ID); @@ -41,13 +40,9 @@ suite('Smoke: Registration Checks', function () { assert.ok(api, 'API not exported'); // Wait for environment managers to register (happens async in setImmediate) - // This may fail in CI if the pet binary is not available + // This will fail if the pet binary is not available - which is correct behavior const result = await waitForApiReady(api, 45_000); - managersReady = result.ready; - if (!result.ready) { - console.log(`[WARN] Managers not ready: ${result.error}`); - console.log('[WARN] Some tests will be skipped'); - } + assert.ok(result.ready, `Environment managers failed to initialize: ${result.error}`); }); // ========================================================================= @@ -264,12 +259,6 @@ suite('Smoke: Registration Checks', function () { // ========================================================================= test('Built-in environment managers are registered', async function () { - // Skip if managers aren't ready (e.g., pet binary not available in CI) - if (!managersReady) { - this.skip(); - return; - } - // Get all environments to verify managers are working const environments = await api.getEnvironments('all');