Skip to content
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

Jump to bottom

Unit tests Recipe download feature #1946

Merged
merged 1 commit into from
Jul 31, 2019
Merged

Unit tests Recipe download feature #1946

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions pythonforandroid/recipe.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,10 +156,10 @@ def report_hook(index, blksize, size):
while True:
try:
urlretrieve(url, target, report_hook)
except OSError as e:
except OSError:
attempts += 1
if attempts >= 5:
raise e
raise
Copy link
Member Author

Choose a reason for hiding this comment

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

Minor changes that I came across, doesn't harm, but sightly simplify things.
More serious refactoring will come after this module is properly covered by unit tests

stdout.write('Download failed retrying in a second...')
time.sleep(1)
continue
Expand Down
64 changes: 64 additions & 0 deletions tests/test_recipe.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,28 @@
import types
import unittest
import warnings
import mock
from backports import tempfile
from pythonforandroid.build import Context
from pythonforandroid.recipe import Recipe, import_recipe


def patch_logger(level):
return mock.patch('pythonforandroid.recipe.{}'.format(level))


def patch_logger_info():
return patch_logger('info')


def patch_logger_debug():
return patch_logger('debug')


class DummyRecipe(Recipe):
pass


class TestRecipe(unittest.TestCase):

def test_recipe_dirs(self):
Expand Down Expand Up @@ -58,3 +76,49 @@ def test_import_recipe(self):
module = import_recipe(name, pathname)
assert module is not None
assert recorded_warnings == []

def test_download_if_necessary(self):
"""
Download should happen via `Recipe.download()` only if the recipe
specific environment variable is not set.
"""
# download should happen as the environment variable is not set
recipe = DummyRecipe()
with mock.patch.object(Recipe, 'download') as m_download:
recipe.download_if_necessary()
assert m_download.call_args_list == [mock.call()]
# after setting it the download should be skipped
env_var = 'P4A_test_recipe_DIR'
env_dict = {env_var: '1'}
with mock.patch.object(Recipe, 'download') as m_download, mock.patch.dict(os.environ, env_dict):
recipe.download_if_necessary()
assert m_download.call_args_list == []

def test_download(self):
"""
Verifies the actual download gets triggered when the URL is set.
"""
# test with no URL set
recipe = DummyRecipe()
with patch_logger_info() as m_info:
recipe.download()
assert m_info.call_args_list == [
mock.call('Skipping test_recipe download as no URL is set')]
# when the URL is set `Recipe.download_file()` should be called
filename = 'Python-3.7.4.tgz'
url = 'https://www.python.org/ftp/python/3.7.4/{}'.format(filename)
recipe._url = url
recipe.ctx = Context()
with (
patch_logger_debug()) as m_debug, (
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of this line break styling, but I'm willing to believe there's no great option for with statements.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not fan either, I usually use backslash \ for context manager, but @opacam suggested he would prefer to avoid it #1928 (comment)
I didn't like much how not aligned the context managers were with his suggestion so I tried something else here while still avoiding the backslashes.
Any other suggestion are more than welcome. I think black can't come soon enough...

Copy link
Member

Choose a reason for hiding this comment

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

I also dislike backslashes (as is well documented :p), but I think they would be nicer here. I don't mind though, when there's no good solution we have to choose something.

I've just googled and found https://docs.python.org/3/library/contextlib.html#contextlib.ExitStack which is new to me. With this method you would do:


with ExitStack() as stack:
    m_debug = stack.enter_context(patch_logger_debug())
    ...

It's obviously less nice in that you have to know what ExitStack is to understand it, but at least the code is nicely formatted. I think I wouldn't be averse to using this, but I'm not saying we should either.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for sharing!
Yes I also came across that stackoverflow answer and wasn't so convinced either so I scrolled down further and saw that parenthesis alternative. Maybe it's also a matter of getting used to it.
So yes it feels uneasy to find a satisfying approach on that one it seems. I'll keep it this way then until maybe we find a consensus on how to approach it

Copy link
Member

Choose a reason for hiding this comment

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

Ok, my three cents to this:

as per formatting, I would go for the following, basically because I usually trigger an script when I have a doubt (that runs python's black module targeting the file I want)...and this is what I got for the discussed lines:

        with (patch_logger_debug()) as m_debug, (
            mock.patch.object(Recipe, 'download_file')
        ) as m_download_file, (
            mock.patch('pythonforandroid.recipe.sh.touch')
        ) as m_touch, (
            tempfile.TemporaryDirectory()
        ) as temp_dir:
            recipe.ctx.setup_dirs(temp_dir)
            recipe.download()

Do you noticed that the aligned parenthesis helps with the reading? also there is some more space available...but...still may be confusing...anyway...I really prefer decorators when possible...it's cleaner I think, so I would split the test in two using mock.patch as a decorator, something like:

    @patch_logger_info()
    def test_download_url_not_set(self, m_info):
        """
        Verifies that no download happens when URL is not set.
        """
        # test with no URL set
        recipe = DummyRecipe()
        recipe.download()
        assert m_info.call_args_list == [
            mock.call('Skipping test_recipe download as no URL is set')]

    @mock.patch.object(Recipe, 'download_file')
    @mock.patch('pythonforandroid.recipe.sh.touch')
    @patch_logger_debug()
    def test_download_url_is_set(self, m_debug, m_touch, m_download_file):
        """
        Verifies the actual download gets triggered when the URL is set.
        """
        filename = 'Python-3.7.4.tgz'
        url = 'https://www.python.org/ftp/python/3.7.4/{}'.format(filename)
        recipe = DummyRecipe()
        recipe._url = url
        recipe.ctx = Context()
        with tempfile.TemporaryDirectory() as temp_dir:
            recipe.ctx.setup_dirs(temp_dir)
            recipe.download()
        assert m_download_file.call_args_list == [mock.call(url, filename)]
        assert m_debug.call_args_list == [
            mock.call(
                'Downloading test_recipe from '
                'https://www.python.org/ftp/python/3.7.4/Python-3.7.4.tgz')]
        assert m_touch.call_count == 1

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for your input.
Black will definitely help if we add it. I'm just worrying because I know it will completely mess up the git blame output.
Yes for splitting the test, I also actually prefer to split per concerns, but I know sometimes people get mad that I do so. For instance I like inline functions.
However I'm not a huge fan of decorator because I like to narrow contexts as much as possible. I agree that stacking decorators is often easier to read. But I still prefer context managers because of the way it allows to precisely document where the patching needs to happen

mock.patch.object(Recipe, 'download_file')) as m_download_file, (
mock.patch('pythonforandroid.recipe.sh.touch')) as m_touch, (
tempfile.TemporaryDirectory()) as temp_dir:
recipe.ctx.setup_dirs(temp_dir)
recipe.download()
assert m_download_file.call_args_list == [mock.call(url, filename)]
assert m_debug.call_args_list == [
mock.call(
'Downloading test_recipe from '
'https://www.python.org/ftp/python/3.7.4/Python-3.7.4.tgz')]
assert m_touch.call_count == 1
1 change: 1 addition & 0 deletions tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ deps =
pytest
virtualenv
py3: coveralls
backports.tempfile
Copy link
Member Author

Choose a reason for hiding this comment

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

Basically backports Python TemporaryDirectory context manager to Python2.
It clearly simply things up as it avoids having to reinvent the wheel while still supporting Python2. This dep is just a test dep and doesn't impact the rests of the project.

# makes it possible to override pytest args, e.g.
# tox -- tests/test_graph.py
commands = pytest {posargs:tests/}
Expand Down