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

Add api 21 into CI #1496

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Add api 21 into CI #1496

wants to merge 16 commits into from

Conversation

Jaehwa-Noh
Copy link
Contributor

@Jaehwa-Noh Jaehwa-Noh commented Jun 8, 2024

What I have done and why

Cover api 21 with Continuous Integration to detect early crash from minSdk api version.

Using decodeFromString instead decodeFromStream api 21 below version, reference by Kotlin/kotlinx.serialization#2457 (comment)

Fix #1490

Change-Id: Icf428ebfe7e153e132e112c2dc6926bd40ab3951
Change-Id: I07dbb58813bc891f407773fddab7f1487f1ed24f
Change-Id: I86d99fa9e74a8475c0b2bad202cfb4697ab1016b
Change-Id: Idb7fc085e889f8012234d14ad7bc0a4713073d6e
Change-Id: I7e1406da9bf7d8aca44f48d9d7dfeb123d67a562
Change-Id: Ibac659c7aff6d8be99f52d012d603f8251bbd23d
@keyboardsurfer
Copy link
Member

Thanks for your PR. Currently there are conflicts that need to be resolved before we can move on to possibly accepting your contribution. Please resolve the conflicts.

@dturner
Copy link
Collaborator

dturner commented Dec 18, 2024

The older emulators are notoriously flaky and should not be added to the Build workflow. Please remove the API 21 emulator. Please also update the PR description to more accurately describe what this fixes.

@Jaehwa-Noh
Copy link
Contributor Author

Jaehwa-Noh commented Dec 19, 2024

@dturner Could you check this PR #1485 (comment)

This PR will crashes at startup on an API 21 device.

The developers are hard to notice all dependencies and can't ensure that works lowest api 21, be supported in this project, well.
We can early catch the crash on API 21 with Instrumented tests.

@SimonMarquis
Copy link
Contributor

To be honest, I don't think it is worth to have an entire CI pipeline just to catch one specific issue on this specific library that was not properly documented in the first place.
A comment should suffice, or a lint detector if we really want to not reuse this. But API 21 is like 0.1% of active users on the play store...

@Jaehwa-Noh
Copy link
Contributor Author

@SimonMarquis From your comment, why don't we change lowest api 21 to 22 or so?

@Jaehwa-Noh
Copy link
Contributor Author

When nobody cares lowest api 21, It would be better to shift up the lowest api.

@SimonMarquis
Copy link
Contributor

It depends I guess. Sometimes we have to bump the lowest compatible version when using dependencies that are themselves forcing a higher limit. Using the same value as the default Android template from Android Studio might be a good fit (not sure what it is nowadays).
But anyways, this will be the maintainer's choice.

@Jaehwa-Noh
Copy link
Contributor Author

Jaehwa-Noh commented Dec 19, 2024

@dturner I am just as missing your point, the purpose of this PR is API 21 on CI makes work.
and the reason of needing API21 CI is this comment.
#1496 (comment)

If you decide API 21 doesn't need any more, closing this PR.
Else, let me know keep going.

@dturner
Copy link
Collaborator

dturner commented Dec 19, 2024

@Jaehwa-Noh To be clear, we should definitely fix the code that is causing the app to crash on API 21. Thanks for addressing this.

However, my point (and @SimonMarquis as well) is that because API 21 has such a small market share it doesn't make sense to add it to CI. Granted, the benefit would be that we could spot problems running the app on API 21 early. But the cost is we have to spin up an API 21 emulator for every single commit, and as I mentioned before, the old emulators are flaky which means we'd almost certainly get a lot of false negatives, something which has significantly hindered our development velocity recently.

In summary, please carry on fixing the API 21 problem on this PR but remove the update to the Build workflow.

Side note: The Android Studio default API level for new projects is 24, giving 97.4% coverage as of today, Thu 19th Dec 2024. If we run into any more issues with APIs <24 then we should update to minSDK=24.

Change-Id: Iecf9c37a262bf6ae59953ac50bca2523bcab8843

# Conflicts:
#	.github/workflows/Build.yaml
@dturner dturner added the waiting Waiting on (blocked by) someone or something label Dec 19, 2024
Change-Id: I15df6f18343bcf36d2d583a19cd47d6344942201
Change-Id: Ibf7fca9de5461e7285ed6d68092e36d70408f3bd
Change-Id: I4b58c250e76f6d41e794087ff3b467fc61c88eca
Change-Id: If8cc6c27bd399fcdf8446ec411626f13d39e707c
Change-Id: Ib9d2de121c797d97d94e6074cc1da4a26d880a2c
Change-Id: I8076fabed4b4f2f460c74b43ad1c3e38a5268005
Change-Id: I19336b061e74a378cc70470fbd6af8ee4c7533e0
@Jaehwa-Noh
Copy link
Contributor Author

Change-Id: Idac609c7773a83fe1ab2b37d7976a099e390ace9
Change-Id: Ie13a81d4a4534ea278e3d52cb233ed4c54add8ba
@Jaehwa-Noh
Copy link
Contributor Author

@dturner @keyboardsurfer Ready to get review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting Waiting on (blocked by) someone or something
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FR]: Add android API 21 in CI
4 participants