Skip to content

Commit 1bd9612

Browse files
testing: refine compact discovery payload review feedback (#26000)
Follow-up to #25982 addressing review feedback. ## What changed - **Shared `isAbsolutePath` helper.** Moved the cross-platform absolute-path check out of `testDiscoveryHandler.ts` and into `src/client/common/platform/fs-paths.ts` so it can be reused. The implementation now delegates to `path.posix.isAbsolute || path.win32.isAbsolute` instead of a hand-rolled regex. This is the right behavior here because the paths come from a Python subprocess and may be formatted for an OS different from the one currently running the extension. - **Tightened `create_class_node` annotation.** Replaced `Any` with `pytest.Class | DescribeBlockType` in `python_files/vscode_pytest/__init__.py`. `DescribeBlockType` is imported inside the existing `if TYPE_CHECKING:` block so it stays optional at runtime (matching the runtime `DescribeBlock: Any = None` fallback when `pytest_describe` is not installed). - **Unit tests for `isAbsolutePath`.** Added a 4-test suite in `fs-paths.unit.test.ts` covering POSIX absolute, Windows drive-letter, Windows UNC, and relative paths. ## Verification - `tsc --noEmit - cleanp .` - `eslint` on touched cleanfiles - `ruff check vscode_ cleanpytest/` - `pyright python_files/vscode_pytest/__init__. 0 errorspy` - 4 new `isAbsolutePath` unit passtests - All 17 `TestDiscoveryHandler` unit still passtests - 3 `compact_discovery_payload` pytest passtests ## Notes for reviewers No behavior change is intended. The new `isAbsolutePath` is functionally equivalent to the regex it replaces (both recognize `/foo`, `C:\foo`, `c:/foo`, and `\\server\share`). The `create_class_node` signature change is purely a type-check improvement and has no runtime effect. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent a478402 commit 1bd9612

4 files changed

Lines changed: 42 additions & 6 deletions

File tree

python_files/vscode_pytest/__init__.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525

2626
if TYPE_CHECKING:
2727
from pluggy import Result
28+
from pytest_describe.plugin import DescribeBlock as DescribeBlockType
2829
from typing_extensions import NotRequired
2930

3031
USES_PYTEST_DESCRIBE = False
@@ -873,7 +874,7 @@ def create_session_node(session: pytest.Session) -> TestNode:
873874
}
874875

875876

876-
def create_class_node(class_module: Any) -> TestNode:
877+
def create_class_node(class_module: pytest.Class | DescribeBlockType) -> TestNode:
877878
"""Creates a class node from a pytest class object.
878879
879880
Keyword arguments:

src/client/common/platform/fs-paths.ts

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,19 @@ export function normCasePath(filePath: string): string {
148148
return normCase(nodepath.normalize(filePath));
149149
}
150150

151+
/**
152+
* Returns true if the given path is absolute on either POSIX or Windows.
153+
*
154+
* Node's `path.isAbsolute` is platform-dependent, so a POSIX path like `/foo`
155+
* is not recognized as absolute on Windows, and a Windows path like `C:\foo`
156+
* is not recognized as absolute on POSIX. Use this helper when the path could
157+
* have been produced on a different OS than the one currently running (e.g.
158+
* paths received in a JSON payload from a Python subprocess).
159+
*/
160+
export function isAbsolutePath(value: string): boolean {
161+
return nodepath.posix.isAbsolute(value) || nodepath.win32.isAbsolute(value);
162+
}
163+
151164
export function normCase(s: string): string {
152165
return getOSType() === OSType.Windows ? s.toUpperCase() : s;
153166
}

src/client/testing/testController/common/testDiscoveryHandler.ts

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,7 @@ import { createErrorTestItem } from './testItemUtilities';
1111
import { buildErrorNodeOptions, populateTestTree } from './utils';
1212
import { TestItemIndex } from './testItemIndex';
1313
import { PROJECT_ID_SEPARATOR } from './projectUtils';
14-
15-
function isAbsolutePath(value: string): boolean {
16-
return /^([a-zA-Z]:[\\/]|\\\\)/.test(value) || value.startsWith('/');
17-
}
14+
import { isAbsolutePath } from '../../../common/platform/fs-paths';
1815

1916
function joinWithBase(base: string, relativePath: string): string {
2017
if (!base || isAbsolutePath(relativePath)) {

src/test/common/platform/fs-paths.unit.test.ts

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
import { expect } from 'chai';
55
import * as path from 'path';
66
import * as TypeMoq from 'typemoq';
7-
import { FileSystemPathUtils } from '../../../client/common/platform/fs-paths';
7+
import { FileSystemPathUtils, isAbsolutePath } from '../../../client/common/platform/fs-paths';
88
import { getNamesAndValues } from '../../../client/common/utils/enum';
99
import { OSType } from '../../../client/common/utils/platform';
1010

@@ -112,3 +112,28 @@ suite('FileSystem - Path Utils', () => {
112112
});
113113
});
114114
});
115+
116+
suite('FileSystem - isAbsolutePath', () => {
117+
test('Recognizes POSIX absolute paths', () => {
118+
expect(isAbsolutePath('/foo/bar')).to.equal(true);
119+
expect(isAbsolutePath('/')).to.equal(true);
120+
});
121+
122+
test('Recognizes Windows drive-letter absolute paths', () => {
123+
expect(isAbsolutePath('C:\\foo\\bar')).to.equal(true);
124+
expect(isAbsolutePath('c:/foo/bar')).to.equal(true);
125+
expect(isAbsolutePath('Z:\\')).to.equal(true);
126+
});
127+
128+
test('Recognizes Windows UNC absolute paths', () => {
129+
expect(isAbsolutePath('\\\\server\\share\\foo')).to.equal(true);
130+
});
131+
132+
test('Rejects relative paths', () => {
133+
expect(isAbsolutePath('foo/bar')).to.equal(false);
134+
expect(isAbsolutePath('./foo')).to.equal(false);
135+
expect(isAbsolutePath('../foo')).to.equal(false);
136+
expect(isAbsolutePath('foo\\bar')).to.equal(false);
137+
expect(isAbsolutePath('')).to.equal(false);
138+
});
139+
});

0 commit comments

Comments
 (0)