-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Changes from all commits
11df8e0
b242289
bd1018b
add66c2
52978e1
04be9cb
5e2c08f
5616cbd
3b0478e
bbf4d64
c4deb10
7061cc5
18bda8e
054eb02
6f3758a
a68522a
6370455
bc93051
ed8524f
fd4cbc0
d3d6e1d
5cca1f5
a95b7e9
0d48b40
47bba46
f77d693
13a48ea
2e73dc2
b27376b
fcc71f2
80104c4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 |
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess you're using the def __init__(self, *args, **kwargs):
if self.recipe_name is not None:
... There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 I just made a test with ...so...@AndreMiras, let's go with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That recaps my feeling pretty well. I'm not against |
||
|
||
@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() |
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): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need to inherit from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We actually need it because nor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ...this fuc*** There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Damn it, you got the point 😂 |
||
""" | ||
An unittest for recipe :mod:`~pythonforandroid.recipes.freetype` | ||
""" | ||
recipe_name = "freetype" | ||
sh_command_calls = ["./configure"] |
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): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also here, I think we can kill the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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): | ||
""" | ||
|
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"] |
There was a problem hiding this comment.
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? 👏
There was a problem hiding this comment.
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 inget_recipe_env
, since our default compiler wasgcc
, but with the NDK-r19 migration thing, we useclang
, so we can make use ofarch.get_clang_exe
and remove some lines 😄)