Skip to content

bpo-32138: Skip on Android test_faulthandler tests that raise SIGSEGV #4604

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.

Sign up for GitHub

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

Merged
xdegaye merged 2 commits into from
Nov 29, 2017
Merged

bpo-32138: Skip on Android test_faulthandler tests that raise SIGSEGV #4604

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7204c22
bpo-32138: Skip on Android test_faulthandler tests that raise SIGSEGV
xdegaye Nov 28, 2017
7d2d012
Use a new skip_segfault_on_android decorator.
xdegaye Nov 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 2 additions & 15 deletions Lib/test/support/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@
"bigmemtest", "bigaddrspacetest", "cpython_only", "get_attribute",
"requires_IEEE_754", "skip_unless_xattr", "requires_zlib",
"anticipate_failure", "load_package_tests", "detect_api_mismatch",
"check__all__", "requires_android_level", "requires_multiprocessing_queue",
"check__all__", "requires_multiprocessing_queue",
"skip_unless_bind_unix_socket",
# sys
"is_jython", "is_android", "check_impl_detail", "unix_shell",
Expand Down Expand Up @@ -773,13 +773,7 @@ def dec(*args, **kwargs):

is_jython = sys.platform.startswith('java')

try:
# constant used by requires_android_level()
_ANDROID_API_LEVEL = sys.getandroidapilevel()
is_android = True
except AttributeError:
# sys.getandroidapilevel() is only available on Android
is_android = False
is_android = True if hasattr(sys, 'getandroidapilevel') else False

@vstinner vstinner Nov 28, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just "hasattr(sys, 'getandroidapilevel')"??


if sys.platform != 'win32':
unix_shell = '/system/bin/sh' if is_android else '/bin/sh'
Expand Down Expand Up @@ -1778,13 +1772,6 @@ def requires_resource(resource):
else:
return unittest.skip("resource {0!r} is not enabled".format(resource))

def requires_android_level(level, reason):

@vstinner vstinner Nov 28, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to keep it. I'm in touch with a developer who wants to support older Android versions, and in the future, we may have to use it again for features only working on newer Android versions.

@xdegaye xdegaye Nov 28, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer not to clutter test.support with dead code and it is easy to restore requires_android_level with git show <sha1> | git apply -R.

if is_android and _ANDROID_API_LEVEL < level:
return unittest.skip('%s at Android API level %d' %
(reason, _ANDROID_API_LEVEL))
else:
return _id

def cpython_only(test):
"""
Decorator for tests only applicable on CPython.
Expand Down
22 changes: 9 additions & 13 deletions Lib/test/test_faulthandler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import subprocess
import sys
from test import support
from test.support import script_helper, is_android, requires_android_level
from test.support import script_helper, is_android
import tempfile
import threading
import unittest
Expand Down Expand Up @@ -37,10 +37,6 @@ def temporary_filename():
finally:
support.unlink(filename)

def requires_raise(test):
return (test if not is_android else
requires_android_level(24, 'raise() is buggy')(test))

class FaultHandlerTests(unittest.TestCase):
def get_output(self, code, filename=None, fd=None):
"""
Expand Down Expand Up @@ -140,7 +136,7 @@ def test_read_null(self):
3,
'access violation')

@requires_raise
@unittest.skipIf(is_android, 'raising SIGSEGV on Android is unreliable')

@vstinner vstinner Nov 28, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please write a function like the current requires_raise() for Android, since you copied/pasted this line many times. Something like:

def skip_on_android():
    return unittest.skipIf(is_android, 'raising SIGSEGV on Android is unreliable')

Maybe "skip_segfault_on_android"?

Add a comment on this function mentionning "bpo-32138" and explain that raise(SIGSEGV) doesn't crash on Android (but does nothing if I understood correctly).

Then just use @skip_on_android.

def test_sigsegv(self):
self.check_fatal_error("""
import faulthandler
Expand Down Expand Up @@ -182,7 +178,7 @@ def test_sigfpe(self):

@unittest.skipIf(_testcapi is None, 'need _testcapi')
@unittest.skipUnless(hasattr(signal, 'SIGBUS'), 'need signal.SIGBUS')
@requires_raise
@unittest.skipIf(is_android, 'raising SIGSEGV on Android is unreliable')
def test_sigbus(self):
self.check_fatal_error("""
import _testcapi
Expand All @@ -197,7 +193,7 @@ def test_sigbus(self):

@unittest.skipIf(_testcapi is None, 'need _testcapi')
@unittest.skipUnless(hasattr(signal, 'SIGILL'), 'need signal.SIGILL')
@requires_raise
@unittest.skipIf(is_android, 'raising SIGSEGV on Android is unreliable')
def test_sigill(self):
self.check_fatal_error("""
import _testcapi
Expand Down Expand Up @@ -241,7 +237,7 @@ def test_stack_overflow(self):
'(?:Segmentation fault|Bus error)',
other_regex='unable to raise a stack overflow')

@requires_raise
@unittest.skipIf(is_android, 'raising SIGSEGV on Android is unreliable')
def test_gil_released(self):
self.check_fatal_error("""
import faulthandler
Expand All @@ -251,7 +247,7 @@ def test_gil_released(self):
3,
'Segmentation fault')

@requires_raise
@unittest.skipIf(is_android, 'raising SIGSEGV on Android is unreliable')
def test_enable_file(self):
with temporary_filename() as filename:
self.check_fatal_error("""
Expand All @@ -266,7 +262,7 @@ def test_enable_file(self):

@unittest.skipIf(sys.platform == "win32",
"subprocess doesn't support pass_fds on Windows")
@requires_raise
@unittest.skipIf(is_android, 'raising SIGSEGV on Android is unreliable')
def test_enable_fd(self):
with tempfile.TemporaryFile('wb+') as fp:
fd = fp.fileno()
Expand All @@ -280,7 +276,7 @@ def test_enable_fd(self):
'Segmentation fault',
fd=fd)

@requires_raise
@unittest.skipIf(is_android, 'raising SIGSEGV on Android is unreliable')
def test_enable_single_thread(self):
self.check_fatal_error("""
import faulthandler
Expand All @@ -291,7 +287,7 @@ def test_enable_single_thread(self):
'Segmentation fault',
all_threads=False)

@requires_raise
@unittest.skipIf(is_android, 'raising SIGSEGV on Android is unreliable')
def test_disable(self):
code = """
import faulthandler
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Tests/2017-11-27-16-18-58.bpo-32138.QsTvf-.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Skip on Android test_faulthandler tests that raise SIGSEGV and remove the
test.support.requires_android_level decorator.