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

opacam · 2019-09-12T12:05:38Z

AndreMiras · 2019-09-14T10:02:48Z

AndreMiras

AndreMiras · 2019-09-14T10:04:24Z

pythonforandroid/recipes/jpeg/__init__.py

-                        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)),


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

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 😄)

AndreMiras · 2019-09-14T10:15:54Z

tests/recipes/recipe_lib_test.py

+            print(f"We are testing recipe: {obj.recipe_name}")
+            obj.recipes.append(obj.recipe_name)
+            obj.recipe_build_order.insert(1, obj.recipe_name)
+            return obj


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: ...

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__?

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.

AndreMiras · 2019-09-14T10:19:21Z

tests/recipes/recipe_lib_test.py

+        obj = super(BaseTestForMakeRecipe, cls).__new__(cls)
+        if obj.recipe_name is not None:
+            print(f"We are testing recipe: {obj.recipe_name}")
+            obj.recipes.append(obj.recipe_name)


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? 😄

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)

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

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 👍

AndreMiras · 2019-09-14T10:22:21Z

tests/recipes/recipe_lib_test.py

+            self.recipe.build_arch(self.arch)
+
+            # make sure that the mocked methods are actually called
+            mock_glob.assert_called()


Only the call needs to be in the context manager. The rest can be outside

AndreMiras · 2019-09-14T10:23:14Z

tests/recipes/recipe_lib_test.py

+            self.recipe.build_arch(self.arch)
+
+            # make sure that the mocked methods are actually called
+            mock_glob.assert_called()


Same here, you can take this one level down (if you want). It doesn't need to be in the context manager

AndreMiras · 2019-09-14T10:24:16Z

tests/recipes/test_freetype.py

+from tests.recipes.recipe_lib_test import BaseTestForMakeRecipe
+
+
+class TestFreetypeRecipe(BaseTestForMakeRecipe, unittest.TestCase):


Why do we need to inherit from TestCase suddenly?

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

<nitpick-troll>
What is TestCase providing that you need in this case? Would tests fail or not run if we drop it?
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>

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:

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

AndreMiras · 2019-09-14T10:24:55Z

tests/recipes/test_gevent.py



-class TestGeventRecipe(unittest.TestCase):
+class TestGeventRecipe(RecipeCtx, unittest.TestCase):


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

The same than this

AndreMiras · 2019-09-14T10:26:13Z

tests/recipes/test_libgeos.py

+    ):
+        # We overwrite the base test method because we
+        # want to avoid any file/directory creation
+        super(TestLibgeosRecipe, self).test_build_arch()


I think we can actually ditch all theses Python2 style super() call and use the Python3 one if we wanted. At least in unit tests

AndreMiras · 2019-09-14T10:32:23Z

opacam · 2019-09-14T16:59:35Z

AndreMiras

AndreMiras · 2019-09-14T17:41:27Z

tests/recipes/recipe_lib_test.py

+                    mock.call(command),
+                    mock_sh_command.mock_calls,
+                )
+            mock_make.assert_called()


FYI actually this check can also be outside the context manager. The only thing that needs to be in the context manager is the call that you want to be mocked, hence self.recipe.build_arch(self.arch)

AndreMiras · 2019-09-14T17:43:50Z

tests/recipes/recipe_lib_test.py

+            self.recipe.build_arch(self.arch)
+
+            mock_cmake.assert_called()
+            mock_make.assert_called()


Same here it could also be outside. I know it can feel weird at first because we assume the variable is scoped to the that bock, but not. Unlike javascript let this Python variable is function scoped, just like javascript var.

Thanks man, you are right 😄, let me change this and try if a force-push with a rebase on top of develop branch triggers the build...

AndreMiras · 2019-09-14T17:46:10Z

opacam · 2019-09-14T18:01:49Z

AndreMiras

AndreMiras · 2019-09-14T18:34:28Z

opacam · 2019-09-14T18:36:51Z

AndreMiras · 2019-09-14T18:38:35Z

opacam · 2019-09-14T18:40:00Z

opacam · 2019-09-14T22:10:10Z

@TheSin-

@TheSin-

AndreMiras previously approved these changes Sep 14, 2019

View reviewed changes

opacam dismissed AndreMiras’s stale review via 4f54abc September 14, 2019 17:30

AndreMiras previously approved these changes Sep 14, 2019

View reviewed changes

opacam added 22 commits September 14, 2019 20:20

[test] Add module tests.recipe_ctx

11df8e0

A helper module to test recipes. Here we will initialize a `Context` to test recipes.

[test] Refactor setUp/tearDown for test_icu

b242289

[test] Refactor setUp/tearDown for test_pyicu

bd1018b

[test] Refactor setUp for test_reportlab

add66c2

[test] Refactor setUp for test_gevent

52978e1

[test] Add module tests.recipe_lib_test

04be9cb

A helper module to test recipes which will allow to test any recipe using `configure/make` commands.

[test] Add test for libffi

5e2c08f

[test] Add test for libexpat

5616cbd

[test] Add test for libcurl

3b0478e

[test] Add test for libiconv

bbf4d64

[test] Add test for libogg

c4deb10

[test] Add test for libpq

7061cc5

[test] Add test for libsecp256k1

18bda8e

[test] Add test for libshine

054eb02

[test] Add test for libvorbis

6f3758a

[test] Add test for libx264

a68522a

[test] Add test for libxml2

6370455

[test] Add test for libxslt

bc93051

[test] Add test for png

ed8524f

[test] Add test for freetype

fd4cbc0

[test] Add test for harfbuzz

d3d6e1d

[test] Add test for openssl

5cca1f5

opacam added 9 commits September 14, 2019 20:20

[test] Add BaseTestForCmakeRecipe

a95b7e9

[test] Add test for jpeg and clean code

0d48b40

We can remove the `get_recipe_env` because the environment that we use is already using a clang as default compiler (not the case when we migrated the jpeg recipe to use `cmake`)...so we can do a little refactor :)

[test] Add test for snappy

47bba46

[test] Add test for leveldb

f77d693

[test] Add test for libgeos

13a48ea

[test] Add test for libmysqlclient

2e73dc2

[test] Add test for openal

b27376b

[test] Make the super calls Python3 style

fcc71f2

[test] Move mock tests outside context manager...

80104c4

Because there is no need to do it there. Also rewrote the inline comments.

opacam dismissed AndreMiras’s stale review via 80104c4 September 14, 2019 18:25

opacam force-pushed the feature-test-libs branch from 4f54abc to 80104c4 Compare September 14, 2019 18:25

AndreMiras approved these changes Sep 14, 2019

View reviewed changes

opacam merged commit be7abe9 into kivy:develop Sep 14, 2019

opacam deleted the feature-test-libs branch September 14, 2019 22:11

opacam mentioned this pull request Sep 15, 2019

Recipes tests enhancements #1984

Merged

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

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

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!