gh-111912: Run test_posix on Windows #111913
Conversation
serhiy-storchaka
commented
Nov 9, 2023
•
serhiy-storchaka
commented
- Issue: Run test_posix on Windows #111912
serhiy-storchaka
added
tests
Lib/test/test_posix.py
Outdated
| @@ -1220,6 +1227,7 @@ def test_sched_setaffinity(self): | |||
| self.assertRaises(OSError, posix.sched_setaffinity, -1, mask) | |||
|
|
|||
| @unittest.skipIf(support.is_wasi, "No dynamic linking on WASI") | |||
| @unittest.skipUnless(os.name == 'posix', "requires Posix") | |||
There was a problem hiding this comment.
The test is called "posix" and other tests are run. It's surprising that a test is skipped because it "requires Posix".
Maybe skip the test on Windows, is support.MS_WINDOWS is true? With a message like "Windows doesn't have RTLD constants".
There was a problem hiding this comment.
The message for the skip test as is would better be "requires posix module". But I think Victor's suggest is even better. It there were a 3rd OS module, we don't know that the test would need to be skipped.
There was a problem hiding this comment.
I did not invent this message, it is one of common messages already used in many other tests with this condition. Grep 'skip.*posix'. Other common messages are "only supported on Unix" (technically incorrect, Linux is not Unix), 'tests requires a posix system.', "test meaningful only on posix systems", 'POSIX-only test', 'POSIX only test'. Only one of them spells "Posix" correctly.
But unification of skipping messages is a matter of different issue.
There was a problem hiding this comment.
Windows provides a POSIX-like API for many years. You say that Linux is not Unix. Well, I would still prefer to say "Windows doesn't have RTLD constants" than just "requires Posix". By the way, POSIX is an accronym which stands for "Portable Operating System Interface", and so should be written in uppercase: https://en.wikipedia.org/wiki/POSIX ;-)
Python is misleading since os.name is just "posix" on all platforms but Windows (where it's "nt"): https://pythondev.readthedocs.io/platforms.html#sys-platform-versus-os-name Is it the name of the operating system ("OS") or the name of the C extension? The "os" name is not called "os?" :-)
There was a problem hiding this comment.
I must have confused him with FORTRAN that is now simply Fortran.
Lib/test/test_posix.py
Outdated
| try: | ||
| import nt as posix | ||
| except ImportError: | ||
| raise unittest.SkipTest("requires 'posix' or 'nt' module") |
There was a problem hiding this comment.
Hum, when is this code path supposed to be taken? On Python implementations other than CPython? If the compilation of the posix/nt extension failed?
I would prefer to get an ImportError rather than skipping silently the test. If there is a legit use case to skip the test, in that ok, sure, we can skip it. But now it's unclear to me why posix and nt would miss.
PyPy has a posix module on Linux for example.
There was a problem hiding this comment.
I do not know, but maybe on some custom builds of Python on some exotic platforms.
Currently the os module only supports posix and nt.
There was a problem hiding this comment.
I do not know, but maybe on some custom builds of Python on some exotic platforms.
If someone has a custom build of Python, I suppose that they can modify one line of test_posix.py to skip the whole file on their exotic platform. I would prefer to be safe by default. For me, it's more likely that the test would be skipped by mistake, than causing troubles on exotic platforms. Also, we don't support exotic platforms (PEP 11).
|
There is currently a test_ntpath corresponding to test_posixpath and a test_os for os functions not part the Posix standard, but no test_nt corresponding to test_posix. So this fill what seems a gap in testing the Posix functions in 'nt', and with minimal fuss. +1 except possibly for details. |
|
I'm considering the split test_os into a test package and sub-files. Maybe test_os, test_posix, test_ntpath, etc. can be moved there. But that's should be done later. |
| try: | ||
| import posix | ||
| except ImportError: | ||
| import nt as posix |
There was a problem hiding this comment.
It's a little bit surprising, but well, it's convenient for these tests :-)
The C extension should be called _os instead of posix or nt :-(
|
Thank you Victor and Terry for your review. |
|
Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12. |
(cherry picked from commit 64fea32) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
(cherry picked from commit 64fea32) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
|
GH-111953 is a backport of this pull request to the 3.12 branch. |
|
GH-111954 is a backport of this pull request to the 3.11 branch. |
