From 75ed73e8b139f5f785e8292fac047ae23185ca63 Mon Sep 17 00:00:00 2001 From: Eleanor Boyd Date: Mon, 19 Feb 2024 19:05:24 -0800 Subject: [PATCH] Fix Bug with Pytest when using symlinked workspaces (#22885) fixes https://github.com/microsoft/vscode-python/issues/22658 also implements switching to arg mapping which is this issue here: https://github.com/microsoft/vscode-python/issues/22076 --------- Co-authored-by: Karthik Nadig --- ThirdPartyNotices-Repository.txt | 29 +++ .../expected_discovery_test_output.py | 75 ++++++++ pythonFiles/tests/pytestadapter/helpers.py | 18 ++ .../tests/pytestadapter/test_discovery.py | 42 ++++- pythonFiles/vscode_pytest/__init__.py | 53 +++++- .../testing/testController/common/utils.ts | 80 ++++++++ .../pytest/pytestDiscoveryAdapter.ts | 19 +- .../pytest/pytestExecutionAdapter.ts | 21 ++- .../testing/common/testingAdapter.test.ts | 113 ++++++++++++ .../pytestDiscoveryAdapter.unit.test.ts | 68 +++++++ .../pytestExecutionAdapter.unit.test.ts | 7 +- .../testing/testController/utils.unit.test.ts | 174 ++++++++++++++++++ 12 files changed, 681 insertions(+), 18 deletions(-) diff --git a/ThirdPartyNotices-Repository.txt b/ThirdPartyNotices-Repository.txt index 9e7e822af1bb..bbb00d523f91 100644 --- a/ThirdPartyNotices-Repository.txt +++ b/ThirdPartyNotices-Repository.txt @@ -17,6 +17,7 @@ Microsoft Python extension for Visual Studio Code incorporates third party mater 11. vscode-cpptools (https://github.com/microsoft/vscode-cpptools) 12. mocha (https://github.com/mochajs/mocha) 13. get-pip (https://github.com/pypa/get-pip) +14. vscode-js-debug (https://github.com/microsoft/vscode-js-debug) %% Go for Visual Studio Code NOTICES, INFORMATION, AND LICENSE BEGIN HERE @@ -1032,3 +1033,31 @@ WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. ========================================= END OF get-pip NOTICES, INFORMATION, AND LICENSE + + +%% vscode-js-debug NOTICES, INFORMATION, AND LICENSE BEGIN HERE +========================================= + +MIT License + +Copyright (c) Microsoft Corporation. All rights reserved. + +Permission is hereby granted, free of charge, to any person obtaining a copy +of this software and associated documentation files (the "Software"), to deal +in the Software without restriction, including without limitation the rights +to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +copies of the Software, and to permit persons to whom the Software is +furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice shall be included in all +copies or substantial portions of the Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +SOFTWARE +========================================= +END OF vscode-js-debug NOTICES, INFORMATION, AND LICENSE diff --git a/pythonFiles/tests/pytestadapter/expected_discovery_test_output.py b/pythonFiles/tests/pytestadapter/expected_discovery_test_output.py index 2d86710e776b..7fbb0c5c43e7 100644 --- a/pythonFiles/tests/pytestadapter/expected_discovery_test_output.py +++ b/pythonFiles/tests/pytestadapter/expected_discovery_test_output.py @@ -994,3 +994,78 @@ ], "id_": str(TEST_DATA_PATH), } +SYMLINK_FOLDER_PATH = TEST_DATA_PATH / "symlink_folder" +SYMLINK_FOLDER_PATH_TESTS = TEST_DATA_PATH / "symlink_folder" / "tests" +SYMLINK_FOLDER_PATH_TESTS_TEST_A = ( + TEST_DATA_PATH / "symlink_folder" / "tests" / "test_a.py" +) +SYMLINK_FOLDER_PATH_TESTS_TEST_B = ( + TEST_DATA_PATH / "symlink_folder" / "tests" / "test_b.py" +) + +symlink_expected_discovery_output = { + "name": "symlink_folder", + "path": str(SYMLINK_FOLDER_PATH), + "type_": "folder", + "children": [ + { + "name": "tests", + "path": str(SYMLINK_FOLDER_PATH_TESTS), + "type_": "folder", + "id_": str(SYMLINK_FOLDER_PATH_TESTS), + "children": [ + { + "name": "test_a.py", + "path": str(SYMLINK_FOLDER_PATH_TESTS_TEST_A), + "type_": "file", + "id_": str(SYMLINK_FOLDER_PATH_TESTS_TEST_A), + "children": [ + { + "name": "test_a_function", + "path": str(SYMLINK_FOLDER_PATH_TESTS_TEST_A), + "lineno": find_test_line_number( + "test_a_function", + os.path.join(tests_path, "test_a.py"), + ), + "type_": "test", + "id_": get_absolute_test_id( + "tests/test_a.py::test_a_function", + SYMLINK_FOLDER_PATH_TESTS_TEST_A, + ), + "runID": get_absolute_test_id( + "tests/test_a.py::test_a_function", + SYMLINK_FOLDER_PATH_TESTS_TEST_A, + ), + } + ], + }, + { + "name": "test_b.py", + "path": str(SYMLINK_FOLDER_PATH_TESTS_TEST_B), + "type_": "file", + "id_": str(SYMLINK_FOLDER_PATH_TESTS_TEST_B), + "children": [ + { + "name": "test_b_function", + "path": str(SYMLINK_FOLDER_PATH_TESTS_TEST_B), + "lineno": find_test_line_number( + "test_b_function", + os.path.join(tests_path, "test_b.py"), + ), + "type_": "test", + "id_": get_absolute_test_id( + "tests/test_b.py::test_b_function", + SYMLINK_FOLDER_PATH_TESTS_TEST_B, + ), + "runID": get_absolute_test_id( + "tests/test_b.py::test_b_function", + SYMLINK_FOLDER_PATH_TESTS_TEST_B, + ), + } + ], + }, + ], + } + ], + "id_": str(SYMLINK_FOLDER_PATH), +} diff --git a/pythonFiles/tests/pytestadapter/helpers.py b/pythonFiles/tests/pytestadapter/helpers.py index 2d36da59956b..a3ed21cc5538 100644 --- a/pythonFiles/tests/pytestadapter/helpers.py +++ b/pythonFiles/tests/pytestadapter/helpers.py @@ -1,6 +1,7 @@ # Copyright (c) Microsoft Corporation. All rights reserved. # Licensed under the MIT License. +import contextlib import io import json import os @@ -27,6 +28,23 @@ def get_absolute_test_id(test_id: str, testPath: pathlib.Path) -> str: return absolute_test_id +@contextlib.contextmanager +def create_symlink(root: pathlib.Path, target_ext: str, destination_ext: str): + try: + destination = root / destination_ext + target = root / target_ext + if destination.exists(): + print("destination already exists", destination) + try: + destination.symlink_to(target) + except Exception as e: + print("error occurred when attempting to create a symlink", e) + yield target, destination + finally: + destination.unlink() + print("destination unlinked", destination) + + def create_server( host: str = "127.0.0.1", port: int = 0, diff --git a/pythonFiles/tests/pytestadapter/test_discovery.py b/pythonFiles/tests/pytestadapter/test_discovery.py index 9956b82a6345..b28a2b307ae2 100644 --- a/pythonFiles/tests/pytestadapter/test_discovery.py +++ b/pythonFiles/tests/pytestadapter/test_discovery.py @@ -1,5 +1,6 @@ # Copyright (c) Microsoft Corporation. All rights reserved. # Licensed under the MIT License. +import json import os import pathlib import shutil @@ -14,7 +15,7 @@ from tests.tree_comparison_helper import is_same_tree from . import expected_discovery_test_output -from .helpers import TEST_DATA_PATH, runner, runner_with_cwd +from .helpers import TEST_DATA_PATH, runner, runner_with_cwd, create_symlink def test_import_error(tmp_path): @@ -205,6 +206,45 @@ def test_pytest_collect(file, expected_const): assert is_same_tree(actual_item.get("tests"), expected_const) +def test_symlink_root_dir(): + """ + Test to test pytest discovery with the command line arg --rootdir specified as a symlink path. + Discovery should succeed and testids should be relative to the symlinked root directory. + """ + with create_symlink(TEST_DATA_PATH, "root", "symlink_folder") as ( + source, + destination, + ): + assert destination.is_symlink() + + # Run pytest with the cwd being the resolved symlink path (as it will be when we run the subprocess from node). + actual = runner_with_cwd( + ["--collect-only", f"--rootdir={os.fspath(destination)}"], source + ) + expected = expected_discovery_test_output.symlink_expected_discovery_output + assert actual + actual_list: List[Dict[str, Any]] = actual + if actual_list is not None: + assert actual_list.pop(-1).get("eot") + actual_item = actual_list.pop(0) + try: + # Check if all requirements + assert all( + item in actual_item.keys() for item in ("status", "cwd", "error") + ), "Required keys are missing" + assert actual_item.get("status") == "success", "Status is not 'success'" + assert actual_item.get("cwd") == os.fspath( + destination + ), f"CWD does not match: {os.fspath(destination)}" + assert ( + actual_item.get("tests") == expected + ), "Tests do not match expected value" + except AssertionError as e: + # Print the actual_item in JSON format if an assertion fails + print(json.dumps(actual_item, indent=4)) + pytest.fail(str(e)) + + def test_pytest_root_dir(): """ Test to test pytest discovery with the command line arg --rootdir specified to be a subfolder diff --git a/pythonFiles/vscode_pytest/__init__.py b/pythonFiles/vscode_pytest/__init__.py index a565fbf930a6..256f2bfdb099 100644 --- a/pythonFiles/vscode_pytest/__init__.py +++ b/pythonFiles/vscode_pytest/__init__.py @@ -57,6 +57,7 @@ def __init__(self, message): collected_tests_so_far = list() TEST_PORT = os.getenv("TEST_PORT") TEST_UUID = os.getenv("TEST_UUID") +SYMLINK_PATH = None def pytest_load_initial_conftests(early_config, parser, args): @@ -75,6 +76,25 @@ def pytest_load_initial_conftests(early_config, parser, args): global IS_DISCOVERY IS_DISCOVERY = True + # check if --rootdir is in the args + for arg in args: + if "--rootdir=" in arg: + rootdir = arg.split("--rootdir=")[1] + if not os.path.exists(rootdir): + raise VSCodePytestError( + f"The path set in the argument --rootdir={rootdir} does not exist." + ) + if ( + os.path.islink(rootdir) + and pathlib.Path(os.path.realpath(rootdir)) == pathlib.Path.cwd() + ): + print( + f"Plugin info[vscode-pytest]: rootdir argument, {rootdir}, is identified as a symlink to the cwd, {pathlib.Path.cwd()}.", + "Therefore setting symlink path to rootdir argument.", + ) + global SYMLINK_PATH + SYMLINK_PATH = pathlib.Path(rootdir) + def pytest_internalerror(excrepr, excinfo): """A pytest hook that is called when an internal error occurs. @@ -326,6 +346,13 @@ def pytest_sessionfinish(session, exitstatus): Exit code 5: No tests were collected """ cwd = pathlib.Path.cwd() + if SYMLINK_PATH: + print("Plugin warning[vscode-pytest]: SYMLINK set, adjusting cwd.") + # Get relative between the cwd (resolved path) and the node path. + rel_path = os.path.relpath(cwd, pathlib.Path.cwd()) + # Calculate the new node path by making it relative to the symlink path. + cwd = pathlib.Path(os.path.join(SYMLINK_PATH, rel_path)) + if IS_DISCOVERY: if not (exitstatus == 0 or exitstatus == 1 or exitstatus == 5): errorNode: TestNode = { @@ -388,6 +415,11 @@ def build_test_tree(session: pytest.Session) -> TestNode: class_nodes_dict: Dict[str, TestNode] = {} function_nodes_dict: Dict[str, TestNode] = {} + # Check to see if the global variable for symlink path is set + if SYMLINK_PATH: + session_node["path"] = SYMLINK_PATH + session_node["id_"] = os.fspath(SYMLINK_PATH) + for test_case in session.items: test_node = create_test_node(test_case) if isinstance(test_case.parent, pytest.Class): @@ -645,13 +677,31 @@ class EOTPayloadDict(TypedDict): def get_node_path(node: Any) -> pathlib.Path: - """A function that returns the path of a node given the switch to pathlib.Path.""" + """ + A function that returns the path of a node given the switch to pathlib.Path. + It also evaluates if the node is a symlink and returns the equivalent path. + """ path = getattr(node, "path", None) or pathlib.Path(node.fspath) if not path: raise VSCodePytestError( f"Unable to find path for node: {node}, node.path: {node.path}, node.fspath: {node.fspath}" ) + + # Check for the session node since it has the symlink already. + if SYMLINK_PATH and not isinstance(node, pytest.Session): + # Get relative between the cwd (resolved path) and the node path. + try: + rel_path = path.relative_to(pathlib.Path.cwd()) + + # Calculate the new node path by making it relative to the symlink path. + sym_path = pathlib.Path(os.path.join(SYMLINK_PATH, rel_path)) + return sym_path + except Exception as e: + raise VSCodePytestError( + f"Error occurred while calculating symlink equivalent from node path: {e}" + "\n SYMLINK_PATH: {SYMLINK_PATH}, \n node path: {path}, \n cwd: {{pathlib.Path.cwd()}}" + ) return path @@ -687,7 +737,6 @@ def post_response(cwd: str, session_node: TestNode) -> None: cwd (str): Current working directory. session_node (TestNode): Node information of the test session. """ - payload: DiscoveryPayloadDict = { "cwd": cwd, "status": "success" if not ERRORS else "error", diff --git a/src/client/testing/testController/common/utils.ts b/src/client/testing/testController/common/utils.ts index 23ee881a405a..0e81154a899c 100644 --- a/src/client/testing/testController/common/utils.ts +++ b/src/client/testing/testController/common/utils.ts @@ -349,3 +349,83 @@ export function splitTestNameWithRegex(testName: string): [string, string] { } return [testName, testName]; } + +/** + * Converts an array of strings (with or without '=') into a map. + * If a string contains '=', it is split into a key-value pair, with the portion + * before the '=' as the key and the portion after the '=' as the value. + * If no '=' is found in the string, the entire string becomes a key with a value of null. + * + * @param args - Readonly array of strings to be converted to a map. + * @returns A map representation of the input strings. + */ +export const argsToMap = (args: ReadonlyArray): { [key: string]: string | null | undefined } => { + const map: { [key: string]: string | null } = {}; + for (const arg of args) { + const delimiter = arg.indexOf('='); + if (delimiter === -1) { + map[arg] = null; + } else { + map[arg.slice(0, delimiter)] = arg.slice(delimiter + 1); + } + } + + return map; +}; + +/** + * Converts a map into an array of strings. + * Each key-value pair in the map is transformed into a string. + * If the value is null, only the key is represented in the string. + * If the value is defined (and not null), the string is in the format "key=value". + * If a value is undefined, the key-value pair is skipped. + * + * @param map - The map to be converted to an array of strings. + * @returns An array of strings representation of the input map. + */ +export const mapToArgs = (map: { [key: string]: string | null | undefined }): string[] => { + const out: string[] = []; + for (const key of Object.keys(map)) { + const value = map[key]; + if (value === undefined) { + // eslint-disable-next-line no-continue + continue; + } + + out.push(value === null ? key : `${key}=${value}`); + } + + return out; +}; + +/** + * Adds an argument to the map only if it doesn't already exist. + * + * @param map - The map of arguments. + * @param argKey - The argument key to be checked and added. + * @param argValue - The value to set for the argument if it's not already in the map. + * @returns The updated map. + */ +export function addArgIfNotExist( + map: { [key: string]: string | null | undefined }, + argKey: string, + argValue: string | null, +): { [key: string]: string | null | undefined } { + // Only add the argument if it doesn't exist in the map. + if (map[argKey] === undefined) { + map[argKey] = argValue; + } + + return map; +} + +/** + * Checks if an argument key exists in the map. + * + * @param map - The map of arguments. + * @param argKey - The argument key to be checked. + * @returns True if the argument key exists in the map, false otherwise. + */ +export function argKeyExists(map: { [key: string]: string | null | undefined }, argKey: string): boolean { + return map[argKey] !== undefined; +} diff --git a/src/client/testing/testController/pytest/pytestDiscoveryAdapter.ts b/src/client/testing/testController/pytest/pytestDiscoveryAdapter.ts index 39dc87ed12c1..ab44c96821e5 100644 --- a/src/client/testing/testController/pytest/pytestDiscoveryAdapter.ts +++ b/src/client/testing/testController/pytest/pytestDiscoveryAdapter.ts @@ -2,6 +2,7 @@ // Licensed under the MIT License. import * as path from 'path'; import { Uri } from 'vscode'; +import * as fs from 'fs'; import { ExecutionFactoryCreateWithEnvironmentOptions, IPythonExecutionFactory, @@ -10,7 +11,7 @@ import { import { IConfigurationService, ITestOutputChannel } from '../../../common/types'; import { Deferred, createDeferred } from '../../../common/utils/async'; import { EXTENSION_ROOT_DIR } from '../../../constants'; -import { traceError, traceInfo, traceVerbose } from '../../../logging'; +import { traceError, traceInfo, traceVerbose, traceWarn } from '../../../logging'; import { DataReceivedEvent, DiscoveredTestPayload, @@ -24,6 +25,9 @@ import { createEOTPayload, createTestingDeferred, fixLogLinesNoTrailing, + argsToMap, + addArgIfNotExist, + mapToArgs, } from '../common/utils'; import { IEnvironmentVariablesProvider } from '../../../common/variables/types'; @@ -66,9 +70,18 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter { const relativePathToPytest = 'pythonFiles'; const fullPluginPath = path.join(EXTENSION_ROOT_DIR, relativePathToPytest); const settings = this.configSettings.getSettings(uri); - const { pytestArgs } = settings.testing; + let pytestArgsMap = argsToMap(settings.testing.pytestArgs); const cwd = settings.testing.cwd && settings.testing.cwd.length > 0 ? settings.testing.cwd : uri.fsPath; + // check for symbolic path + const stats = fs.lstatSync(cwd); + if (stats.isSymbolicLink()) { + traceWarn( + "The cwd is a symbolic link, adding '--rootdir' to pytestArgsMap only if it doesn't already exist.", + ); + pytestArgsMap = addArgIfNotExist(pytestArgsMap, '--rootdir', cwd); + } + // get and edit env vars const mutableEnv = { ...(await this.envVarsService?.getEnvironmentVariables(uri)), @@ -98,7 +111,7 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter { }; const execService = await executionFactory?.createActivatedEnvironment(creationOptions); // delete UUID following entire discovery finishing. - const execArgs = ['-m', 'pytest', '-p', 'vscode_pytest', '--collect-only'].concat(pytestArgs); + const execArgs = ['-m', 'pytest', '-p', 'vscode_pytest', '--collect-only'].concat(mapToArgs(pytestArgsMap)); traceVerbose(`Running pytest discovery with command: ${execArgs.join(' ')} for workspace ${uri.fsPath}.`); const deferredTillExecClose: Deferred = createTestingDeferred(); diff --git a/src/client/testing/testController/pytest/pytestExecutionAdapter.ts b/src/client/testing/testController/pytest/pytestExecutionAdapter.ts index 8995a182a774..d366bdfc9718 100644 --- a/src/client/testing/testController/pytest/pytestExecutionAdapter.ts +++ b/src/client/testing/testController/pytest/pytestExecutionAdapter.ts @@ -129,15 +129,16 @@ export class PytestTestExecutionAdapter implements ITestExecutionAdapter { try { // Remove positional test folders and files, we will add as needed per node const testArgs = removePositionalFoldersAndFiles(pytestArgs); + let testArgsMap = utils.argsToMap(testArgs); // if user has provided `--rootdir` then use that, otherwise add `cwd` - if (testArgs.filter((a) => a.startsWith('--rootdir')).length === 0) { - // Make sure root dir is set so pytest can find the relative paths - testArgs.splice(0, 0, '--rootdir', uri.fsPath); - } + // root dir is required so pytest can find the relative paths and for symlinks + utils.addArgIfNotExist(testArgsMap, '--rootdir', cwd); - if (debugBool && !testArgs.some((a) => a.startsWith('--capture') || a === '-s')) { - testArgs.push('--capture', 'no'); + // -s and --capture are both command line options that control how pytest captures output. + // if neither are set, then set --capture=no to prevent pytest from capturing output. + if (debugBool && !utils.argKeyExists(testArgsMap, '-s')) { + testArgsMap = utils.addArgIfNotExist(testArgsMap, '--capture', 'no'); } // add port with run test ids to env vars @@ -162,7 +163,7 @@ export class PytestTestExecutionAdapter implements ITestExecutionAdapter { const pytestUUID = uuid.toString(); const launchOptions: LaunchOptions = { cwd, - args: testArgs, + args: utils.mapToArgs(testArgsMap), token: spawnOptions.token, testProvider: PYTEST_PROVIDER, pytestPort, @@ -170,7 +171,9 @@ export class PytestTestExecutionAdapter implements ITestExecutionAdapter { runTestIdsPort: pytestRunTestIdsPort.toString(), }; traceInfo( - `Running DEBUG pytest with arguments: ${testArgs.join(' ')} for workspace ${uri.fsPath} \r\n`, + `Running DEBUG pytest with arguments: ${utils.mapToArgs(testArgsMap).join(' ')} for workspace ${ + uri.fsPath + } \r\n`, ); await debugLauncher!.launchDebugger(launchOptions, () => { deferredTillEOT?.resolve(); @@ -180,7 +183,7 @@ export class PytestTestExecutionAdapter implements ITestExecutionAdapter { const deferredTillExecClose: Deferred = utils.createTestingDeferred(); // combine path to run script with run args const scriptPath = path.join(fullPluginPath, 'vscode_pytest', 'run_pytest_script.py'); - const runArgs = [scriptPath, ...testArgs]; + const runArgs = [scriptPath, ...utils.mapToArgs(testArgsMap)]; traceInfo(`Running pytest with arguments: ${runArgs.join(' ')} for workspace ${uri.fsPath} \r\n`); let resultProc: ChildProcess | undefined; diff --git a/src/test/testing/common/testingAdapter.test.ts b/src/test/testing/common/testingAdapter.test.ts index a9ed25194fa9..8e2f6200003e 100644 --- a/src/test/testing/common/testingAdapter.test.ts +++ b/src/test/testing/common/testingAdapter.test.ts @@ -5,6 +5,7 @@ import { TestController, TestRun, Uri } from 'vscode'; import * as typeMoq from 'typemoq'; import * as path from 'path'; import * as assert from 'assert'; +import * as fs from 'fs'; import { PytestTestDiscoveryAdapter } from '../../../client/testing/testController/pytest/pytestDiscoveryAdapter'; import { ITestController, ITestResultResolver } from '../../../client/testing/testController/common/types'; import { PythonTestServer } from '../../../client/testing/testController/common/server'; @@ -59,8 +60,25 @@ suite('End to End Tests: test adapters', () => { 'testTestingRootWkspc', 'discoveryErrorWorkspace', ); + const rootPathDiscoverySymlink = path.join( + EXTENSION_ROOT_DIR_FOR_TESTS, + 'src', + 'testTestingRootWkspc', + 'symlinkWorkspace', + ); suiteSetup(async () => { serviceContainer = (await initialize()).serviceContainer; + + // create symlink for specific symlink test + const target = rootPathSmallWorkspace; + const dest = rootPathDiscoverySymlink; + fs.symlink(target, dest, 'dir', (err) => { + if (err) { + console.error(err); + } else { + console.log('Symlink created successfully for end to end tests.'); + } + }); }); setup(async () => { @@ -96,6 +114,17 @@ suite('End to End Tests: test adapters', () => { teardown(async () => { pythonTestServer.dispose(); }); + suiteTeardown(async () => { + // remove symlink + const dest = rootPathDiscoverySymlink; + fs.unlink(dest, (err) => { + if (err) { + console.error(err); + } else { + console.log('Symlink removed successfully after tests.'); + } + }); + }); test('unittest discovery adapter small workspace', async () => { // result resolver and saved data for assertions let actualData: { @@ -232,6 +261,90 @@ suite('End to End Tests: test adapters', () => { assert.strictEqual(callCount, 1, 'Expected _resolveDiscovery to be called once'); }); }); + test('pytest discovery adapter small workspace with symlink', async () => { + // result resolver and saved data for assertions + let actualData: { + cwd: string; + tests?: unknown; + status: 'success' | 'error'; + error?: string[]; + }; + // set workspace to test workspace folder + const testSimpleSymlinkPath = path.join(rootPathDiscoverySymlink, 'test_simple.py'); + workspaceUri = Uri.parse(rootPathDiscoverySymlink); + const stats = fs.lstatSync(rootPathDiscoverySymlink); + + // confirm that the path is a symbolic link + assert.ok(stats.isSymbolicLink(), 'The path is not a symbolic link but must be for this test.'); + + resultResolver = new PythonResultResolver(testController, pytestProvider, workspaceUri); + let callCount = 0; + resultResolver._resolveDiscovery = async (payload, _token?) => { + traceLog(`resolveDiscovery ${payload}`); + callCount = callCount + 1; + actualData = payload; + return Promise.resolve(); + }; + // run pytest discovery + const discoveryAdapter = new PytestTestDiscoveryAdapter( + pythonTestServer, + configService, + testOutputChannel.object, + resultResolver, + envVarsService, + ); + + await discoveryAdapter.discoverTests(workspaceUri, pythonExecFactory).finally(() => { + // verification after discovery is complete + + // 1. Check the status is "success" + assert.strictEqual( + actualData.status, + 'success', + `Expected status to be 'success' instead status is ${actualData.status}`, + ); // 2. Confirm no errors + assert.strictEqual(actualData.error?.length, 0, "Expected no errors in 'error' field"); + // 3. Confirm tests are found + assert.ok(actualData.tests, 'Expected tests to be present'); + // 4. Confirm that the cwd returned is the symlink path and the test's path is also using the symlink as the root + if (process.platform === 'win32') { + // covert string to lowercase for windows as the path is case insensitive + traceLog('windows machine detected, converting path to lowercase for comparison'); + const a = actualData.cwd.toLowerCase(); + const b = rootPathDiscoverySymlink.toLowerCase(); + const testSimpleActual = (actualData.tests as { + children: { + path: string; + }[]; + }).children[0].path.toLowerCase(); + const testSimpleExpected = testSimpleSymlinkPath.toLowerCase(); + assert.strictEqual(a, b, `Expected cwd to be the symlink path actual: ${a} expected: ${b}`); + assert.strictEqual( + testSimpleActual, + testSimpleExpected, + `Expected test path to be the symlink path actual: ${testSimpleActual} expected: ${testSimpleExpected}`, + ); + } else { + assert.strictEqual( + path.join(actualData.cwd), + path.join(rootPathDiscoverySymlink), + 'Expected cwd to be the symlink path, check for non-windows machines', + ); + assert.strictEqual( + (actualData.tests as { + children: { + path: string; + }[]; + }).children[0].path, + testSimpleSymlinkPath, + 'Expected test path to be the symlink path, check for non windows machines', + ); + } + + // 5. Confirm that resolveDiscovery was called once + assert.strictEqual(callCount, 1, 'Expected _resolveDiscovery to be called once'); + }); + }); test('pytest discovery adapter large workspace', async () => { // result resolver and saved data for assertions let actualData: { diff --git a/src/test/testing/testController/pytest/pytestDiscoveryAdapter.unit.test.ts b/src/test/testing/testController/pytest/pytestDiscoveryAdapter.unit.test.ts index 7badb5a0350d..45f6d4ffa8eb 100644 --- a/src/test/testing/testController/pytest/pytestDiscoveryAdapter.unit.test.ts +++ b/src/test/testing/testController/pytest/pytestDiscoveryAdapter.unit.test.ts @@ -6,6 +6,8 @@ import { Uri } from 'vscode'; import * as typeMoq from 'typemoq'; import * as path from 'path'; import { Observable } from 'rxjs/Observable'; +import * as fs from 'fs'; +import * as sinon from 'sinon'; import { IConfigurationService, ITestOutputChannel } from '../../../../client/common/types'; import { PytestTestDiscoveryAdapter } from '../../../../client/testing/testController/pytest/pytestDiscoveryAdapter'; import { ITestServer } from '../../../../client/testing/testController/common/types'; @@ -94,6 +96,9 @@ suite('pytest test discovery adapter', () => { }; }); }); + teardown(() => { + sinon.restore(); + }); test('Discovery should call exec with correct basic args', async () => { // set up exec mock deferred = createDeferred(); @@ -104,6 +109,7 @@ suite('pytest test discovery adapter', () => { deferred.resolve(); return Promise.resolve(execService.object); }); + sinon.stub(fs, 'lstatSync').returns({ isFile: () => true, isSymbolicLink: () => false } as fs.Stats); adapter = new PytestTestDiscoveryAdapter(testServer.object, configService, outputChannel.object); adapter.discoverTests(uri, execFactory.object); // add in await and trigger @@ -143,6 +149,8 @@ suite('pytest test discovery adapter', () => { }), } as unknown) as IConfigurationService; + sinon.stub(fs, 'lstatSync').returns({ isFile: () => true, isSymbolicLink: () => false } as fs.Stats); + // set up exec mock deferred = createDeferred(); execFactory = typeMoq.Mock.ofType(); @@ -161,6 +169,7 @@ suite('pytest test discovery adapter', () => { mockProc.trigger('close'); // verification + const expectedArgs = ['-m', 'pytest', '-p', 'vscode_pytest', '--collect-only', '.', 'abc', 'xyz']; execService.verify( (x) => @@ -176,4 +185,63 @@ suite('pytest test discovery adapter', () => { typeMoq.Times.once(), ); }); + test('Test discovery adds cwd to pytest args when path is symlink', async () => { + sinon.stub(fs, 'lstatSync').returns({ + isFile: () => true, + isSymbolicLink: () => true, + } as fs.Stats); + + // set up a config service with different pytest args + const configServiceNew: IConfigurationService = ({ + getSettings: () => ({ + testing: { + pytestArgs: ['.', 'abc', 'xyz'], + cwd: expectedPath, + }, + }), + } as unknown) as IConfigurationService; + + // set up exec mock + deferred = createDeferred(); + execFactory = typeMoq.Mock.ofType(); + execFactory + .setup((x) => x.createActivatedEnvironment(typeMoq.It.isAny())) + .returns(() => { + deferred.resolve(); + return Promise.resolve(execService.object); + }); + + adapter = new PytestTestDiscoveryAdapter(testServer.object, configServiceNew, outputChannel.object); + adapter.discoverTests(uri, execFactory.object); + // add in await and trigger + await deferred.promise; + await deferred2.promise; + mockProc.trigger('close'); + + // verification + const expectedArgs = [ + '-m', + 'pytest', + '-p', + 'vscode_pytest', + '--collect-only', + '.', + 'abc', + 'xyz', + `--rootdir=${expectedPath}`, + ]; + execService.verify( + (x) => + x.execObservable( + expectedArgs, + typeMoq.It.is((options) => { + assert.deepEqual(options.env, expectedExtraVariables); + assert.equal(options.cwd, expectedPath); + assert.equal(options.throwOnStdErr, true); + return true; + }), + ), + typeMoq.Times.once(), + ); + }); }); diff --git a/src/test/testing/testController/pytest/pytestExecutionAdapter.unit.test.ts b/src/test/testing/testController/pytest/pytestExecutionAdapter.unit.test.ts index a097df654360..d2ab19368b2d 100644 --- a/src/test/testing/testController/pytest/pytestExecutionAdapter.unit.test.ts +++ b/src/test/testing/testController/pytest/pytestExecutionAdapter.unit.test.ts @@ -171,7 +171,8 @@ suite('pytest test execution adapter', () => { const pathToPythonFiles = path.join(EXTENSION_ROOT_DIR, 'pythonFiles'); const pathToPythonScript = path.join(pathToPythonFiles, 'vscode_pytest', 'run_pytest_script.py'); - const expectedArgs = [pathToPythonScript, '--rootdir', myTestPath]; + const rootDirArg = `--rootdir=${myTestPath}`; + const expectedArgs = [pathToPythonScript, rootDirArg]; const expectedExtraVariables = { PYTHONPATH: pathToPythonFiles, TEST_UUID: 'uuid123', @@ -238,7 +239,7 @@ suite('pytest test execution adapter', () => { const pathToPythonFiles = path.join(EXTENSION_ROOT_DIR, 'pythonFiles'); const pathToPythonScript = path.join(pathToPythonFiles, 'vscode_pytest', 'run_pytest_script.py'); - const expectedArgs = [pathToPythonScript, '--rootdir', myTestPath]; + const expectedArgs = [pathToPythonScript, `--rootdir=${newCwd}`]; const expectedExtraVariables = { PYTHONPATH: pathToPythonFiles, TEST_UUID: 'uuid123', @@ -305,7 +306,7 @@ suite('pytest test execution adapter', () => { x.launchDebugger( typeMoq.It.is((launchOptions) => { assert.equal(launchOptions.cwd, uri.fsPath); - assert.deepEqual(launchOptions.args, ['--rootdir', myTestPath, '--capture', 'no']); + assert.deepEqual(launchOptions.args, [`--rootdir=${myTestPath}`, '--capture=no']); assert.equal(launchOptions.testProvider, 'pytest'); assert.equal(launchOptions.pytestPort, '12345'); assert.equal(launchOptions.pytestUUID, 'uuid123'); diff --git a/src/test/testing/testController/utils.unit.test.ts b/src/test/testing/testController/utils.unit.test.ts index 014261a40232..5bcf8dfa10c8 100644 --- a/src/test/testing/testController/utils.unit.test.ts +++ b/src/test/testing/testController/utils.unit.test.ts @@ -9,6 +9,10 @@ import { ExtractJsonRPCData, parseJsonRPCHeadersAndData, splitTestNameWithRegex, + mapToArgs, + addArgIfNotExist, + argKeyExists, + argsToMap, } from '../../../client/testing/testController/common/utils'; suite('Test Controller Utils: JSON RPC', () => { @@ -158,4 +162,174 @@ ${data}${secondPayload}`; }); }); }); + suite('Test Controller Utils: Args Mapping', () => { + test('Converts map with mixed values to array of strings', async () => { + const inputMap = { + key1: 'value1', + key2: null, + key3: undefined, + key4: 'value4', + }; + const expectedOutput = ['key1=value1', 'key2', 'key4=value4']; + + const result = mapToArgs(inputMap); + + assert.deepStrictEqual(result, expectedOutput); + }); + + test('Returns an empty array for an empty map', async () => { + const inputMap = {}; + const expectedOutput: unknown[] = []; + + const result = mapToArgs(inputMap); + + assert.deepStrictEqual(result, expectedOutput); + }); + + test('Skips undefined values', async () => { + const inputMap = { + key1: undefined, + key2: undefined, + }; + const expectedOutput: unknown[] = []; + + const result = mapToArgs(inputMap); + + assert.deepStrictEqual(result, expectedOutput); + }); + + test('Handles null values correctly', async () => { + const inputMap = { + key1: null, + key2: null, + }; + const expectedOutput = ['key1', 'key2']; + + const result = mapToArgs(inputMap); + + assert.deepStrictEqual(result, expectedOutput); + }); + test('Adds new argument if it does not exist', () => { + const map = {}; + const argKey = 'newKey'; + const argValue = 'newValue'; + + const updatedMap = addArgIfNotExist(map, argKey, argValue); + + assert.deepStrictEqual(updatedMap, { [argKey]: argValue }); + }); + + test('Does not overwrite existing argument', () => { + const map = { existingKey: 'existingValue' }; + const argKey = 'existingKey'; + const argValue = 'newValue'; + + const updatedMap = addArgIfNotExist(map, argKey, argValue); + + assert.deepStrictEqual(updatedMap, { [argKey]: 'existingValue' }); + }); + + test('Handles null value for new key', () => { + const map = {}; + const argKey = 'nullKey'; + const argValue = null; + + const updatedMap = addArgIfNotExist(map, argKey, argValue); + + assert.deepStrictEqual(updatedMap, { [argKey]: argValue }); + }); + + test('Ignores addition if key exists with null value', () => { + const map = { nullKey: null }; + const argKey = 'nullKey'; + const argValue = 'newValue'; + + const updatedMap = addArgIfNotExist(map, argKey, argValue); + + assert.deepStrictEqual(updatedMap, { [argKey]: null }); + }); + + test('Accepts addition if key exists with undefined value', () => { + const map = { undefinedKey: undefined }; + const argKey = 'undefinedKey'; + const argValue = 'newValue'; + + // Attempting to add a key that is explicitly set to undefined + const updatedMap = addArgIfNotExist(map, argKey, argValue); + + // Expect the map to remain unchanged because the key exists as undefined + assert.strictEqual(map[argKey], argValue); + assert.deepStrictEqual(updatedMap, { [argKey]: argValue }); + }); + test('Complex test for argKeyExists with various key types', () => { + const map = { + stringKey: 'stringValue', + nullKey: null, + // Note: not adding an 'undefinedKey' explicitly since it's not present and hence undefined by default + }; + + // Should return true for keys that are present, even with a null value + assert.strictEqual( + argKeyExists(map, 'stringKey'), + true, + "Failed to recognize 'stringKey' which has a string value.", + ); + assert.strictEqual( + argKeyExists(map, 'nullKey'), + true, + "Failed to recognize 'nullKey' which has a null value.", + ); + + // Should return false for keys that are not present + assert.strictEqual( + argKeyExists(map, 'undefinedKey'), + false, + "Incorrectly recognized 'undefinedKey' as existing.", + ); + }); + test('Converts array of strings with "=" into a map', () => { + const args = ['key1=value1', 'key2=value2']; + const expectedMap = { key1: 'value1', key2: 'value2' }; + + const resultMap = argsToMap(args); + + assert.deepStrictEqual(resultMap, expectedMap); + }); + + test('Assigns null to keys without "="', () => { + const args = ['key1', 'key2']; + const expectedMap = { key1: null, key2: null }; + + const resultMap = argsToMap(args); + + assert.deepStrictEqual(resultMap, expectedMap); + }); + + test('Handles mixed keys with and without "="', () => { + const args = ['key1=value1', 'key2']; + const expectedMap = { key1: 'value1', key2: null }; + + const resultMap = argsToMap(args); + + assert.deepStrictEqual(resultMap, expectedMap); + }); + + test('Handles strings with multiple "=" characters', () => { + const args = ['key1=part1=part2']; + const expectedMap = { key1: 'part1=part2' }; + + const resultMap = argsToMap(args); + + assert.deepStrictEqual(resultMap, expectedMap); + }); + + test('Returns an empty map for an empty input array', () => { + const args: ReadonlyArray = []; + const expectedMap = {}; + + const resultMap = argsToMap(args); + + assert.deepStrictEqual(resultMap, expectedMap); + }); + }); });