Unit tests Recipe download feature #1946
Conversation
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.
601f03c to
721e7c5
Compare
| @@ -8,6 +8,7 @@ deps = | |||
| pytest | |||
| virtualenv | |||
| py3: coveralls | |||
| backports.tempfile | |||
There was a problem hiding this comment.
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.
| attempts += 1 | ||
| if attempts >= 5: | ||
| raise e | ||
| raise |
There was a problem hiding this comment.
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
| recipe._url = url | ||
| recipe.ctx = Context() | ||
| with ( | ||
| patch_logger_debug()) as m_debug, ( |
There was a problem hiding this comment.
I'm not a fan of this line break styling, but I'm willing to believe there's no great option for with statements.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
Currently unit tests the following:
Recipe.download_if_necessary()Recipe.download()Next up is
Recipe.download_file(). This is more difficult and will beaddressed in subsequent pull request.