Skip to content

Basic toolchain.py unit tests #1928

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.

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
merged 1 commit into from
Jul 26, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
114 changes: 68 additions & 46 deletions pythonforandroid/build.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,66 @@
RECOMMENDED_NDK_API, RECOMMENDED_TARGET_API)


def get_cython_path():
for cython_fn in ("cython", "cython3", "cython2", "cython-2.7"):
cython = sh.which(cython_fn)
if cython:
return cython
raise BuildInterruptingException('No cython binary found.')


def get_ndk_platform_dir(ndk_dir, ndk_api, arch):
ndk_platform_dir_exists = True
platform_dir = arch.platform_dir
ndk_platform = join(
ndk_dir,
'platforms',
'android-{}'.format(ndk_api),
platform_dir)
if not exists(ndk_platform):
warning("ndk_platform doesn't exist: {}".format(ndk_platform))
ndk_platform_dir_exists = False
return ndk_platform, ndk_platform_dir_exists


def get_toolchain_versions(ndk_dir, arch):
toolchain_versions = []
toolchain_path_exists = True
toolchain_prefix = arch.toolchain_prefix
toolchain_path = join(ndk_dir, 'toolchains')
if isdir(toolchain_path):
toolchain_contents = glob.glob('{}/{}-*'.format(toolchain_path,
toolchain_prefix))
toolchain_versions = [split(path)[-1][len(toolchain_prefix) + 1:]
for path in toolchain_contents]
else:
warning('Could not find toolchain subdirectory!')
toolchain_path_exists = False
return toolchain_versions, toolchain_path_exists


def get_targets(sdk_dir):
if exists(join(sdk_dir, 'tools', 'bin', 'avdmanager')):
avdmanager = sh.Command(join(sdk_dir, 'tools', 'bin', 'avdmanager'))
targets = avdmanager('list', 'target').stdout.decode('utf-8').split('\n')
elif exists(join(sdk_dir, 'tools', 'android')):
android = sh.Command(join(sdk_dir, 'tools', 'android'))
targets = android('list').stdout.decode('utf-8').split('\n')
else:
raise BuildInterruptingException(
'Could not find `android` or `sdkmanager` binaries in Android SDK',
instructions='Make sure the path to the Android SDK is correct')
return targets


def get_available_apis(sdk_dir):
targets = get_targets(sdk_dir)
apis = [s for s in targets if re.match(r'^ *API level: ', s)]
apis = [re.findall(r'[0-9]+', s) for s in apis]
apis = [int(s[0]) for s in apis if s]
return apis


class Context(object):
'''A build context. If anything will be built, an instance this class
will be instantiated and used to hold all the build state.'''
Expand Down Expand Up @@ -238,20 +298,7 @@ def prepare_build_environment(self,
self.android_api = android_api

check_target_api(android_api, self.archs[0].arch)

if exists(join(sdk_dir, 'tools', 'bin', 'avdmanager')):
avdmanager = sh.Command(join(sdk_dir, 'tools', 'bin', 'avdmanager'))
targets = avdmanager('list', 'target').stdout.decode('utf-8').split('\n')
elif exists(join(sdk_dir, 'tools', 'android')):
android = sh.Command(join(sdk_dir, 'tools', 'android'))
targets = android('list').stdout.decode('utf-8').split('\n')
else:
raise BuildInterruptingException(
'Could not find `android` or `sdkmanager` binaries in Android SDK',
instructions='Make sure the path to the Android SDK is correct')
apis = [s for s in targets if re.match(r'^ *API level: ', s)]
apis = [re.findall(r'[0-9]+', s) for s in apis]
apis = [int(s[0]) for s in apis if s]
apis = get_available_apis(self.sdk_dir)
info('Available Android APIs are ({})'.format(
', '.join(map(str, apis))))
if android_api in apis:
Expand Down Expand Up @@ -327,46 +374,21 @@ def prepare_build_environment(self,
if not self.ccache:
info('ccache is missing, the build will not be optimized in the '
'future.')
for cython_fn in ("cython", "cython3", "cython2", "cython-2.7"):
cython = sh.which(cython_fn)
if cython:
self.cython = cython
break
else:
raise BuildInterruptingException('No cython binary found.')
if not self.cython:
ok = False
warning("Missing requirement: cython is not installed")
self.cython = get_cython_path()

# This would need to be changed if supporting multiarch APKs
arch = self.archs[0]
platform_dir = arch.platform_dir
toolchain_prefix = arch.toolchain_prefix
toolchain_version = None
self.ndk_platform = join(
self.ndk_dir,
'platforms',
'android-{}'.format(self.ndk_api),
platform_dir)
if not exists(self.ndk_platform):
warning('ndk_platform doesn\'t exist: {}'.format(
self.ndk_platform))
ok = False
self.ndk_platform, ndk_platform_dir_exists = get_ndk_platform_dir(
self.ndk_dir, self.ndk_api, arch)
ok = ok and ndk_platform_dir_exists

py_platform = sys.platform
if py_platform in ['linux2', 'linux3']:
py_platform = 'linux'

toolchain_versions = []
toolchain_path = join(self.ndk_dir, 'toolchains')
if isdir(toolchain_path):
toolchain_contents = glob.glob('{}/{}-*'.format(toolchain_path,
toolchain_prefix))
toolchain_versions = [split(path)[-1][len(toolchain_prefix) + 1:]
for path in toolchain_contents]
else:
warning('Could not find toolchain subdirectory!')
ok = False
toolchain_versions, toolchain_path_exists = get_toolchain_versions(
self.ndk_dir, arch)
ok = ok and toolchain_path_exists
toolchain_versions.sort()

toolchain_versions_gcc = []
Expand Down
89 changes: 89 additions & 0 deletions tests/test_toolchain.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
import sys
import pytest
import mock
from pythonforandroid.toolchain import ToolchainCL
from pythonforandroid.util import BuildInterruptingException


def patch_sys_argv(argv):
return mock.patch('sys.argv', argv)


def patch_argparse_print_help():
return mock.patch('argparse.ArgumentParser.print_help')


def raises_system_exit():
return pytest.raises(SystemExit)


class TestToolchainCL:

def test_help(self):
"""
Calling with `--help` should print help and exit 0.
"""
argv = ['toolchain.py', '--help', '--storage-dir=/tmp']
with patch_sys_argv(argv), raises_system_exit(
) as ex_info, patch_argparse_print_help() as m_print_help:
ToolchainCL()
Copy link

@ghost ghost Jul 25, 2019

Choose a reason for hiding this comment

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

Wouldn't something like this be a little easier to understand:

subprocess.check_output([
    sys.executable, "-c", "from pythonforandroid.toolchain import main(); main", "--help"
], cwd=os.path.join(os.path.dirname(__file__), ".."))

Or is that only me? Mock is fine but if it's not needed I would still rather use standard methods


There is also a lot of repetition of this block in particular:

        with mock.patch('sys.argv', argv), pytest.raises(SystemExit) as ex_info, \
                mock.patch('argparse.ArgumentParser.print_help') as m_print_help:
            ToolchainCL()

No matter if we end up using mock or not, maybe a helper function for that would be useful?

Copy link
Member Author

Choose a reason for hiding this comment

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

The subprocess is overkill to me, it's really too much top level integration test.
Regarding the patching helper, yes I agree, I actually do this in my projects and at work, but since it's not common in p4a I didn't dare to introduce it.
I'll then

Copy link

@ghost ghost Jul 26, 2019

Choose a reason for hiding this comment

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

it's really too much top level integration test.

I'll let you decide but just as a note: that seems like an irrelevant point to me, because isn't this what the entire purpose is? Check if the overall tool responds fine to --help? It seems a little to me with this particular argument you're trying to isolate a component here with no benefit other than possibly complicating the test, and also removing it further from the actual usage later. But of course there are other completely valid reasons to stick with your current solution, e.g. if you find it more readable (which I don't but I can see others maybe would)

But keep in mind my proposal would also possibly reduce the maintenance burden if toolchain.py is ever heavily refactored to also adjust the test here, because it nicely stays out of how the functionality needs to be performed - even more so than the mock code I mean - as long as it works right without a crash, which I think is an additional benefit!

But that's just my personal opinion of course and I don't think this should hold off a merge

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I definitely don't share this point of view regarding readability, I find that subprocess.check_output absolutely ugly. Not to say about parsing eventual errors that it could output or the fact that's unclear which python executable it would run, the fact that you need to play around with the cwd. For me it's just a big nope. I've hardly ever seen people writing unit tests calling subprocess 😕

Copy link

@ghost ghost Jul 26, 2019

Choose a reason for hiding this comment

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

it's sys.executable (=just the same interpreter that is already running) do people not usually know that? 😄 but it's fine, leave it with mocking I am clearly making you confused 😂 (I'm weird that's normal)

The mocking really isn't too bad I just seem to have unusual preferences here

assert ex_info.value.code == 0
assert m_print_help.call_args_list == [mock.call()]

@pytest.mark.skipif(sys.version_info < (3, 0), reason="requires python3")
def test_unknown(self):
"""
Calling with unknown args should print help and exit 1.
"""
argv = ['toolchain.py', '--unknown']
with patch_sys_argv(argv), raises_system_exit(
) as ex_info, patch_argparse_print_help() as m_print_help:
ToolchainCL()
assert ex_info.value.code == 1
assert m_print_help.call_args_list == [mock.call()]

def test_create(self):
"""
Basic `create` distribution test.
"""
argv = [
'toolchain.py',
'create',
'--sdk-dir=/tmp/android-sdk',
'--ndk-dir=/tmp/android-ndk',
'--dist-name=test_toolchain',
]
with patch_sys_argv(argv), mock.patch(
'pythonforandroid.build.get_available_apis'
) as m_get_available_apis, mock.patch(
'pythonforandroid.build.get_toolchain_versions'
) as m_get_toolchain_versions, mock.patch(
'pythonforandroid.build.get_ndk_platform_dir'
) as m_get_ndk_platform_dir, mock.patch(
'pythonforandroid.build.get_cython_path'
) as m_get_cython_path, mock.patch(
'pythonforandroid.toolchain.build_dist_from_args'
) as m_build_dist_from_args:
m_get_available_apis.return_value = [27]
m_get_toolchain_versions.return_value = (['4.9'], True)
m_get_ndk_platform_dir.return_value = (
'/tmp/android-ndk/platforms/android-21/arch-arm', True)
ToolchainCL()
assert m_get_available_apis.call_args_list == [
mock.call('/tmp/android-sdk')]
assert m_get_toolchain_versions.call_args_list == [
mock.call('/tmp/android-ndk', mock.ANY)]
assert m_get_cython_path.call_args_list == [mock.call()]
assert m_build_dist_from_args.call_count == 1

def test_create_no_sdk_dir(self):
"""
The `--sdk-dir` is mandatory to `create` a distribution.
"""
argv = ['toolchain.py', 'create']
with mock.patch('sys.argv', argv), pytest.raises(
BuildInterruptingException
) as ex_info:
ToolchainCL()
assert ex_info.value.message == (
'Android SDK dir was not specified, exiting.')