Basic toolchain.py unit tests

[AndreMiras]

First simple set of tests for toolchain.py
Also refactors Context.prepare_build_environment() slightly. Splits
concerns to improve readability and ease unit testing.

[AndreMiras]

The toolchain.py looks painful to unit test. Still we should slowly, but surely get there.
This already increased the coverage slightly COVERAGE INCREASED (+4.4%) TO 36.775%, refs https://coveralls.io/jobs/51258249
BTW I saw @KeyWeeUsr was requested for review, that was a misclick, sorry for the spam

[opacam]

Good, some more coverage is welcome, thanks to take care of this 😄

Mmm...it seems that my local tests fails:

opacam@DEPOBOX:~/Devel/python-for-android$ tox -r -e py37 -- tests/test_toolchain.py
GLOB sdist-make: /home/opacam/Devel/python-for-android/setup.py
py37 recreate: /home/opacam/Devel/python-for-android/.tox/py37
py37 installdeps: mock, pytest, virtualenv
py37 inst: /home/opacam/Devel/python-for-android/.tox/.tmp/package/3/python-for-android-2019.7.8.1.dev0.zip
py37 installed: appdirs==1.4.3,atomicwrites==1.3.0,attrs==19.1.0,colorama==0.4.1,importlib-metadata==0.18,Jinja2==2.10.1,MarkupSafe==1.1.1,mock==3.0.5,more-itertools==7.2.0,packaging==19.0,pep517==0.5.0,pluggy==0.12.0,py==1.8.0,pyparsing==2.4.1.1,pytest==5.0.1,python-for-android==2019.7.8.1.dev0,pytoml==0.1.21,sh==1.12.14,six==1.12.0,virtualenv==16.7.0,wcwidth==0.1.7,zipp==0.5.2
py37 run-test-pre: PYTHONHASHSEED='1110765035'
py37 run-test: commands[0] | pytest tests/test_toolchain.py
============================================================================================================================= test session starts ==============================================================================================================================
platform linux -- Python 3.7.4, pytest-5.0.1, py-1.8.0, pluggy-0.12.0
cachedir: .tox/py37/.pytest_cache
rootdir: /home/opacam/Devel/python-for-android
collected 4 items                                                                                                                                                                                                                                                              

tests/test_toolchain.py ..F.                                                                                                                                                                                                                                             [100%]

=================================================================================================================================== FAILURES ===================================================================================================================================
_________________________________________________________________________________________________________________________ TestToolchainCL.test_create __________________________________________________________________________________________________________________________

self = <test_toolchain.TestToolchainCL object at 0x7f28b7837c10>

    def test_create(self):
        """
        Basic `create` distribution test.
        """
        argv = ['toolchain.py', 'create', '--sdk-dir=/tmp/android-sdk', '--ndk-dir=/tmp/android-ndk']
        with mock.patch('sys.argv', argv), \
                mock.patch('pythonforandroid.build.get_available_apis') as m_get_available_apis, \
                mock.patch('pythonforandroid.build.get_toolchain_versions') as m_get_toolchain_versions, \
                mock.patch('pythonforandroid.build.get_ndk_platform_dir') as m_get_ndk_platform_dir, \
                mock.patch('pythonforandroid.build.get_cython_path') as m_get_cython_path, \
                mock.patch('pythonforandroid.toolchain.build_dist_from_args') as m_build_dist_from_args:
            m_get_available_apis.return_value = [27]
            m_get_toolchain_versions.return_value = (['4.9'], True)
            m_get_ndk_platform_dir.return_value = (
                '/tmp/android-ndk/platforms/android-21/arch-arm', True)
            ToolchainCL()
        assert m_get_available_apis.call_args_list == [mock.call('/tmp/android-sdk')]
        assert m_get_toolchain_versions.call_args_list == [
            mock.call('/tmp/android-ndk', mock.ANY)]
        assert m_get_cython_path.call_args_list == [mock.call()]
>       assert m_build_dist_from_args.call_count == 1
E       AssertionError: assert 0 == 1
E        +  where 0 = <MagicMock name='build_dist_from_args' id='139812854396752'>.call_count

tests/test_toolchain.py:53: AssertionError
----------------------------------------------------------------------------------------------------------------------------- Captured stderr call ---------------------------------------------------------------------------------------------------------------------------

The same error when running for python2...it seems that build_dist_from_args is never called in my local tests, but it clearly works for CI tests...:thinking:

[opacam]

Again here, the backslashes in this context manager:

        with mock.patch('sys.argv', argv), mock.patch(
            'pythonforandroid.build.get_available_apis'
        ) as m_get_available_apis, mock.patch(
            'pythonforandroid.build.get_toolchain_versions'
        ) as m_get_toolchain_versions, mock.patch(
            'pythonforandroid.build.get_ndk_platform_dir'
        ) as m_get_ndk_platform_dir, mock.patch(
            'pythonforandroid.build.get_cython_path'
        ) as m_get_cython_path, mock.patch(
            'pythonforandroid.toolchain.build_dist_from_args'
        ) as m_build_dist_from_args:
            m_get_available_apis.return_value = [27]
            m_get_toolchain_versions.return_value = (['4.9'], True)
            m_get_ndk_platform_dir.return_value = (
                '/tmp/android-ndk/platforms/android-21/arch-arm',
                True,
            )

This will also be pep8 friendly

[AndreMiras]

I also hate backslashes, but for context managers, I haven't found a more readable way to write it so far.
I'm not yet convinced this way is more readable, but I gave it a try nonetheless 😄

[opacam]

Ei, @AndreMiras, good work 👍

Note: I know that there is no general consents about keeping lines below 80 (so I stick to pep 8 standards/recommendations) ...I think that produces more readable code and github diffs, so I always try to keep a maximum of 79 character

[ghost]

Wouldn't something like this be a little easier to understand:

subprocess.check_output([
    sys.executable, "-c", "from pythonforandroid.toolchain import main(); main", "--help"
], cwd=os.path.join(os.path.dirname(__file__), ".."))

Or is that only me? Mock is fine but if it's not needed I would still rather use standard methods

---

There is also a lot of repetition of this block in particular:

        with mock.patch('sys.argv', argv), pytest.raises(SystemExit) as ex_info, \
                mock.patch('argparse.ArgumentParser.print_help') as m_print_help:
            ToolchainCL()

No matter if we end up using mock or not, maybe a helper function for that would be useful?

[AndreMiras]

The subprocess is overkill to me, it's really too much top level integration test.
Regarding the patching helper, yes I agree, I actually do this in my projects and at work, but since it's not common in p4a I didn't dare to introduce it.
I'll then

[ghost]

it's really too much top level integration test.

I'll let you decide but just as a note: that seems like an irrelevant point to me, because isn't this what the entire purpose is? Check if the overall tool responds fine to --help? It seems a little to me with this particular argument you're trying to isolate a component here with no benefit other than possibly complicating the test, and also removing it further from the actual usage later. But of course there are other completely valid reasons to stick with your current solution, e.g. if you find it more readable (which I don't but I can see others maybe would)

But keep in mind my proposal would also possibly reduce the maintenance burden if toolchain.py is ever heavily refactored to also adjust the test here, because it nicely stays out of how the functionality needs to be performed - even more so than the mock code I mean - as long as it works right without a crash, which I think is an additional benefit!

But that's just my personal opinion of course and I don't think this should hold off a merge

[AndreMiras]

Yes I definitely don't share this point of view regarding readability, I find that subprocess.check_output absolutely ugly. Not to say about parsing eventual errors that it could output or the fact that's unclear which python executable it would run, the fact that you need to play around with the cwd. For me it's just a big nope. I've hardly ever seen people writing unit tests calling subprocess 😕

[ghost]

it's sys.executable (=just the same interpreter that is already running) do people not usually know that? 😄 but it's fine, leave it with mocking I am clearly making you confused 😂 (I'm weird that's normal)

The mocking really isn't too bad I just seem to have unusual preferences here

[opacam]

¡¡Thanks again @AndreMiras!!

Pretty nice job ❤️

😄