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

Fix #5093: Fix start-up crash on SDK versions <24 #5291

Merged
merged 47 commits into from
Mar 1, 2024
Merged
Show file tree
Hide file tree
Changes from 45 commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
667bf89
Fix a variety of dev platform-specific issues.
BenHenning Aug 22, 2023
fb59232
Tidy some things up, and revert fragment test.
BenHenning Aug 28, 2023
2551d4b
Fix test behavior inconsistency in test.
BenHenning Sep 11, 2023
714f3ea
Merge branch 'develop' into fix-platform-specific-issues
BenHenning Sep 11, 2023
7488b9f
Fix broken ProfileAndDeviceIdFragmentTest test.
BenHenning Sep 13, 2023
e3d4091
Merge branch 'develop' into fix-platform-specific-issues
BenHenning Oct 26, 2023
5671673
Post-merge fix.
BenHenning Oct 26, 2023
4689609
Fix API 21-23 (incl.) crash.
BenHenning Dec 28, 2023
50e92e2
Merge branch 'develop' into fix-api-21-22-23-crash
BenHenning Dec 28, 2023
bd1466d
Merge branch 'develop' into fix-platform-specific-issues
BenHenning Jan 16, 2024
79878cd
Some more robustness fixes.
BenHenning Jan 19, 2024
bd97611
Fix BUILD file lint issue.
BenHenning Jan 20, 2024
211487c
Merge branch 'develop' into fix-platform-specific-issues
BenHenning Feb 6, 2024
14b7ea5
Merge branch 'develop' into fix-api-21-22-23-crash
BenHenning Feb 6, 2024
1a666a9
Post-merge fixes.
BenHenning Feb 6, 2024
a2ee5d3
Add smoke tests for instr. binaries & tests.
BenHenning Feb 7, 2024
d5b9012
Some minor refactoring for readability.
BenHenning Feb 7, 2024
e6ec869
Fixed broken instrumentation builds.
BenHenning Feb 7, 2024
9f50469
Merge branch 'develop' into fix-instrumentation-build-failure
BenHenning Feb 7, 2024
8dbf4dc
Add missing CODEOWNERS line.
BenHenning Feb 7, 2024
9c38057
Remove old files from CODEOWNERS.
BenHenning Feb 7, 2024
f2ff4e3
Merge branch 'fix-instrumentation-build-failure' into fix-platform-sp…
BenHenning Feb 7, 2024
fad48ed
Add missing tests for TestBlazeWorkspace changes.
BenHenning Feb 8, 2024
e341d5e
Merge branch 'fix-platform-specific-issues' into fix-api-21-22-23-crash
BenHenning Feb 8, 2024
e1802c8
Add missing Firebase auth tests.
BenHenning Feb 8, 2024
00c32f6
Merge branch 'fix-instrumentation-build-failure' into fix-platform-sp…
BenHenning Feb 8, 2024
2e3c13e
Merge branch 'fix-platform-specific-issues' into fix-api-21-22-23-crash
BenHenning Feb 8, 2024
d3cbd93
Post-merge fixes for previously missed tests.
BenHenning Feb 8, 2024
b1254c1
Merge branch 'fix-platform-specific-issues' into fix-api-21-22-23-crash
BenHenning Feb 8, 2024
9b85cd3
Fix some broken tests.
BenHenning Feb 8, 2024
50f41d2
Merge branch 'develop' into fix-instrumentation-build-failure
BenHenning Feb 8, 2024
c8df70e
Merge branch 'fix-instrumentation-build-failure' into fix-platform-sp…
BenHenning Feb 8, 2024
44a5003
Merge branch 'fix-platform-specific-issues' into fix-api-21-22-23-crash
BenHenning Feb 8, 2024
c85d421
Merge branch 'develop' into fix-instrumentation-build-failure
BenHenning Feb 9, 2024
dcf27b6
Merge branch 'fix-instrumentation-build-failure' into fix-platform-sp…
BenHenning Feb 9, 2024
90e162c
Merge branch 'fix-platform-specific-issues' into fix-api-21-22-23-crash
BenHenning Feb 9, 2024
2369514
Merge branch 'develop' into fix-platform-specific-issues
BenHenning Feb 9, 2024
a6cd25e
Follow-up doc and readability improvements.
BenHenning Feb 9, 2024
91a5807
Merge branch 'fix-platform-specific-issues' into fix-api-21-22-23-crash
BenHenning Feb 9, 2024
2916770
Update scripts/src/javatests/org/oppia/android/scripts/common/GitClie…
BenHenning Feb 14, 2024
f2d810a
Merge branch 'develop' into fix-platform-specific-issues
BenHenning Feb 14, 2024
2f1b54e
Merge branch 'fix-platform-specific-issues' into fix-api-21-22-23-crash
BenHenning Feb 14, 2024
6856e3c
Merge branch 'develop' into fix-platform-specific-issues
BenHenning Feb 14, 2024
430c487
Merge branch 'fix-platform-specific-issues' into fix-api-21-22-23-crash
BenHenning Feb 14, 2024
d5b921b
Merge branch 'develop' into fix-api-21-22-23-crash
BenHenning Feb 20, 2024
b6274f2
Merge branch 'develop' into fix-api-21-22-23-crash
BenHenning Mar 1, 2024
bef917c
Address review comments.
BenHenning Mar 1, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -852,8 +852,10 @@ class HtmlParserTest {
}
}

private fun createDisplayLocaleImpl(context: OppiaLocaleContext): DisplayLocaleImpl =
DisplayLocaleImpl(context, machineLocale, androidLocaleFactory, formatterFactory)
private fun createDisplayLocaleImpl(context: OppiaLocaleContext): DisplayLocaleImpl {
val formattingLocale = androidLocaleFactory.createOneOffAndroidLocale(context)
adhiamboperes marked this conversation as resolved.
Show resolved Hide resolved
return DisplayLocaleImpl(context, formattingLocale, machineLocale, formatterFactory)
}

private fun <A : Activity> ActivityScenario<A>.getDimensionPixelSize(
@DimenRes dimenResId: Int
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1044,8 +1044,10 @@ class ListItemLeadingMarginSpanTest {
return valueCaptor.value
}

private fun createDisplayLocaleImpl(context: OppiaLocaleContext): DisplayLocaleImpl =
DisplayLocaleImpl(context, machineLocale, androidLocaleFactory, formatterFactory)
private fun createDisplayLocaleImpl(context: OppiaLocaleContext): DisplayLocaleImpl {
val formattingLocale = androidLocaleFactory.createOneOffAndroidLocale(context)
return DisplayLocaleImpl(context, formattingLocale, machineLocale, formatterFactory)
}

private fun setUpTestApplicationComponent() {
ApplicationProvider.getApplicationContext<TestApplication>().inject(this)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,8 @@ class LocaleController @Inject constructor(
private val asyncDataSubscriptionManager: AsyncDataSubscriptionManager,
private val machineLocale: MachineLocale,
private val androidLocaleFactory: AndroidLocaleFactory,
private val formatterFactory: OppiaBidiFormatter.Factory
private val formatterFactory: OppiaBidiFormatter.Factory,
private val androidLocaleProfileFactory: AndroidLocaleProfile.Factory
) {
private val definitionsLock = ReentrantLock()
private lateinit var supportedLanguages: SupportedLanguages
Expand Down Expand Up @@ -115,9 +116,8 @@ class LocaleController @Inject constructor(
* for cases in which the user changed their selected language).
*/
fun reconstituteDisplayLocale(oppiaLocaleContext: OppiaLocaleContext): DisplayLocale {
return DisplayLocaleImpl(
oppiaLocaleContext, machineLocale, androidLocaleFactory, formatterFactory
)
val formattingLocale = androidLocaleFactory.createOneOffAndroidLocale(oppiaLocaleContext)
return DisplayLocaleImpl(oppiaLocaleContext, formattingLocale, machineLocale, formatterFactory)
}

/**
Expand Down Expand Up @@ -257,7 +257,7 @@ class LocaleController @Inject constructor(

private fun getSystemLocaleProfile(): DataProvider<AndroidLocaleProfile> {
return dataProviders.createInMemoryDataProvider(ANDROID_SYSTEM_LOCALE_DATA_PROVIDER_ID) {
AndroidLocaleProfile.createFrom(getSystemLocale())
androidLocaleProfileFactory.createFrom(getSystemLocale())
}
}

Expand Down Expand Up @@ -293,7 +293,7 @@ class LocaleController @Inject constructor(
@Suppress("DEPRECATION") // Old API is needed for SDK versions < N.
private fun getDefaultLocale(configuration: Configuration): Locale = configuration.locale

private suspend fun <T : OppiaLocale> computeLocaleResult(
private suspend inline fun <reified T : OppiaLocale> computeLocaleResult(
language: OppiaLanguage,
systemLocaleProfile: AndroidLocaleProfile,
usageMode: LanguageUsageMode
Expand All @@ -302,7 +302,6 @@ class LocaleController @Inject constructor(
// internal weirdness that would lead to a wrong type being produced from the generic helpers.
// This shouldn't actually ever happen in practice, but this code gracefully fails to a null
// (and thus a failure).
@Suppress("UNCHECKED_CAST") // as? should always be a safe cast, even if unchecked.
val locale = computeLocale(language, systemLocaleProfile, usageMode) as? T
return locale?.let {
AsyncResult.Success(it)
Expand All @@ -324,7 +323,13 @@ class LocaleController @Inject constructor(
retrieveLanguageDefinition(languageDefinition.fallbackMacroLanguage)?.let {
fallbackLanguageDefinition = it
}
regionDefinition = retrieveRegionDefinition(systemLocaleProfile.regionCode)
val regionCode = when (systemLocaleProfile) {
is AndroidLocaleProfile.LanguageAndRegionProfile -> systemLocaleProfile.regionCode
is AndroidLocaleProfile.RegionOnlyProfile -> systemLocaleProfile.regionCode
is AndroidLocaleProfile.LanguageAndWildcardRegionProfile,
is AndroidLocaleProfile.LanguageOnlyProfile, is AndroidLocaleProfile.RootProfile -> ""
}
regionDefinition = retrieveRegionDefinition(regionCode)
this.usageMode = usageMode
}.build()

Expand All @@ -343,8 +348,10 @@ class LocaleController @Inject constructor(
}

return when (usageMode) {
APP_STRINGS ->
DisplayLocaleImpl(localeContext, machineLocale, androidLocaleFactory, formatterFactory)
APP_STRINGS -> {
val formattingLocale = androidLocaleFactory.createAndroidLocaleAsync(localeContext).await()
DisplayLocaleImpl(localeContext, formattingLocale, machineLocale, formatterFactory)
}
CONTENT_STRINGS, AUDIO_TRANSLATIONS -> ContentLocaleImpl(localeContext)
USAGE_MODE_UNSPECIFIED, UNRECOGNIZED -> null
}
Expand Down Expand Up @@ -413,9 +420,7 @@ class LocaleController @Inject constructor(
return@mapNotNull definition.retrieveAppLanguageProfile()?.let { profile ->
profile to definition
}
}.find { (profile, _) ->
localeProfile.matches(machineLocale, profile)
}?.let { (_, definition) -> definition }
}.find { (profile, _) -> localeProfile.matches(profile) }?.let { (_, definition) -> definition }
}

private suspend fun retrieveRegionDefinition(countryCode: String): RegionSupportDefinition {
Expand All @@ -431,7 +436,7 @@ class LocaleController @Inject constructor(
} ?: RegionSupportDefinition.newBuilder().apply {
region = OppiaRegion.REGION_UNSPECIFIED
regionId = RegionSupportDefinition.IetfBcp47RegionId.newBuilder().apply {
ietfRegionTag = countryCode
ietfRegionTag = machineLocale.run { countryCode.toMachineUpperCase() }
BenHenning marked this conversation as resolved.
Show resolved Hide resolved
}.build()
}.build()
}
Expand All @@ -455,10 +460,10 @@ class LocaleController @Inject constructor(
private fun LanguageSupportDefinition.retrieveAppLanguageProfile(): AndroidLocaleProfile? {
return when (appStringId.languageTypeCase) {
LanguageSupportDefinition.LanguageId.LanguageTypeCase.IETF_BCP47_ID ->
AndroidLocaleProfile.createFromIetfDefinitions(appStringId, regionDefinition = null)
androidLocaleProfileFactory.createFromIetfDefinitions(appStringId, regionDefinition = null)
LanguageSupportDefinition.LanguageId.LanguageTypeCase.MACARONIC_ID -> {
// Likely won't match against system languages.
AndroidLocaleProfile.createFromMacaronicLanguage(appStringId)
androidLocaleProfileFactory.createFromMacaronicLanguage(appStringId)
}
LanguageSupportDefinition.LanguageId.LanguageTypeCase.LANGUAGETYPE_NOT_SET, null -> null
}
Expand All @@ -473,9 +478,12 @@ class LocaleController @Inject constructor(
// must be part of the language definitions. Support for app strings is exposed so that a locale
// can be constructed from it.
appStringId = LanguageSupportDefinition.LanguageId.newBuilder().apply {
ietfBcp47Id = LanguageSupportDefinition.IetfBcp47LanguageId.newBuilder().apply {
ietfLanguageTag = systemLocaleProfile.computeIetfLanguageTag()
}.build()
// The root profile is assumed if there is no specific language ID to use.
if (systemLocaleProfile !is AndroidLocaleProfile.RootProfile) {
ietfBcp47Id = LanguageSupportDefinition.IetfBcp47LanguageId.newBuilder().apply {
ietfLanguageTag = systemLocaleProfile.ietfLanguageTag
}.build()
}
BenHenning marked this conversation as resolved.
Show resolved Hide resolved
}.build()
}.build()

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,12 +114,16 @@ class LocaleControllerTest {
}

@Test
fun testReconstituteDisplayLocale_defaultContext_returnsDisplayLocaleForContext() {
fun testReconstituteDisplayLocale_defaultContext_throwsException() {
BenHenning marked this conversation as resolved.
Show resolved Hide resolved
val context = OppiaLocaleContext.getDefaultInstance()

val locale = localeController.reconstituteDisplayLocale(context)
val exception = assertThrows<IllegalStateException>() {
localeController.reconstituteDisplayLocale(context)
}

assertThat(locale.localeContext).isEqualToDefaultInstance()
// A default locale context isn't valid by itself (though it can represent the root locale when
// at least the app strings context is present & default).
assertThat(exception).hasMessageThat().contains("Invalid language case")
}

@Test
Expand Down Expand Up @@ -243,6 +247,25 @@ class LocaleControllerTest {
assertThat(context.regionDefinition.regionId.ietfRegionTag).isEqualTo("MC")
}

@Test
fun testAppStringLocale_rootLocale_defaultLang_returnsRootLocale() {
forceDefaultLocale(Locale.ROOT)

val localeProvider = localeController.retrieveAppStringDisplayLocale(LANGUAGE_UNSPECIFIED)

// The locale will be forced to the root locale. The root locale also should never provide an
// IETF BCP-47 ID.
val locale = monitorFactory.waitForNextSuccessfulResult(localeProvider)
val context = locale.localeContext
val languageDefinition = context.languageDefinition
assertThat(languageDefinition.language).isEqualTo(LANGUAGE_UNSPECIFIED)
assertThat(languageDefinition.fallbackMacroLanguage).isEqualTo(LANGUAGE_UNSPECIFIED)
assertThat(languageDefinition.appStringId.hasIetfBcp47Id()).isFalse()
assertThat(context.hasFallbackLanguageDefinition()).isFalse()
assertThat(context.regionDefinition.region).isEqualTo(REGION_UNSPECIFIED)
assertThat(context.regionDefinition.regionId.ietfRegionTag).isEmpty()
}

@Test
fun testAppStringLocale_newSystemLocale_doesNotNotifyProvider() {
forceDefaultLocale(Locale.US)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import org.oppia.android.app.model.AppLanguageSelection.SelectionTypeCase.USE_SY
import org.oppia.android.app.model.AudioTranslationLanguageSelection
import org.oppia.android.app.model.HtmlTranslationList
import org.oppia.android.app.model.LanguageSupportDefinition.LanguageId.LanguageTypeCase.IETF_BCP47_ID
import org.oppia.android.app.model.LanguageSupportDefinition.LanguageId.LanguageTypeCase.LANGUAGETYPE_NOT_SET
import org.oppia.android.app.model.OppiaLanguage
import org.oppia.android.app.model.OppiaLanguage.ARABIC
import org.oppia.android.app.model.OppiaLanguage.BRAZILIAN_PORTUGUESE
Expand Down Expand Up @@ -112,9 +113,7 @@ class TranslationControllerTest {
val appStringId = context.languageDefinition.appStringId
assertThat(context.usageMode).isEqualTo(APP_STRINGS)
assertThat(context.languageDefinition.language).isEqualTo(LANGUAGE_UNSPECIFIED)
assertThat(appStringId.languageTypeCase).isEqualTo(IETF_BCP47_ID)
assertThat(appStringId.ietfBcp47Id.ietfLanguageTag).isEmpty()
assertThat(appStringId.androidResourcesLanguageId.languageCode).isEmpty()
assertThat(appStringId.languageTypeCase).isEqualTo(LANGUAGETYPE_NOT_SET)
BenHenning marked this conversation as resolved.
Show resolved Hide resolved
assertThat(context.regionDefinition.region).isEqualTo(REGION_UNSPECIFIED)
}

Expand Down Expand Up @@ -290,12 +289,28 @@ class TranslationControllerTest {
val appStringId = context.languageDefinition.appStringId
assertThat(context.usageMode).isEqualTo(APP_STRINGS)
assertThat(context.languageDefinition.language).isEqualTo(LANGUAGE_UNSPECIFIED)
assertThat(appStringId.languageTypeCase).isEqualTo(IETF_BCP47_ID)
assertThat(appStringId.ietfBcp47Id.ietfLanguageTag).isEmpty()
assertThat(appStringId.androidResourcesLanguageId.languageCode).isEmpty()
assertThat(appStringId.languageTypeCase).isEqualTo(LANGUAGETYPE_NOT_SET)
assertThat(context.regionDefinition.region).isEqualTo(REGION_UNSPECIFIED)
}

@Test
fun testGetAppLanguageLocale_ptBrDefLocale_returnsLocaleWithIetfAndAndroidResourcesLangIds() {
forceDefaultLocale(BRAZIL_PORTUGUESE_LOCALE)

val localeProvider = translationController.getAppLanguageLocale(PROFILE_ID_0)

val locale = monitorFactory.waitForNextSuccessfulResult(localeProvider)
val context = locale.localeContext
val appStringId = context.languageDefinition.appStringId
assertThat(context.usageMode).isEqualTo(APP_STRINGS)
assertThat(context.languageDefinition.language).isEqualTo(BRAZILIAN_PORTUGUESE)
assertThat(appStringId.languageTypeCase).isEqualTo(IETF_BCP47_ID)
assertThat(appStringId.ietfBcp47Id.ietfLanguageTag).isEqualTo("pt-BR")
assertThat(appStringId.androidResourcesLanguageId.languageCode).isEqualTo("pt")
assertThat(appStringId.androidResourcesLanguageId.regionCode).isEqualTo("BR")
assertThat(context.regionDefinition.region).isEqualTo(BRAZIL)
}

@Test
fun testGetAppLanguageLocale_updateLanguageToEnglish_returnsEnglishLocale() {
forceDefaultLocale(Locale.ROOT)
Expand Down Expand Up @@ -1937,6 +1952,7 @@ class TranslationControllerTest {

private companion object {
private val BRAZIL_ENGLISH_LOCALE = Locale("en", "BR")
private val BRAZIL_PORTUGUESE_LOCALE = Locale("pt", "BR")
private val INDIA_HINDI_LOCALE = Locale("hi", "IN")
private val KENYA_KISWAHILI_LOCALE = Locale("sw", "KE")

Expand Down
7 changes: 7 additions & 0 deletions scripts/assets/file_content_validation_checks.textproto
Original file line number Diff line number Diff line change
Expand Up @@ -598,3 +598,10 @@ file_content_checks {
exempted_file_name: "scripts/src/javatests/org/oppia/android/scripts/regex/RegexPatternValidationCheckTest.kt"
exempted_file_name: "testing/src/test/java/org/oppia/android/testing/espresso/TextInputActionTest.kt"
}
file_content_checks {
file_path_regex: ".+?\\.kt"
prohibited_content_regex: "computeIfAbsent\\("
failure_message: "computeIfAbsent won't desugar and requires Java 8 support (SDK 24+). Suggest using an atomic Kotlin-specific solution, instead."
exempted_file_name: "scripts/src/javatests/org/oppia/android/scripts/regex/RegexPatternValidationCheckTest.kt"
exempted_file_name: "utility/src/main/java/org/oppia/android/util/caching/testing/FakeAssetRepository.kt"
BenHenning marked this conversation as resolved.
Show resolved Hide resolved
}
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,9 @@ class RegexPatternValidationCheckTest {
"ActivityScenarioRule can result in order dependence when static state leaks across tests" +
" (such as static module variables), and can make staging much more difficult for platform" +
" parameters. Use ActivityScenario directly, instead."
private val referenceComputeIfAbsent =
"computeIfAbsent won't desugar and requires Java 8 support (SDK 24+). Suggest using an atomic" +
" Kotlin-specific solution, instead."
private val wikiReferenceNote =
"Refer to https://github.com/oppia/oppia-android/wiki/Static-Analysis-Checks" +
"#regexpatternvalidation-check for more details on how to fix this."
Expand Down Expand Up @@ -2728,6 +2731,28 @@ class RegexPatternValidationCheckTest {
)
}

@Test
fun testFileContent_includesReferenceToComputeIfAbsent_fileContentIsNotCorrect() {
val prohibitedContent =
"""
someMap.computeIfAbsent(key) { createOtherValue() }
""".trimIndent()
tempFolder.newFolder("testfiles", "app", "src", "main", "java", "org", "oppia", "android")
val stringFilePath = "app/src/main/java/org/oppia/android/TestPresenter.kt"
tempFolder.newFile("testfiles/$stringFilePath").writeText(prohibitedContent)

val exception = assertThrows<Exception>() { runScript() }

assertThat(exception).hasMessageThat().contains(REGEX_CHECK_FAILED_OUTPUT_INDICATOR)
assertThat(outContent.toString().trim())
.isEqualTo(
"""
$stringFilePath:1: $referenceComputeIfAbsent
$wikiReferenceNote
""".trimIndent()
)
}

/** Runs the regex_pattern_validation_check. */
private fun runScript() {
main(File(tempFolder.root, "testfiles").absolutePath)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import kotlinx.coroutines.flow.launchIn
import kotlinx.coroutines.flow.onEach
import kotlinx.coroutines.flow.transform
import kotlinx.coroutines.launch
import org.oppia.android.util.data.DataProviders.Companion.transform
import org.oppia.android.util.logging.ExceptionLogger
import org.oppia.android.util.threading.BackgroundDispatcher
import java.util.concurrent.atomic.AtomicBoolean
Expand Down Expand Up @@ -74,7 +75,12 @@ class DataProviders @Inject constructor(
override fun getId(): Any = newId

override suspend fun retrieveData(): AsyncResult<O> {
return [email protected]().transformAsync(function)
return try {
[email protected]().transformAsync(function)
} catch (e: Exception) {
BenHenning marked this conversation as resolved.
Show resolved Hide resolved
dataProviders.exceptionLogger.logException(e)
AsyncResult.Failure(e)
}
}
}
}
Expand Down
Loading
Loading