Repository navigation
Fix other.test_argv0_node with Windows 8.3 short paths - #28195
Conversation
Node sets process.argv[1] via fs.realpathSync(), which resolves symlinks (such as /var -> /private/var on macOS) without expanding Windows 8.3 short names (such as C:/Users/RUNNER~1/... when %TEMP% belongs to a username longer than 8 characters like runneradmin). Python 3.8+'s os.path.realpath() on Windows uses GetFinalPathNameByHandleW and expands 8.3 short names, so normalize both the expected path and the path reported in argv[0] with os.path.realpath().
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates a test in test/test_other.py to extract a path from the output using a regular expression and compare it using assertPathsIdentical to handle platform-specific path resolution. The reviewer identified a potential issue on Windows where carriage return characters could cause the regular expression match to fail, and provided a suggestion to make the regex pattern more robust.
| # Resolve both paths with realpath: Node's process.argv[1] resolves symlinks | ||
| # (e.g. /var -> /private/var on macOS) without expanding Windows 8.3 short | ||
| # names (e.g. RUNNER~1 -> runneradmin), whereas Python's realpath resolves both. | ||
| self.assertPathsIdentical(os.path.realpath(self.in_dir('code.js')), os.path.realpath(m.group(1))) |
There was a problem hiding this comment.
Can you do something like this:
expected = os.path.realpath(self.in_dir('code.js))
self.assertContains(output, f'I am ${expected}')
?
There was a problem hiding this comment.
You have to run realpath on both the expected path and the output. But here's a version that's simpler and doesn't need the regex.
| # Resolve both paths with realpath: Node's process.argv[1] resolves symlinks | ||
| # (e.g. /var -> /private/var on macOS) without expanding Windows 8.3 short | ||
| # names (e.g. RUNNER~1 -> runneradmin), whereas Python's realpath resolves both. | ||
| self.assertPathsIdentical(os.path.realpath(self.in_dir('code.js')), os.path.realpath(output)) |
There was a problem hiding this comment.
Whats wrong with keeping the assertContained version? I'm not sure what assertPathsIdentical does?
There was a problem hiding this comment.
assertPathsIdentical just calls utils.normalize_path (which just swaps windows for Unix forward slashes) on each path. I guess it's jsut a bit more robust.
Also I think the problem with the assertContained version is that you want to run realpath on both the expected output (which your suggestion does) and also the actual output (which it doesn't).
Node sets process.argv[1] via fs.realpathSync(), which resolves symlinks (such as /var -> /private/var on macOS) without expanding Windows 8.3 short names (such as C:/Users/RUNNER~1/... when %TEMP% belongs to a username longer than 8 characters like runneradmin). Python 3.8+'s os.path.realpath() on Windows uses GetFinalPathNameByHandleW and expands 8.3 short names, so normalize both the expected path and the path reported in argv[0] with os.path.realpath().