Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix regex split for subtest names #22107

Merged
merged 2 commits into from
Sep 28, 2023
Merged

fix regex split for subtest names #22107

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
610507c
fix regex split for subtest names
eleanorjboyd Sep 28, 2023
0468200
fixing commenting
eleanorjboyd Sep 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 9 additions & 10 deletions src/client/testing/testController/common/resultResolver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { clearAllChildren, createErrorTestItem, getTestCaseNodes } from './testI
import { sendTelemetryEvent } from '../../../telemetry';
import { EventName } from '../../../telemetry/constants';
import { splitLines } from '../../../common/stringUtils';
import { buildErrorNodeOptions, fixLogLines, populateTestTree } from './utils';
import { buildErrorNodeOptions, fixLogLines, populateTestTree, splitTestNameWithRegex } from './utils';
import { Deferred } from '../../../common/utils/async';

export class PythonResultResolver implements ITestResultResolver {
Expand Down Expand Up @@ -216,9 +216,8 @@ export class PythonResultResolver implements ITestResultResolver {
});
}
} else if (rawTestExecData.result[keyTemp].outcome === 'subtest-failure') {
// split on " " since the subtest ID has the parent test ID in the first part of the ID.
const parentTestCaseId = keyTemp.split(' ')[0];
const subtestId = keyTemp.split(' ')[1];
// split on [] or () based on how the subtest is setup.
const [parentTestCaseId, subtestId] = splitTestNameWithRegex(keyTemp);
const parentTestItem = this.runIdToTestItem.get(parentTestCaseId);
const data = rawTestExecData.result[keyTemp];
// find the subtest's parent test item
Expand All @@ -227,7 +226,10 @@ export class PythonResultResolver implements ITestResultResolver {
if (subtestStats) {
subtestStats.failed += 1;
} else {
this.subTestStats.set(parentTestCaseId, { failed: 1, passed: 0 });
this.subTestStats.set(parentTestCaseId, {
failed: 1,
passed: 0,
});
runInstance.appendOutput(fixLogLines(`${parentTestCaseId} [subtests]:\r\n`));
// clear since subtest items don't persist between runs
clearAllChildren(parentTestItem);
Expand All @@ -253,11 +255,8 @@ export class PythonResultResolver implements ITestResultResolver {
throw new Error('Parent test item not found');
}
} else if (rawTestExecData.result[keyTemp].outcome === 'subtest-success') {
// split only on first " [" since the subtest ID has the parent test ID in the first part of the ID.
const index = keyTemp.indexOf(' [');
const parentTestCaseId = keyTemp.substring(0, index);
// add one to index to remove the space from the start of the subtest ID
const subtestId = keyTemp.substring(index + 1, keyTemp.length);
// split on [] or () based on how the subtest is setup.
const [parentTestCaseId, subtestId] = splitTestNameWithRegex(keyTemp);
const parentTestItem = this.runIdToTestItem.get(parentTestCaseId);

// find the subtest's parent test item
Expand Down
17 changes: 17 additions & 0 deletions src/client/testing/testController/common/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,3 +320,20 @@ export function createEOTPayload(executionBool: boolean): EOTTestPayload {
eot: true,
} as EOTTestPayload;
}

/**
* Splits a test name into its parent test name and subtest unique section.
*
* @param testName The full test name string.
* @returns A tuple where the first item is the parent test name and the second item is the subtest section or `testName` if no subtest section exists.
*/
export function splitTestNameWithRegex(testName: string): [string, string] {
// If a match is found, return the parent test name and the subtest (whichever was captured between parenthesis or square brackets).
// Otherwise, return the entire testName for the parent and entire testName for the subtest.
const regex = /^(.*?) ([\[(].*[\])])$/;
const match = testName.match(regex);
if (match) {
return [match[1].trim(), match[2] || match[3] || testName];
}
return [testName, testName];
}
56 changes: 56 additions & 0 deletions src/test/testing/testController/utils.unit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
JSONRPC_UUID_HEADER,
ExtractJsonRPCData,
parseJsonRPCHeadersAndData,
splitTestNameWithRegex,
} from '../../../client/testing/testController/common/utils';

suite('Test Controller Utils: JSON RPC', () => {
Expand Down Expand Up @@ -65,3 +66,58 @@ suite('Test Controller Utils: JSON RPC', () => {
assert.deepStrictEqual(rpcContent.remainingRawData, rawDataString);
});
});

suite('Test Controller Utils: Other', () => {
interface TestCase {
name: string;
input: string;
expectedParent: string;
expectedSubtest: string;
}

const testCases: Array<TestCase> = [
{
name: 'Single parameter, named',
input: 'test_package.ClassName.test_method (param=value)',
expectedParent: 'test_package.ClassName.test_method',
expectedSubtest: '(param=value)',
},
{
name: 'Single parameter, unnamed',
input: 'test_package.ClassName.test_method [value]',
expectedParent: 'test_package.ClassName.test_method',
expectedSubtest: '[value]',
},
{
name: 'Multiple parameters, named',
input: 'test_package.ClassName.test_method (param1=value1, param2=value2)',
expectedParent: 'test_package.ClassName.test_method',
expectedSubtest: '(param1=value1, param2=value2)',
},
{
name: 'Multiple parameters, unnamed',
input: 'test_package.ClassName.test_method [value1, value2]',
Copy link

@alisonatwork alisonatwork Oct 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @eleanorjboyd and @rzhao271 / @roblourens. I already commented on #21733, but I didn't realize you were parsing the unittest output directly and therefore have to deal with whatever extra brackets it is inserting. In that case, this test is correct but also misleading, because you can't do with self.subTest(value1, value2) - that will result in a Python TypeError, on 3.10 at least.

As per the subTest documentation (https://docs.python.org/3/library/unittest.html#unittest.TestCase.subTest), the only choices are to pass a string (msg) or to pass kwargs (**params) or to pass both. I just tested now and if you pass both a string and a kwargs, then it will print both the string in square brackets and the kwargs in round brackets, so your current regex may not handle that case.

It might be useful to have an end-to-end test for this stuff, i.e. first create some real-world Python unit tests that are run with the targeted versions of Python, then use the output of those as the input for the VS Code parser. This way you can be sure that if the Python default test runner changes its output format in a future version, it won't mysteriously break the VS Code parsing which was expecting something different. Alternatively - and I am not sure how hard this would be to do - VS Code could have its own test runner, which should allow access to the internal data structures that Python unittest is using, and then you could create a nice JSON structure to enable all kinds of cool front end features (e.g. filter by individual kwarg).

expectedParent: 'test_package.ClassName.test_method',
expectedSubtest: '[value1, value2]',
},
{
name: 'Names with special characters',
input: 'test_package.ClassName.test_method (param1=value/1, param2=value+2)',
expectedParent: 'test_package.ClassName.test_method',
expectedSubtest: '(param1=value/1, param2=value+2)',
},
{
name: 'Names with spaces',
input: 'test_package.ClassName.test_method ["a b c d"]',
expectedParent: 'test_package.ClassName.test_method',
expectedSubtest: '["a b c d"]',
},
];

testCases.forEach((testCase) => {
test(`splitTestNameWithRegex: ${testCase.name}`, () => {
const splitResult = splitTestNameWithRegex(testCase.input);
assert.deepStrictEqual(splitResult, [testCase.expectedParent, testCase.expectedSubtest]);
});
});
});
Loading