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.

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

Increases toolchain.py test coverage #1933

Merged
merged 1 commit into from
Jul 27, 2019

Conversation

AndreMiras
Copy link
Member

Increases test_create() coverage demonstrating crash referenced in:
#1867 (comment)
Adds test_recipes() checking if it prints out without crashing.

@AndreMiras
Copy link
Member Author

This is still in WIP, because for some reason the test_recipes() later make the tests/recipes/test_reportlab.py fail. I need to investigate, but this probably has something to do with the way we dynamically load recipes.
For records, the error is:

__________________________________________________ TestReportLabRecipe.test_prebuild_arch __________________________________________________

self = <test_reportlab.TestReportLabRecipe testMethod=test_prebuild_arch>

    def test_prebuild_arch(self):
        """
        Makes sure `prebuild_arch()` runs without error and patches `setup.py`
        as expected.
        """
        # `prebuild_arch()` dynamically replaces strings in the `setup.py` file
        setup_path = os.path.join(self.recipe_dir, 'setup.py')
        with open(setup_path, 'w') as setup_file:
            setup_file.write('_FT_LIB_\n')
            setup_file.write('_FT_INC_\n')
    
        # these sh commands are not relevant for the test and need to be mocked
        with \
                patch('sh.patch'), \
                patch('sh.touch'), \
                patch('sh.unzip'), \
                patch('os.path.isfile'):
>           self.recipe.prebuild_arch(self.arch)

tests/recipes/test_reportlab.py:52:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
pythonforandroid/recipes/reportlab/__init__.py:33: in prebuild_arch
    ft_dir = ft.get_build_dir(arch.arch)
pythonforandroid/recipe.py:275: in get_build_dir
    return join(self.get_build_container_dir(arch), self.name)
pythonforandroid/recipe.py:264: in get_build_container_dir
    dir_name, '{}__ndk_target_{}'.format(arch, self.ctx.ndk_api))
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <pythonforandroid.build.Context object at 0x7f86ad48e710>

    @property
    def ndk_api(self):
        '''The API number compile against'''
        if self._ndk_api is None:
>           raise ValueError('Tried to access ndk_api but it has not '
                             'been set - this should not happen, something '
                             'went wrong!')
E           ValueError: Tried to access ndk_api but it has not been set - this should not happen, something went wrong!

pythonforandroid/build.py:203: ValueError
----------------------------------------------------------- Captured stderr call -----------------------------------------------------------

Increases `test_create()` coverage demonstrating crash referenced in:
<kivy#1867 (comment)>
Adds `test_recipes()` checking if it prints out without crashing.
@AndreMiras AndreMiras changed the title WIP Increases toolchain.py test coverage Increases toolchain.py test coverage Jul 27, 2019
@AndreMiras
Copy link
Member Author

I've fixed the test and we now have a +15.8% of coverage (to 53.148%), awesome.
The problem cames from the static Recipe.recipes attribute. We may encounter similar issues in the future as we write more tests. I think the fix I've have is OK enough for now as I don't want to bring tearDown, fixtures or context manager yet.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

in overall it seems to go into the right direction, but how much the one mock test has blown up worries me a little. it just seems to me like doing a sort of small integration-style test (I know I suggest this a lot 😂 but hear me out 😄 ) might end up more maintainable in the long run - but obviously just my opinion 🤷‍♀️

but after some thinking that does seem unavoidable given how central this function is and how long it'd otherwise run, so well done 👍 I still want to test it though before approving it, hang on

tests/test_toolchain.py Show resolved Hide resolved
tests/test_toolchain.py Show resolved Hide resolved
tests/test_toolchain.py Show resolved Hide resolved
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

the tests ran through locally on my pc fine. they look like a neat addition! apart from that one formatting comment (and the misguided resolved others, ignore those 😂 ) I have nothing else to suggest 🙂 👍

@AndreMiras
Copy link
Member Author

Thanks for the quick review @Jonast. Yes I do agree with your strike-through comment regarding the integration test. It's indeed far from being a strickly unit test indeed, but it was easy enough to write it that way while also helping raising issues, increasing coverage and not introducing any kind of lag/flakkness in our testing, so it feel good enough for now

@AndreMiras
Copy link
Member Author

@opacam I'm merging it prior your review, but I'll keep an eye on your post merge comments if you have any so I can address on subsequent PRs

@AndreMiras AndreMiras merged commit df687c5 into kivy:develop Jul 27, 2019
@AndreMiras AndreMiras deleted the feature/test_toolchain branch July 27, 2019 22:53
conflicts: ['python2']
optional depends: ['sqlite3', 'libffi', 'openssl']
```
"""
Copy link
Member

Choose a reason for hiding this comment

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

minor: I'm not sure how it will be rendered this docstring in sphinx...maybe I would use a code-block directive, something like:

        Prints recipes basic info, e.g.
        .. code-block:: bash

            python3      3.7.1
            depends: ['hostpython3', 'sqlite3', 'openssl', 'libffi']
            conflicts: ['python2']
            optional depends: ['sqlite3', 'libffi', 'openssl']

But, @AndreMiras, don't need to change this, if I remember well, I think that we don't have enabled the build of technical documentation, so it wont be visible for now.

Apart from this, I would like to mention that overall it looks good, clean and very readable.Also, I made tox test on my machine, with current develop branch , since it's already merged, without any issue.

@AndreMiras, thanks for this PR, good job, as usual 👍

and @Jonast, thanks for your review, I was out for the weekend and I was unable to take care of the review, so yours sped up the merging 😄

Copy link
Member Author

@AndreMiras AndreMiras Jul 31, 2019

Choose a reason for hiding this comment

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

I don't know how I missed that comment. I'd be happy to address it still.
Edit: covered as part of #1948

AndreMiras added a commit to AndreMiras/python-for-android that referenced this pull request Jul 31, 2019
This context manager will makes it possible to rollback an object state
after leaving the context manager.
This is a follow up of
<kivy#1867 (comment)>

Also address docstring format as suggested by @opacam in
<kivy#1933 (comment)>
AndreMiras added a commit to AndreMiras/python-for-android that referenced this pull request Jul 31, 2019
This context manager will makes it possible to rollback an object state
after leaving the context manager.
This is a follow up of
<kivy#1867 (comment)>

Also address docstring format as suggested by @opacam in
<kivy#1933 (comment)>
AndreMiras added a commit to AndreMiras/python-for-android that referenced this pull request Jul 31, 2019
This context manager will makes it possible to rollback an object state
after leaving the context manager.
This is a follow up of
<kivy#1867 (comment)>

Also address docstring format as suggested by @opacam in
<kivy#1933 (comment)>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants