-
Notifications
You must be signed in to change notification settings - Fork 2k
Unify most of the test apps into on device unit test app
#2046
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
Conversation
AndreMiras
left a comment
There was a problem hiding this 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 ❤️
| 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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/th e/the/
| - A non-gui unittests app | ||
| module_import = None | ||
| If you install/build this app via the `setup_test_app.py` file, an file named |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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/
|
Thanks @opacam. Please let me review before merging :) |
8b760a8 to
b4c4736
Compare
|
@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 A quick "run in desktop" tutorialIf 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 Hack: you can avoid do the build if you create the file |
AndreMiras
left a comment
There was a problem hiding this 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 |
There was a problem hiding this comment.
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
e43febf to
b42643f
Compare
9eb5325 to
237de3f
Compare
inclement
left a comment
There was a problem hiding this 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).
237de3f to
293d009
Compare
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
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)
This reverts commit 0ed4d70.
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
293d009 to
6365157
Compare
|
Ok, I fixed the merge conflicts and restored the |
inclement
left a comment
There was a problem hiding this 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.
* 📦 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
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
CItests (since we remove thetestapp_sqlite3_openssl... alongside with some more test apps).There are 3 app modes:
app_requirements.txt)app_requirements.txt)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-guiapp is built, then the results must be checked viaadb logcat.Also be aware that this PR adds more artifacts for gh-actions via an
matrix, so now we will haveon device unit test appfor:Python2 & arm64-v8a (tested on device)Python2 & armeabi-v7a (tested on device)Python2 & x86_64A quick view of the situation:
testapp_keyboardandtestapp_service, now lives in theon device unit test apppillow,matplotlibandencryptionhas been moved into the unit tests (so we can also test those recipes via the flask app)flaskhas been also moved/reworked into the newon device unit test app, this allow us to share the resources with the kivy app (which are the same).app_requirements.txtwhich 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 (whensetup_test_app.pyis ran)...so buildozer builds will build an basic test app, no matter what recipes are introduced intobuildozer.specfile, because theapp_requirements.txtwill only be created if thesetup_test_app.pyfile is ran.Notes:
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).importto add python2 compatibility to this new unit test app...imho...I thinks it's worth it 😉WIPbecause:flask unittest testappwithout building anything, for now you can download thekivy testappat gh-actionsbuildozerbuild, anyone could test it?on_device_unit_test_app#2046 #2073Some future improvements we could make:
testapp_vispytestlauncher_setuptestlauncherreboot_setuprebuild updated recipes(I have some job done in this point but not included at here)