gh-114099: Add test exclusions to support running the test suite on iOS #114889
Conversation
freakboy3742
requested review from
berkerpeksag,
erlend-aasland,
vsajip,
1st1,
asvetlov,
gvanrossum,
kumaraditya303 and
willingc
as code owners
|
I'm not a project member, so I can't resolve my conversations, but they've all now been addressed. |
| @@ -1874,7 +1874,7 @@ async def runner(): | |||
| wsock.close() | |||
|
|
|||
|
|
|||
| @unittest.skipUnless(hasattr(os, 'fork'), 'requires os.fork()') | |||
| @support.requires_fork() | |||
| @@ -160,6 +161,7 @@ def setUp(self): | |||
| self.console = code.InteractiveConsole(local_exit=True) | |||
| self.mock_sys() | |||
|
|
|||
| @unittest.skipIf(sys.flags.no_site, "exit() isn't defined unless there's a site module") | |||
There was a problem hiding this comment.
Does this work if the import site does not exist in the file?
There was a problem hiding this comment.
It does - the import site was a leftover from a previous attempt to fix the problem that was ultimately resolved with the no_site-based skip. I've removed the import.
|
Are you ready to land this, @freakboy3742? |
|
@erlend-aasland I'm ready if everyone else is :-) |
|
AFAICS, @willingc's remarks have been addressed; the rest are thumbs up. Let's go :) |
…e on iOS (python#114889) Add test annotations required to run the test suite on iOS (PEP 730). The majority of the change involve annotating tests that use subprocess, but are skipped on Emscripten/WASI for other reasons, and including iOS/tvOS/watchOS under the same umbrella as macOS/darwin checks. `is_apple` and `is_apple_mobile` test helpers have been added to identify *any* Apple platform, and "any Apple platform except macOS", respectively.
…e on iOS (python#114889) Add test annotations required to run the test suite on iOS (PEP 730). The majority of the change involve annotating tests that use subprocess, but are skipped on Emscripten/WASI for other reasons, and including iOS/tvOS/watchOS under the same umbrella as macOS/darwin checks. `is_apple` and `is_apple_mobile` test helpers have been added to identify *any* Apple platform, and "any Apple platform except macOS", respectively.
| from test.support import ( | ||
| cpython_only, get_pagesize, is_apple, requires_subprocess, verbose | ||
| ) |
There was a problem hiding this comment.
You seem to do this change often. In the future, could you add a trailing comma, to make future diffs nicer?
(Or simply add another line if the current one gets too long:
from test.support import verbose, cpython_only, get_pagesize
from test.support import is_apple, requires_subprocess
)
There was a problem hiding this comment.
@encukou Sure, no problem.
One of the issues I've hit is that the "preferred" style for imports varies wildly between files. I appreciate there's 30 years of historical baggage in the existing code - but is there a reason either of those two approaches is preferred over isort/black style syntax:
from test.support import (
cpython_only,
get_pagesize,
is_apple,
requires_subprocess,
verbose,
)
Running black over every file I touch would make for unreadable patches; but if little black touches when the line is already being altered would be looked on favourably, then that's an easy tweak to make.
There was a problem hiding this comment.
That works too -- there's a trailing comma, so future diffs won't need to add it.
(The 30 years of baggage also means there is no shared preference for stuff that isn't in PEP 8. I'm partial to repeating from <name> because it's searchable with grep -- you don't need a code analyzer -- but that's just personal opinion.)
if little black touches when the line is already being altered would be looked on favourably
Sure! As long as it helps readability :)
Part of the PEP 730 work to add iOS support.
Adds test annotations required to run the CPython test suite on iOS.
The majority of these changes involve:
is_appleandis_apple_mobiletest helpers have been added to identify any Apple platform, and "any Apple platform except macOS", respectively.Refs #114099.