Skip to content

Commit

Permalink
Revert arg map handler for test arguments (#23207)
Browse files Browse the repository at this point in the history 
This reverts commit e3ff798 which
restores the original behavior without the argmap and instead keeps all
testing args as a list

Co-authored-by: Eleanor Boyd <[email protected]>
  • Loading branch information
@anthonykim1 @eleanorjboyd
anthonykim1 and eleanorjboyd authored Apr 9, 2024
1 parent 1d7c764 commit 0a935bd
Show file tree
Hide file tree
Showing 6 changed files with 102 additions and 327 deletions.
29 changes: 0 additions & 29 deletions ThirdPartyNotices-Repository.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
116 changes: 26 additions & 90 deletions src/client/testing/testController/common/utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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;
}
14 changes: 5 additions & 9 deletions src/client/testing/testController/pytest/pytestDiscoveryAdapter.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,7 @@ import {
createEOTPayload,
createTestingDeferred,
fixLogLinesNoTrailing,
argsToMap,
addArgIfNotExist,
mapToArgs,
addValueIfKeyNotExist,
} from '../common/utils';
import { IEnvironmentVariablesProvider } from '../../../common/variables/types';

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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();
Expand Down
19 changes: 7 additions & 12 deletions src/client/testing/testController/pytest/pytestExecutionAdapter.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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();
});
Expand All @@ -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;
Expand Down
45 changes: 31 additions & 14 deletions src/test/testing/common/testingAdapter.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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[] = [];
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down
Loading

0 comments on commit 0a935bd

Please sign in to comment.