-
-
Notifications
You must be signed in to change notification settings - Fork 30.8k
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
gh-114272: fix Windows test_asyncio/test_subprocess
when sys.executable
contains unescaped spaces
#128160
base: main
Are you sure you want to change the base?
Conversation
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
test_subprocess
when sys.executable
contains unescapable spaces
test_subprocess
when sys.executable
contains unescapable spacestest_asyncio/test_subprocess
when sys.executable
contains unescapable spaces
test_asyncio/test_subprocess
when sys.executable
contains unescapable spacestest_asyncio/test_subprocess
when sys.executable
contains unescaped spaces
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.
Are these the only tests that need modification or are there more that can be fixed?
@@ -25,12 +25,26 @@ | |||
if support.check_sanitizer(address=True): | |||
raise unittest.SkipTest("Exposes ASAN flakiness in GitHub CI") | |||
|
|||
|
|||
def get_quoted_sys_executable(): |
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.
Can you refactor the function and use shorter names for the attributes? it's a test file so it doesn't matter what we use. Or use an LRU-cache instead. OTOH, you can setup the test cases so that they have a test attribute (namely cls.sysexec = ...
in setUpClass
and you'd use as self.sysexec
instead of sys.executable
in the tests)
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.
Hi picnixz.
I think caching is overkill. I'd do it for my own code but not for opensource to keep it simple. I'll remove it.
if not ((sysExecutable.startswith('"') and sysExecutable.endswith('"')) or | ||
(sysExecutable.startswith("'") and sysExecutable.endswith("'"))): | ||
if ' ' in (sysExecutable if sys.platform == 'win32' else sysExecutable.replace('\\ ', '')): |
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.
Can't we use some smarter escape mechanism? for instance, something in os.path
or shlex.quote
perhaps?
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.
os.path does not add the quotes and shlex.quote quotes a quoted string ad-nauseam.
i think we should merge this have less failed tests, and somebody later (maybe me) should fix shlex.quote
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.
*fewer failed test
I do not know if other libraries should benefit from this . I just run python -m unittest for the first time, and the only failures are fixed by this. There are some skips, but that's also for another time.
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.
cc @zooba for how we should proceed here.
gh-114272: Fix or skip tests that fail due to spaces in paths
Defines and uses the function get_quoted_sys_executable
if sys.executable is not quoted and it has unescaped spaces, it adds them. Win32 and Linux aware.