Fix #5093: Fix start-up crash on SDK versions <24 #5291
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
These issues were found after I started using a new development environment.
ProfileAndDeviceIdFragmentTest had been updated to use a newer fragment initialization pattern, but that's no longer needed and seems to be causing what appears to be timing discrepancies between local dev and CI.
The issue ultimately arose from test parameters being initialized after they're needed in the launched UI. This type of change was tried earlier in the branch, but reverted since it didn't seem necessary. It is, however, necessary when there are environment differences (e.g. local vs. CI) or when running certain tests individually. Due to the difficulty in finding this issue, ActivityScenarioRule has been added as a prohibited pattern in the static regex checks (along with ActivityTestRule since that's deprecated and discouraged, anyway).
The test was suffering from some proto encoding inconsistencies that seem to occur between some development machines vs. on CI. The fix improves the test's robustness by extracting the raw encoded string, verifying that the other outputs in the intent message correctly correspond to that string, and that the string (as a parsed proto) contains the correct values. As a result, the test no longer depends on a hardcoded encoding value to be present for verification. This does result in a bit more logic than is generally good to have in a test (and it lengthened the test code quite a bit), but it seems necessary in this particular case.
The main problem was the use of computeIfAbsent() which is a Java 8 API that does NOT desugar during building, and Java 8 APIs are only available on SDK 24+. The actual fix introduced a lot of complexity when working with locale profiles since the utilization of memoized locale profiles now requires using a coroutine to ensure thread safety (i.e. by leveraging a blocking dispatcher). This introduced some complexity in the process for creating locales since a livelock was possible when trying to use the blocking dispatcher for the system locale (since a blocking dispatcher is used farther up in its call chain). Due to these changes, a refactor of AndroidLocaleProfile has been included to largely simplify a lot of the previous complexity around the different states the class could take (these are now explicitly defined as separate subclasses to the now sealed class AndroidLocaleProfile). DataProviders.transformAsync() has also been updated to handle caught exceptions in much the same way as transform(). This helped during debugging the original crash, so it seems like a good idea to include.
|
Hi @BenHenning, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
oppiabot
bot
added
the
stale
Vishwajith-Shettigar
removed
the
stale
The main change here is ensuring that Bazel 4.0.0 is used & bzlmod disabled in newer versions of Bazel when running operations in a test Bazel environment. This commit also introduces some more timing tweaks on CommandExecutor for some tests, though these only affect very specific tests (as many script tests directly call a script's main() function and thus don't overwrite its executor behavior). This commit attempted to introduce "--batch" mode to runs, but the isolation didn't actually seem to improve stability and, instead, substantially slowed down some of the tests.
|
Hi @BenHenning, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
oppiabot
bot
added
the
stale
oppiabot
bot
removed
the
stale
This was done by removing the //testing dependency and, instead, having instrumentation targets depend on the direct module within //testing that they need to build. This module & its corresponding implementation binding (and tests) needed to be moved out of //testing and into their own /firebase package.
BenHenning
requested review from
adhiamboperes
and removed request for
a team
BenHenning
changed the base branch from
fix-platform-specific-issues
to
develop
BenHenning
changed the base branch from
develop
to
fix-platform-specific-issues
|
PTAL @adhiamboperes. |
BenHenning
changed the title
adhiamboperes
requested changes
Feb 26, 2024
utility/src/main/java/org/oppia/android/util/locale/DisplayLocaleImpl.kt
Outdated
Show resolved
Hide resolved
utility/src/main/java/org/oppia/android/util/locale/AndroidLocaleProfile.kt
Show resolved
Hide resolved
utility/src/main/java/org/oppia/android/util/locale/AndroidLocaleProfile.kt
Show resolved
Hide resolved
utility/src/main/java/org/oppia/android/util/locale/AndroidLocaleFactory.kt
Outdated
Show resolved
Hide resolved
utility/src/test/java/org/oppia/android/util/locale/AndroidLocaleProfileTest.kt
Show resolved
Hide resolved
BenHenning
commented
Mar 1, 2024
utility/src/main/java/org/oppia/android/util/locale/AndroidLocaleFactory.kt
Outdated
Show resolved
Hide resolved
utility/src/main/java/org/oppia/android/util/locale/AndroidLocaleProfile.kt
Show resolved
Hide resolved
utility/src/main/java/org/oppia/android/util/locale/AndroidLocaleProfile.kt
Show resolved
Hide resolved
utility/src/main/java/org/oppia/android/util/locale/DisplayLocaleImpl.kt
Outdated
Show resolved
Hide resolved
utility/src/test/java/org/oppia/android/util/locale/AndroidLocaleProfileTest.kt
Show resolved
Hide resolved
|
Thanks @adhiamboperes! PTAL at latest. |
adhiamboperes
approved these changes
Mar 1, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation
Fixes #5093
This replaces #5211.
The core crash has come from using Java 8's Map.computeIfAbsent on devices running SDK <24. This function isn't supported on these SDK versions because:
computeIfAbsentshould be present, it's not clear why it isn't (it could be an out-of-date desugarer, or some other issue).Only two uses for
computeIfAbsentare present:AndroidLocaleFactory: the main cause of the crash.FakeAssetRepository: only used in tests, so ignored in this change (since it would require changing the production API forAssetRepositoryto fix).A new regex pattern check was added to ensure this method can't inadvertently be used in the future since it won't work on lower API versions.
This PR mainly focuses on fixing
AndroidLocaleFactory.Need to review the code & finish it, and document the changes. #5211 explored different methods that we could take to keep synchronization when updating the factory to move away from
computeIfAbsent, but all of them required introducing locking mechanisms which could cause deadlocking. I also considered in this PR removing the memoization altogether, but this doesn't seem ideal either since the creation of profiles is non-trivial and locales are frequently fetched by nearly all core lesson data providers throughout the app. I landed on a solution that leveraged the app's blocking dispatcher and made profile creation asynchronous. This has some specific implications:DisplayLocaleImplget more complicated since creation of theLocaleis now asynchronous. This was adjusted by passing in the display locale'sLocaleobject rather than having it compute it, and callsites often already are operating within a coroutine context which makes the asynchronous aspect ofLocalecreation a bit simpler.Localeobject specifically for these purposes. This method should only be used when necessary since it avoids the performance benefits of the memoized version.During development, there were two other changes that were made to ease development:
AndroidLocaleProfilewas refactored to use a sealed class to improve its typing, and to better facilitate the introduction of a root profile (which was added for better system compatibility in cases when the default app locale isn't supported on the system). Note that the new root profile only applies to app strings since it can actually have correct default behavior (whereas for content strings & audio voiceovers it can't actually be used effectively). The profile class was also updated to compute its ownLocale(which is a simplification since before there were multiple ways to create a "correct"Locale), and have an application-injectable factory.DataProviders.transformAsyncwas updated to handle caught exceptions in the same way asDataProviders.transformwhich helped while debugging the crash, and seems like it would have downstream benefits in the future. The method's tests were correspondingly updated.The changes here led to some testing changes:
testCreateDisplayLocaleImpl_defaultInstance_hasDefaultInstanceContextwas removed since using the default context isn't valid in most cases (see below point).testReconstituteDisplayLocale_defaultContext_returnsDisplayLocaleForContextwas renamed & a new test added to better represent that the default context is invalid except for app strings where it can represent root locale. For non-app cases, an exception should be thrown for default. This is also whytestCreateLocale_appStrings_allIncompat_invalidLangType_throwsExceptionwas updated to instead verify that the root locale is returned.TranslationControllerTest,testGetSystemLanguageLocale_rootLocale_returnsLocaleWithBlankContextandtestGetAppLanguageLocale_uninitialized_returnsLocaleWithSystemLanguagewere updated to correctly indicate that there is no specified language for the root locale cases. To ensure coverage for valid IETF BCP-47 & Android resource language IDs, a new test was added:testGetAppLanguageLocale_ptBrDefLocale_returnsLocaleWithIetfAndAndroidResourcesLangIds.Finally, please note that this is fixing a release blocking issue for the upcoming 0.13 beta release of the app.
Essential Checklist
For UI-specific PRs only
This is mainly an infrastructure change. The only user behavior impacted is that the app no longer crashes on startup on SDK versions below 24.
Attempting to open the app on an SDK 23 emulator on develop (4a07d8d):
Attempting to open the app with the fixes from this branch:
