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

Unify most of the test apps into on device unit test app #2046

Merged
merged 15 commits into from
Mar 31, 2020

Conversation

opacam
Copy link
Member

@opacam opacam commented Dec 27, 2019

So depending on the requested recipes at built time, a different test app will be ran (as well as different unit tests). We also set this unified test app the default for our CI tests (since we remove the testapp_sqlite3_openssl ... alongside with some more test apps).

There are 3 app modes:

  • kivy (if kivy detected in app_requirements.txt)
  • flask (if flask detected in app_requirements.txt)
  • no-gui (if above cases are not taken)

In all the cases mentioned the unittests will be run. In the two first use cases (kivy and flask) the result of the unit tests can be seen via the app. If an no-gui app is built, then the results must be checked via adb logcat.

Also be aware that this PR adds more artifacts for gh-actions via an matrix, so now we will have on device unit test app for:

  • Python3 & arm64-v8a (tested on device)
  • Python3 & armeabi-v7a (tested on device)
  • Python3 & x86_64
  • Python3 & x86
  • Python2 & arm64-v8a (tested on device)
  • Python2 & armeabi-v7a (tested on device)
  • Python2 & x86_64

A quick view of the situation:

  • the removed testapp_keyboard and testapp_service, now lives in the on device unit test app
  • the testapps for pillow, matplotlib and encryption has been moved into the unit tests (so we can also test those recipes via the flask app)
  • the testapp for flask has been also moved/reworked into the new on device unit test app, this allow us to share the resources with the kivy app (which are the same).
  • Furthermore, we generate a file at build time app_requirements.txt which will hold the supplied recipes at build time. This file will allow us to test these recipes via our unittests. Just noting that the recipes we want to test, must be explicitly mentioned. Also should be mentioned that this feature its only available for direct build with p4a (when setup_test_app.py is ran)...so buildozer builds will build an basic test app, no matter what recipes are introduced into buildozer.spec file, because the app_requirements.txt will only be created if the setup_test_app.py file is ran.

Notes:

  • @inclement, I included you as author of this thing (alongside myself), I think that we are the authors of the removed test apps and you are also the author of the base code for the on device unit test app (@AndreMiras and anyone looking into this, I think that you are not involved with these test apps, please, let me know if I'm mistaken and I will include you as author as well).
  • I avoided python3's specific code in purpose, so the new test app could be also run from python2 (this way we can remove the python2 test apps). BTW, we only have one import to add python2 compatibility to this new unit test app...imho...I thinks it's worth it 😉
  • I labeled this WIP because:
    • I want to add a couple of commits, in order to force to generate the flask testapp via gh-actions and restore it to kivy, so this way, you guys can see the result of this flask unittest testapp without building anything, for now you can download the kivy testapp at gh-actions
    • I have mixed feelings about this PR...maybe the whole thing it's a crazy idea of mine...mmmm...let me know if this is the case and I will close this, no problem at all
  • I added a font file (it's very light...less than 10Kb and it's free for commercial/personal use). This font has two purposes:
    • used for some titles in kivy/flask apps
    • to perform the unittests for the Pillow recipe
  • @inclement, the testapp for keyboard that I deleted and included at the new unit test app...I'm not sure if its working as expected...it would be great that you could download one of the generated tests apps via gh-actions and give your thoughts, @AndreMiras, it would be awesome to have yours as well.
  • I did not test the buildozer build, anyone could test it?
  • Closes Playing with on_device_unit_test_app #2046 #2073

Some future improvements we could make:

  • remove/integrate some more test apps. Merging this PR, we will only have the following test apps:
    • testapp_vispy
    • testlauncher_setup
    • testlauncherreboot_setup
  • make gh-actions create a new artifact for rebuild updated recipes (I have some job done in this point but not included at here)
  • add tests for all the recipes we have (we have a basic set but we could go further)
  • Improve design and features, specially for the flask app (yea..I put more love with the kivy app 😄)
  • test the artifacts we generate with gh-actions on device...@AndreMiras, you were working with that for travis...maybe now it will be easier to do that with gh-actions?
  • you tell me...

@opacam opacam added the WIP label Dec 27, 2019
AndreMiras
AndreMiras previously approved these changes Dec 27, 2019
Copy link
Member

@AndreMiras AndreMiras left a comment

Choose a reason for hiding this comment

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

That's some pretty nice work, well done.
I wanted to refactor our test apps for a while now and never did.
Thanks for giving it a try with some interesting approach 👏
I haven't tried it on device yet but only looking at the code it already seems like a great improvement.
Also thank you for keeping the documentation up to date ❤️

@@ -4,18 +4,38 @@ On device unit tests
This test app runs a set of unit tests, to help confirm that the
python-for-android build is actually working properly.

Also its dynamic, because it will run one app or another depending on the
Copy link
Member

Choose a reason for hiding this comment

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

s/its/it's/


You can use the provided file `setup_test_app.py`. Check our `Makefile
<https://github.com/kivy/python-for-android/blob/develop/Makefile>`__ to guess
how to build th e test app, or also you can look at `testing pull requests documentation
Copy link
Member

Choose a reason for hiding this comment

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

s/th e/the/


module_import = None
If you install/build this app via the `setup_test_app.py` file, an file named
Copy link
Member

Choose a reason for hiding this comment

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

s/an file/a file/

self.assertIsNotNone(
self.module_import,
'module_import is not set (was default None)')
.. note:: This app it's made to be working on desktop and on an android device.
Copy link
Member

Choose a reason for hiding this comment

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

s/it's made/is made/

.. note:: This app it's made to be working on desktop and on an android device.
Be aware that some of the functionality of this app will only work on
an android device. Also should be mentioned that it will run for
python2 and python3.
Copy link
Member

Choose a reason for hiding this comment

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

I know we still support target Python2, but I would keep it a secret by removing that last sentence 😛

these on desktop just by editing the file `app_requirements.txt`,
which should be located at the same location than this file. This
`app_requirements.txt` file, it's autogenerated when the
`setup_test_app.py` is ran, so in certain circumstances, you may need
Copy link
Member

Choose a reason for hiding this comment

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

s/so in/so in/

@inclement
Copy link
Member

Thanks @opacam. Please let me review before merging :)

@opacam opacam force-pushed the feature-testapps-rework branch 6 times, most recently from 8b760a8 to b4c4736 Compare December 30, 2019 01:42
@opacam
Copy link
Member Author

opacam commented Jan 17, 2020

@inclement, I fixed the @AndreMiras comments and also made some additional changes (hope that the commits are clear enough). I leave this WIP awaiting your review (@AndreMiras...and yours again if you want) you probably want to change something, so let me know or make the changes that you want directly in here (you should be able to do so, because I always mark the checkbox allow edit from maintainers), whatever it's fine to me.

Also, here you have the download links for the generated apks via gh-actions (you can collect yourself this via the artifacts dropdown of the gh-actions corresponding job, but I collected the relevant ones for you guys 😉):

Note: Since I consider this very useful for us (specially in combination with gh-actions) and I don't want to rush you with the review, I'm planning to open a draft pull request with this branch squashed and merged on top of develop branch , so I can play a little with the requirements of the on device unit test app, just to test some recipes which I know that are not working (and I will try to fix it there), so we all can see this thing in action and maybe extract some fixes for the non working recipes.

A quick "run in desktop" tutorial

If you just want to see how it will look the kivy/flask apps from your PC, you can run the main.py file with the proper python environment/installation. If you don't make any build nor touch setup_test_app.py, then the kivy app should run (of course you need kivy installed), if you edit the mentioned file replacing kivy for flask in requirements (setup_test_app.py or via cli) and make a build as we used to do with the removed setup_testapp_python3_sqlite_openssl.py, well...flask app should raise (you need flash installed and a browser to see the results)

Hack: you can avoid do the build if you create the file app_requirements.txt file inside testapps/on_device_unit_tests/test_app folder with the requirements you want to test. The content of such file will look like as following (without touching setup_test_app.py and performing a build of an apk):

sqlite3
libffi
openssl
pyjnius
kivy
python3
requests

AndreMiras
AndreMiras previously approved these changes Jan 18, 2020
Copy link
Member

@AndreMiras AndreMiras left a comment

Choose a reason for hiding this comment

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

Impressive PR and very useful, can't wait to see it merged.
Thanks a mil for pulling that up 👏
Let's see what inclement thinks about it :)

our requirements we can build an kivy, flask or a non-gui app. We default to an
kivy app, since the python-for-android project its a sister project of kivy.

The parameter `requirements` it's crucial to determine the unit tests we will
Copy link
Member

Choose a reason for hiding this comment

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

it's crucial -> is crucial

Copy link
Member

@inclement inclement left a comment

Choose a reason for hiding this comment

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

Looks awesome, thanks yet again.

I didn't see any issues other than a few minor documentation wording issues that I decided not to fix right away, they'll be easy to do later if necessary. The changes are too big to really check everything in detail, but it's such a big improvement (and so test-focused anyway) that it should be easy to fix anything that comes up.

The only minor things are:

  • This has a few merge conflicts, especially after the recent big merges, could you fix them up?
  • I guess you'll want to get rid of the remaining Python 2 references now that the removal ticket is closed?
  • I'd quite like to keep a "setup_testapp_python3_openssl_sqlite.py" back into the testapps folder, since it's so useful to just test a quick build. Maybe that's just me? It would be nice to have it there if you wouldn't mind (and I think it would be fine if it's just a shim into the new on device unit tests).

inclement
inclement previously approved these changes Mar 30, 2020
opacam added 6 commits March 31, 2020 00:02
So depending on the supplied recipes, a different test app will be ran.

The supported test apps are:
  - kivy
  - flask
  - no-gui
for our CI providers and `rebuild_updated_recipes.py` because we recently removed
the old test app that we made use of.
Remove sentence regarding python2 compatibility...
because python2 it's almost at the end of his life
opacam added 8 commits March 31, 2020 00:02
So we can get an apk via gh-actions

Notes:
  - this way the reviewers can install the flask apk without making the build
  - these changes will be reverted in the next
    commit...since we want a kivy app as our default)
in case that we detect that p4a will build an `service_only` app.

Note: if we don't do this we wil get errors once we try to create our apk with the target distribution
and change gh-actions job title (because it looks better)
Because it looks better and is more readable.
Because reducing filename to `setup.py` will make our cli arguments shorter and source code cleaner
So our matrix gh-actions builds will not be cancelled if some arch fails
which is very useful to fix specific arch issues.

See also: kivy#2073
@opacam opacam force-pushed the feature-testapps-rework branch from 293d009 to 6365157 Compare March 30, 2020 22:04
make `testapps/%` support multiple archs

Notes: this completes the job started at 9ea53fc & afd4413
@opacam opacam removed the WIP label Mar 31, 2020
@opacam
Copy link
Member Author

opacam commented Mar 31, 2020

Ok, I fixed the merge conflicts and restored the setup_testapp_python3_openssl_sqlite.py. BTW...I also use it, but lately I began to use the on_device_unit_test_app as a replacement because I find it more versatile since it does the same than setup_testapp_python3_openssl_sqlite.py but a little more 😄. Anyway, I have no problem on leaving it in the project, after all...it has been a lot of helpful to me 😉

Copy link
Member

@inclement inclement left a comment

Choose a reason for hiding this comment

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

Sounds good, thanks opacam.

@inclement inclement merged commit 16155c0 into kivy:develop Mar 31, 2020
zworkb pushed a commit to zworkb/python-for-android that referenced this pull request Apr 15, 2020
* 📦 Refactor (move) `PythonTestMixIn` into `tests.mixin`

* 📦 Unify most of the test apps into one app

So depending on the supplied recipes, a different test app will be ran.

The supported test apps are:
  - kivy
  - flask
  - no-gui

* 🏗️ Make test app `on_device_unit_tests` the default...

for our CI providers and `rebuild_updated_recipes.py` because we recently removed
the old test app that we made use of.

* 📝 Update docs

* ✅ Create a `x86_64` testapp for gh-actions

* 📝 Fix typo errors and...

Remove sentence regarding python2 compatibility...
because python2 it's almost at the end of his life

* ✅ Make a `flask` testapp...

So we can get an apk via gh-actions

Notes:
  - this way the reviewers can install the flask apk without making the build
  - these changes will be reverted in the next
    commit...since we want a kivy app as our default)

* ✅ Revert "Make a `flask` testapp..."

This reverts commit 0ed4d70.

* 🔥 Remove `orientation` kwarg...

in case that we detect that p4a will build an `service_only` app.

Note: if we don't do this we wil get errors once we try to create our apk with the target distribution

* ✅ Create a `x86` testapp for gh-actions

and change gh-actions job title (because it looks better)

* 🏗️ Rename test app

Because it looks better and is more readable.

* 📝 Fix a syntactical error at docstring

* 🚚 Rename `setup_test_app.py` to `setup.py`

Because reducing filename to `setup.py` will make our cli arguments shorter and source code cleaner

* 🔧 Disable gh-actions `fail-fast`

So our matrix gh-actions builds will not be cancelled if some arch fails
which is very useful to fix specific arch issues.

See also: kivy#2073

* 💚 Add `testapps-with-numpy/%` & ...

make `testapps/%` support multiple archs

Notes: this completes the job started at 9ea53fc & afd4413
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