Skip to content

Commit e3ff798

Browse files
eleanorjboydeleanorjboyd
authored
revert arg map change to remove from stable release (#23160)
remove these edits from the release so instead it will start in insiders then be released as a point release
1 parent d5ec67d commit e3ff798

File tree

6 files changed

+327
-102
lines changed

6 files changed

+327
-102
lines changed

‎ThirdPartyNotices-Repository.txt‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ 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)
2021

2122
%%
2223
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.
10321033

10331034
=========================================
10341035
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: 90 additions & 26 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, traceInfo, traceLog, traceVerbose } from '../../../logging';
6+
import { traceError, traceLog, traceVerbose } from '../../../logging';
77

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

353353
/**
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
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.
359361
*/
360-
export function addValueIfKeyNotExist(args: string[], key: string, value: string | null): string[] {
362+
export const argsToMap = (args: ReadonlyArray<string>): { [key: string]: Array<string> | null | undefined } => {
363+
const map: { [key: string]: Array<string> | null } = {};
361364
for (const arg of args) {
362-
if (arg.includes(key)) {
363-
traceInfo(`arg: ${key} already exists in args, not adding.`);
364-
return 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+
}
365381
}
366382
}
367-
if (value) {
368-
args.push(`${key}=${value}`);
369-
} else {
370-
args.push(`${key}`);
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+
}
371413
}
372-
return args;
373-
}
414+
415+
return out;
416+
};
374417

375418
/**
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
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.
381425
*/
382-
export function argKeyExists(args: string[], key: string): boolean {
383-
for (const arg of args) {
384-
if (arg.includes(key)) {
385-
return true;
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];
386438
}
387439
}
388-
return false;
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;
389453
}

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

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

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

7476
// check for symbolic path
7577
const stats = fs.lstatSync(cwd);
7678
if (stats.isSymbolicLink()) {
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);
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);
7983
}
8084

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

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

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

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -128,16 +128,17 @@ 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-
let testArgs = removePositionalFoldersAndFiles(pytestArgs);
131+
const testArgs = removePositionalFoldersAndFiles(pytestArgs);
132+
let testArgsMap = utils.argsToMap(testArgs);
132133

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

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

143144
// add port with run test ids to env vars
@@ -162,14 +163,18 @@ export class PytestTestExecutionAdapter implements ITestExecutionAdapter {
162163
const pytestUUID = uuid.toString();
163164
const launchOptions: LaunchOptions = {
164165
cwd,
165-
args: testArgs,
166+
args: utils.mapToArgs(testArgsMap),
166167
token: spawnOptions.token,
167168
testProvider: PYTEST_PROVIDER,
168169
pytestPort,
169170
pytestUUID,
170171
runTestIdsPort: pytestRunTestIdsPort.toString(),
171172
};
172-
traceInfo(`Running DEBUG pytest with arguments: ${testArgs} for workspace ${uri.fsPath} \r\n`);
173+
traceInfo(
174+
`Running DEBUG pytest with arguments: ${utils.mapToArgs(testArgsMap).join(' ')} for workspace ${
175+
uri.fsPath
176+
} \r\n`,
177+
);
173178
await debugLauncher!.launchDebugger(launchOptions, () => {
174179
deferredTillEOT?.resolve();
175180
});
@@ -178,7 +183,7 @@ export class PytestTestExecutionAdapter implements ITestExecutionAdapter {
178183
const deferredTillExecClose: Deferred<void> = utils.createTestingDeferred();
179184
// combine path to run script with run args
180185
const scriptPath = path.join(fullPluginPath, 'vscode_pytest', 'run_pytest_script.py');
181-
const runArgs = [scriptPath, ...testArgs];
186+
const runArgs = [scriptPath, ...utils.mapToArgs(testArgsMap)];
182187
traceInfo(`Running pytest with arguments: ${runArgs.join(' ')} for workspace ${uri.fsPath} \r\n`);
183188

184189
let resultProc: ChildProcess | undefined;

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

Lines changed: 14 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -72,17 +72,13 @@ suite('End to End Tests: test adapters', () => {
7272
// create symlink for specific symlink test
7373
const target = rootPathSmallWorkspace;
7474
const dest = rootPathDiscoverySymlink;
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-
}
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+
});
8682
});
8783

8884
setup(async () => {
@@ -121,17 +117,13 @@ suite('End to End Tests: test adapters', () => {
121117
suiteTeardown(async () => {
122118
// remove symlink
123119
const dest = rootPathDiscoverySymlink;
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-
}
120+
fs.unlink(dest, (err) => {
121+
if (err) {
122+
console.error(err);
123+
} else {
124+
console.log('Symlink removed successfully after tests.');
125+
}
126+
});
135127
});
136128
test('unittest discovery adapter small workspace', async () => {
137129
// result resolver and saved data for assertions
@@ -301,7 +293,6 @@ suite('End to End Tests: test adapters', () => {
301293
resultResolver,
302294
envVarsService,
303295
);
304-
configService.getSettings(workspaceUri).testing.pytestArgs = [];
305296

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

382373
// set workspace to test workspace folder
383374
workspaceUri = Uri.parse(rootPathLargeWorkspace);
384-
configService.getSettings(workspaceUri).testing.pytestArgs = [];
385375

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

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

660649
// set workspace to test workspace folder
661650
workspaceUri = Uri.parse(rootPathLargeWorkspace);
662-
configService.getSettings(workspaceUri).testing.pytestArgs = [];
663651

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

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

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

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

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

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

938922
// set workspace to test workspace folder
939923
workspaceUri = Uri.parse(rootPathErrorWorkspace);
940-
configService.getSettings(workspaceUri).testing.pytestArgs = [];
941924

942925
// run pytest execution
943926
const executionAdapter = new PytestTestExecutionAdapter(

0 commit comments

Comments
 (0)