Unit tests Recipe download feature

[AndreMiras]

Currently unit tests the following:

• Recipe.download_if_necessary()
• Recipe.download()

Next up is Recipe.download_file(). This is more difficult and will be
addressed in subsequent pull request.

[AndreMiras]

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.

[AndreMiras]

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

[inclement]

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

[AndreMiras]

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

[inclement]

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.

[AndreMiras]

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

[opacam]

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

[AndreMiras]

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

[inclement]

Thanks!