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

Simple run_pymodules_install test, refs #1898 #1899

Conversation

AndreMiras
Copy link
Member

Regression test for the fix introduced in #1898.

@AndreMiras
Copy link
Member Author

The aim is definitely not to deeply test anything, but to at least check that the bug address in #1898 is now covered by a simple test

try:
from unittest import mock
except ImportError:
# `Python 2` or lower than `Python 3.3` does not
Copy link
Member

Choose a reason for hiding this comment

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

This reminds me we still need to decide when p4a drops python2 support. Soon would probably be good, it's especially annoying nowadays to do python2 compatibility stuff when it will be necessary for only a short time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Completely agree. Also I can't wait to use f-strings and annotations :smile

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I'm also waiting for python 3's f-strings, but this will require python >= 3.6...I have no problem with that

As a side note: we could already use python 3 f-strings but now it would require the install of some extra python package like fmt (I've been tempted of doing this in some prs 😆)

inclement
inclement previously approved these changes Jun 28, 2019
from pythonforandroid.build import run_pymodules_install


class TestBuildBasic:
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason to non inherit from unittest.TestCase?

I think that inheriting from unittest.TestCase has several advantages:

  • Allows some ides to detect the unittest class and you can run it from the ide with a couple of mouse clicks (pycharm case...probably eclipse too...but has been years since I used it...so I'm not sure of this last one)
  • Allows us to use the assert methods of unit.TestCase, which increases the readability

Also I may add that the class unittest.TestCase has been made specially to deal with tests, I don't see the reason of not using it...am I missing something?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes the reason is, don't use something if you don't need it, as simple as that 😄
Also known as YAGNI https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it
I mean we use pytest from tox which advocates for light code with no unnecessary boiler plate. https://docs.pytest.org/en/latest/assert.html

pytest has support for showing the values of the most common subexpressions including calls, attributes, comparisons, and binary and unary operators. (...). This allows you to use the idiomatic python constructs without boilerplate code while not losing introspection information.

I hope nowadays IDE can work with pytest, but I don't know because I've never used any.

As for the readability I actually think assert is far more readable than self.assertSomething.
The human eye just has to scan for assert it's always this keyword with always a space straight after. So no silly camelCase (shame on you Python!), no self. and no variations (assertEqual, assertTrue, assertIsNone, assertIsNotNone...). So I honestly thing the core unittest module makes test less readable than going straight with pytest and plan assert.

Nevertheless, I've added the inheritance boilerplate for you, I know it's gonna take time to convince people, so let's try one step at a time 😄

Regression test for the fix introduced in kivy#1898.
@AndreMiras AndreMiras force-pushed the feature/ticket1898_run_pymodules_install_regression_test branch from fe85374 to 8b3e218 Compare June 29, 2019 10:16
Copy link
Member

@opacam opacam left a comment

Choose a reason for hiding this comment

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

LGTM!

Thanks @AndreMiras 😄

@inclement inclement merged commit 4b4e899 into kivy:develop Jun 29, 2019
@AndreMiras AndreMiras deleted the feature/ticket1898_run_pymodules_install_regression_test branch June 29, 2019 14:06
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.

3 participants