gh-114272: fix Windows test_asyncio/test_subprocess when sys.executable contains unescaped spaces - #128160
gh-114272: fix Windows test_asyncio/test_subprocess when sys.executable contains unescaped spaces#128160andyrosa2 wants to merge 2 commits into
test_asyncio/test_subprocess when sys.executable contains unescaped spaces#128160Conversation
|
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 |
picnixz
changed the title
test_subprocess when sys.executable contains unescapable spaces
picnixz
changed the title
test_subprocess when sys.executable contains unescapable spacestest_asyncio/test_subprocess when sys.executable contains unescapable spaces
picnixz
changed the title
test_asyncio/test_subprocess when sys.executable contains unescapable spacestest_asyncio/test_subprocess when sys.executable contains unescaped spaces
picnixz
left a comment
picnixz
left a comment
There was a problem hiding this comment.
Are these the only tests that need modification or are there more that can be fixed?
| raise unittest.SkipTest("Exposes ASAN flakiness in GitHub CI") | ||
|
|
||
|
|
||
| def get_quoted_sys_executable(): |
There was a problem hiding this comment.
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.
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.
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.
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.
*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.
I haven't looked into what this is being used for, but subprocess is meant to handle quoting itself. Why isn't that working?
shlex is only really for Unix. There are a number of discussions elsewhere about making it more portable, though Windows is tough (different shells have different quoting rules; app parsing rules are app-specific; the most commonly used rules are different between argv[0] and argv[1:]).
|
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 |
kumaraditya303
left a comment
kumaraditya303
left a comment
There was a problem hiding this comment.
I think this is already fixed by #114451, is this still an issue?
4 participants
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.