revert symlink changes which switched to using argMap for testing args (microsoft/vscode-python#23148)

eleanorjboyd · web-flow · commit af90bbdd50c1 · 2024-04-01T12:20:15.000-05:00
fixes microsoft/vscode-python#22999 reverts some changes made here: microsoft/vscode-python#22885
diff --git a/extensions/positron-python/ThirdPartyNotices-Repository.txt b/extensions/positron-python/ThirdPartyNotices-Repository.txt
@@ -17,7 +17,6 @@ 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
@@ -1033,31 +1032,3 @@ 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/extensions/positron-python/src/client/testing/testController/common/utils.ts b/extensions/positron-python/src/client/testing/testController/common/utils.ts
@@ -3,7 +3,7 @@
 import * as net from 'net';
 import * as path from 'path';
 import { CancellationToken, Position, TestController, TestItem, Uri, Range } from 'vscode';
-import { traceError, traceLog, traceVerbose } from '../../../logging';
+import { traceError, traceInfo, traceLog, traceVerbose } from '../../../logging';
 
 import { EnableTestAdapterRewrite } from '../../../common/experiments/groups';
 import { IExperimentService } from '../../../common/types';
@@ -351,103 +351,39 @@ export function splitTestNameWithRegex(testName: string): [string, string] {
 }
 
 /**
- * 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.
+ * Takes a list of arguments and adds an key-value pair to the list if the key doesn't already exist. Searches each element
+ * in the array for the key to see if it is contained within the element.
+ * @param args list of arguments to search
+ * @param argToAdd argument to add if it doesn't already exist
+ * @returns the list of arguments with the key-value pair added if it didn't already exist
  */
-export const argsToMap = (args: ReadonlyArray<string>): { [key: string]: Array<string> | null | undefined } => {
-    const map: { [key: string]: Array<string> | null } = {};
+export function addValueIfKeyNotExist(args: string[], key: string, value: string | null): string[] {
     for (const arg of args) {
-        const delimiter = arg.indexOf('=');
-        if (delimiter === -1) {
-            // If no delimiter is found, the entire string becomes a key with a value of null.
-            map[arg] = null;
-        } else {
-            const key = arg.slice(0, delimiter);
-            const value = arg.slice(delimiter + 1);
-            if (map[key]) {
-                // add to the array
-                const arr = map[key] as string[];
-                arr.push(value);
-                map[key] = arr;
-            } else {
-                // create a new array
-                map[key] = [value];
-            }
+        if (arg.includes(key)) {
+            traceInfo(`arg: ${key} already exists in args, not adding.`);
+            return args;
         }
     }
-
-    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]: Array<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;
-        }
-        if (value === null) {
-            out.push(key);
-        } else {
-            const values = Array.isArray(value) ? (value as string[]) : [value];
-            for (const v of values) {
-                out.push(`${key}=${v}`);
-            }
-        }
+    if (value) {
+        args.push(`${key}=${value}`);
+    } else {
+        args.push(`${key}`);
     }
-
-    return out;
-};
+    return args;
+}
 
 /**
- * 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.
+ * Checks if a key exists in a list of arguments. Searches each element in the array
+ *  for the key to see if it is contained within the element.
+ * @param args list of arguments to search
+ * @param key string to search for
+ * @returns true if the key exists in the list of arguments, false otherwise
  */
-export function addArgIfNotExist(
-    map: { [key: string]: Array<string> | null | undefined },
-    argKey: string,
-    argValue: string | null,
-): { [key: string]: Array<string> | null | undefined } {
-    // Only add the argument if it doesn't exist in the map.
-    if (map[argKey] === undefined) {
-        // if null then set to null, otherwise set to an array with the value
-        if (argValue === null) {
-            map[argKey] = null;
-        } else {
-            map[argKey] = [argValue];
+export function argKeyExists(args: string[], key: string): boolean {
+    for (const arg of args) {
+        if (arg.includes(key)) {
+            return true;
         }
     }
-
-    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]: Array<string> | null | undefined }, argKey: string): boolean {
-    return map[argKey] !== undefined;
+    return false;
 }
diff --git a/extensions/positron-python/src/client/testing/testController/pytest/pytestDiscoveryAdapter.ts b/extensions/positron-python/src/client/testing/testController/pytest/pytestDiscoveryAdapter.ts
@@ -25,9 +25,7 @@ import {
     createEOTPayload,
     createTestingDeferred,
     fixLogLinesNoTrailing,
-    argsToMap,
-    addArgIfNotExist,
-    mapToArgs,
+    addValueIfKeyNotExist,
 } from '../common/utils';
 import { IEnvironmentVariablesProvider } from '../../../common/variables/types';
 
@@ -70,16 +68,14 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
         const relativePathToPytest = 'python_files';
         const fullPluginPath = path.join(EXTENSION_ROOT_DIR, relativePathToPytest);
         const settings = this.configSettings.getSettings(uri);
-        let pytestArgsMap = argsToMap(settings.testing.pytestArgs);
+        let { pytestArgs } = settings.testing;
         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);
+            traceWarn("The cwd is a symbolic link, adding '--rootdir' to pytestArgs only if it doesn't already exist.");
+            pytestArgs = addValueIfKeyNotExist(pytestArgs, '--rootdir', cwd);
         }
 
         // get and edit env vars
@@ -111,7 +107,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(mapToArgs(pytestArgsMap));
+        const execArgs = ['-m', 'pytest', '-p', 'vscode_pytest', '--collect-only'].concat(pytestArgs);
         traceVerbose(`Running pytest discovery with command: ${execArgs.join(' ')} for workspace ${uri.fsPath}.`);
 
         const deferredTillExecClose: Deferred<void> = createTestingDeferred();
diff --git a/extensions/positron-python/src/client/testing/testController/pytest/pytestExecutionAdapter.ts b/extensions/positron-python/src/client/testing/testController/pytest/pytestExecutionAdapter.ts
@@ -128,17 +128,16 @@ export class PytestTestExecutionAdapter implements ITestExecutionAdapter {
         const execService = await executionFactory?.createActivatedEnvironment(creationOptions);
         try {
             // Remove positional test folders and files, we will add as needed per node
-            const testArgs = removePositionalFoldersAndFiles(pytestArgs);
-            let testArgsMap = utils.argsToMap(testArgs);
+            let testArgs = removePositionalFoldersAndFiles(pytestArgs);
 
             // if user has provided `--rootdir` then use that, otherwise add `cwd`
             // root dir is required so pytest can find the relative paths and for symlinks
-            utils.addArgIfNotExist(testArgsMap, '--rootdir', cwd);
+            utils.addValueIfKeyNotExist(testArgs, '--rootdir', cwd);
 
             // -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');
+            if (debugBool && !utils.argKeyExists(testArgs, '-s')) {
+                testArgs = utils.addValueIfKeyNotExist(testArgs, '--capture', 'no');
             }
 
             // add port with run test ids to env vars
@@ -163,18 +162,14 @@ export class PytestTestExecutionAdapter implements ITestExecutionAdapter {
                 const pytestUUID = uuid.toString();
                 const launchOptions: LaunchOptions = {
                     cwd,
-                    args: utils.mapToArgs(testArgsMap),
+                    args: testArgs,
                     token: spawnOptions.token,
                     testProvider: PYTEST_PROVIDER,
                     pytestPort,
                     pytestUUID,
                     runTestIdsPort: pytestRunTestIdsPort.toString(),
                 };
-                traceInfo(
-                    `Running DEBUG pytest with arguments: ${utils.mapToArgs(testArgsMap).join(' ')} for workspace ${
-                        uri.fsPath
-                    } \r\n`,
-                );
+                traceInfo(`Running DEBUG pytest with arguments: ${testArgs} for workspace ${uri.fsPath} \r\n`);
                 await debugLauncher!.launchDebugger(launchOptions, () => {
                     deferredTillEOT?.resolve();
                 });
@@ -183,7 +178,7 @@ export class PytestTestExecutionAdapter implements ITestExecutionAdapter {
                 const deferredTillExecClose: Deferred<void> = utils.createTestingDeferred();
                 // combine path to run script with run args
                 const scriptPath = path.join(fullPluginPath, 'vscode_pytest', 'run_pytest_script.py');
-                const runArgs = [scriptPath, ...utils.mapToArgs(testArgsMap)];
+                const runArgs = [scriptPath, ...testArgs];
                 traceInfo(`Running pytest with arguments: ${runArgs.join(' ')} for workspace ${uri.fsPath} \r\n`);
 
                 let resultProc: ChildProcess | undefined;
diff --git a/extensions/positron-python/src/test/testing/common/testingAdapter.test.ts b/extensions/positron-python/src/test/testing/common/testingAdapter.test.ts
@@ -72,13 +72,17 @@ suite('End to End Tests: test adapters', () => {
         // 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.');
-            }
-        });
+        try {
+            fs.symlink(target, dest, 'dir', (err) => {
+                if (err) {
+                    console.error(err);
+                } else {
+                    console.log('Symlink created successfully for end to end tests.');
+                }
+            });
+        } catch (err) {
+            console.error(err);
+        }
     });
 
     setup(async () => {
@@ -117,13 +121,17 @@ suite('End to End Tests: test adapters', () => {
     suiteTeardown(async () => {
         // remove symlink
         const dest = rootPathDiscoverySymlink;
-        fs.unlink(dest, (err) => {
-            if (err) {
-                console.error(err);
-            } else {
-                console.log('Symlink removed successfully after tests.');
-            }
-        });
+        if (fs.existsSync(dest)) {
+            fs.unlink(dest, (err) => {
+                if (err) {
+                    console.error(err);
+                } else {
+                    console.log('Symlink removed successfully after tests.');
+                }
+            });
+        } else {
+            console.log('Symlink was not found to remove after tests, exiting successfully');
+        }
     });
     test('unittest discovery adapter small workspace', async () => {
         // result resolver and saved data for assertions
@@ -293,6 +301,7 @@ suite('End to End Tests: test adapters', () => {
             resultResolver,
             envVarsService,
         );
+        configService.getSettings(workspaceUri).testing.pytestArgs = [];
 
         await discoveryAdapter.discoverTests(workspaceUri, pythonExecFactory).finally(() => {
             // verification after discovery is complete
@@ -372,6 +381,7 @@ suite('End to End Tests: test adapters', () => {
 
         // set workspace to test workspace folder
         workspaceUri = Uri.parse(rootPathLargeWorkspace);
+        configService.getSettings(workspaceUri).testing.pytestArgs = [];
 
         await discoveryAdapter.discoverTests(workspaceUri, pythonExecFactory).finally(() => {
             // verification after discovery is complete
@@ -558,6 +568,7 @@ suite('End to End Tests: test adapters', () => {
         };
         // set workspace to test workspace folder
         workspaceUri = Uri.parse(rootPathSmallWorkspace);
+        configService.getSettings(workspaceUri).testing.pytestArgs = [];
 
         // run pytest execution
         const executionAdapter = new PytestTestExecutionAdapter(
@@ -648,6 +659,7 @@ suite('End to End Tests: test adapters', () => {
 
         // set workspace to test workspace folder
         workspaceUri = Uri.parse(rootPathLargeWorkspace);
+        configService.getSettings(workspaceUri).testing.pytestArgs = [];
 
         // generate list of test_ids
         const testIds: string[] = [];
@@ -728,6 +740,7 @@ suite('End to End Tests: test adapters', () => {
 
         // set workspace to test workspace folder
         workspaceUri = Uri.parse(rootPathDiscoveryErrorWorkspace);
+        configService.getSettings(workspaceUri).testing.unittestArgs = ['-s', '.', '-p', '*test*.py'];
 
         const discoveryAdapter = new UnittestTestDiscoveryAdapter(
             pythonTestServer,
@@ -799,6 +812,8 @@ suite('End to End Tests: test adapters', () => {
 
         // set workspace to test workspace folder
         workspaceUri = Uri.parse(rootPathDiscoveryErrorWorkspace);
+        configService.getSettings(workspaceUri).testing.pytestArgs = [];
+
         await discoveryAdapter.discoverTests(workspaceUri, pythonExecFactory).finally(() => {
             // verification after discovery is complete
             assert.ok(
@@ -860,6 +875,7 @@ suite('End to End Tests: test adapters', () => {
 
         // set workspace to test workspace folder
         workspaceUri = Uri.parse(rootPathErrorWorkspace);
+        configService.getSettings(workspaceUri).testing.unittestArgs = ['-s', '.', '-p', '*test*.py'];
 
         // run pytest execution
         const executionAdapter = new UnittestTestExecutionAdapter(
@@ -921,6 +937,7 @@ suite('End to End Tests: test adapters', () => {
 
         // set workspace to test workspace folder
         workspaceUri = Uri.parse(rootPathErrorWorkspace);
+        configService.getSettings(workspaceUri).testing.pytestArgs = [];
 
         // run pytest execution
         const executionAdapter = new PytestTestExecutionAdapter(
diff --git a/extensions/positron-python/src/test/testing/testController/utils.unit.test.ts b/extensions/positron-python/src/test/testing/testController/utils.unit.test.ts
