Skip to content

Commit 9e4098c

Browse files
Copilotrchiodo
andcommitted
Fix PATH separator issue when appending to environment variable collection
Co-authored-by: rchiodo <19672699+rchiodo@users.noreply.github.com>
1 parent 2076083 commit 9e4098c

File tree

3 files changed

+60
-95
lines changed

3 files changed

+60
-95
lines changed

package-lock.json

Lines changed: 29 additions & 5 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/extension/noConfigDebugInit.ts

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -78,12 +78,8 @@ export async function registerNoConfigDebug(
7878
const noConfigScriptsDir = path.join(extPath, 'bundled', 'scripts', 'noConfigScripts');
7979
const pathSeparator = process.platform === 'win32' ? ';' : ':';
8080

81-
// Check if the current PATH already ends with a path separator to avoid double separators
82-
const currentPath = process.env.PATH || '';
83-
const needsSeparator = currentPath.length > 0 && !currentPath.endsWith(pathSeparator);
84-
const pathValueToAppend = needsSeparator ? `${pathSeparator}${noConfigScriptsDir}` : noConfigScriptsDir;
85-
86-
collection.append('PATH', pathValueToAppend);
81+
// Always prepend separator when appending to PATH since append() concatenates to existing value
82+
collection.append('PATH', `${pathSeparator}${noConfigScriptsDir}`);
8783

8884
const bundledDebugPath = path.join(extPath, 'bundled', 'libs', 'debugpy');
8985
collection.replace('BUNDLED_DEBUGPY_PATH', bundledDebugPath);

src/test/unittest/noConfigDebugInit.unit.test.ts

Lines changed: 29 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,8 @@ suite('setup for no-config debug scenario', function () {
7171
.setup((x) => x.append(TypeMoq.It.isAny(), TypeMoq.It.isAny()))
7272
.callback((key, value) => {
7373
if (key === 'PATH') {
74-
assert(value.includes(noConfigScriptsDir));
74+
const pathSeparator = process.platform === 'win32' ? ';' : ':';
75+
assert(value === `${pathSeparator}${noConfigScriptsDir}`);
7576
}
7677
})
7778
.returns(envVarCollectionAppendStub);
@@ -88,100 +89,44 @@ suite('setup for no-config debug scenario', function () {
8889
sinon.assert.calledOnce(envVarCollectionAppendStub);
8990
});
9091

91-
test('should not add extra separator when PATH already ends with separator', async () => {
92+
test('should always add separator when appending to PATH', async () => {
9293
const environmentVariableCollectionMock = TypeMoq.Mock.ofType<any>();
9394
envVarCollectionReplaceStub = sinon.stub();
9495
envVarCollectionAppendStub = sinon.stub();
9596

96-
// Simulate a PATH that already ends with a separator to test the fix
97+
// The separator should always be prepended regardless of process.env.PATH
9798
const pathSeparator = process.platform === 'win32' ? ';' : ':';
98-
const originalPath = process.env.PATH;
99-
process.env.PATH = `/some/path${pathSeparator}`;
10099

101-
try {
102-
environmentVariableCollectionMock
103-
.setup((x) => x.replace(TypeMoq.It.isAny(), TypeMoq.It.isAny()))
104-
.returns(envVarCollectionReplaceStub);
105-
106-
environmentVariableCollectionMock
107-
.setup((x) => x.append(TypeMoq.It.isAny(), TypeMoq.It.isAny()))
108-
.callback((key, value) => {
109-
if (key === 'PATH') {
110-
// Since PATH already ends with separator, we should NOT add another one
111-
assert(value === noConfigScriptsDir);
112-
assert(!value.startsWith(pathSeparator));
113-
}
114-
})
115-
.returns(envVarCollectionAppendStub);
116-
117-
context
118-
.setup((c) => c.environmentVariableCollection)
119-
.returns(() => environmentVariableCollectionMock.object);
120-
121-
setupFileSystemWatchers();
122-
123-
// run init for no config debug
124-
await registerNoConfigDebug(context.object.environmentVariableCollection, context.object.extensionPath);
125-
126-
// assert that append was called for PATH
127-
sinon.assert.calledOnce(envVarCollectionAppendStub);
128-
} finally {
129-
// Restore original PATH
130-
if (originalPath !== undefined) {
131-
process.env.PATH = originalPath;
132-
} else {
133-
delete process.env.PATH;
134-
}
135-
}
136-
});
100+
environmentVariableCollectionMock
101+
.setup((x) => x.replace(TypeMoq.It.isAny(), TypeMoq.It.isAny()))
102+
.returns(envVarCollectionReplaceStub);
137103

138-
test('should add separator when PATH does not end with separator', async () => {
139-
const environmentVariableCollectionMock = TypeMoq.Mock.ofType<any>();
140-
envVarCollectionReplaceStub = sinon.stub();
141-
envVarCollectionAppendStub = sinon.stub();
104+
environmentVariableCollectionMock
105+
.setup((x) => x.append(TypeMoq.It.isAny(), TypeMoq.It.isAny()))
106+
.callback((key, value) => {
107+
if (key === 'PATH') {
108+
// Should always add separator when appending
109+
assert(value === `${pathSeparator}${noConfigScriptsDir}`);
110+
assert(value.startsWith(pathSeparator));
111+
}
112+
})
113+
.returns(envVarCollectionAppendStub);
142114

143-
// Simulate a PATH that does NOT end with a separator
144-
const pathSeparator = process.platform === 'win32' ? ';' : ':';
145-
const originalPath = process.env.PATH;
146-
process.env.PATH = '/some/path';
115+
context
116+
.setup((c) => c.environmentVariableCollection)
117+
.returns(() => environmentVariableCollectionMock.object);
147118

148-
try {
149-
environmentVariableCollectionMock
150-
.setup((x) => x.replace(TypeMoq.It.isAny(), TypeMoq.It.isAny()))
151-
.returns(envVarCollectionReplaceStub);
152-
153-
environmentVariableCollectionMock
154-
.setup((x) => x.append(TypeMoq.It.isAny(), TypeMoq.It.isAny()))
155-
.callback((key, value) => {
156-
if (key === 'PATH') {
157-
// Since PATH does NOT end with separator, we should add one
158-
assert(value === `${pathSeparator}${noConfigScriptsDir}`);
159-
assert(value.startsWith(pathSeparator));
160-
}
161-
})
162-
.returns(envVarCollectionAppendStub);
163-
164-
context
165-
.setup((c) => c.environmentVariableCollection)
166-
.returns(() => environmentVariableCollectionMock.object);
167-
168-
setupFileSystemWatchers();
169-
170-
// run init for no config debug
171-
await registerNoConfigDebug(context.object.environmentVariableCollection, context.object.extensionPath);
172-
173-
// assert that append was called for PATH
174-
sinon.assert.calledOnce(envVarCollectionAppendStub);
175-
} finally {
176-
// Restore original PATH
177-
if (originalPath !== undefined) {
178-
process.env.PATH = originalPath;
179-
} else {
180-
delete process.env.PATH;
181-
}
182-
}
119+
setupFileSystemWatchers();
120+
121+
// run init for no config debug
122+
await registerNoConfigDebug(context.object.environmentVariableCollection, context.object.extensionPath);
123+
124+
// assert that append was called for PATH
125+
sinon.assert.calledOnce(envVarCollectionAppendStub);
183126
});
184127

128+
129+
185130
test('should create file system watcher for debuggerAdapterEndpointFolder', async () => {
186131
// Arrange
187132
const environmentVariableCollectionMock = TypeMoq.Mock.ofType<any>();

0 commit comments

Comments
 (0)