Skip to content

Proof of concept - A bunch of tests for library recipes #1982

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 31 commits into from
Sep 14, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
11df8e0
[test] Add module `tests.recipe_ctx`
opacam Sep 10, 2019
b242289
[test] Refactor `setUp/tearDown` for `test_icu`
opacam Sep 10, 2019
bd1018b
[test] Refactor `setUp/tearDown` for `test_pyicu`
opacam Sep 10, 2019
add66c2
[test] Refactor `setUp` for `test_reportlab`
opacam Sep 10, 2019
52978e1
[test] Refactor `setUp` for `test_gevent`
opacam Sep 10, 2019
04be9cb
[test] Add module `tests.recipe_lib_test`
opacam Sep 11, 2019
5e2c08f
[test] Add test for `libffi`
opacam Sep 11, 2019
5616cbd
[test] Add test for `libexpat`
opacam Sep 11, 2019
3b0478e
[test] Add test for `libcurl`
opacam Sep 11, 2019
bbf4d64
[test] Add test for `libiconv`
opacam Sep 11, 2019
c4deb10
[test] Add test for `libogg`
opacam Sep 11, 2019
7061cc5
[test] Add test for `libpq`
opacam Sep 11, 2019
18bda8e
[test] Add test for `libsecp256k1`
opacam Sep 11, 2019
054eb02
[test] Add test for `libshine`
opacam Sep 11, 2019
6f3758a
[test] Add test for `libvorbis`
opacam Sep 11, 2019
a68522a
[test] Add test for `libx264`
opacam Sep 11, 2019
6370455
[test] Add test for `libxml2`
opacam Sep 11, 2019
bc93051
[test] Add test for `libxslt`
opacam Sep 11, 2019
ed8524f
[test] Add test for `png`
opacam Sep 11, 2019
fd4cbc0
[test] Add test for `freetype`
opacam Sep 11, 2019
d3d6e1d
[test] Add test for `harfbuzz`
opacam Sep 11, 2019
5cca1f5
[test] Add test for `openssl`
opacam Sep 11, 2019
a95b7e9
[test] Add `BaseTestForCmakeRecipe`
opacam Sep 11, 2019
0d48b40
[test] Add test for `jpeg` and clean code
opacam Sep 11, 2019
47bba46
[test] Add test for `snappy`
opacam Sep 11, 2019
f77d693
[test] Add test for `leveldb`
opacam Sep 11, 2019
13a48ea
[test] Add test for `libgeos`
opacam Sep 11, 2019
2e73dc2
[test] Add test for `libmysqlclient`
opacam Sep 11, 2019
b27376b
[test] Add test for `openal`
opacam Sep 11, 2019
fcc71f2
[test] Make the `super` calls Python3 style
opacam Sep 14, 2019
80104c4
[test] Move mock tests outside context manager...
opacam Sep 14, 2019
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
19 changes: 3 additions & 16 deletions pythonforandroid/recipes/jpeg/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
from pythonforandroid.logger import shprint
from pythonforandroid.util import current_directory
from os.path import join
from os import environ, uname
import sh


Expand Down Expand Up @@ -35,10 +34,9 @@ def build_arch(self, arch):
'-DCMAKE_POSITION_INDEPENDENT_CODE=1',
'-DCMAKE_ANDROID_ARCH_ABI={arch}'.format(arch=arch.arch),
'-DCMAKE_ANDROID_NDK=' + self.ctx.ndk_dir,
'-DCMAKE_C_COMPILER={toolchain}/bin/clang'.format(
toolchain=env['TOOLCHAIN']),
'-DCMAKE_CXX_COMPILER={toolchain}/bin/clang++'.format(
toolchain=env['TOOLCHAIN']),
'-DCMAKE_C_COMPILER={cc}'.format(cc=arch.get_clang_exe()),
'-DCMAKE_CXX_COMPILER={cc_plus}'.format(
cc_plus=arch.get_clang_exe(plus_plus=True)),
Copy link
Member

Choose a reason for hiding this comment

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

So you actually did find a bug already while covering it with your tests? 👏

Copy link
Member Author

Choose a reason for hiding this comment

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

Aaa...this is more a little refactor (since at the time we changed JPEG to be built with Cmake, the function arch.get_clang_exe does not existed, we were forced to got these values in get_recipe_env, since our default compiler was gcc, but with the NDK-r19 migration thing, we use clang, so we can make use of arch.get_clang_exe and remove some lines 😄)

'-DCMAKE_BUILD_TYPE=Release',
'-DCMAKE_INSTALL_PREFIX=./install',
'-DCMAKE_TOOLCHAIN_FILE=' + toolchain_file,
Expand All @@ -54,16 +52,5 @@ def build_arch(self, arch):
_env=env)
shprint(sh.make, _env=env)

def get_recipe_env(self, arch=None, with_flags_in_cc=False):
env = environ.copy()

build_platform = '{system}-{machine}'.format(
system=uname()[0], machine=uname()[-1]).lower()
env['TOOLCHAIN'] = join(self.ctx.ndk_dir, 'toolchains/llvm/'
'prebuilt/{build_platform}'.format(
build_platform=build_platform))

return env


recipe = JpegRecipe()
52 changes: 52 additions & 0 deletions tests/recipes/recipe_ctx.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import os

from pythonforandroid.bootstrap import Bootstrap
from pythonforandroid.distribution import Distribution
from pythonforandroid.recipe import Recipe
from pythonforandroid.build import Context
from pythonforandroid.archs import ArchAarch_64


class RecipeCtx:
"""
An base class for unit testing a recipe. This will create a context so we
can test any recipe using this context. Implement `setUp` and `tearDown`
methods used by unit testing.
"""

ctx = None
arch = None
recipe = None

recipe_name = ""
"The name of the recipe to test."

recipes = []
"""A List of recipes to pass to `Distribution.get_distribution`. Should
contain the target recipe to test as well as a python recipe."""
recipe_build_order = []
"""A recipe_build_order which should take into account the recipe we want
to test as well as the possible dependant recipes"""

def setUp(self):
self.ctx = Context()
self.ctx.ndk_api = 21
self.ctx.android_api = 27
self.ctx._sdk_dir = "/opt/android/android-sdk"
self.ctx._ndk_dir = "/opt/android/android-ndk"
self.ctx.setup_dirs(os.getcwd())
self.ctx.bootstrap = Bootstrap().get_bootstrap("sdl2", self.ctx)
self.ctx.bootstrap.distribution = Distribution.get_distribution(
self.ctx, name="sdl2", recipes=self.recipes
)
self.ctx.recipe_build_order = self.recipe_build_order
self.ctx.python_recipe = Recipe.get_recipe("python3", self.ctx)
self.arch = ArchAarch_64(self.ctx)
self.ctx.ndk_platform = (
f"{self.ctx._ndk_dir}/platforms/"
f"android-{self.ctx.ndk_api}/{self.arch.platform_dir}"
)
self.recipe = Recipe.get_recipe(self.recipe_name, self.ctx)

def tearDown(self):
self.ctx = None
157 changes: 157 additions & 0 deletions tests/recipes/recipe_lib_test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,157 @@
from unittest import mock
from tests.recipes.recipe_ctx import RecipeCtx


class BaseTestForMakeRecipe(RecipeCtx):
"""
An unittest for testing any recipe using the standard build commands
(`configure/make`).

.. note:: Note that Some cmake recipe may need some more specific testing
...but this should cover the basics.
"""

recipe_name = None
recipes = ["python3", "kivy"]
recipe_build_order = ["hostpython3", "python3", "sdl2", "kivy"]
expected_compiler = (
"{android_ndk}/toolchains/llvm/prebuilt/linux-x86_64/bin/clang"
)

sh_command_calls = ["./configure"]
"""The expected commands that the recipe runs via `sh.command`."""

extra_env_flags = {}
"""
This must be a dictionary containing pairs of key (env var) and value.
"""

def __new__(cls, *args):
obj = super().__new__(cls)
if obj.recipe_name is not None:
print(f"We are testing recipe: {obj.recipe_name}")
obj.recipes.append(obj.recipe_name)
Copy link
Member

Choose a reason for hiding this comment

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

It always confuse me how Python can deal with class attributes vs instance attributes.
For instance initially recipes is a class attribute, right? Then we access it from the instance to alter it. Would this modify also the class attribute or just for this instance? 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, class attributes are supposed to be shared for all instances...so I guess that the answer is yes, we are also modifying class attributes (the thing is that recipes will grow up as long as we perform the tests...but shouldn't affect the test itself at all)

Copy link
Member

Choose a reason for hiding this comment

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

Exactly, in this case why not making it explicit and access it through cls? That would avoid confusion. Do you see what I mean? This is rather nitpicking, but at the same time I personally feel it's important to get the habit about thinking/clarifying theses when possible since it can cause nasty side effects sometimes

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 you are right, I'm preparing another PR to address all this we recently talk that cannot be done here. Later I will push it.

Thanks again @AndreMiras, your reviews always makes the code better 👍

obj.recipe_build_order.insert(1, obj.recipe_name)
return obj
Copy link
Member

Choose a reason for hiding this comment

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

I guess you're using the __new__ over the __init__() for a reason, but I have to ask. Why can't this be dealt with it? Something like:

def __init__(self, *args, **kwargs):
    if self.recipe_name is not None:
...

Copy link
Member Author

Choose a reason for hiding this comment

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

Aaa... I faced several issues along the way with this: duplicated tests, tests not initialized...:laughing:..finally using __new__ is what it worked and I forgot about this lines until now...

I just made a test with __init__ and it also can work so I'm considering to change __new__ with __init__ (I think it's better to not mess with __new__ if we can do it with __init__)

...so...@AndreMiras, let's go with __init__?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to not mess with __new__ if we can do it with __init__

That recaps my feeling pretty well. I'm not against __new__ in general, but it needs to be clearly justified. But yes again if you can't make it work consistently with __init__ then I'm go for it.


@mock.patch("pythonforandroid.recipe.Recipe.check_recipe_choices")
@mock.patch("pythonforandroid.build.ensure_dir")
@mock.patch("pythonforandroid.archs.glob")
@mock.patch("pythonforandroid.archs.find_executable")
def test_get_recipe_env(
self,
mock_find_executable,
mock_glob,
mock_ensure_dir,
mock_check_recipe_choices,
):
"""
Test that get_recipe_env contains some expected arch flags and that
some internal methods has been called.
"""
mock_find_executable.return_value = self.expected_compiler.format(
android_ndk=self.ctx._ndk_dir
)
mock_glob.return_value = ["llvm"]
mock_check_recipe_choices.return_value = sorted(
self.ctx.recipe_build_order
)

# make sure the arch flags are in env
env = self.recipe.get_recipe_env(self.arch)
for flag in self.arch.arch_cflags:
self.assertIn(flag, env["CFLAGS"])
self.assertIn(
f"-target {self.arch.target}",
env["CFLAGS"],
)

for flag, value in self.extra_env_flags.items():
self.assertIn(value, env[flag])

# make sure that the mocked methods are actually called
mock_glob.assert_called()
mock_ensure_dir.assert_called()
mock_find_executable.assert_called()
mock_check_recipe_choices.assert_called()

@mock.patch("pythonforandroid.util.chdir")
@mock.patch("pythonforandroid.build.ensure_dir")
@mock.patch("pythonforandroid.archs.glob")
@mock.patch("pythonforandroid.archs.find_executable")
def test_build_arch(
self,
mock_find_executable,
mock_glob,
mock_ensure_dir,
mock_current_directory,
):
mock_find_executable.return_value = self.expected_compiler.format(
android_ndk=self.ctx._ndk_dir
)
mock_glob.return_value = ["llvm"]

# Since the following mocks are dynamic,
# we mock it inside a Context Manager
with mock.patch(
f"pythonforandroid.recipes.{self.recipe_name}.sh.Command"
) as mock_sh_command, mock.patch(
f"pythonforandroid.recipes.{self.recipe_name}.sh.make"
) as mock_make:
self.recipe.build_arch(self.arch)

# make sure that the mocked methods are actually called
for command in self.sh_command_calls:
self.assertIn(
mock.call(command),
mock_sh_command.mock_calls,
)
mock_make.assert_called()
mock_glob.assert_called()
mock_ensure_dir.assert_called()
mock_current_directory.assert_called()
mock_find_executable.assert_called()


class BaseTestForCmakeRecipe(BaseTestForMakeRecipe):
"""
An unittest for testing any recipe using `cmake`. It inherits from
`BaseTestForMakeRecipe` but we override the build method to match the cmake
build method.

.. note:: Note that Some cmake recipe may need some more specific testing
...but this should cover the basics.
"""

@mock.patch("pythonforandroid.util.chdir")
@mock.patch("pythonforandroid.build.ensure_dir")
@mock.patch("pythonforandroid.archs.glob")
@mock.patch("pythonforandroid.archs.find_executable")
def test_build_arch(
self,
mock_find_executable,
mock_glob,
mock_ensure_dir,
mock_current_directory,
):
mock_find_executable.return_value = self.expected_compiler.format(
android_ndk=self.ctx._ndk_dir
)
mock_glob.return_value = ["llvm"]

# Since the following mocks are dynamic,
# we mock it inside a Context Manager
with mock.patch(
f"pythonforandroid.recipes.{self.recipe_name}.sh.make"
) as mock_make, mock.patch(
f"pythonforandroid.recipes.{self.recipe_name}.sh.cmake"
) as mock_cmake:
self.recipe.build_arch(self.arch)

# make sure that the mocked methods are actually called
mock_cmake.assert_called()
mock_make.assert_called()
mock_glob.assert_called()
mock_ensure_dir.assert_called()
mock_current_directory.assert_called()
mock_find_executable.assert_called()
10 changes: 10 additions & 0 deletions tests/recipes/test_freetype.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import unittest
from tests.recipes.recipe_lib_test import BaseTestForMakeRecipe


class TestFreetypeRecipe(BaseTestForMakeRecipe, unittest.TestCase):
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to inherit from TestCase suddenly?

Copy link
Member Author

Choose a reason for hiding this comment

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

We actually need it because nor RecipeCtx nor BaseTestForMakeRecipe inherits from unittest.TestCase

Copy link
Member

Choose a reason for hiding this comment

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

<nitpick-troll>
What is TestCase providing that you need in this case? Would tests fail or not run if we drop it? :trollface:
We don't even use setup/teardown here. Yes I know it's hard to say unittest.TestCase goodbye 😂
Honestly, feel free to not address that comment, I'm just challenging your habits 😉
</nitpick-troll>

Copy link
Member Author

Choose a reason for hiding this comment

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

Jajajaja...No man, we need this or the tests won't run...we are using setUp/teardown for all library recipes we introduced/modified, since all of them inherits from RecipeCtx...or directly or via BaseTestForMakeRecipe/BaseTestForCmakeRecipe

...this fuc*** Python Inheritance makes the things hard to track sometimes...:laughing:

Copy link
Member

Choose a reason for hiding this comment

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

Damn it, you got the point 😂
But yes definitely OOP sometimes 😅
Thanks man

"""
An unittest for recipe :mod:`~pythonforandroid.recipes.freetype`
"""
recipe_name = "freetype"
sh_command_calls = ["./configure"]
16 changes: 3 additions & 13 deletions tests/recipes/test_gevent.py
Original file line number Diff line number Diff line change
@@ -1,21 +1,11 @@
import unittest
from mock import patch
from pythonforandroid.archs import ArchARMv7_a
from pythonforandroid.build import Context
from pythonforandroid.recipe import Recipe
from tests.recipes.recipe_ctx import RecipeCtx


class TestGeventRecipe(unittest.TestCase):
class TestGeventRecipe(RecipeCtx, unittest.TestCase):
Copy link
Member

Choose a reason for hiding this comment

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

Also here, I think we can kill the TestCase inheritance, can't we?

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 same than this


def setUp(self):
"""
Setups recipe and context.
"""
self.context = Context()
self.context.ndk_api = 21
self.context.android_api = 27
self.arch = ArchARMv7_a(self.context)
self.recipe = Recipe.get_recipe('gevent', self.context)
recipe_name = "gevent"

def test_get_recipe_env(self):
"""
Expand Down
10 changes: 10 additions & 0 deletions tests/recipes/test_harfbuzz.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import unittest
from tests.recipes.recipe_lib_test import BaseTestForMakeRecipe


class TestHarfbuzzRecipe(BaseTestForMakeRecipe, unittest.TestCase):
"""
An unittest for recipe :mod:`~pythonforandroid.recipes.harfbuzz`
"""
recipe_name = "harfbuzz"
sh_command_calls = ["./configure"]
Loading