search

inaturalist / iNaturalistAndroid

Android app for iNaturalist.org
https://market.android.com/details?id=org.inaturalist.android
MIT License
155 stars 56 forks source link

Update Toolchain & CI/CD #1323

Closed hamiltont closed 5 months ago

hamiltont commented 6 months ago

Happy holidays to the iNat team 👋 In my free mornings over the break I have decided to revisit this codebase. My understanding that the React Native transition is targeting an MVP release in ~6 months. Considering the typical user adoption curves, it's likely that this native application will remain active for at least another year. In light of this, keeping the current app maintained will help keep users engaged and happy, so I'm taking a look for low-effort high-impact edits.

This pull request introduces a toolchain upgrade, no application code changes. This leads to around a ~25% reduction in the application's size (exact number will depend on the specifications of the device that's downloading the application), which will help users with lower-end devices

Changes

budowski commented 5 months ago

Thanks @hamiltont - very thorough work! 🙏 Looks good, just only one thing - can you please fix the failing test for Android 14?

hamiltont commented 5 months ago

Hey sorry real life work has caught up. I'll have to leave this here. Feel free to close it if it's not something you want.

As mentioned in the PR, the problem with test flakiness is getting a cloud emulator to boot consistently. If I remember correctly, I capture screenshots of the emulator before I run the test. If you take a look at those screenshots, you can likely see the problem. It'll typically be something like a system dialogue showing that the launcher is still booting.

Fundamentally using emulators with new system images as part of a CICD pipeline is quite difficult. On some of the older system images it's a bit more predictable because they do quite a bit less when running Android init and the launchers are much faster. Unfortunately, as the Android ecosystem has grown in complexity boot times, especially in non-hardware accelerated environments, can be painfully unpredictable. The system properties that used to be quite useful for determining whether or not a system was ready for running testing (such as the boot completed property) are no longer reliable given the significant number of things that happen after the initial process is done booting. There is work you can do to optimize this, but it typically involves a very manual and difficult approach such as creating a custom emulation image that uses in launcher tuned for CICD and has system privileges to to set a custom system property you can wait on.

All in all, it's super a pain and arguably the easiest solution is to either ignore or disable the flaky tests on modern versions. If you do want the ability to test modern versions as part of the CICD pipeline, my recommendation would be to see if there's any service out there that will give a non-profit free access to real devices for testing.

Given that this project has so few unit tests at this time, and especially as your migrating to a new system, I doubt there's going to be a ton of effort put into creating new to unit tests, you might consider just making it a build check in the CICD pipeline. If I remember correctly. The main thing that the device tests are actually testing at this time is that when you click on various menu items in the about screen, it opens the correct URL. Good thing to have if you have a massive large test suite, but if that's literally the only thing in the test suite, it's easy enough to keep an eye on that in the PRs

Ps - the other thing I worked on while I had a break was transitioning the observation to a recycler view. I've mentioned this in a earlier issue, I'm bringing it up again because I just want to make sure you're aware that in my testing the observation list is almost 10x faster. It's truly an unbelievable speed up, in addition to helping a number of the concurrency bugs disappear with the cleaner support for isolating item by item changes. I haven't submitted a PR because doing this requires changing a number of the underlying classes which causes a cascade of changes. Most of these are fairly simple things, like taking your class that's already called view holder and actually making it subclass the recycler views view holder implementation. Still, it has so many changes that the PR starts to look scary and I haven't had time to carefully stage it so that it looks step by step reasonable. However, if you're willing to take on the challenge, I think it would buy iNaturalist quite a bit more usable lifetime of the current Android application while you're working on the new application.

Cheers! Ham

On Sat, Jan 13, 2024, 21:45 budowski @.***> wrote:

Thanks @hamiltont https://github.com/hamiltont - very thorough work! 🙏 Looks good, just only one thing - can you please fix the failing test for Android 14?

— Reply to this email directly, view it on GitHub https://github.com/inaturalist/iNaturalistAndroid/pull/1323#issuecomment-1890829381, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACKRZGJ325X2OEW4C3JS33YONBGFAVCNFSM6AAAAABBG2IY2KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOJQHAZDSMZYGE . You are receiving this because you were mentioned.Message ID: @.***>