-
Notifications
You must be signed in to change notification settings - Fork 99
Always quote Python executable path #964
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.
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
Changes from 4 commits
a3d46e4
3668184
ef13e45
32822e0
745998a
6df15dc
a0927d1
2601ca5
f4c64e5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ import * as path from 'path'; | |
| import { EXTENSION_ROOT_DIR } from '../../../extension/common/constants'; | ||
| import '../../../extension/common/promiseUtils'; | ||
| import * as launchers from '../../../extension/debugger/adapter/remoteLaunchers'; | ||
| import { fileToCommandArgumentForPythonExt } from '../../../extension/common/stringUtils'; | ||
|
|
||
| suite('External debugpy Debugger Launcher', () => { | ||
| [ | ||
|
|
@@ -18,7 +19,7 @@ suite('External debugpy Debugger Launcher', () => { | |
| }, | ||
| { | ||
| testName: 'When path to debugpy contains spaces', | ||
| path: path.join('path', 'to', 'debugpy', 'with spaces'), | ||
| path: fileToCommandArgumentForPythonExt(path.join('path', 'to', 'debugpy', 'with spaces')), | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps something like this?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't feel like this is the right way to test this as you're reusing the internal function to test itself.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thought the same, although in the end we want just to validate that the "normalised" input is equivalent. What if we replace(//, ) the input here, simulating the same behaviour, without exposing the internals?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All in all, it seems to me that the platform-specific behaviour is in
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure what you're seeing but my expectation is that The unit tests should be checking that quotes are around a path, they don't really care about the separator in the paths.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
So yeah tests data is encoded as "path/to/something/with space" but of course Windows would generate "path\to\something\with spaces". I kept the sane logic in the source code and updated the test code to use platform-specific data. |
||
| expectedPath: '"path/to/debugpy/with spaces"', | ||
| }, | ||
| ].forEach((testParams) => { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is the correct change. On non windows machines it may have the wrong path separators.
The unit tests are failing only because the old code did the wrong thing. The unit tests are wrong. They need to be platform specific too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh ok, I excluded that because I suggested amending previously pushed tests but we discarded that. Let me re-push it once more
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay this leaves the 'bug' that was there before. It should be checking OS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, restored the OS-checking path as you recommended before.