Skip to content

Commit c5a383b

Browse files
eleanorjboydeleanorjboyd
authored
revert symlink changes which switched to using argMap for testing args (#23148)
fixes #22999 reverts some changes made here: #22885
1 parent d8a5c46 commit c5a383b

File tree

6 files changed

+102
-327
lines changed

6 files changed

+102
-327
lines changed

‎ThirdPartyNotices-Repository.txt

Lines changed: 0 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ Microsoft Python extension for Visual Studio Code incorporates third party mater
1717
11. vscode-cpptools (https://github.com/microsoft/vscode-cpptools)
1818
12. mocha (https://github.com/mochajs/mocha)
1919
13. get-pip (https://github.com/pypa/get-pip)
20-
14. vscode-js-debug (https://github.com/microsoft/vscode-js-debug)
2120

2221
%%
2322
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.
10331032

10341033
=========================================
10351034
END OF get-pip NOTICES, INFORMATION, AND LICENSE
1036-
1037-
1038-
%% vscode-js-debug NOTICES, INFORMATION, AND LICENSE BEGIN HERE
1039-
=========================================
1040-
1041-
MIT License
1042-
1043-
Copyright (c) Microsoft Corporation. All rights reserved.
1044-
1045-
Permission is hereby granted, free of charge, to any person obtaining a copy
1046-
of this software and associated documentation files (the "Software"), to deal
1047-
in the Software without restriction, including without limitation the rights
1048-
to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
1049-
copies of the Software, and to permit persons to whom the Software is
1050-
furnished to do so, subject to the following conditions:
1051-
1052-
The above copyright notice and this permission notice shall be included in all
1053-
copies or substantial portions of the Software.
1054-
1055-
THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
1056-
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
1057-
FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
1058-
AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
1059-
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
1060-
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
1061-
SOFTWARE
1062-
=========================================
1063-
END OF vscode-js-debug NOTICES, INFORMATION, AND LICENSE

‎src/client/testing/testController/common/utils.ts

Lines changed: 26 additions & 90 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
import * as net from 'net';
44
import * as path from 'path';
55
import { CancellationToken, Position, TestController, TestItem, Uri, Range } from 'vscode';
6-
import { traceError, traceLog, traceVerbose } from '../../../logging';
6+
import { traceError, traceInfo, traceLog, traceVerbose } from '../../../logging';
77

88
import { EnableTestAdapterRewrite } from '../../../common/experiments/groups';
99
import { IExperimentService } from '../../../common/types';
@@ -351,103 +351,39 @@ export function splitTestNameWithRegex(testName: string): [string, string] {
351351
}
352352

353353
/**
354-
* Converts an array of strings (with or without '=') into a map.
355-
* If a string contains '=', it is split into a key-value pair, with the portion
356-
* before the '=' as the key and the portion after the '=' as the value.
357-
* If no '=' is found in the string, the entire string becomes a key with a value of null.
358-
*
359-
* @param args - Readonly array of strings to be converted to a map.
360-
* @returns A map representation of the input strings.
354+
* Takes a list of arguments and adds an key-value pair to the list if the key doesn't already exist. Searches each element
355+
* in the array for the key to see if it is contained within the element.
356+
* @param args list of arguments to search
357+
* @param argToAdd argument to add if it doesn't already exist
358+
* @returns the list of arguments with the key-value pair added if it didn't already exist
361359
*/
362-
export const argsToMap = (args: ReadonlyArray<string>): { [key: string]: Array<string> | null | undefined } => {
363-
const map: { [key: string]: Array<string> | null } = {};
360+
export function addValueIfKeyNotExist(args: string[], key: string, value: string | null): string[] {
364361
for (const arg of args) {
365-
const delimiter = arg.indexOf('=');
366-
if (delimiter === -1) {
367-
// If no delimiter is found, the entire string becomes a key with a value of null.
368-
map[arg] = null;
369-
} else {
370-
const key = arg.slice(0, delimiter);
371-
const value = arg.slice(delimiter + 1);
372-
if (map[key]) {
373-
// add to the array
374-
const arr = map[key] as string[];
375-
arr.push(value);
376-
map[key] = arr;
377-
} else {
378-
// create a new array
379-
map[key] = [value];
380-
}
362+
if (arg.includes(key)) {
363+
traceInfo(`arg: ${key} already exists in args, not adding.`);
364+
return args;
381365
}
382366
}
383-
384-
return map;
385-
};
386-
387-
/**
388-
* Converts a map into an array of strings.
389-
* Each key-value pair in the map is transformed into a string.
390-
* If the value is null, only the key is represented in the string.
391-
* If the value is defined (and not null), the string is in the format "key=value".
392-
* If a value is undefined, the key-value pair is skipped.
393-
*
394-
* @param map - The map to be converted to an array of strings.
395-
* @returns An array of strings representation of the input map.
396-
*/
397-
export const mapToArgs = (map: { [key: string]: Array<string> | null | undefined }): string[] => {
398-
const out: string[] = [];
399-
for (const key of Object.keys(map)) {
400-
const value = map[key];
401-
if (value === undefined) {
402-
// eslint-disable-next-line no-continue
403-
continue;
404-
}
405-
if (value === null) {
406-
out.push(key);
407-
} else {
408-
const values = Array.isArray(value) ? (value as string[]) : [value];
409-
for (const v of values) {
410-
out.push(`${key}=${v}`);
411-
}
412-
}
367+
if (value) {
368+
args.push(`${key}=${value}`);
369+
} else {
370+
args.push(`${key}`);
413371
}
414-
415-
return out;
416-
};
372+
return args;
373+
}
417374

418375
/**
419-
* Adds an argument to the map only if it doesn't already exist.
420-
*
421-
* @param map - The map of arguments.
422-
* @param argKey - The argument key to be checked and added.
423-
* @param argValue - The value to set for the argument if it's not already in the map.
424-
* @returns The updated map.
376+
* Checks if a key exists in a list of arguments. Searches each element in the array
377+
* for the key to see if it is contained within the element.
378+
* @param args list of arguments to search
379+
* @param key string to search for
380+
* @returns true if the key exists in the list of arguments, false otherwise
425381
*/
426-
export function addArgIfNotExist(
427-
map: { [key: string]: Array<string> | null | undefined },
428-
argKey: string,
429-
argValue: string | null,
430-
): { [key: string]: Array<string> | null | undefined } {
431-
// Only add the argument if it doesn't exist in the map.
432-
if (map[argKey] === undefined) {
433-
// if null then set to null, otherwise set to an array with the value
434-
if (argValue === null) {
435-
map[argKey] = null;
436-
} else {
437-
map[argKey] = [argValue];
382+
export function argKeyExists(args: string[], key: string): boolean {
383+
for (const arg of args) {
384+
if (arg.includes(key)) {
385+
return true;
438386
}
439387
}
440-
441-
return map;
442-
}
443-
444-
/**
445-
* Checks if an argument key exists in the map.
446-
*
447-
* @param map - The map of arguments.
448-
* @param argKey - The argument key to be checked.
449-
* @returns True if the argument key exists in the map, false otherwise.
450-
*/
451-
export function argKeyExists(map: { [key: string]: Array<string> | null | undefined }, argKey: string): boolean {
452-
return map[argKey] !== undefined;
388+
return false;
453389
}

‎src/client/testing/testController/pytest/pytestDiscoveryAdapter.ts

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -25,9 +25,7 @@ import {
2525
createEOTPayload,
2626
createTestingDeferred,
2727
fixLogLinesNoTrailing,
28-
argsToMap,
29-
addArgIfNotExist,
30-
mapToArgs,
28+
addValueIfKeyNotExist,
3129
} from '../common/utils';
3230
import { IEnvironmentVariablesProvider } from '../../../common/variables/types';
3331

@@ -70,16 +68,14 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
7068
const relativePathToPytest = 'python_files';
7169
const fullPluginPath = path.join(EXTENSION_ROOT_DIR, relativePathToPytest);
7270
const settings = this.configSettings.getSettings(uri);
73-
let pytestArgsMap = argsToMap(settings.testing.pytestArgs);
71+
let { pytestArgs } = settings.testing;
7472
const cwd = settings.testing.cwd && settings.testing.cwd.length > 0 ? settings.testing.cwd : uri.fsPath;
7573

7674
// check for symbolic path
7775
const stats = fs.lstatSync(cwd);
7876
if (stats.isSymbolicLink()) {
79-
traceWarn(
80-
"The cwd is a symbolic link, adding '--rootdir' to pytestArgsMap only if it doesn't already exist.",
81-
);
82-
pytestArgsMap = addArgIfNotExist(pytestArgsMap, '--rootdir', cwd);
77+
traceWarn("The cwd is a symbolic link, adding '--rootdir' to pytestArgs only if it doesn't already exist.");
78+
pytestArgs = addValueIfKeyNotExist(pytestArgs, '--rootdir', cwd);
8379
}
8480

8581
// get and edit env vars
@@ -111,7 +107,7 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
111107
};
112108
const execService = await executionFactory?.createActivatedEnvironment(creationOptions);
113109
// delete UUID following entire discovery finishing.
114-
const execArgs = ['-m', 'pytest', '-p', 'vscode_pytest', '--collect-only'].concat(mapToArgs(pytestArgsMap));
110+
const execArgs = ['-m', 'pytest', '-p', 'vscode_pytest', '--collect-only'].concat(pytestArgs);
115111
traceVerbose(`Running pytest discovery with command: ${execArgs.join(' ')} for workspace ${uri.fsPath}.`);
116112

117113
const deferredTillExecClose: Deferred<void> = createTestingDeferred();

‎src/client/testing/testController/pytest/pytestExecutionAdapter.ts

Lines changed: 7 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -128,17 +128,16 @@ export class PytestTestExecutionAdapter implements ITestExecutionAdapter {
128128
const execService = await executionFactory?.createActivatedEnvironment(creationOptions);
129129
try {
130130
// Remove positional test folders and files, we will add as needed per node
131-
const testArgs = removePositionalFoldersAndFiles(pytestArgs);
132-
let testArgsMap = utils.argsToMap(testArgs);
131+
let testArgs = removePositionalFoldersAndFiles(pytestArgs);
133132

134133
// if user has provided `--rootdir` then use that, otherwise add `cwd`
135134
// root dir is required so pytest can find the relative paths and for symlinks
136-
utils.addArgIfNotExist(testArgsMap, '--rootdir', cwd);
135+
utils.addValueIfKeyNotExist(testArgs, '--rootdir', cwd);
137136

138137
// -s and --capture are both command line options that control how pytest captures output.
139138
// if neither are set, then set --capture=no to prevent pytest from capturing output.
140-
if (debugBool && !utils.argKeyExists(testArgsMap, '-s')) {
141-
testArgsMap = utils.addArgIfNotExist(testArgsMap, '--capture', 'no');
139+
if (debugBool && !utils.argKeyExists(testArgs, '-s')) {
140+
testArgs = utils.addValueIfKeyNotExist(testArgs, '--capture', 'no');
142141
}
143142

144143
// add port with run test ids to env vars
@@ -163,18 +162,14 @@ export class PytestTestExecutionAdapter implements ITestExecutionAdapter {
163162
const pytestUUID = uuid.toString();
164163
const launchOptions: LaunchOptions = {
165164
cwd,
166-
args: utils.mapToArgs(testArgsMap),
165+
args: testArgs,
167166
token: spawnOptions.token,
168167
testProvider: PYTEST_PROVIDER,
169168
pytestPort,
170169
pytestUUID,
171170
runTestIdsPort: pytestRunTestIdsPort.toString(),
172171
};
173-
traceInfo(
174-
`Running DEBUG pytest with arguments: ${utils.mapToArgs(testArgsMap).join(' ')} for workspace ${
175-
uri.fsPath
176-
} \r\n`,
177-
);
172+
traceInfo(`Running DEBUG pytest with arguments: ${testArgs} for workspace ${uri.fsPath} \r\n`);
178173
await debugLauncher!.launchDebugger(launchOptions, () => {
179174
deferredTillEOT?.resolve();
180175
});
@@ -183,7 +178,7 @@ export class PytestTestExecutionAdapter implements ITestExecutionAdapter {
183178
const deferredTillExecClose: Deferred<void> = utils.createTestingDeferred();
184179
// combine path to run script with run args
185180
const scriptPath = path.join(fullPluginPath, 'vscode_pytest', 'run_pytest_script.py');
186-
const runArgs = [scriptPath, ...utils.mapToArgs(testArgsMap)];
181+
const runArgs = [scriptPath, ...testArgs];
187182
traceInfo(`Running pytest with arguments: ${runArgs.join(' ')} for workspace ${uri.fsPath} \r\n`);
188183

189184
let resultProc: ChildProcess | undefined;

‎src/test/testing/common/testingAdapter.test.ts

Lines changed: 31 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -72,13 +72,17 @@ suite('End to End Tests: test adapters', () => {
7272
// create symlink for specific symlink test
7373
const target = rootPathSmallWorkspace;
7474
const dest = rootPathDiscoverySymlink;
75-
fs.symlink(target, dest, 'dir', (err) => {
76-
if (err) {
77-
console.error(err);
78-
} else {
79-
console.log('Symlink created successfully for end to end tests.');
80-
}
81-
});
75+
try {
76+
fs.symlink(target, dest, 'dir', (err) => {
77+
if (err) {
78+
console.error(err);
79+
} else {
80+
console.log('Symlink created successfully for end to end tests.');
81+
}
82+
});
83+
} catch (err) {
84+
console.error(err);
85+
}
8286
});
8387

8488
setup(async () => {
@@ -117,13 +121,17 @@ suite('End to End Tests: test adapters', () => {
117121
suiteTeardown(async () => {
118122
// remove symlink
119123
const dest = rootPathDiscoverySymlink;
120-
fs.unlink(dest, (err) => {
121-
if (err) {
122-
console.error(err);
123-
} else {
124-
console.log('Symlink removed successfully after tests.');
125-
}
126-
});
124+
if (fs.existsSync(dest)) {
125+
fs.unlink(dest, (err) => {
126+
if (err) {
127+
console.error(err);
128+
} else {
129+
console.log('Symlink removed successfully after tests.');
130+
}
131+
});
132+
} else {
133+
console.log('Symlink was not found to remove after tests, exiting successfully');
134+
}
127135
});
128136
test('unittest discovery adapter small workspace', async () => {
129137
// result resolver and saved data for assertions
@@ -293,6 +301,7 @@ suite('End to End Tests: test adapters', () => {
293301
resultResolver,
294302
envVarsService,
295303
);
304+
configService.getSettings(workspaceUri).testing.pytestArgs = [];
296305

297306
await discoveryAdapter.discoverTests(workspaceUri, pythonExecFactory).finally(() => {
298307
// verification after discovery is complete
@@ -372,6 +381,7 @@ suite('End to End Tests: test adapters', () => {
372381

373382
// set workspace to test workspace folder
374383
workspaceUri = Uri.parse(rootPathLargeWorkspace);
384+
configService.getSettings(workspaceUri).testing.pytestArgs = [];
375385

376386
await discoveryAdapter.discoverTests(workspaceUri, pythonExecFactory).finally(() => {
377387
// verification after discovery is complete
@@ -558,6 +568,7 @@ suite('End to End Tests: test adapters', () => {
558568
};
559569
// set workspace to test workspace folder
560570
workspaceUri = Uri.parse(rootPathSmallWorkspace);
571+
configService.getSettings(workspaceUri).testing.pytestArgs = [];
561572

562573
// run pytest execution
563574
const executionAdapter = new PytestTestExecutionAdapter(
@@ -648,6 +659,7 @@ suite('End to End Tests: test adapters', () => {
648659

649660
// set workspace to test workspace folder
650661
workspaceUri = Uri.parse(rootPathLargeWorkspace);
662+
configService.getSettings(workspaceUri).testing.pytestArgs = [];
651663

652664
// generate list of test_ids
653665
const testIds: string[] = [];
@@ -728,6 +740,7 @@ suite('End to End Tests: test adapters', () => {
728740

729741
// set workspace to test workspace folder
730742
workspaceUri = Uri.parse(rootPathDiscoveryErrorWorkspace);
743+
configService.getSettings(workspaceUri).testing.unittestArgs = ['-s', '.', '-p', '*test*.py'];
731744

732745
const discoveryAdapter = new UnittestTestDiscoveryAdapter(
733746
pythonTestServer,
@@ -799,6 +812,8 @@ suite('End to End Tests: test adapters', () => {
799812

800813
// set workspace to test workspace folder
801814
workspaceUri = Uri.parse(rootPathDiscoveryErrorWorkspace);
815+
configService.getSettings(workspaceUri).testing.pytestArgs = [];
816+
802817
await discoveryAdapter.discoverTests(workspaceUri, pythonExecFactory).finally(() => {
803818
// verification after discovery is complete
804819
assert.ok(
@@ -860,6 +875,7 @@ suite('End to End Tests: test adapters', () => {
860875

861876
// set workspace to test workspace folder
862877
workspaceUri = Uri.parse(rootPathErrorWorkspace);
878+
configService.getSettings(workspaceUri).testing.unittestArgs = ['-s', '.', '-p', '*test*.py'];
863879

864880
// run pytest execution
865881
const executionAdapter = new UnittestTestExecutionAdapter(
@@ -921,6 +937,7 @@ suite('End to End Tests: test adapters', () => {
921937

922938
// set workspace to test workspace folder
923939
workspaceUri = Uri.parse(rootPathErrorWorkspace);
940+
configService.getSettings(workspaceUri).testing.pytestArgs = [];
924941

925942
// run pytest execution
926943
const executionAdapter = new PytestTestExecutionAdapter(

0 commit comments

Comments
 (0)