Skip to content

Commit 59e72f7

Browse files
wenytang-msCopilot
andcommitted
style: trim verbose comments in pathUtil and tests
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 5fa77fe commit 59e72f7

2 files changed

Lines changed: 18 additions & 65 deletions

File tree

src/pathUtil.ts

Lines changed: 4 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -4,25 +4,11 @@
44
/**
55
* Builds the value to append to PATH for the noConfigScripts directory.
66
*
7-
* `vscode.EnvironmentVariableCollection.append()` performs literal string
8-
* concatenation and does NOT insert a path separator. We always prepend one so
9-
* we never glue our directory onto the last entry of the user's PATH (e.g.
10-
* `...;C:\Program Files\jreleaser\c:\Users\...\noConfigScripts`).
7+
* `vscode.EnvironmentVariableCollection.append()` does literal string
8+
* concatenation, so we always prepend a separator to avoid gluing our
9+
* directory onto the last entry of the user's PATH (see issue #1637).
1110
*
12-
* We cannot rely on `process.env.PATH` ending with a separator: the integrated
13-
* terminal's PATH may differ from the extension host's PATH (it can be modified
14-
* by `terminal.integrated.env.*` or by other extensions' env-var collections).
15-
*
16-
* A leading separator is always safe: if the resolved PATH already ends with
17-
* one, the resulting empty PATH entry is harmless on both Windows and POSIX
18-
* shells.
19-
*
20-
* This module has no `vscode` import so it can be unit-tested in plain Node.
21-
*
22-
* @param scriptsDir absolute path to the noConfigScripts directory
23-
* @param platform the target platform; defaults to `process.platform`. Made
24-
* injectable so unit tests can exercise both Windows and
25-
* POSIX behavior on a single host.
11+
* @param platform defaults to `process.platform`; injectable for tests.
2612
*/
2713
export function buildNoConfigPathAppendValue(
2814
scriptsDir: string,

test/pathUtil.test.ts

Lines changed: 14 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,7 @@ import * as assert from "assert";
55

66
import { buildNoConfigPathAppendValue } from "../src/pathUtil";
77

8-
// Regression tests for issue #1637: the extension was appending its
9-
// noConfigScripts directory to PATH without a separator on some terminal
10-
// PATH configurations, gluing it onto the last entry of the user's PATH.
8+
// Regression tests for issue #1637.
119
suite("buildNoConfigPathAppendValue", () => {
1210

1311
const winDir = "C:\\Users\\me\\.vscode\\extensions\\vscjava.vscode-java-debug-0.59.0\\bundled\\scripts\\noConfigScripts";
@@ -30,69 +28,38 @@ suite("buildNoConfigPathAppendValue", () => {
3028

3129
test("always starts with a path separator (Windows)", () => {
3230
const result = buildNoConfigPathAppendValue(winDir, "win32");
33-
assert.ok(result.startsWith(";"), `expected leading ';', got: ${result}`);
31+
assert.ok(result.startsWith(";"));
3432
});
3533

3634
test("always starts with a path separator (POSIX)", () => {
3735
const result = buildNoConfigPathAppendValue(posixDir, "linux");
38-
assert.ok(result.startsWith(":"), `expected leading ':', got: ${result}`);
36+
assert.ok(result.startsWith(":"));
3937
});
4038

4139
test("never collapses scriptsDir into the previous PATH entry on Windows", () => {
42-
// Simulates the exact scenario from issue #1637: a user PATH whose
43-
// last entry has no trailing separator. After append, the script dir
44-
// must not be glued onto 'jreleaser\'.
40+
// #1637 scenario: last user PATH entry has no trailing separator.
4541
const userPath = "C:\\foo;C:\\Program Files\\jreleaser\\";
46-
const finalPath = userPath + buildNoConfigPathAppendValue(winDir, "win32");
47-
48-
const entries = finalPath.split(";");
49-
assert.ok(
50-
entries.includes("C:\\Program Files\\jreleaser\\"),
51-
`expected 'jreleaser\\' to remain a standalone PATH entry, got entries: ${JSON.stringify(entries)}`,
52-
);
53-
assert.ok(
54-
entries.includes(winDir),
55-
`expected scripts dir to be a standalone PATH entry, got entries: ${JSON.stringify(entries)}`,
56-
);
42+
const entries = (userPath + buildNoConfigPathAppendValue(winDir, "win32")).split(";");
43+
assert.ok(entries.includes("C:\\Program Files\\jreleaser\\"));
44+
assert.ok(entries.includes(winDir));
5745
});
5846

5947
test("never collapses scriptsDir into the previous PATH entry on POSIX", () => {
6048
const userPath = "/usr/bin:/opt/jreleaser/bin";
61-
const finalPath = userPath + buildNoConfigPathAppendValue(posixDir, "linux");
62-
63-
const entries = finalPath.split(":");
64-
assert.ok(
65-
entries.includes("/opt/jreleaser/bin"),
66-
`expected '/opt/jreleaser/bin' to remain a standalone PATH entry, got entries: ${JSON.stringify(entries)}`,
67-
);
68-
assert.ok(
69-
entries.includes(posixDir),
70-
`expected scripts dir to be a standalone PATH entry, got entries: ${JSON.stringify(entries)}`,
71-
);
49+
const entries = (userPath + buildNoConfigPathAppendValue(posixDir, "linux")).split(":");
50+
assert.ok(entries.includes("/opt/jreleaser/bin"));
51+
assert.ok(entries.includes(posixDir));
7252
});
7353

7454
test("yields only an empty (harmless) entry when the user's PATH already ends with a separator", () => {
75-
// If the resolved terminal PATH already ends with ';', append produces
76-
// ';;'. The empty middle entry is ignored by Windows and (in this
77-
// position) effectively a no-op on POSIX shells.
7855
const userPath = "C:\\foo;C:\\bar;";
79-
const finalPath = userPath + buildNoConfigPathAppendValue(winDir, "win32");
80-
81-
const entries = finalPath.split(";");
82-
// The scripts dir must still be a standalone, valid entry.
83-
assert.ok(
84-
entries.includes(winDir),
85-
`expected scripts dir to remain standalone, got entries: ${JSON.stringify(entries)}`,
86-
);
87-
// No real entry should be merged with our scripts dir.
88-
assert.ok(
89-
!entries.some((e) => e !== winDir && e.endsWith(winDir)),
90-
`no entry should be glued to scripts dir, got entries: ${JSON.stringify(entries)}`,
91-
);
56+
const entries = (userPath + buildNoConfigPathAppendValue(winDir, "win32")).split(";");
57+
assert.ok(entries.includes(winDir));
58+
assert.ok(!entries.some((e) => e !== winDir && e.endsWith(winDir)));
9259
});
9360

9461
test("scriptsDir appears unchanged at the end of the appended value", () => {
9562
const result = buildNoConfigPathAppendValue(winDir, "win32");
96-
assert.ok(result.endsWith(winDir), `expected value to end with scriptsDir, got: ${result}`);
63+
assert.ok(result.endsWith(winDir));
9764
});
9865
});

0 commit comments

Comments
 (0)