From e049c8eaa5b625e387d90be93c72495f5d8fc8b3 Mon Sep 17 00:00:00 2001 From: Adhiambo Peres <59600948+adhiamboperes@users.noreply.github.com> Date: Tue, 20 Aug 2024 23:37:12 +0300 Subject: [PATCH 1/8] Test workflow with ActivityComponent changes --- .../java/org/oppia/android/app/activity/ActivityComponent.kt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/src/main/java/org/oppia/android/app/activity/ActivityComponent.kt b/app/src/main/java/org/oppia/android/app/activity/ActivityComponent.kt index 402161f88ea..df7d722f1ab 100644 --- a/app/src/main/java/org/oppia/android/app/activity/ActivityComponent.kt +++ b/app/src/main/java/org/oppia/android/app/activity/ActivityComponent.kt @@ -9,3 +9,5 @@ import org.oppia.android.app.utility.datetime.DateTimeUtil * Instances of this subcomponent should be created using [ActivityComponentFactory]. */ interface ActivityComponent : AppLanguageActivityInjector, DateTimeUtil.Injector + +// # Some test comment \ No newline at end of file From 1c6c9f7f430b2bdc1b13fb5057ca34fb59bffcc8 Mon Sep 17 00:00:00 2001 From: Adhiambo Peres <59600948+adhiamboperes@users.noreply.github.com> Date: Tue, 20 Aug 2024 23:49:20 +0300 Subject: [PATCH 2/8] Test workflow with TextInputActionTest changes --- .../org/oppia/android/testing/espresso/TextInputActionTest.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/testing/src/test/java/org/oppia/android/testing/espresso/TextInputActionTest.kt b/testing/src/test/java/org/oppia/android/testing/espresso/TextInputActionTest.kt index 34d7f816950..a6b703c0427 100644 --- a/testing/src/test/java/org/oppia/android/testing/espresso/TextInputActionTest.kt +++ b/testing/src/test/java/org/oppia/android/testing/espresso/TextInputActionTest.kt @@ -53,7 +53,7 @@ class TextInputActionTest { } } - @Test + /*@Test fun testTextExistsMatcher_descriptionMatchesExpectedDescription() { val errorText = "Incorrect Administrator PIN. Please try again." val expectedDescription = @@ -87,7 +87,7 @@ class TextInputActionTest { val result: Boolean = errorTextNotExisted.matches(textInputLayout) assertThat(result).isFalse() } - } + }*/ @Test fun testTextDoesNotExistMatcher_descriptionMatchesExpectedDescription() { From 02eadc16aed4014e800c2ab8496014514562ad59 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Fri, 23 Aug 2024 01:05:49 +0000 Subject: [PATCH 3/8] Fixes part of #4938: Use TranslationController as the source of truth for the audio language setting (#5487) ## Explanation Fixes part of #4938 This primarily updates ``ProfileManagementController`` to use ``TranslationController`` as the source of truth for audio language (rather than using the audio language property stored within the ``Profile`` proto). This has some noteworthy advantages: - It allows for proper translation setting fallback behaviors to be enabled for the audio language setting without needing to migrate UI code over to ``TranslationController``. - It isolates changes to the domain layer (with one exception: reading the audio language setting now uses a new getter in ``ProfileManagementController`` rather than reading the profile directly). Some peripheral changes were also needed as part of this: - A bunch of tests needed to be disabled in Gradle now that different options UIs (including for reading text) may depend on ``TranslationController`` (which is only fully configured in Bazel builds). - The ``Profile`` proto was updated to remove its audio language setting. This means that existing users will revert back to whichever default ``TranslationController`` decides for them (most likely English, but it depends on several factors). This is considered a reasonable regression since most users are unlikely to depend on the automatic audio language setting, and the app is currently in a beta state so regressions like this should be expected. - French and Chinese were removed from the list of ``AudioLanguage``s since they are not currently supported by the Oppia Android app (per ``OppiaLanguage`` which, plus the configured supported language textproto files, determine for which languages the app is guaranteeing support). - ``ProfileManagementControllerTest`` was updated to have much more thorough testing around audio language. - ``TranslationController`` was updated to use a ``PersistentCacheStore`` backing for audio and written translation languages (in addition to app language which was already supported). - As part of the previous change, ``TranslationController.updateAppLanguage()`` and its related tests were updated to verify the _previous_ language is returned, not the current (for consistency with voiceover and written translations). Long-term, ``AudioLanguage`` should be removed (along with its corresponding functionality in ``ProfileManagementController``) in favor of using ``TranslationController`` and ``OppiaLocale`` as the bases for managing language functionality in the UI layer. Note that the Gradle version of the app will have increased degraded functionality in the options menu due to no supported language configuration being included in the build for that app (see the corresponding comment thread in this PR for more context). This is considered a reasonable medium-term gap as developers ought to be using the Bazel build of the app, anyway, as the Gradle version has significant functional limitations for all aspects of language selection and management (due to the lack of language configuration). ## Essential Checklist - [x] The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".) - [x] Any changes to [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets) files have their rationale included in the PR explanation. - [x] The PR follows the [style guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide). - [x] The PR does not contain any unnecessary code changes from Android Studio ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)). - [x] The PR is made from a branch that's **not** called "develop" and is up-to-date with "develop". - [x] The PR is **assigned** to the appropriate reviewers ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)). ## For UI-specific PRs only Options screen (without changes): ![image](https://github.com/user-attachments/assets/065caa9f-5815-42a5-98ec-278186fa8dcd) Options screen (with changes): ![image](https://github.com/user-attachments/assets/f4ece283-bf0e-4490-a1bb-57e77bb7a834) I also verified setting a profile audio setting (Portuguese) on a ``develop`` branch build and then upgrading to a build from this branch. There are no crashes or stability issues, and (per my device setup) the audio language does revert back to English as expected. I also verified that, like before, the audio setting persists across app instances when changed and is distinct between multiple profiles. --- app/build.gradle | 3 +- .../app/options/OptionControlsViewModel.kt | 31 +-- .../player/audio/AudioFragmentPresenter.kt | 27 ++- .../player/audio/LanguageDialogFragment.kt | 2 - .../translation/AppLanguageResourceHandler.kt | 2 - .../app/options/AudioLanguageFragmentTest.kt | 6 +- .../options/ReadingTextSizeFragmentTest.kt | 4 + .../AppLanguageResourceHandlerTest.kt | 2 - .../profile/ProfileManagementController.kt | 75 +++++--- .../translation/TranslationController.kt | 157 ++++++++-------- .../ProfileManagementControllerTest.kt | 176 +++++++++++++++++- .../translation/TranslationControllerTest.kt | 14 +- model/src/main/proto/profile.proto | 10 +- 13 files changed, 341 insertions(+), 168 deletions(-) diff --git a/app/build.gradle b/app/build.gradle index 13fae2de07a..7c94678f75c 100644 --- a/app/build.gradle +++ b/app/build.gradle @@ -107,7 +107,8 @@ def filesToExclude = [ '**/*AppLanguageLocaleHandlerTest*.kt', '**/*AppLanguageResourceHandlerTest*.kt', '**/*AppLanguageWatcherMixinTest*.kt', - '**/*ActivityLanguageLocaleHandlerTest*.kt' + '**/*ActivityLanguageLocaleHandlerTest*.kt', + '**/*OptionsFragmentTest*.kt', // Excludes 2 tests. ] _excludeSourceFiles(filesToExclude) diff --git a/app/src/main/java/org/oppia/android/app/options/OptionControlsViewModel.kt b/app/src/main/java/org/oppia/android/app/options/OptionControlsViewModel.kt index cd03b74450a..8bc4515efd1 100644 --- a/app/src/main/java/org/oppia/android/app/options/OptionControlsViewModel.kt +++ b/app/src/main/java/org/oppia/android/app/options/OptionControlsViewModel.kt @@ -5,8 +5,8 @@ import androidx.databinding.ObservableField import androidx.lifecycle.LiveData import androidx.lifecycle.MutableLiveData import androidx.lifecycle.Transformations -import androidx.lifecycle.ViewModel import org.oppia.android.app.fragment.FragmentScope +import org.oppia.android.app.model.AudioLanguage import org.oppia.android.app.model.OppiaLanguage import org.oppia.android.app.model.Profile import org.oppia.android.app.model.ProfileId @@ -20,7 +20,8 @@ import org.oppia.android.util.data.DataProviders.Companion.combineWith import org.oppia.android.util.data.DataProviders.Companion.toLiveData import javax.inject.Inject -/** [ViewModel] for [OptionsFragment]. */ +private const val OPTIONS_ITEM_VIEW_MODEL_APP_AUDIO_LANGUAGE_PROVIDER_ID = + "OPTIONS_ITEM_VIEW_MODEL_APP_AUDIO_LANGUAGE_PROVIDER_ID" private const val OPTIONS_ITEM_VIEW_MODEL_LIST_PROVIDER_ID = "OPTIONS_ITEM_VIEW_MODEL_LIST_PROVIDER_ID" @@ -67,11 +68,14 @@ class OptionControlsViewModel @Inject constructor( } private fun createOptionsItemViewModelProvider(): DataProvider> { + val appAudioLangProvider = + translationController.getAppLanguage(profileId).combineWith( + profileManagementController.getAudioLanguage(profileId), + OPTIONS_ITEM_VIEW_MODEL_APP_AUDIO_LANGUAGE_PROVIDER_ID + ) { appLanguage, audioLanguage -> appLanguage to audioLanguage } return profileManagementController.getProfile(profileId).combineWith( - translationController.getAppLanguage(profileId), - OPTIONS_ITEM_VIEW_MODEL_LIST_PROVIDER_ID, - ::processViewModelList - ) + appAudioLangProvider, OPTIONS_ITEM_VIEW_MODEL_LIST_PROVIDER_ID + ) { profile, (appLang, audioLang) -> processViewModelList(profile, appLang, audioLang) } } private fun processViewModelListsResult( @@ -93,12 +97,13 @@ class OptionControlsViewModel @Inject constructor( private fun processViewModelList( profile: Profile, - oppiaLanguage: OppiaLanguage + appLanguage: OppiaLanguage, + audioLanguage: AudioLanguage ): List { return listOfNotNull( createReadingTextSizeViewModel(profile), - createAppLanguageViewModel(oppiaLanguage), - createAudioLanguageViewModel(profile) + createAppLanguageViewModel(appLanguage), + createAudioLanguageViewModel(audioLanguage) ) } @@ -117,12 +122,14 @@ class OptionControlsViewModel @Inject constructor( ) } - private fun createAudioLanguageViewModel(profile: Profile): OptionsAudioLanguageViewModel { + private fun createAudioLanguageViewModel( + audioLanguage: AudioLanguage + ): OptionsAudioLanguageViewModel { return OptionsAudioLanguageViewModel( routeToAudioLanguageListListener, loadAudioLanguageListListener, - profile.audioLanguage, - resourceHandler.computeLocalizedDisplayName(profile.audioLanguage) + audioLanguage, + resourceHandler.computeLocalizedDisplayName(audioLanguage) ) } } diff --git a/app/src/main/java/org/oppia/android/app/player/audio/AudioFragmentPresenter.kt b/app/src/main/java/org/oppia/android/app/player/audio/AudioFragmentPresenter.kt index 69b433d8aa9..60f4214458e 100644 --- a/app/src/main/java/org/oppia/android/app/player/audio/AudioFragmentPresenter.kt +++ b/app/src/main/java/org/oppia/android/app/player/audio/AudioFragmentPresenter.kt @@ -17,7 +17,6 @@ import org.oppia.android.R import org.oppia.android.app.fragment.FragmentScope import org.oppia.android.app.model.AudioLanguage import org.oppia.android.app.model.CellularDataPreference -import org.oppia.android.app.model.Profile import org.oppia.android.app.model.ProfileId import org.oppia.android.app.model.Spotlight import org.oppia.android.app.model.State @@ -147,15 +146,15 @@ class AudioFragmentPresenter @Inject constructor( ) as? SpotlightManager } - private fun getProfileData(): LiveData { + private fun retrieveAudioLanguageCode(): LiveData { return Transformations.map( - profileManagementController.getProfile(profileId).toLiveData(), - ::processGetProfileResult + profileManagementController.getAudioLanguage(profileId).toLiveData(), + ::processAudioLanguageResult ) } private fun subscribeToAudioLanguageLiveData() { - getProfileData().observe( + retrieveAudioLanguageCode().observe( activity, Observer { result -> audioViewModel.selectedLanguageCode = result @@ -165,11 +164,9 @@ class AudioFragmentPresenter @Inject constructor( } /** Gets language code by [AudioLanguage]. */ - private fun getAudioLanguage(audioLanguage: AudioLanguage): String { + private fun computeLanguageCode(audioLanguage: AudioLanguage): String { return when (audioLanguage) { AudioLanguage.HINDI_AUDIO_LANGUAGE -> "hi" - AudioLanguage.FRENCH_AUDIO_LANGUAGE -> "fr" - AudioLanguage.CHINESE_AUDIO_LANGUAGE -> "zh" AudioLanguage.BRAZILIAN_PORTUGUESE_LANGUAGE -> "pt" AudioLanguage.ARABIC_LANGUAGE -> "ar" AudioLanguage.NIGERIAN_PIDGIN_LANGUAGE -> "pcm" @@ -178,16 +175,16 @@ class AudioFragmentPresenter @Inject constructor( } } - private fun processGetProfileResult(profileResult: AsyncResult): String { - val profile = when (profileResult) { + private fun processAudioLanguageResult(languageResult: AsyncResult): String { + val audioLanguage = when (languageResult) { is AsyncResult.Failure -> { - oppiaLogger.e("AudioFragment", "Failed to retrieve profile", profileResult.error) - Profile.getDefaultInstance() + oppiaLogger.e("AudioFragment", "Failed to retrieve audio language", languageResult.error) + AudioLanguage.AUDIO_LANGUAGE_UNSPECIFIED } - is AsyncResult.Pending -> Profile.getDefaultInstance() - is AsyncResult.Success -> profileResult.value + is AsyncResult.Pending -> AudioLanguage.AUDIO_LANGUAGE_UNSPECIFIED + is AsyncResult.Success -> languageResult.value } - return getAudioLanguage(profile.audioLanguage) + return computeLanguageCode(audioLanguage) } /** Sets selected language code in presenter and ViewModel. */ diff --git a/app/src/main/java/org/oppia/android/app/player/audio/LanguageDialogFragment.kt b/app/src/main/java/org/oppia/android/app/player/audio/LanguageDialogFragment.kt index 178c93c57cb..158d59290c7 100644 --- a/app/src/main/java/org/oppia/android/app/player/audio/LanguageDialogFragment.kt +++ b/app/src/main/java/org/oppia/android/app/player/audio/LanguageDialogFragment.kt @@ -80,8 +80,6 @@ class LanguageDialogFragment : InjectableDialogFragment() { for (languageCode in languageCodeArrayList) { val audioLanguage = when (machineLocale.run { languageCode.toMachineLowerCase() }) { "hi" -> AudioLanguage.HINDI_AUDIO_LANGUAGE - "fr" -> AudioLanguage.FRENCH_AUDIO_LANGUAGE - "zh" -> AudioLanguage.CHINESE_AUDIO_LANGUAGE "pt", "pt-br" -> AudioLanguage.BRAZILIAN_PORTUGUESE_LANGUAGE "ar" -> AudioLanguage.ARABIC_LANGUAGE "pcm" -> AudioLanguage.NIGERIAN_PIDGIN_LANGUAGE diff --git a/app/src/main/java/org/oppia/android/app/translation/AppLanguageResourceHandler.kt b/app/src/main/java/org/oppia/android/app/translation/AppLanguageResourceHandler.kt index 05ad59fac13..be2fa522409 100644 --- a/app/src/main/java/org/oppia/android/app/translation/AppLanguageResourceHandler.kt +++ b/app/src/main/java/org/oppia/android/app/translation/AppLanguageResourceHandler.kt @@ -154,8 +154,6 @@ class AppLanguageResourceHandler @Inject constructor( fun computeLocalizedDisplayName(audioLanguage: AudioLanguage): String { return when (audioLanguage) { AudioLanguage.HINDI_AUDIO_LANGUAGE -> getLocalizedDisplayName("hi") - AudioLanguage.FRENCH_AUDIO_LANGUAGE -> getLocalizedDisplayName("fr") - AudioLanguage.CHINESE_AUDIO_LANGUAGE -> getLocalizedDisplayName("zh") AudioLanguage.BRAZILIAN_PORTUGUESE_LANGUAGE -> getLocalizedDisplayName("pt", "BR") AudioLanguage.ARABIC_LANGUAGE -> getLocalizedDisplayName("ar", "EG") AudioLanguage.NIGERIAN_PIDGIN_LANGUAGE -> diff --git a/app/src/sharedTest/java/org/oppia/android/app/options/AudioLanguageFragmentTest.kt b/app/src/sharedTest/java/org/oppia/android/app/options/AudioLanguageFragmentTest.kt index b25607c9158..7ad7462af04 100644 --- a/app/src/sharedTest/java/org/oppia/android/app/options/AudioLanguageFragmentTest.kt +++ b/app/src/sharedTest/java/org/oppia/android/app/options/AudioLanguageFragmentTest.kt @@ -112,9 +112,9 @@ import javax.inject.Singleton class AudioLanguageFragmentTest { private companion object { private const val ENGLISH_BUTTON_INDEX = 0 - private const val PORTUGUESE_BUTTON_INDEX = 4 - private const val ARABIC_BUTTON_INDEX = 5 - private const val NIGERIAN_PIDGIN_BUTTON_INDEX = 6 + private const val PORTUGUESE_BUTTON_INDEX = 2 + private const val ARABIC_BUTTON_INDEX = 3 + private const val NIGERIAN_PIDGIN_BUTTON_INDEX = 4 } @get:Rule val initializeDefaultLocaleRule = InitializeDefaultLocaleRule() diff --git a/app/src/sharedTest/java/org/oppia/android/app/options/ReadingTextSizeFragmentTest.kt b/app/src/sharedTest/java/org/oppia/android/app/options/ReadingTextSizeFragmentTest.kt index 75e9b20db35..5e4b14be4aa 100644 --- a/app/src/sharedTest/java/org/oppia/android/app/options/ReadingTextSizeFragmentTest.kt +++ b/app/src/sharedTest/java/org/oppia/android/app/options/ReadingTextSizeFragmentTest.kt @@ -74,7 +74,9 @@ import org.oppia.android.domain.platformparameter.PlatformParameterModule import org.oppia.android.domain.platformparameter.PlatformParameterSingletonModule import org.oppia.android.domain.question.QuestionModule import org.oppia.android.domain.workmanager.WorkManagerConfigurationModule +import org.oppia.android.testing.BuildEnvironment import org.oppia.android.testing.OppiaTestRule +import org.oppia.android.testing.RunOn import org.oppia.android.testing.TestLogReportingModule import org.oppia.android.testing.firebase.TestAuthenticationModule import org.oppia.android.testing.junit.InitializeDefaultLocaleRule @@ -180,8 +182,10 @@ class ReadingTextSizeFragmentTest { } } + // Requires language configurations. @Test @Config(qualifiers = "sw600dp") + @RunOn(buildEnvironments = [BuildEnvironment.BAZEL]) fun testTextSize_changeTextSizeToMedium_mediumItemIsSelected() { launch(createOptionActivityIntent(0, true)).use { testCoroutineDispatchers.runCurrent() diff --git a/app/src/test/java/org/oppia/android/app/translation/AppLanguageResourceHandlerTest.kt b/app/src/test/java/org/oppia/android/app/translation/AppLanguageResourceHandlerTest.kt index 76811f7f5b3..9c6e56f953a 100644 --- a/app/src/test/java/org/oppia/android/app/translation/AppLanguageResourceHandlerTest.kt +++ b/app/src/test/java/org/oppia/android/app/translation/AppLanguageResourceHandlerTest.kt @@ -494,8 +494,6 @@ class AppLanguageResourceHandlerTest { // TODO(#3793): Remove this once OppiaLanguage is used as the source of truth. @Test @Iteration("hi", "lang=HINDI_AUDIO_LANGUAGE", "expectedDisplayText=हिन्दी") - @Iteration("fr", "lang=FRENCH_AUDIO_LANGUAGE", "expectedDisplayText=Français") - @Iteration("zh", "lang=CHINESE_AUDIO_LANGUAGE", "expectedDisplayText=中文") @Iteration("pr-pt", "lang=BRAZILIAN_PORTUGUESE_LANGUAGE", "expectedDisplayText=Português") @Iteration("ar", "lang=ARABIC_LANGUAGE", "expectedDisplayText=العربية") @Iteration("pcm", "lang=NIGERIAN_PIDGIN_LANGUAGE", "expectedDisplayText=Naijá") diff --git a/domain/src/main/java/org/oppia/android/domain/profile/ProfileManagementController.kt b/domain/src/main/java/org/oppia/android/domain/profile/ProfileManagementController.kt index 51db3f941bb..983caebf6db 100644 --- a/domain/src/main/java/org/oppia/android/domain/profile/ProfileManagementController.kt +++ b/domain/src/main/java/org/oppia/android/domain/profile/ProfileManagementController.kt @@ -9,7 +9,9 @@ import android.provider.MediaStore import androidx.exifinterface.media.ExifInterface import kotlinx.coroutines.Deferred import org.oppia.android.app.model.AudioLanguage +import org.oppia.android.app.model.AudioTranslationLanguageSelection import org.oppia.android.app.model.DeviceSettings +import org.oppia.android.app.model.OppiaLanguage import org.oppia.android.app.model.Profile import org.oppia.android.app.model.ProfileAvatar import org.oppia.android.app.model.ProfileDatabase @@ -22,6 +24,7 @@ import org.oppia.android.domain.oppialogger.LoggingIdentifierController import org.oppia.android.domain.oppialogger.OppiaLogger import org.oppia.android.domain.oppialogger.analytics.LearnerAnalyticsLogger import org.oppia.android.domain.oppialogger.exceptions.ExceptionsController +import org.oppia.android.domain.translation.TranslationController import org.oppia.android.util.data.AsyncResult import org.oppia.android.util.data.DataProvider import org.oppia.android.util.data.DataProviders @@ -64,8 +67,8 @@ private const val SET_CURRENT_PROFILE_ID_PROVIDER_ID = "set_current_profile_id_p private const val UPDATE_READING_TEXT_SIZE_PROVIDER_ID = "update_reading_text_size_provider_id" private const val UPDATE_APP_LANGUAGE_PROVIDER_ID = "update_app_language_provider_id" -private const val UPDATE_AUDIO_LANGUAGE_PROVIDER_ID = - "update_audio_language_provider_id" +private const val GET_AUDIO_LANGUAGE_PROVIDER_ID = "get_audio_language_provider_id" +private const val UPDATE_AUDIO_LANGUAGE_PROVIDER_ID = "update_audio_language_provider_id" private const val UPDATE_LEARNER_ID_PROVIDER_ID = "update_learner_id_provider_id" private const val SET_SURVEY_LAST_SHOWN_TIMESTAMP_PROVIDER_ID = "record_survey_last_shown_timestamp_provider_id" @@ -93,7 +96,8 @@ class ProfileManagementController @Inject constructor( private val enableLearnerStudyAnalytics: PlatformParameterValue, @EnableLoggingLearnerStudyIds private val enableLoggingLearnerStudyIds: PlatformParameterValue, - private val profileNameValidator: ProfileNameValidator + private val profileNameValidator: ProfileNameValidator, + private val translationController: TranslationController ) { private var currentProfileId: Int = DEFAULT_LOGGED_OUT_INTERNAL_PROFILE_ID private val profileDataStore = @@ -275,7 +279,6 @@ class ProfileManagementController @Inject constructor( dateCreatedTimestampMs = oppiaClock.getCurrentTimeMs() this.isAdmin = isAdmin readingTextSize = ReadingTextSize.MEDIUM_TEXT_SIZE - audioLanguage = AudioLanguage.ENGLISH_AUDIO_LANGUAGE numberOfLogins = 0 if (enableLoggingLearnerStudyIds.value) { @@ -628,34 +631,52 @@ class ProfileManagementController @Inject constructor( } } + /** + * Returns the current audio language configured for the specified profile ID, as possibly set by + * [updateAudioLanguage]. + * + * The return [DataProvider] will automatically update for subsequent calls to + * [updateAudioLanguage] for this [profileId]. + */ + fun getAudioLanguage(profileId: ProfileId): DataProvider { + return translationController.getAudioTranslationContentLanguage( + profileId + ).transform(GET_AUDIO_LANGUAGE_PROVIDER_ID) { oppiaLanguage -> + when (oppiaLanguage) { + OppiaLanguage.UNRECOGNIZED, OppiaLanguage.LANGUAGE_UNSPECIFIED, OppiaLanguage.HINGLISH, + OppiaLanguage.PORTUGUESE, OppiaLanguage.SWAHILI -> AudioLanguage.AUDIO_LANGUAGE_UNSPECIFIED + OppiaLanguage.ARABIC -> AudioLanguage.ARABIC_LANGUAGE + OppiaLanguage.ENGLISH -> AudioLanguage.ENGLISH_AUDIO_LANGUAGE + OppiaLanguage.HINDI -> AudioLanguage.HINDI_AUDIO_LANGUAGE + OppiaLanguage.BRAZILIAN_PORTUGUESE -> AudioLanguage.BRAZILIAN_PORTUGUESE_LANGUAGE + OppiaLanguage.NIGERIAN_PIDGIN -> AudioLanguage.NIGERIAN_PIDGIN_LANGUAGE + } + } + } + /** * Updates the audio language of the profile. * - * @param profileId the ID corresponding to the profile being updated. - * @param audioLanguage New audio language for the profile being updated. - * @return a [DataProvider] that indicates the success/failure of this update operation. + * @param profileId the ID corresponding to the profile being updated + * @param audioLanguage New audio language for the profile being updated + * @return a [DataProvider] that indicates the success/failure of this update operation */ fun updateAudioLanguage(profileId: ProfileId, audioLanguage: AudioLanguage): DataProvider { - val deferred = profileDataStore.storeDataWithCustomChannelAsync( - updateInMemoryCache = true - ) { - val profile = - it.profilesMap[profileId.internalId] ?: return@storeDataWithCustomChannelAsync Pair( - it, - ProfileActionStatus.PROFILE_NOT_FOUND - ) - val updatedProfile = profile.toBuilder().setAudioLanguage(audioLanguage).build() - val profileDatabaseBuilder = it.toBuilder().putProfiles( - profileId.internalId, - updatedProfile - ) - Pair(profileDatabaseBuilder.build(), ProfileActionStatus.SUCCESS) - } - return dataProviders.createInMemoryDataProviderAsync( - UPDATE_AUDIO_LANGUAGE_PROVIDER_ID - ) { - return@createInMemoryDataProviderAsync getDeferredResult(profileId, null, deferred) - } + val audioSelection = AudioTranslationLanguageSelection.newBuilder().apply { + this.selectedLanguage = when (audioLanguage) { + AudioLanguage.UNRECOGNIZED, AudioLanguage.AUDIO_LANGUAGE_UNSPECIFIED, + AudioLanguage.NO_AUDIO -> OppiaLanguage.LANGUAGE_UNSPECIFIED + AudioLanguage.ENGLISH_AUDIO_LANGUAGE -> OppiaLanguage.ENGLISH + AudioLanguage.HINDI_AUDIO_LANGUAGE -> OppiaLanguage.HINDI + AudioLanguage.BRAZILIAN_PORTUGUESE_LANGUAGE -> OppiaLanguage.BRAZILIAN_PORTUGUESE + AudioLanguage.ARABIC_LANGUAGE -> OppiaLanguage.ARABIC + AudioLanguage.NIGERIAN_PIDGIN_LANGUAGE -> OppiaLanguage.NIGERIAN_PIDGIN + } + }.build() + // The transformation is needed to reinterpreted the result of the update to 'Any?'. + return translationController.updateAudioTranslationContentLanguage( + profileId, audioSelection + ).transform(UPDATE_AUDIO_LANGUAGE_PROVIDER_ID) { value -> value } } /** diff --git a/domain/src/main/java/org/oppia/android/domain/translation/TranslationController.kt b/domain/src/main/java/org/oppia/android/domain/translation/TranslationController.kt index a916d935ac4..11f54e9cfba 100644 --- a/domain/src/main/java/org/oppia/android/domain/translation/TranslationController.kt +++ b/domain/src/main/java/org/oppia/android/domain/translation/TranslationController.kt @@ -1,5 +1,6 @@ package org.oppia.android.domain.translation +import com.google.protobuf.MessageLite import org.oppia.android.app.model.AppLanguageSelection import org.oppia.android.app.model.AudioTranslationLanguageSelection import org.oppia.android.app.model.LanguageSupportDefinition @@ -28,10 +29,8 @@ import org.oppia.android.util.data.DataProviders.Companion.combineWithAsync import org.oppia.android.util.data.DataProviders.Companion.transform import org.oppia.android.util.data.DataProviders.Companion.transformAsync import org.oppia.android.util.locale.OppiaLocale -import java.util.concurrent.locks.ReentrantLock import javax.inject.Inject import javax.inject.Singleton -import kotlin.concurrent.withLock private const val SYSTEM_LANGUAGE_LOCALE_DATA_PROVIDER_ID = "system_language_locale" private const val APP_LANGUAGE_DATA_PROVIDER_ID = "app_language" @@ -56,6 +55,10 @@ private const val AUDIO_TRANSLATION_CONTENT_SELECTION_DATA_PROVIDER_ID = private const val UPDATE_AUDIO_TRANSLATION_CONTENT_DATA_PROVIDER_ID = "update_audio_translation_content" private const val APP_LANGUAGE_CONTENT_DATABASE = "app_language_content_database" +private const val WRITTEN_TRANSLATION_LANGUAGE_CONTENT_DATABASE = + "written_language_content_database" +private const val AUDIO_TRANSLATION_LANGUAGE_CONTENT_DATABASE = + "audio_translation_language_content_database" private const val RETRIEVED_CONTENT_LANGUAGE_DATA_PROVIDER_ID = "retrieved_content_language_data_provider_id" @@ -76,17 +79,12 @@ class TranslationController @Inject constructor( private val cacheStoreFactory: PersistentCacheStore.Factory, private val oppiaLogger: OppiaLogger, ) { - // TODO(#4938): Finish this implementation. The implementation below saves/restores per-profile app - // language, but not audio language. - - private val dataLock = ReentrantLock() - private val writtenTranslationLanguageSettings = - mutableMapOf() - private val audioVoiceoverLanguageSettings = - mutableMapOf() - - private val cacheStoreMap = + private val appLanguageCacheStoreMap = mutableMapOf>() + private val writtenTranslationLanguageCacheStoreMap = + mutableMapOf>() + private val audioTranslationLanguageCacheStoreMap = + mutableMapOf>() /** * Returns a data provider for an app string [OppiaLocale.DisplayLocale] corresponding to the @@ -144,7 +142,7 @@ class TranslationController @Inject constructor( * the underlying configured selection. */ fun getAppLanguageSelection(profileId: ProfileId): DataProvider = - retrieveLanguageContentCacheStore(profileId) + retrieveAppLanguageContentCacheStore(profileId) /** * Updates the language to be used by the specified user for app string translations. Note that @@ -156,18 +154,17 @@ class TranslationController @Inject constructor( * language matches a supported language, otherwise the app defaults to English). * * @return a [DataProvider] which succeeds only if the update succeeds, otherwise fails. The - * payload of the data provider is the *current* selection state. + * payload of the data provider is the *previous* selection state. */ fun updateAppLanguage( profileId: ProfileId, selection: AppLanguageSelection ): DataProvider { - val cacheStore = retrieveLanguageContentCacheStore(profileId) - val deferred = cacheStore.storeDataAsync(updateInMemoryCache = true) { selection } - + val cacheStore = retrieveAppLanguageContentCacheStore(profileId) return dataProviders.createInMemoryDataProviderAsync(UPDATE_APP_LANGUAGE_DATA_PROVIDER_ID) { - deferred.await() - AsyncResult.Success(cacheStore.readDataAsync().await()) + AsyncResult.Success(cacheStore.readDataAsync().await()).also { + cacheStore.storeDataAsync(updateInMemoryCache = true) { selection }.await() + } } } @@ -216,12 +213,8 @@ class TranslationController @Inject constructor( */ fun getWrittenTranslationContentLanguageSelection( profileId: ProfileId - ): DataProvider { - val providerId = WRITTEN_TRANSLATION_CONTENT_SELECTION_DATA_PROVIDER_ID - return dataProviders.createInMemoryDataProvider(providerId) { - retrieveWrittenTranslationContentLanguageSelection(profileId) - } - } + ): DataProvider = + retrieveWrittenTranslationLanguageContentCacheStore(profileId) /** * Updates the language to be used by the specified user for written content string translations. @@ -240,9 +233,13 @@ class TranslationController @Inject constructor( profileId: ProfileId, selection: WrittenTranslationLanguageSelection ): DataProvider { - val providerId = UPDATE_WRITTEN_TRANSLATION_CONTENT_DATA_PROVIDER_ID - return dataProviders.createInMemoryDataProviderAsync(providerId) { - AsyncResult.Success(updateWrittenTranslationContentLanguageSelection(profileId, selection)) + val cacheStore = retrieveWrittenTranslationLanguageContentCacheStore(profileId) + return dataProviders.createInMemoryDataProviderAsync( + UPDATE_WRITTEN_TRANSLATION_CONTENT_DATA_PROVIDER_ID + ) { + AsyncResult.Success(cacheStore.readDataAsync().await()).also { + cacheStore.storeDataAsync(updateInMemoryCache = true) { selection }.await() + } } } @@ -290,12 +287,8 @@ class TranslationController @Inject constructor( */ fun getAudioTranslationContentLanguageSelection( profileId: ProfileId - ): DataProvider { - val providerId = AUDIO_TRANSLATION_CONTENT_SELECTION_DATA_PROVIDER_ID - return dataProviders.createInMemoryDataProvider(providerId) { - retrieveAudioTranslationContentLanguageSelection(profileId) - } - } + ): DataProvider = + retrieveAudioTranslationLanguageContentCacheStore(profileId) /** * Updates the language to be used by the specified user for audio voiceover selection. Note that @@ -314,9 +307,13 @@ class TranslationController @Inject constructor( profileId: ProfileId, selection: AudioTranslationLanguageSelection ): DataProvider { - val providerId = UPDATE_AUDIO_TRANSLATION_CONTENT_DATA_PROVIDER_ID - return dataProviders.createInMemoryDataProviderAsync(providerId) { - AsyncResult.Success(updateAudioTranslationContentLanguageSelection(profileId, selection)) + val cacheStore = retrieveAudioTranslationLanguageContentCacheStore(profileId) + return dataProviders.createInMemoryDataProviderAsync( + UPDATE_AUDIO_TRANSLATION_CONTENT_DATA_PROVIDER_ID + ) { + AsyncResult.Success(cacheStore.readDataAsync().await()).also { + cacheStore.storeDataAsync(updateInMemoryCache = true) { selection }.await() + } } } @@ -414,15 +411,49 @@ class TranslationController @Inject constructor( } } - private fun retrieveLanguageContentCacheStore( + private fun retrieveAppLanguageContentCacheStore( profileId: ProfileId ): PersistentCacheStore { - return cacheStoreMap.getOrPut(profileId) { + return retrieveContentCacheStore( + profileId, + APP_LANGUAGE_CONTENT_DATABASE, + AppLanguageSelection.getDefaultInstance(), + appLanguageCacheStoreMap + ) + } + + private fun retrieveWrittenTranslationLanguageContentCacheStore( + profileId: ProfileId + ): PersistentCacheStore { + return retrieveContentCacheStore( + profileId, + WRITTEN_TRANSLATION_LANGUAGE_CONTENT_DATABASE, + WrittenTranslationLanguageSelection.getDefaultInstance(), + writtenTranslationLanguageCacheStoreMap + ) + } + + private fun retrieveAudioTranslationLanguageContentCacheStore( + profileId: ProfileId + ): PersistentCacheStore { + return retrieveContentCacheStore( + profileId, + AUDIO_TRANSLATION_LANGUAGE_CONTENT_DATABASE, + AudioTranslationLanguageSelection.getDefaultInstance(), + audioTranslationLanguageCacheStoreMap + ) + } + + private fun retrieveContentCacheStore( + profileId: ProfileId, + databaseName: String, + defaultCacheValue: T, + cacheMap: MutableMap> + ): PersistentCacheStore { + return cacheMap.getOrPut(profileId) { cacheStoreFactory.createPerProfile( - APP_LANGUAGE_CONTENT_DATABASE, - AppLanguageSelection.getDefaultInstance(), - profileId - ).also> { + databaseName, defaultCacheValue, profileId + ).also> { it.primeInMemoryAndDiskCacheAsync( updateMode = PersistentCacheStore.UpdateMode.UPDATE_IF_NEW_CACHE, publishMode = PersistentCacheStore.PublishMode.PUBLISH_TO_IN_MEMORY_CACHE @@ -439,46 +470,6 @@ class TranslationController @Inject constructor( } } - private fun retrieveWrittenTranslationContentLanguageSelection( - profileId: ProfileId - ): WrittenTranslationLanguageSelection { - return dataLock.withLock { - writtenTranslationLanguageSettings[profileId] - ?: WrittenTranslationLanguageSelection.getDefaultInstance() - } - } - - private suspend fun updateWrittenTranslationContentLanguageSelection( - profileId: ProfileId, - selection: WrittenTranslationLanguageSelection - ): WrittenTranslationLanguageSelection { - return dataLock.withLock { - writtenTranslationLanguageSettings.put(profileId, selection) - }.also { - asyncDataSubscriptionManager.notifyChange(WRITTEN_TRANSLATION_CONTENT_LOCALE_DATA_PROVIDER_ID) - } ?: WrittenTranslationLanguageSelection.getDefaultInstance() - } - - private fun retrieveAudioTranslationContentLanguageSelection( - profileId: ProfileId - ): AudioTranslationLanguageSelection { - return dataLock.withLock { - audioVoiceoverLanguageSettings[profileId] - ?: AudioTranslationLanguageSelection.getDefaultInstance() - } - } - - private suspend fun updateAudioTranslationContentLanguageSelection( - profileId: ProfileId, - selection: AudioTranslationLanguageSelection - ): AudioTranslationLanguageSelection { - return dataLock.withLock { - audioVoiceoverLanguageSettings.put(profileId, selection) - }.also { - asyncDataSubscriptionManager.notifyChange(AUDIO_TRANSLATION_CONTENT_LOCALE_DATA_PROVIDER_ID) - } ?: AudioTranslationLanguageSelection.getDefaultInstance() - } - private fun getSystemLanguage(): DataProvider = localeController.retrieveSystemLanguage() diff --git a/domain/src/test/java/org/oppia/android/domain/profile/ProfileManagementControllerTest.kt b/domain/src/test/java/org/oppia/android/domain/profile/ProfileManagementControllerTest.kt index ba6082fa1c5..91d58907646 100644 --- a/domain/src/test/java/org/oppia/android/domain/profile/ProfileManagementControllerTest.kt +++ b/domain/src/test/java/org/oppia/android/domain/profile/ProfileManagementControllerTest.kt @@ -17,10 +17,14 @@ import kotlinx.coroutines.async import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import org.junit.After +import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith -import org.oppia.android.app.model.AudioLanguage -import org.oppia.android.app.model.AudioLanguage.FRENCH_AUDIO_LANGUAGE +import org.oppia.android.app.model.AudioLanguage.ARABIC_LANGUAGE +import org.oppia.android.app.model.AudioLanguage.BRAZILIAN_PORTUGUESE_LANGUAGE +import org.oppia.android.app.model.AudioLanguage.ENGLISH_AUDIO_LANGUAGE +import org.oppia.android.app.model.AudioLanguage.HINDI_AUDIO_LANGUAGE +import org.oppia.android.app.model.AudioLanguage.NIGERIAN_PIDGIN_LANGUAGE import org.oppia.android.app.model.Profile import org.oppia.android.app.model.ProfileDatabase import org.oppia.android.app.model.ProfileId @@ -31,7 +35,10 @@ import org.oppia.android.domain.oppialogger.ApplicationIdSeed import org.oppia.android.domain.oppialogger.LogStorageModule import org.oppia.android.domain.oppialogger.LoggingIdentifierController import org.oppia.android.domain.oppialogger.analytics.ApplicationLifecycleModule +import org.oppia.android.testing.BuildEnvironment import org.oppia.android.testing.FakeAnalyticsEventLogger +import org.oppia.android.testing.OppiaTestRule +import org.oppia.android.testing.RunOn import org.oppia.android.testing.TestLogReportingModule import org.oppia.android.testing.data.DataProviderTestMonitor import org.oppia.android.testing.logging.EventLogSubject.Companion.assertThat @@ -74,6 +81,7 @@ import javax.inject.Singleton @LooperMode(LooperMode.Mode.PAUSED) @Config(application = ProfileManagementControllerTest.TestApplication::class) class ProfileManagementControllerTest { + @get:Rule val oppiaTestRule = OppiaTestRule() @Inject lateinit var context: Context @Inject lateinit var profileTestHelper: ProfileTestHelper @Inject lateinit var profileManagementController: ProfileManagementController @@ -129,7 +137,6 @@ class ProfileManagementControllerTest { assertThat(profile.allowDownloadAccess).isEqualTo(true) assertThat(profile.id.internalId).isEqualTo(0) assertThat(profile.readingTextSize).isEqualTo(MEDIUM_TEXT_SIZE) - assertThat(profile.audioLanguage).isEqualTo(AudioLanguage.ENGLISH_AUDIO_LANGUAGE) assertThat(profile.numberOfLogins).isEqualTo(0) assertThat(profile.isContinueButtonAnimationSeen).isEqualTo(false) assertThat(File(getAbsoluteDirPath("0")).isDirectory).isTrue() @@ -197,7 +204,6 @@ class ProfileManagementControllerTest { assertThat(profile.allowDownloadAccess).isEqualTo(false) assertThat(profile.id.internalId).isEqualTo(3) assertThat(profile.readingTextSize).isEqualTo(MEDIUM_TEXT_SIZE) - assertThat(profile.audioLanguage).isEqualTo(AudioLanguage.ENGLISH_AUDIO_LANGUAGE) } @Test @@ -715,18 +721,168 @@ class ProfileManagementControllerTest { assertThat(profile.readingTextSize).isEqualTo(MEDIUM_TEXT_SIZE) } + // Requires language configurations. @Test - fun testUpdateAudioLanguage_addProfiles_updateWithFrenchLanguage_checkUpdateIsSuccessful() { + @RunOn(buildEnvironments = [BuildEnvironment.BAZEL]) + fun testGetAudioLanguage_initialProfileCreation_defaultsToEnglish() { + setUpTestApplicationComponent() + + addTestProfiles() + + val audioLanguageProvider = profileManagementController.getAudioLanguage(PROFILE_ID_2) + val audioLanguage = monitorFactory.waitForNextSuccessfulResult(audioLanguageProvider) + assertThat(audioLanguage).isEqualTo(ENGLISH_AUDIO_LANGUAGE) + } + + @Test + fun testUpdateAudioLanguage_updateToHindi_updateIsSuccessful() { setUpTestApplicationComponent() addTestProfiles() val updateProvider = - profileManagementController.updateAudioLanguage(PROFILE_ID_2, FRENCH_AUDIO_LANGUAGE) - val profileProvider = profileManagementController.getProfile(PROFILE_ID_2) + profileManagementController.updateAudioLanguage(PROFILE_ID_2, HINDI_AUDIO_LANGUAGE) + val monitor = monitorFactory.createMonitor(updateProvider) + testCoroutineDispatchers.runCurrent() - monitorFactory.waitForNextSuccessfulResult(updateProvider) - val profile = monitorFactory.waitForNextSuccessfulResult(profileProvider) - assertThat(profile.audioLanguage).isEqualTo(FRENCH_AUDIO_LANGUAGE) + monitor.ensureNextResultIsSuccess() + } + + @Test + fun testUpdateAudioLanguage_updateToBrazilianPortuguese_updateIsSuccessful() { + setUpTestApplicationComponent() + addTestProfiles() + + val updateProvider = + profileManagementController.updateAudioLanguage(PROFILE_ID_2, BRAZILIAN_PORTUGUESE_LANGUAGE) + val monitor = monitorFactory.createMonitor(updateProvider) + testCoroutineDispatchers.runCurrent() + + monitor.ensureNextResultIsSuccess() + } + + @Test + fun testUpdateAudioLanguage_updateToArabic_updateIsSuccessful() { + setUpTestApplicationComponent() + addTestProfiles() + + val updateProvider = + profileManagementController.updateAudioLanguage(PROFILE_ID_2, ARABIC_LANGUAGE) + val monitor = monitorFactory.createMonitor(updateProvider) + testCoroutineDispatchers.runCurrent() + + monitor.ensureNextResultIsSuccess() + } + + @Test + fun testUpdateAudioLanguage_updateToNigerianPidgin_updateIsSuccessful() { + setUpTestApplicationComponent() + addTestProfiles() + + val updateProvider = + profileManagementController.updateAudioLanguage(PROFILE_ID_2, NIGERIAN_PIDGIN_LANGUAGE) + val monitor = monitorFactory.createMonitor(updateProvider) + testCoroutineDispatchers.runCurrent() + + monitor.ensureNextResultIsSuccess() + } + + // Requires language configurations. + @Test + @RunOn(buildEnvironments = [BuildEnvironment.BAZEL]) + fun testUpdateAudioLanguage_updateToHindi_updateChangesAudioLanguage() { + setUpTestApplicationComponent() + addTestProfiles() + + val updateProvider = + profileManagementController.updateAudioLanguage(PROFILE_ID_2, HINDI_AUDIO_LANGUAGE) + monitorFactory.ensureDataProviderExecutes(updateProvider) + + val audioLanguageProvider = profileManagementController.getAudioLanguage(PROFILE_ID_2) + val audioLanguage = monitorFactory.waitForNextSuccessfulResult(audioLanguageProvider) + assertThat(audioLanguage).isEqualTo(HINDI_AUDIO_LANGUAGE) + } + + // Requires language configurations. + @Test + @RunOn(buildEnvironments = [BuildEnvironment.BAZEL]) + fun testUpdateAudioLanguage_updateToBrazilianPortuguese_updateChangesAudioLanguage() { + setUpTestApplicationComponent() + addTestProfiles() + + val updateProvider = + profileManagementController.updateAudioLanguage(PROFILE_ID_2, BRAZILIAN_PORTUGUESE_LANGUAGE) + monitorFactory.ensureDataProviderExecutes(updateProvider) + + val audioLanguageProvider = profileManagementController.getAudioLanguage(PROFILE_ID_2) + val audioLanguage = monitorFactory.waitForNextSuccessfulResult(audioLanguageProvider) + assertThat(audioLanguage).isEqualTo(BRAZILIAN_PORTUGUESE_LANGUAGE) + } + + // Requires language configurations. + @Test + @RunOn(buildEnvironments = [BuildEnvironment.BAZEL]) + fun testUpdateAudioLanguage_updateToArabic_updateChangesAudioLanguage() { + setUpTestApplicationComponent() + addTestProfiles() + + val updateProvider = + profileManagementController.updateAudioLanguage(PROFILE_ID_2, ARABIC_LANGUAGE) + monitorFactory.ensureDataProviderExecutes(updateProvider) + + val audioLanguageProvider = profileManagementController.getAudioLanguage(PROFILE_ID_2) + val audioLanguage = monitorFactory.waitForNextSuccessfulResult(audioLanguageProvider) + assertThat(audioLanguage).isEqualTo(ARABIC_LANGUAGE) + } + + // Requires language configurations. + @Test + @RunOn(buildEnvironments = [BuildEnvironment.BAZEL]) + fun testUpdateAudioLanguage_updateToNigerianPidgin_updateChangesAudioLanguage() { + setUpTestApplicationComponent() + addTestProfiles() + + val updateProvider = + profileManagementController.updateAudioLanguage(PROFILE_ID_2, NIGERIAN_PIDGIN_LANGUAGE) + monitorFactory.ensureDataProviderExecutes(updateProvider) + + val audioLanguageProvider = profileManagementController.getAudioLanguage(PROFILE_ID_2) + val audioLanguage = monitorFactory.waitForNextSuccessfulResult(audioLanguageProvider) + assertThat(audioLanguage).isEqualTo(NIGERIAN_PIDGIN_LANGUAGE) + } + + // Requires language configurations. + @Test + @RunOn(buildEnvironments = [BuildEnvironment.BAZEL]) + fun testUpdateAudioLanguage_updateToArabicThenEnglish_updateChangesAudioLanguageToEnglish() { + setUpTestApplicationComponent() + addTestProfiles() + val updateProvider1 = + profileManagementController.updateAudioLanguage(PROFILE_ID_2, NIGERIAN_PIDGIN_LANGUAGE) + monitorFactory.ensureDataProviderExecutes(updateProvider1) + + val updateProvider2 = + profileManagementController.updateAudioLanguage(PROFILE_ID_2, ENGLISH_AUDIO_LANGUAGE) + monitorFactory.ensureDataProviderExecutes(updateProvider2) + + val audioLanguageProvider = profileManagementController.getAudioLanguage(PROFILE_ID_2) + val audioLanguage = monitorFactory.waitForNextSuccessfulResult(audioLanguageProvider) + assertThat(audioLanguage).isEqualTo(ENGLISH_AUDIO_LANGUAGE) + } + + // Requires language configurations. + @Test + @RunOn(buildEnvironments = [BuildEnvironment.BAZEL]) + fun testUpdateAudioLanguage_updateProfile1ToArabic_profile2IsUnchanged() { + setUpTestApplicationComponent() + addTestProfiles() + + val updateProvider = + profileManagementController.updateAudioLanguage(PROFILE_ID_1, ARABIC_LANGUAGE) + monitorFactory.ensureDataProviderExecutes(updateProvider) + + val audioLanguageProvider = profileManagementController.getAudioLanguage(PROFILE_ID_2) + val audioLanguage = monitorFactory.waitForNextSuccessfulResult(audioLanguageProvider) + assertThat(audioLanguage).isEqualTo(ENGLISH_AUDIO_LANGUAGE) } @Test diff --git a/domain/src/test/java/org/oppia/android/domain/translation/TranslationControllerTest.kt b/domain/src/test/java/org/oppia/android/domain/translation/TranslationControllerTest.kt index b3b501ff94c..c483bf494dd 100644 --- a/domain/src/test/java/org/oppia/android/domain/translation/TranslationControllerTest.kt +++ b/domain/src/test/java/org/oppia/android/domain/translation/TranslationControllerTest.kt @@ -490,7 +490,7 @@ class TranslationControllerTest { } @Test - fun testUpdateAppLanguage_uninitializedToSystem_returnsDefaultSelection() { + fun testUpdateAppLanguage_uninitializedToSystem_returnsUninitialized() { forceDefaultLocale(Locale.ROOT) val languageSelection = AppLanguageSelection.newBuilder().apply { @@ -503,11 +503,11 @@ class TranslationControllerTest { // The previous selection was uninitialized. val selection = monitorFactory.waitForNextSuccessfulResult(updateProvider) - assertThat(selection).isEqualTo(languageSelection) + assertThat(selection).isEqualToDefaultInstance() } @Test - fun testUpdateAppLanguage_uninitializedToEnglish_returnsEnglishSelection() { + fun testUpdateAppLanguage_uninitializedToEnglish_returnsUninitialized() { forceDefaultLocale(Locale.ROOT) val expectedLanguageSelection = AppLanguageSelection.newBuilder().apply { @@ -520,7 +520,7 @@ class TranslationControllerTest { // The previous selection was uninitialized. val selection = monitorFactory.waitForNextSuccessfulResult(updateProvider) - assertThat(selection).isEqualTo(expectedLanguageSelection) + assertThat(selection).isEqualToDefaultInstance() } @Test @@ -535,11 +535,11 @@ class TranslationControllerTest { // The previous selection was system language. val selection = monitorFactory.waitForNextSuccessfulResult(updateProvider) - assertThat(selection.selectionTypeCase).isEqualTo(SELECTED_APP_LANGUAGE) + assertThat(selection.selectionTypeCase).isEqualTo(USE_SYSTEM_LANGUAGE_OR_APP_DEFAULT) } @Test - fun testUpdateAppLanguage_englishToPortuguese_returnsPortugueseSelection() { + fun testUpdateAppLanguage_englishToPortuguese_returnsEnglishSelection() { forceDefaultLocale(Locale.ROOT) ensureAppLanguageIsUpdatedTo(PROFILE_ID_0, ENGLISH) @@ -551,7 +551,7 @@ class TranslationControllerTest { // The previous selection was English. val selection = monitorFactory.waitForNextSuccessfulResult(updateProvider) assertThat(selection.selectionTypeCase).isEqualTo(SELECTED_APP_LANGUAGE) - assertThat(selection.selectedLanguage).isEqualTo(BRAZILIAN_PORTUGUESE) + assertThat(selection.selectedLanguage).isEqualTo(ENGLISH) } /* Tests for written translation content functions */ diff --git a/model/src/main/proto/profile.proto b/model/src/main/proto/profile.proto index cc395949209..bffdb1ec194 100644 --- a/model/src/main/proto/profile.proto +++ b/model/src/main/proto/profile.proto @@ -64,8 +64,8 @@ message Profile { // Represents user selected reading-text-size. ReadingTextSize reading_text_size = 10; - // Represents user selected audio-language. - AudioLanguage audio_language = 11; + // Represented user selected audio-language (now deprecated). + reserved 11; // Reserve 12 which was used before as using it might cause import issues for older profiles. reserved 12; @@ -137,8 +137,10 @@ enum AudioLanguage { NO_AUDIO = 1; ENGLISH_AUDIO_LANGUAGE = 2; HINDI_AUDIO_LANGUAGE = 3; - FRENCH_AUDIO_LANGUAGE = 4; - CHINESE_AUDIO_LANGUAGE = 5; + // Previously corresponded to French. + reserved 4; + // Previously corresponded to Chinese. + reserved 5; BRAZILIAN_PORTUGUESE_LANGUAGE = 6; ARABIC_LANGUAGE = 7; NIGERIAN_PIDGIN_LANGUAGE = 8; From 496ca4c7995c1ca1866bf64d7dc0b8d2db68d44d Mon Sep 17 00:00:00 2001 From: "Mr. 17" Date: Fri, 23 Aug 2024 07:55:52 +0530 Subject: [PATCH 4/8] Fix part of #5344: Add classroom label to stories on view all screen (#5502) ## Explanation Fixes part of #5344 This PR adds classroom label to the promoted stories on view all screen. ## Screenshots |Light Mode|Dark Mode| |--|--| |![image](https://github.com/user-attachments/assets/c97cbdf7-4e2a-43bf-a3f4-d57c0c53811f)|![image](https://github.com/user-attachments/assets/caff0087-cf33-4de3-ad74-44fa375f4f7c)| ## Essential Checklist - [x] The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".) - [x] Any changes to [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets) files have their rationale included in the PR explanation. - [x] The PR follows the [style guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide). - [x] The PR does not contain any unnecessary code changes from Android Studio ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)). - [x] The PR is made from a branch that's **not** called "develop" and is up-to-date with "develop". - [x] The PR is **assigned** to the appropriate reviewers ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)). ## For UI-specific PRs only If your PR includes UI-related changes, then: - Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes - For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see [RTL guide](https://github.com/oppia/oppia-android/wiki/RTL-Guidelines)) - Add a video showing the full UX flow with a screen reader enabled (see [accessibility guide](https://github.com/oppia/oppia-android/wiki/Accessibility-A11y-Guide)) - For PRs introducing new UI elements or color changes, both light and dark mode screenshots must be included - Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing --- .../recentlyplayed/PromotedStoryViewModel.kt | 7 + .../recentlyplayed/RecentlyPlayedViewModel.kt | 7 + .../classroom_label_text_background.xml | 12 ++ .../res/layout/recently_played_story_card.xml | 21 +- .../app/home/RecentlyPlayedFragmentTest.kt | 202 +++++++++++++++++- 5 files changed, 246 insertions(+), 3 deletions(-) create mode 100644 app/src/main/res/drawable/classroom_label_text_background.xml diff --git a/app/src/main/java/org/oppia/android/app/home/recentlyplayed/PromotedStoryViewModel.kt b/app/src/main/java/org/oppia/android/app/home/recentlyplayed/PromotedStoryViewModel.kt index f93fba2613c..9dca136cefd 100755 --- a/app/src/main/java/org/oppia/android/app/home/recentlyplayed/PromotedStoryViewModel.kt +++ b/app/src/main/java/org/oppia/android/app/home/recentlyplayed/PromotedStoryViewModel.kt @@ -18,6 +18,7 @@ class PromotedStoryViewModel( private val promotedStoryClickListener: PromotedStoryClickListener, private val position: Int, private val resourceHandler: AppLanguageResourceHandler, + val showClassroomLabel: Boolean, translationController: TranslationController ) : RecentlyPlayedItemViewModel() { /** Sets the story title of the recently played story. */ @@ -38,6 +39,12 @@ class PromotedStoryViewModel( promotedStory.nextChapterTitle, promotedStory.nextChapterWrittenTranslationContext ) } + /** Sets the classroom of the recently played story. */ + val classroomTitle by lazy { + translationController.extractString( + promotedStory.classroomTitle, promotedStory.classroomWrittenTranslationContext + ) + } /** * Starts [ResumeLessonActivity] if a saved exploration is selected or [ExplorationActivity] if an diff --git a/app/src/main/java/org/oppia/android/app/home/recentlyplayed/RecentlyPlayedViewModel.kt b/app/src/main/java/org/oppia/android/app/home/recentlyplayed/RecentlyPlayedViewModel.kt index 7716fdf11f5..1542e63bb5a 100644 --- a/app/src/main/java/org/oppia/android/app/home/recentlyplayed/RecentlyPlayedViewModel.kt +++ b/app/src/main/java/org/oppia/android/app/home/recentlyplayed/RecentlyPlayedViewModel.kt @@ -13,6 +13,8 @@ import org.oppia.android.domain.translation.TranslationController import org.oppia.android.util.data.AsyncResult import org.oppia.android.util.data.DataProviders.Companion.toLiveData import org.oppia.android.util.parser.html.StoryHtmlParserEntityType +import org.oppia.android.util.platformparameter.EnableMultipleClassrooms +import org.oppia.android.util.platformparameter.PlatformParameterValue import javax.inject.Inject /** View model for [RecentlyPlayedFragment]. */ @@ -22,6 +24,7 @@ class RecentlyPlayedViewModel private constructor( @StoryHtmlParserEntityType private val entityType: String, private val resourceHandler: AppLanguageResourceHandler, private val translationController: TranslationController, + private val enableMultipleClassrooms: PlatformParameterValue, private val promotedStoryClickListener: PromotedStoryClickListener, private val profileId: ProfileId, ) { @@ -33,6 +36,8 @@ class RecentlyPlayedViewModel private constructor( @StoryHtmlParserEntityType private val entityType: String, private val resourceHandler: AppLanguageResourceHandler, private val translationController: TranslationController, + @EnableMultipleClassrooms + private val enableMultipleClassrooms: PlatformParameterValue, ) { /** Creates an instance of [RecentlyPlayedViewModel]. */ @@ -46,6 +51,7 @@ class RecentlyPlayedViewModel private constructor( entityType, resourceHandler, translationController, + enableMultipleClassrooms, promotedStoryClickListener, profileId, ) @@ -166,6 +172,7 @@ class RecentlyPlayedViewModel private constructor( promotedStoryClickListener, index, resourceHandler, + enableMultipleClassrooms.value, translationController ) } diff --git a/app/src/main/res/drawable/classroom_label_text_background.xml b/app/src/main/res/drawable/classroom_label_text_background.xml new file mode 100644 index 00000000000..3a316756f7d --- /dev/null +++ b/app/src/main/res/drawable/classroom_label_text_background.xml @@ -0,0 +1,12 @@ + + + + + diff --git a/app/src/main/res/layout/recently_played_story_card.xml b/app/src/main/res/layout/recently_played_story_card.xml index 6dff08a05de..0994de8f994 100755 --- a/app/src/main/res/layout/recently_played_story_card.xml +++ b/app/src/main/res/layout/recently_played_story_card.xml @@ -86,10 +86,29 @@ android:text="@{viewModel.topicTitle}" android:textAllCaps="true" android:textColor="@color/component_color_shared_story_card_topic_name_text_color" - app:layout_constraintBottom_toBottomOf="parent" + app:layout_constraintBottom_toTopOf="@id/classroom_name_text_view" app:layout_constraintEnd_toEndOf="parent" app:layout_constraintStart_toStartOf="parent" app:layout_constraintTop_toBottomOf="@id/story_name_text_view" /> + + diff --git a/app/src/sharedTest/java/org/oppia/android/app/home/RecentlyPlayedFragmentTest.kt b/app/src/sharedTest/java/org/oppia/android/app/home/RecentlyPlayedFragmentTest.kt index c45a0580213..9cf3f4ebf3e 100644 --- a/app/src/sharedTest/java/org/oppia/android/app/home/RecentlyPlayedFragmentTest.kt +++ b/app/src/sharedTest/java/org/oppia/android/app/home/RecentlyPlayedFragmentTest.kt @@ -97,7 +97,6 @@ import org.oppia.android.domain.oppialogger.analytics.ApplicationLifecycleModule import org.oppia.android.domain.oppialogger.analytics.CpuPerformanceSnapshotterModule import org.oppia.android.domain.oppialogger.logscheduler.MetricLogSchedulerModule import org.oppia.android.domain.oppialogger.loguploader.LogReportWorkerModule -import org.oppia.android.domain.platformparameter.PlatformParameterModule import org.oppia.android.domain.platformparameter.PlatformParameterSingletonModule import org.oppia.android.domain.question.QuestionModule import org.oppia.android.domain.topic.FRACTIONS_EXPLORATION_ID_0 @@ -111,6 +110,7 @@ import org.oppia.android.testing.firebase.TestAuthenticationModule import org.oppia.android.testing.junit.InitializeDefaultLocaleRule import org.oppia.android.testing.lightweightcheckpointing.ExplorationCheckpointTestHelper import org.oppia.android.testing.lightweightcheckpointing.FRACTIONS_STORY_0_EXPLORATION_0_CURRENT_VERSION +import org.oppia.android.testing.platformparameter.TestPlatformParameterModule import org.oppia.android.testing.profile.ProfileTestHelper import org.oppia.android.testing.robolectric.RobolectricModule import org.oppia.android.testing.story.StoryProgressTestHelper @@ -457,6 +457,66 @@ class RecentlyPlayedFragmentTest { } } + @Test + fun testRecentlyPlayedTestActivity_disableClassrooms_recommendedSection_classroomNameIsNotDisplayed() { // ktlint-disable max-line-length + TestPlatformParameterModule.forceEnableMultipleClassrooms(false) + fakeOppiaClock.setFakeTimeMode(FakeOppiaClock.FakeTimeMode.MODE_UPTIME_MILLIS) + storyProgressTestHelper.markInProgressSavedFractionsStory0Exp0( + profileId = profileId, + timestampOlderThanOneWeek = false + ) + ActivityScenario.launch( + createRecentlyPlayedActivityIntent( + internalProfileId = internalProfileId + ) + ).use { + testCoroutineDispatchers.runCurrent() + onView(withId(R.id.ongoing_story_recycler_view)).perform( + scrollToPosition( + 3 + ) + ) + onView( + atPositionOnView( + recyclerViewId = R.id.ongoing_story_recycler_view, + position = 3, + targetViewId = R.id.classroom_name_text_view + ) + ).check(matches(not(isDisplayed()))) + } + } + + @Test + fun testRecentlyPlayedTestActivity_enableClassrooms_recommendedSection_classroomNameIsCorrect() { + TestPlatformParameterModule.forceEnableMultipleClassrooms(true) + fakeOppiaClock.setFakeTimeMode(FakeOppiaClock.FakeTimeMode.MODE_UPTIME_MILLIS) + storyProgressTestHelper.markInProgressSavedFractionsStory0Exp0( + profileId = profileId, + timestampOlderThanOneWeek = false + ) + ActivityScenario.launch( + createRecentlyPlayedActivityIntent( + internalProfileId = internalProfileId + ) + ).use { + testCoroutineDispatchers.runCurrent() + onView(withId(R.id.ongoing_story_recycler_view)).perform( + scrollToPosition( + 3 + ) + ) + onView( + atPositionOnView( + recyclerViewId = R.id.ongoing_story_recycler_view, + position = 3, + targetViewId = R.id.classroom_name_text_view + ) + ).check( + matches(withText(containsString("MATHS"))) + ) + } + } + @Config(qualifiers = "port") @Test fun testRecentlyPlayedTestActivity_recentlyPlayedItemInRtl_rtlMarginIsCorrect() { @@ -774,6 +834,74 @@ class RecentlyPlayedFragmentTest { } } + @Test + fun testRecentlyPlayedTestActivity_disableClassrooms_classroomNameIsNotDisplayed() { + TestPlatformParameterModule.forceEnableMultipleClassrooms(false) + fakeOppiaClock.setFakeTimeMode(FakeOppiaClock.FakeTimeMode.MODE_UPTIME_MILLIS) + storyProgressTestHelper.markInProgressSavedFractionsStory0Exp0( + profileId = profileId, + timestampOlderThanOneWeek = false + ) + storyProgressTestHelper.markInProgressSavedRatiosStory0Exp0( + profileId = profileId, + timestampOlderThanOneWeek = true + ) + ActivityScenario.launch( + createRecentlyPlayedActivityIntent( + internalProfileId = internalProfileId + ) + ).use { + testCoroutineDispatchers.runCurrent() + onView(withId(R.id.ongoing_story_recycler_view)).perform( + scrollToPosition( + 1 + ) + ) + onView( + atPositionOnView( + recyclerViewId = R.id.ongoing_story_recycler_view, + position = 1, + targetViewId = R.id.classroom_name_text_view + ) + ).check(matches(not(isDisplayed()))) + } + } + + @Test + fun testRecentlyPlayedTestActivity_enableClassrooms_classroomNameIsCorrect() { + TestPlatformParameterModule.forceEnableMultipleClassrooms(true) + fakeOppiaClock.setFakeTimeMode(FakeOppiaClock.FakeTimeMode.MODE_UPTIME_MILLIS) + storyProgressTestHelper.markInProgressSavedFractionsStory0Exp0( + profileId = profileId, + timestampOlderThanOneWeek = false + ) + storyProgressTestHelper.markInProgressSavedRatiosStory0Exp0( + profileId = profileId, + timestampOlderThanOneWeek = true + ) + ActivityScenario.launch( + createRecentlyPlayedActivityIntent( + internalProfileId = internalProfileId + ) + ).use { + testCoroutineDispatchers.runCurrent() + onView(withId(R.id.ongoing_story_recycler_view)).perform( + scrollToPosition( + 1 + ) + ) + onView( + atPositionOnView( + recyclerViewId = R.id.ongoing_story_recycler_view, + position = 1, + targetViewId = R.id.classroom_name_text_view + ) + ).check( + matches(withText(containsString("MATHS"))) + ) + } + } + @Test fun testRecentlyPlayedTestActivity_lessonThumbnailIsCorrect() { fakeOppiaClock.setFakeTimeMode(FakeOppiaClock.FakeTimeMode.MODE_UPTIME_MILLIS) @@ -1256,6 +1384,76 @@ class RecentlyPlayedFragmentTest { } } + @Test + fun testRecentlyPlayedTestActivity_disableClassrooms_configChange_classroomNameIsNotDisplayed() { + TestPlatformParameterModule.forceEnableMultipleClassrooms(false) + fakeOppiaClock.setFakeTimeMode(FakeOppiaClock.FakeTimeMode.MODE_UPTIME_MILLIS) + storyProgressTestHelper.markInProgressSavedFractionsStory0Exp0( + profileId = profileId, + timestampOlderThanOneWeek = false + ) + storyProgressTestHelper.markInProgressSavedRatiosStory0Exp0( + profileId = profileId, + timestampOlderThanOneWeek = true + ) + ActivityScenario.launch( + createRecentlyPlayedActivityIntent( + internalProfileId = internalProfileId + ) + ).use { + testCoroutineDispatchers.runCurrent() + onView(isRoot()).perform(orientationLandscape()) + onView(withId(R.id.ongoing_story_recycler_view)).perform( + scrollToPosition( + 1 + ) + ) + onView( + atPositionOnView( + recyclerViewId = R.id.ongoing_story_recycler_view, + position = 1, + targetViewId = R.id.classroom_name_text_view + ) + ).check(matches(not(isDisplayed()))) + } + } + + @Test + fun testRecentlyPlayedTestActivity_enableClassrooms_configChange_classroomNameIsCorrect() { + TestPlatformParameterModule.forceEnableMultipleClassrooms(true) + fakeOppiaClock.setFakeTimeMode(FakeOppiaClock.FakeTimeMode.MODE_UPTIME_MILLIS) + storyProgressTestHelper.markInProgressSavedFractionsStory0Exp0( + profileId = profileId, + timestampOlderThanOneWeek = false + ) + storyProgressTestHelper.markInProgressSavedRatiosStory0Exp0( + profileId = profileId, + timestampOlderThanOneWeek = true + ) + ActivityScenario.launch( + createRecentlyPlayedActivityIntent( + internalProfileId = internalProfileId + ) + ).use { + testCoroutineDispatchers.runCurrent() + onView(isRoot()).perform(orientationLandscape()) + onView(withId(R.id.ongoing_story_recycler_view)).perform( + scrollToPosition( + 1 + ) + ) + onView( + atPositionOnView( + recyclerViewId = R.id.ongoing_story_recycler_view, + position = 1, + targetViewId = R.id.classroom_name_text_view + ) + ).check( + matches(withText(containsString("MATHS"))) + ) + } + } + @Test fun testRecentlyPlayedTestActivity_configChange_lessonThumbnailIsCorrect() { fakeOppiaClock.setFakeTimeMode(FakeOppiaClock.FakeTimeMode.MODE_UPTIME_MILLIS) @@ -1464,7 +1662,7 @@ class RecentlyPlayedFragmentTest { @Component( modules = [ RobolectricModule::class, - PlatformParameterModule::class, PlatformParameterSingletonModule::class, + TestPlatformParameterModule::class, PlatformParameterSingletonModule::class, TestDispatcherModule::class, ApplicationModule::class, LoggerModule::class, ContinueModule::class, FractionInputModule::class, ItemSelectionInputModule::class, MultipleChoiceInputModule::class, From 01e02ec855899ede5ece4ab7bddb548bed6ddea4 Mon Sep 17 00:00:00 2001 From: "Mr. 17" Date: Fri, 23 Aug 2024 23:44:20 +0530 Subject: [PATCH 5/8] Fix part of #5344: Enable Multiple Classrooms (#5510) ## Explanation Fixes part of https://github.com/oppia/oppia-android/issues/5344 Toggles the Multiple Classrooms Feature Flag on. ## Essential Checklist - [x] The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".) - [x] Any changes to [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets) files have their rationale included in the PR explanation. - [x] The PR follows the [style guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide). - [x] The PR does not contain any unnecessary code changes from Android Studio ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)). - [x] The PR is made from a branch that's **not** called "develop" and is up-to-date with "develop". - [x] The PR is **assigned** to the appropriate reviewers ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)). ## For UI-specific PRs only If your PR includes UI-related changes, then: - Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes - For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see [RTL guide](https://github.com/oppia/oppia-android/wiki/RTL-Guidelines)) - Add a video showing the full UX flow with a screen reader enabled (see [accessibility guide](https://github.com/oppia/oppia-android/wiki/Accessibility-A11y-Guide)) - For PRs introducing new UI elements or color changes, both light and dark mode screenshots must be included - Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing --- .../oppia/android/app/profile/PinPasswordActivityTest.kt | 7 +++---- .../android/util/platformparameter/FeatureFlagConstants.kt | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/app/src/sharedTest/java/org/oppia/android/app/profile/PinPasswordActivityTest.kt b/app/src/sharedTest/java/org/oppia/android/app/profile/PinPasswordActivityTest.kt index 5da1ccdf4e5..1e678cb7cdb 100644 --- a/app/src/sharedTest/java/org/oppia/android/app/profile/PinPasswordActivityTest.kt +++ b/app/src/sharedTest/java/org/oppia/android/app/profile/PinPasswordActivityTest.kt @@ -87,9 +87,7 @@ import org.oppia.android.domain.platformparameter.PlatformParameterSingletonModu import org.oppia.android.domain.question.QuestionModule import org.oppia.android.domain.workmanager.WorkManagerConfigurationModule import org.oppia.android.testing.OppiaTestRule -import org.oppia.android.testing.RunOn import org.oppia.android.testing.TestLogReportingModule -import org.oppia.android.testing.TestPlatform import org.oppia.android.testing.espresso.EditTextInputAction import org.oppia.android.testing.espresso.TextInputAction.Companion.hasErrorText import org.oppia.android.testing.espresso.TextInputAction.Companion.hasNoErrorText @@ -219,6 +217,7 @@ class PinPasswordActivityTest { @Test fun testPinPassword_withAdmin_inputCorrectPin_opensHomeActivity() { + TestPlatformParameterModule.forceEnableMultipleClassrooms(false) ActivityScenario.launch( PinPasswordActivity.createPinPasswordActivityIntent( context = context, @@ -235,7 +234,6 @@ class PinPasswordActivityTest { } @Test - @RunOn(TestPlatform.ESPRESSO) fun testPinPassword_enableClassrooms_withAdmin_inputCorrectPin_opensClassroomListActivity() { TestPlatformParameterModule.forceEnableMultipleClassrooms(true) ActivityScenario.launch( @@ -256,6 +254,7 @@ class PinPasswordActivityTest { @Test fun testPinPassword_withUser_inputCorrectPin_opensHomeActivity() { + TestPlatformParameterModule.forceEnableMultipleClassrooms(false) ActivityScenario.launch( PinPasswordActivity.createPinPasswordActivityIntent( context = context, @@ -272,7 +271,6 @@ class PinPasswordActivityTest { } @Test - @RunOn(TestPlatform.ESPRESSO) fun testPinPassword_enableClassrooms_withUser_inputCorrectPin_opensClassroomListActivity() { TestPlatformParameterModule.forceEnableMultipleClassrooms(true) ActivityScenario.launch( @@ -544,6 +542,7 @@ class PinPasswordActivityTest { @Test fun testPinPassword_withUser_forgot_inputAdminPinAndNewPin_opensHomeActivity() { + TestPlatformParameterModule.forceEnableMultipleClassrooms(false) ActivityScenario.launch( PinPasswordActivity.createPinPasswordActivityIntent( context = context, diff --git a/utility/src/main/java/org/oppia/android/util/platformparameter/FeatureFlagConstants.kt b/utility/src/main/java/org/oppia/android/util/platformparameter/FeatureFlagConstants.kt index c30ae97206c..39a63ec453a 100644 --- a/utility/src/main/java/org/oppia/android/util/platformparameter/FeatureFlagConstants.kt +++ b/utility/src/main/java/org/oppia/android/util/platformparameter/FeatureFlagConstants.kt @@ -178,4 +178,4 @@ annotation class EnableMultipleClassrooms const val ENABLE_MULTIPLE_CLASSROOMS = "enable_multiple_classrooms" /** Default value of the feature flag corresponding to [EnableMultipleClassrooms]. */ -const val ENABLE_MULTIPLE_CLASSROOMS_DEFAULT_VALUE = false +const val ENABLE_MULTIPLE_CLASSROOMS_DEFAULT_VALUE = true From 1a68c9fd3dda7c395f73ed0af023bc24cf023910 Mon Sep 17 00:00:00 2001 From: RD Rama Devi <122200035+Rd4dev@users.noreply.github.com> Date: Sat, 24 Aug 2024 02:25:07 +0530 Subject: [PATCH 6/8] Fix #5486 & part of #5343: Introducing new wiki page for code coverage usage and limitations (#5483) ## Explanation Fixes part of #5343 Fixes #5486 ### Project [PR 2.6 of Project 4.1] ### Changes Made - This PR introduces 2 new wiki pages: - Oppia Android Code Coverage - Writing tests with Good Behavioural Coverage - Fix to the comment upload feature permission issues with PRs created from the forked branches. - Split the code coverage workflow into 2 1. Core code coverage workflow to handle - Collection of changed files - Bucket partitioning - Run coverage in matrices 2. Comment upload workflow to handle - evaluation of reports - generation of md reports - uploading comments - Coverage Status Checks (as the later required `pull_request_target`) - Fix to #5486 - The issue should have arisen as the pr got merged with the branch being deleted while the publish comment job still running, finding it hard to fetch the pr-issue number to publish a comment. - Now the Coverage Check Status was made to be dependent on the comment uploader ie. the final Coverage check job occurs only after the Evaluation and Comment jobs are done, so it will always have the pr reference) ``` check_coverage_results: name: Check Code Coverage Results needs: [ evaluate-code-coverage-reports, comment_coverage_report ] ``` # ### Reasons for splitting the code_coverage workflow The single code_coverage workflow was split into 1. **code_coverage** (to run coverages) 2. **coverage_report** (to generate and publish reports) ### Separating the comment upload job - The primary reason is the need to have ability to upload comments from PRs opened from a fork branch. - [`pull_request_target`](https://docs.github.com/en/actions/writing-workflows/choosing-when-your-workflow-runs/events-that-trigger-workflows#pull_request_target): For workflows that are triggered by the pull_request_target event, the GITHUB_TOKEN is granted read/write repository permission, even when it is triggered from a fork. - Workflows triggered by `pull_request_target` events are run in the context of the base branch. Since the base branch is considered trusted, workflows triggered by these events will always run, regardless of approval settings. [[GitHub Docs](https://docs.github.com/en/actions/managing-workflow-runs-and-deployments/managing-workflow-runs/approving-workflow-runs-from-public-forks#about-workflow-runs-from-public-forks)] ### Separating the evaluation / generation of report job - While initially it was split to help with 'skip files' - no files changed conditional check, with the introduction to 'SKIP' status check, it doesn't continue to serve the mentioned purpose. - But if we still have it as one workflow then the flow will work as such: Workflow 1: **code_coverage** - compute changed files - run coverage (needs compute changed files) - evaluate / generate md - code coverage check result Workflow 2: **coverage_report** - comment publication If no `.kt` files changes are detected - compute changed files - skips run coverage - skips evaluate / generate md - pass code coverage check result As the workflow was concluded as success, the 2nd workflow runs as, - failed comment publication (as no report is generation due to skip) But expectation is to still produce a pass check for the comment publication. (either to at least skip or upload a skip status as coverage comment report) - With moving it to a separate workflow allows us to not make the evaluation / generation jobs rely on the Run coverage job making it independently behave once the code_coverage workflows are completed successfully. - And it checks if pb files are generated and based on that it decides whether to generate PASS, FAIL or SKIP status checks. ### Separating the coverage status check result job There are 2 main reasons to moving it to new workflow. While it would still make sense to have it with the 1st workflow itself after Run coverage, the drawbacks are, - If the check coverage status result was left with the 1st workflow, then when the Run coverage job completes in the 1st workflow the coverage status check result is set to true on success even before the sibling part of upload comment is done, making it an incomplete status result. - #5486 occurred as it lost its reference to the pr number, so making the coverage check status result dependent on (needs) the comment upload job should resolve this by only allowing the PR to be closed once the comment is uploaded. # **Todo:** - **[Done]** Add a new wiki page for "Writing effective tests / Writing test with good behavioural coverage" ## Essential Checklist - [x] The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".) - [x] Any changes to [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets) files have their rationale included in the PR explanation. - [x] The PR follows the [style guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide). - [x] The PR does not contain any unnecessary code changes from Android Studio ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)). - [x] The PR is made from a branch that's **not** called "develop" and is up-to-date with "develop". - [x] The PR is **assigned** to the appropriate reviewers ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)). ## For UI-specific PRs only If your PR includes UI-related changes, then: - Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes - For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see [RTL guide](https://github.com/oppia/oppia-android/wiki/RTL-Guidelines)) - Add a video showing the full UX flow with a screen reader enabled (see [accessibility guide](https://github.com/oppia/oppia-android/wiki/Accessibility-A11y-Guide)) - For PRs introducing new UI elements or color changes, both light and dark mode screenshots must be included - Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing --------- Co-authored-by: Ben Henning --- .github/workflows/build_tests.yml | 2 +- .github/workflows/code_coverage.yml | 33 +- .github/workflows/comment_coverage_report.yml | 81 + .github/workflows/main.yml | 2 +- .github/workflows/static_checks.yml | 2 +- .github/workflows/unit_tests.yml | 2 +- .../coverage/reporter/CoverageReporter.kt | 41 +- .../scripts/coverage/RunCoverageTest.kt | 32 +- .../coverage/reporter/CoverageReporterTest.kt | 55 + wiki/Oppia-Android-Code-Coverage.md | 461 +++++ ...ing-tests-with-good-behavioral-coverage.md | 1599 +++++++++++++++++ wiki/_Sidebar.md | 2 + 12 files changed, 2268 insertions(+), 44 deletions(-) create mode 100644 .github/workflows/comment_coverage_report.yml create mode 100644 wiki/Oppia-Android-Code-Coverage.md create mode 100644 wiki/Writing-tests-with-good-behavioral-coverage.md diff --git a/.github/workflows/build_tests.yml b/.github/workflows/build_tests.yml index 1f293b1282b..7dde621a72f 100644 --- a/.github/workflows/build_tests.yml +++ b/.github/workflows/build_tests.yml @@ -11,7 +11,7 @@ on: - develop concurrency: - group: ${{ github.workflow }}-${{ github.ref }} + group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }} cancel-in-progress: true jobs: diff --git a/.github/workflows/code_coverage.yml b/.github/workflows/code_coverage.yml index 07824dfb179..992a7e86cb1 100644 --- a/.github/workflows/code_coverage.yml +++ b/.github/workflows/code_coverage.yml @@ -12,7 +12,7 @@ on: - develop concurrency: - group: ${{ github.workflow }}-${{ github.ref }} + group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }} cancel-in-progress: true jobs: @@ -255,10 +255,13 @@ jobs: name: coverage-report-${{ env.SHARD_NAME }} # Saving with unique names to avoid conflict path: coverage_reports - evaluate-code-coverage-reports: + evaluate_code_coverage_reports: name: Evaluate Code Coverage Reports runs-on: ubuntu-20.04 needs: code_coverage_run + # The expression if: ${{ !cancelled() }} runs a job or step regardless of its success or failure while responding to cancellations, + # serving as a cancellation-compliant alternative to if: ${{ always() }} in concurrent workflows. + if: ${{ !cancelled() }} env: CACHE_DIRECTORY: ~/.bazel_cache steps: @@ -305,32 +308,10 @@ jobs: name: final-coverage-report path: coverage_reports/CoverageReport.md - publish_coverage_report: - name: Publish Code Coverage Report - needs: evaluate-code-coverage-reports - permissions: - pull-requests: write - - # The expression if: ${{ !cancelled() }} runs a job or step regardless of its success or failure while responding to cancellations, - # serving as a cancellation-compliant alternative to if: ${{ always() }} in concurrent workflows. - if: ${{ !cancelled() }} - runs-on: ubuntu-latest - steps: - - name: Download Generated Markdown Report - uses: actions/download-artifact@v4 - with: - name: final-coverage-report - - - name: Upload Coverage Report as PR Comment - uses: peter-evans/create-or-update-comment@v4 - with: - issue-number: ${{ github.event.pull_request.number }} - body-path: 'CoverageReport.md' - # Reference: https://github.community/t/127354/7. check_coverage_results: name: Check Code Coverage Results - needs: [ compute_changed_files, code_coverage_run, evaluate-code-coverage-reports ] + needs: [ compute_changed_files, code_coverage_run, evaluate_code_coverage_reports ] # The expression if: ${{ !cancelled() }} runs a job or step regardless of its success or failure while responding to cancellations, # serving as a cancellation-compliant alternative to if: ${{ always() }} in concurrent workflows. if: ${{ !cancelled() }} @@ -341,5 +322,5 @@ jobs: run: exit 1 - name: Check that coverage status is passed - if: ${{ needs.evaluate-code-coverage-reports.result != 'success' }} + if: ${{ needs.compute_changed_files.outputs.can_skip_files != 'true' && needs.evaluate_code_coverage_reports.result != 'success' }} run: exit 1 diff --git a/.github/workflows/comment_coverage_report.yml b/.github/workflows/comment_coverage_report.yml new file mode 100644 index 00000000000..9ecb4e2e95c --- /dev/null +++ b/.github/workflows/comment_coverage_report.yml @@ -0,0 +1,81 @@ +# Contains jobs corresponding to publishing coverage reports generated by code_coverage.yml. + +name: Comment Coverage Report + +# Controls when the action will run. Triggers the workflow on pull request events +# (assigned, opened, synchronize, reopened) + +on: + pull_request_target: + types: [assigned, opened, synchronize, reopened] + +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }} + cancel-in-progress: true + +jobs: + check_code_coverage_completed: + name: Check code coverage completed + runs-on: ubuntu-latest + steps: + - name: Wait for code coverage to complete + id: wait-for-coverage + uses: ArcticLampyrid/action-wait-for-workflow@v1.2.0 + with: + workflow: code_coverage.yml + sha: auto + allowed-conclusions: | + success + failure + + comment_coverage_report: + name: Comment Code Coverage Report + needs: check_code_coverage_completed + permissions: + pull-requests: write + + # The expression if: ${{ !cancelled() }} runs a job or step regardless of its success or failure while responding to cancellations, + # serving as a cancellation-compliant alternative to if: ${{ always() }} in concurrent workflows. + if: ${{ !cancelled() }} + runs-on: ubuntu-latest + steps: + - name: Find CI workflow run for PR + id: find-workflow-run + uses: actions/github-script@v7 + continue-on-error: true + with: + script: | + // Find the last successful workflow run for the current PR's head + const { owner, repo } = context.repo; + const runsResponse = await github.rest.actions.listWorkflowRuns({ + owner, + repo, + workflow_id: 'code_coverage.yml', + event: 'pull_request', + head_sha: '${{ github.event.pull_request.head.sha }}', + }); + + const runs = runsResponse.data.workflow_runs; + runs.sort((a, b) => new Date(b.created_at).getTime() - new Date(a.created_at).getTime()); + + const run = runs[0]; + if(!run) { + core.setFailed('Could not find a succesful workflow run for the PR'); + return; + } + + core.setOutput('run-id', run.id); + + - name: Download Generated Markdown Report + uses: actions/download-artifact@v4 + if: ${{ !cancelled() }} # IMPORTANT: Upload reports regardless of success or failure status + with: + name: final-coverage-report + github-token: ${{ secrets.GITHUB_TOKEN }} + run-id: ${{ steps.find-workflow-run.outputs.run-id }} + + - name: Upload Coverage Report as PR Comment + uses: peter-evans/create-or-update-comment@v4 + with: + issue-number: ${{ github.event.pull_request.number }} + body-path: 'CoverageReport.md' diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index ed6cd752264..84cc12006a8 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -13,7 +13,7 @@ on: - develop concurrency: - group: ${{ github.workflow }}-${{ github.ref }} + group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }} cancel-in-progress: true # This workflow has the following jobs: diff --git a/.github/workflows/static_checks.yml b/.github/workflows/static_checks.yml index 1f9500c1dfc..15dd3e28e76 100644 --- a/.github/workflows/static_checks.yml +++ b/.github/workflows/static_checks.yml @@ -10,7 +10,7 @@ on: - develop concurrency: - group: ${{ github.workflow }}-${{ github.ref }} + group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }} cancel-in-progress: true jobs: diff --git a/.github/workflows/unit_tests.yml b/.github/workflows/unit_tests.yml index f59a5b9bee7..82833edfede 100644 --- a/.github/workflows/unit_tests.yml +++ b/.github/workflows/unit_tests.yml @@ -13,7 +13,7 @@ on: - develop concurrency: - group: ${{ github.workflow }}-${{ github.ref }} + group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }} cancel-in-progress: true jobs: diff --git a/scripts/src/java/org/oppia/android/scripts/coverage/reporter/CoverageReporter.kt b/scripts/src/java/org/oppia/android/scripts/coverage/reporter/CoverageReporter.kt index b27ec9283ce..31763392945 100644 --- a/scripts/src/java/org/oppia/android/scripts/coverage/reporter/CoverageReporter.kt +++ b/scripts/src/java/org/oppia/android/scripts/coverage/reporter/CoverageReporter.kt @@ -496,16 +496,37 @@ class CoverageReporter( } } - val finalReportText = "## Coverage Report\n\n" + - "### Results\n" + - "Number of files assessed: ${coverageReportContainer.coverageReportList.size}\n" + - "Overall Coverage: **${"%.2f".format(calculateOverallCoveragePercentage())}%**\n" + - "Coverage Analysis: $status\n" + - "##" + - failureMarkdownTable + - failureMarkdownEntries + - successMarkdownEntries + - testFileExemptedSection + val wikiPageLinkNote = buildString { + val wikiPageReferenceNote = ">To learn more, visit the [Oppia Android Code Coverage]" + + "(https://github.com/oppia/oppia-android/wiki/Oppia-Android-Code-Coverage) wiki page" + append("\n\n") + append("#") + append("\n") + append(wikiPageReferenceNote) + } + + val skipCoverageReportText = buildString { + append("## Coverage Report\n") + append("### Results\n") + append("Coverage Analysis: **SKIP** :next_track_button:\n\n") + append("_This PR did not introduce any changes to Kotlin source or test files._") + append(wikiPageLinkNote) + } + + val finalReportText = coverageReportContainer.coverageReportList.takeIf { it.isNotEmpty() } + ?.let { + "## Coverage Report\n\n" + + "### Results\n" + + "Number of files assessed: ${coverageReportContainer.coverageReportList.size}\n" + + "Overall Coverage: **${"%.2f".format(calculateOverallCoveragePercentage())}%**\n" + + "Coverage Analysis: $status\n" + + "##" + + failureMarkdownTable + + failureMarkdownEntries + + successMarkdownEntries + + testFileExemptedSection + + wikiPageLinkNote + } ?: skipCoverageReportText val finalReportOutputPath = mdReportOutputPath ?.let { it } diff --git a/scripts/src/javatests/org/oppia/android/scripts/coverage/RunCoverageTest.kt b/scripts/src/javatests/org/oppia/android/scripts/coverage/RunCoverageTest.kt index 864ff9beb51..7ef0183f00d 100644 --- a/scripts/src/javatests/org/oppia/android/scripts/coverage/RunCoverageTest.kt +++ b/scripts/src/javatests/org/oppia/android/scripts/coverage/RunCoverageTest.kt @@ -90,6 +90,7 @@ class RunCoverageTest { append("| File | Failure Reason | Status |\n") append("|------|----------------|--------|\n") append("| ${getFilenameAsDetailsSummary(sampleFile)} | $failureMessage | :x: |") + append(oppiaCoverageWikiPageLinkNote) } assertThat(readFinalMdReport()).isEqualTo(expectedMarkdown) @@ -229,6 +230,7 @@ class RunCoverageTest { append("\n\n") append(exemptionsReferenceNote) append("") + append(oppiaCoverageWikiPageLinkNote) } assertThat(readFinalMdReport()).isEqualTo(expectedResult) @@ -278,6 +280,7 @@ class RunCoverageTest { append("\n\n") append(exemptionsReferenceNote) append("") + append(oppiaCoverageWikiPageLinkNote) } assertThat(readFinalMdReport()).isEqualTo(expectedResult) @@ -385,6 +388,7 @@ class RunCoverageTest { append("| File | Failure Reason | Status |\n") append("|------|----------------|--------|\n") append("| //coverage/example:AddNumsTest | $failureMessage | :x: |") + append(oppiaCoverageWikiPageLinkNote) } assertThat(readFinalMdReport()).isEqualTo(expectedMarkdown) @@ -469,6 +473,7 @@ class RunCoverageTest { append("| File | Failure Reason | Status |\n") append("|------|----------------|--------|\n") append("| //coverage/test/java/com/example:SubNumsTest | $failureMessage | :x: |") + append(oppiaCoverageWikiPageLinkNote) } assertThat(readFinalMdReport()).isEqualTo(expectedMarkdown) @@ -655,6 +660,7 @@ class RunCoverageTest { ":white_check_mark: | $MIN_THRESHOLD% |\n\n" ) append("") + append(oppiaCoverageWikiPageLinkNote) } assertThat(readFinalMdReport()).isEqualTo(expectedResult) @@ -843,6 +849,7 @@ class RunCoverageTest { "| ${getFilenameAsDetailsSummary(filePathList.get(0))} | 0.00% | 0 / 4 | " + ":x: | $MIN_THRESHOLD% |\n" ) + append(oppiaCoverageWikiPageLinkNote) } assertThat(readFinalMdReport()).isEqualTo(expectedResult) @@ -934,6 +941,7 @@ class RunCoverageTest { ":x: | 101% _*_ |" ) append("\n\n>**_*_** represents tests with custom overridden pass/fail coverage thresholds") + append(oppiaCoverageWikiPageLinkNote) } assertThat(readFinalMdReport()).isEqualTo(expectedResult) @@ -1021,6 +1029,7 @@ class RunCoverageTest { ) append("\n\n>**_*_** represents tests with custom overridden pass/fail coverage thresholds\n") append("") + append(oppiaCoverageWikiPageLinkNote) } assertThat(readFinalMdReport()).isEqualTo(expectedResult) @@ -1090,6 +1099,7 @@ class RunCoverageTest { ":white_check_mark: | $MIN_THRESHOLD% |\n\n" ) append("") + append(oppiaCoverageWikiPageLinkNote) } assertThat(readFinalMdReport()).isEqualTo(expectedResult) @@ -1162,6 +1172,7 @@ class RunCoverageTest { append("\n\n") append(exemptionsReferenceNote) append("") + append(oppiaCoverageWikiPageLinkNote) } assertThat(readFinalMdReport()).isEqualTo(expectedResult) @@ -1238,6 +1249,7 @@ class RunCoverageTest { append("\n\n") append(exemptionsReferenceNote) append("") + append(oppiaCoverageWikiPageLinkNote) } assertThat(readFinalMdReport()).isEqualTo(expectedResult) @@ -1333,6 +1345,7 @@ class RunCoverageTest { append("\n\n") append(exemptionsReferenceNote) append("") + append(oppiaCoverageWikiPageLinkNote) } assertThat(readFinalMdReport()).isEqualTo(expectedResult) @@ -1438,6 +1451,7 @@ class RunCoverageTest { append("\n\n") append(exemptionsReferenceNote) append("") + append(oppiaCoverageWikiPageLinkNote) } assertThat(readFinalMdReport()).isEqualTo(expectedResult) @@ -1642,6 +1656,7 @@ class RunCoverageTest { ":white_check_mark: | $MIN_THRESHOLD% |\n\n" ) append("") + append(oppiaCoverageWikiPageLinkNote) } assertThat(readFinalMdReport()).isEqualTo(expectedResult) @@ -1722,6 +1737,7 @@ class RunCoverageTest { ":white_check_mark: | $MIN_THRESHOLD% |\n\n" ) append("") + append(oppiaCoverageWikiPageLinkNote) } assertThat(readFinalMdReport()).isEqualTo(expectedResult) @@ -2282,6 +2298,7 @@ class RunCoverageTest { "3 / 4 | :white_check_mark: | $MIN_THRESHOLD% |\n\n" ) append("") + append(oppiaCoverageWikiPageLinkNote) } return markdownText @@ -2575,13 +2592,20 @@ class RunCoverageTest { } private fun getFilenameAsDetailsSummary( - filePath: String, - additionalData: String? = null + filePath: String ): String { val fileName = filePath.substringAfterLast("/") - val additionalDataPart = additionalData?.let { " - $it" } ?: "" - return "
$fileName$additionalDataPart$filePath
" + return "
$fileName$filePath
" + } + + private val oppiaCoverageWikiPageLinkNote = buildString { + val wikiPageReferenceNote = ">To learn more, visit the [Oppia Android Code Coverage]" + + "(https://github.com/oppia/oppia-android/wiki/Oppia-Android-Code-Coverage) wiki page" + append("\n\n") + append("#") + append("\n") + append(wikiPageReferenceNote) } private fun loadCoverageReportProto( diff --git a/scripts/src/javatests/org/oppia/android/scripts/coverage/reporter/CoverageReporterTest.kt b/scripts/src/javatests/org/oppia/android/scripts/coverage/reporter/CoverageReporterTest.kt index cfbfb5d45b2..b0af56a1115 100644 --- a/scripts/src/javatests/org/oppia/android/scripts/coverage/reporter/CoverageReporterTest.kt +++ b/scripts/src/javatests/org/oppia/android/scripts/coverage/reporter/CoverageReporterTest.kt @@ -75,6 +75,7 @@ class CoverageReporterTest { "| 100.00% | 10 / 10 | :white_check_mark: | $MIN_THRESHOLD% |\n\n" ) append("") + append(oppiaCoverageWikiPageLinkNote) } assertThat(readFinalMdReport()).isEqualTo(expectedMarkdown) @@ -116,6 +117,7 @@ class CoverageReporterTest { "| ${getFilenameAsDetailsSummary(filename)} | " + "0.00% | 0 / 10 | :x: | $MIN_THRESHOLD% |\n" ) + append(oppiaCoverageWikiPageLinkNote) } assertThat(readFinalMdReport()).isEqualTo(expectedMarkdown) @@ -152,6 +154,7 @@ class CoverageReporterTest { append("| File | Failure Reason | Status |\n") append("|------|----------------|--------|\n") append("| ://bazelTestTarget | Failure Message | :x: |") + append(oppiaCoverageWikiPageLinkNote) } assertThat(readFinalMdReport()).isEqualTo(expectedMarkdown) @@ -209,6 +212,7 @@ class CoverageReporterTest { append("\n\n") append(exemptionsReferenceNote) append("") + append(oppiaCoverageWikiPageLinkNote) } assertThat(readFinalMdReport()).isEqualTo(expectedMarkdown) @@ -267,6 +271,7 @@ class CoverageReporterTest { append("\n\n") append(exemptionsReferenceNote) append("") + append(oppiaCoverageWikiPageLinkNote) } assertThat(readFinalMdReport()).isEqualTo(expectedMarkdown) @@ -319,6 +324,7 @@ class CoverageReporterTest { "20.00% | 2 / 10 | :x: | 101% _*_ |\n" ) append("\n>**_*_** represents tests with custom overridden pass/fail coverage thresholds") + append(oppiaCoverageWikiPageLinkNote) } assertThat(readFinalMdReport()).isEqualTo(expectedMarkdown) @@ -374,6 +380,7 @@ class CoverageReporterTest { ) append("\n>**_*_** represents tests with custom overridden pass/fail coverage thresholds\n") append("") + append(oppiaCoverageWikiPageLinkNote) } assertThat(readFinalMdReport()).isEqualTo(expectedMarkdown) @@ -483,6 +490,7 @@ class CoverageReporterTest { append("\n\n") append(exemptionsReferenceNote) append("") + append(oppiaCoverageWikiPageLinkNote) } assertThat(readFinalMdReport()).isEqualTo(expectedMarkdown) @@ -908,6 +916,22 @@ class CoverageReporterTest { .contains("Coverage Analysis$BOLD$RED FAILED$RESET") } + @Test + fun testCoverageReporter_emptyCoverageProtoFile_passesCoverageStatus() { + System.setOut(PrintStream(outContent)) + + val protoListTextPath = "protoList.txt" + tempFolder.newFile(protoListTextPath) + + main( + "${tempFolder.root}", + protoListTextPath + ) + + assertThat(outContent.toString().trim()) + .contains("Coverage Analysis$BOLD$GREEN PASSED$RESET") + } + @Test fun testCoverageReporter_listOfCoverageProtoPath_generatesMarkdownReport() { val successProtoPath = "successCoverageReport.pb" @@ -979,6 +1003,28 @@ class CoverageReporterTest { "| 100.00% | 10 / 10 | :white_check_mark: | $MIN_THRESHOLD% |\n\n" ) append("") + append(oppiaCoverageWikiPageLinkNote) + } + + assertThat(readFinalMdReport()).isEqualTo(expectedMarkdown) + } + + @Test + fun testCoverageReporter_emptyCoverageProtoFile_generatesSkipCoverageReport() { + val protoListTextPath = "protoList.txt" + tempFolder.newFile(protoListTextPath) + + main( + "${tempFolder.root}", + protoListTextPath + ) + + val expectedMarkdown = buildString { + append("## Coverage Report\n") + append("### Results\n") + append("Coverage Analysis: **SKIP** :next_track_button:\n\n") + append("_This PR did not introduce any changes to Kotlin source or test files._") + append(oppiaCoverageWikiPageLinkNote) } assertThat(readFinalMdReport()).isEqualTo(expectedMarkdown) @@ -1001,6 +1047,15 @@ class CoverageReporterTest { return "
$fileName$additionalDataPart$filePath
" } + private val oppiaCoverageWikiPageLinkNote = buildString { + val wikiPageReferenceNote = ">To learn more, visit the [Oppia Android Code Coverage]" + + "(https://github.com/oppia/oppia-android/wiki/Oppia-Android-Code-Coverage) wiki page" + append("\n\n") + append("#") + append("\n") + append(wikiPageReferenceNote) + } + private fun createTestFileExemptionsProtoFile(testFileExemptions: TestFileExemptions): String { return tempFolder.newFile("test_file_exemptions.pb").also { it.outputStream().use(testFileExemptions::writeTo) diff --git a/wiki/Oppia-Android-Code-Coverage.md b/wiki/Oppia-Android-Code-Coverage.md new file mode 100644 index 00000000000..e5ed0479619 --- /dev/null +++ b/wiki/Oppia-Android-Code-Coverage.md @@ -0,0 +1,461 @@ +## Table of Contents + +- [Overview](#overview) +- [Understanding Code Coverage](#understanding-code-coverage) +- [Why is Code Coverage Important?](#why-is-code-coverage-important) +- [How to use the code coverage tool?](#how-to-use-the-code-coverage-tool) + - [Continuous Itegration Checks on Pull Request](#1-continuous-integration-checks-on-pull-requests) + - [Understanding the CI Coverage Report](#11-understanding-the-ci-coverage-report) + - [Local Command Line Interface Tools](#2-local-command-line-interface-cli-tools) + - [Understanding the Html Reports](#21-understanding-the-ci-coverage-report) +- [Increasing Code Coverage Metrics](#increasing-code-coverage-metrics) +- [Unit-Centric Coverage Philosophy](#unit-centric-coverage-philosophy) +- [Limitations of the code coverage tool](#limitations-of-the-code-coverage-tool) + +# Overview +In software engineering, code coverage, also called test coverage, is a percentage measure of the degree to which the source code of a program is executed when a particular test suite is run. A program with high code coverage has more of its source code executed during testing, which suggests it has a lower chance of containing undetected software bugs compared to a program with low code coverage. + +# Understanding code coverage +Code coverage measures the extent to which the code is tested by the automated tests. It indicates whether all parts of the code are examined to ensure they function correctly. + +Let's take a look at how code coverage works with a simple Kotlin function. + +Consider a Kotlin function designed to determine if a number is positive or negative: + +```kotlin +fun checkSign(n: Int): String { + return if (n >= 0) { + "Positive" + } else { + "Negative" + } +} +``` + +A test is created to verify that this function works correctly: +Please refer to the [Oppia Android testing documentation](https://github.com/oppia/oppia-android/wiki/Oppia-Android-Testing) to learn more about writing tests. + +``` +import org.junit.Test +import com.google.common.truth.Truth.assertThat + +class checkSignTest { + @Test + fun testCheckSign_withPositiveInteger_returnsPositive() { + assertThat(checkSign(4)).isEqualTo("Positive") + } +} +``` + +This test checks whether passing a positive integer '+4' `checkSign(4)` correctly returns "Positive" as output. +However, it doesn’t test how the function handles negative numbers. This means that only a portion of the function’s behavior is being tested, leaving the handling of negative numbers unverified. + +For thorough testing, the tests should also check how the function responds to negative inputs, this can be done by adding a test case by passing a negative integer to expect a return of "Negative" as output. + +```diff +import org.junit.Test +import com.google.common.truth.Truth.assertThat + +class checkSignTest { + @Test + fun testCheckSign_withPositiveInteger_returnsPositive() { + assertThat(checkSign(4)).isEqualTo("Positive") + } + ++ @Test ++ fun testCheckSign_withNegativeInteger_returnsNegative() { ++ assertThat(checkSign(-7)).isEqualTo("Negative") ++ } +} +``` + +# Why is code coverage important? +- **Minimizes the Risk of Bugs:** + High code coverage means your code is thoroughly tested. This helps find and fix bugs early. + +- **Maintains Code Stability:** + When you make changes to your code, high coverage helps ensure that those changes don’t break existing functionality. + +- **Facilitates Continuous Integration and Deployment:** + With high code coverage, you can confidently integrate and deploy code changes more frequently. Automated tests act as a safety check, allowing you to release updates automatically with reduced risk. + +- **Encourages Comprehensive Testing:** + Striving for high code coverage encourages to write tests for all parts of the code, including edge cases and less common scenarios, while preparing the application to handle unexpected conditions gracefully. + +- **Provides Confidence in Code Quality:** + Achieving a high percentage of code coverage gives confidence in the quality of the code. It also helps in maintaining a high standard of code quality over time. + +# How to use the code coverage tool? + +Oppia Android supports code coverage analysis through two primary methods: +1. Continuous Integration (CI) Checks on Pull Requests (PRs) +2. Local Command-Line Interface (CLI) Tools + +## 1. Continuous Integration Checks on Pull Requests + +Once a pull request (PR) is created, the Continuous Integration (CI) system automatically triggers a coverage check on all modified files. + +Note: The coverage checks are initiated only after the unit tests have successfully passed. + +![Screenshot (1596)](https://github.com/user-attachments/assets/5bacbb09-cf75-4811-b24c-84692f34916e) + +Following the completion of the coverage analysis, a detailed coverage analysis report will be uploaded as a comment on your PR. + +![image](https://github.com/user-attachments/assets/f0b5ae9d-9e64-4431-b493-c35eae94d2c1) + +## 1.1 Understanding the CI Coverage Report + + +Let's look at a sample coverage report to understand the details it provides. + +## Coverage Report + +### Results +Number of files assessed: 5
+Overall Coverage: **94.26%**
+Coverage Analysis: **FAIL** :x:
+## + +### Failure Cases + +| File | Failure Reason | Status | +|------|----------------|:------:| +| File.kt | No appropriate test file found for File.kt. | :x: | + +### Failing coverage + +| File | Coverage | Lines Hit | Status | Min Required | +|------|:--------:|----------:|:------:|:------------:| +|
Fail1.ktscripts/src/java/org/oppia/android/scripts/coverage/Fail1.kt
| 51.38% | 148 / 288 | :x: | 70% | +|
Fail2.ktutility/src/main/java/org/oppia/android/util/Fail2.kt
| 77.78% | 7 / 9 | :x: | 80% _*_ | +|
Fail3.ktdomain/src/main/java/org/oppia/android/domain/classify/Fail3.kt
| 10.00% | 1 / 10 | :x: | 30% _*_ | + +>**_*_** represents tests with custom overridden pass/fail coverage thresholds + +### Passing coverage + +
+Files with passing code coverage
+ +| File | Coverage | Lines Hit | Status | Min Required | +|------|:--------:|----------:|:------:|:------------:| +|
MathTokenizer.ktutility/src/main/java/org/oppia/android/util/math/MathTokenizer.kt
| 94.26% | 197 / 209 | :white_check_mark: | 70% | +
+ +### Exempted coverage +
Files exempted from coverage
+ +| File | Failure Reason | +|------|----------------| +|
TestExemptedFile.ktapp/src/main/java/org/oppia/android/app/activity/TestExemptedFile.kt
| This file is exempted from having a test file; skipping coverage check. | +|
SourceIncompatible.ktapp/src/main/java/org/oppia/android/app/activity/SourceIncompatible.kt
| This file is incompatible with code coverage tooling; skipping coverage check. | + +
+ +# + +The coverage report header provides an overview of the coverage analysis: + +### 1. Report Overview Section + +# + +### Results +Number of files assessed: 5
+Overall Coverage: **94.26%**
+Coverage Analysis: **FAIL** :x:
+ +# + +- **Number of Files Assessed:**
This displays the number of Kotlin files included in the coverage analysis. Files with other extensions, such as .xml, .json, or .md, are excluded from this analysis. + +- **Overall Coverage:**
This reflects the average code coverage percentage for the files modified in the current pull request. It provides a snapshot of how thoroughly the new code is tested. + +- **Coverage Analysis:**
This indicates whether the pull request passes the coverage analysis. A check mark denotes a passing result, while an X mark indicates a failure. + +### 2. Report Details Section + +# + +**2.a. Failure Cases** + +### Failure Cases + +| File | Failure Reason | Status | +|------|----------------|:------:| +| File.kt | No appropriate test file found for File.kt. | :x: | + +# + +This section lists files that failed to acquire coverage data. Failures may occur due to various reasons, including: + +- The absence of an appropriate test file for the respective source file, preventing coverage analysis. +- Bazel failing to collect coverage data for the source file. +- Bazel lacking a reference to the required source file in the collected data. +- Other potential reasons are still under exploration. + + The table has three columns: + +1. **File:** Displays the file for which the error occurred (Clicking the drop-down reveals the file's path in the codebase). +2. **Failure Reason:** Describes the reason for the failure. +3. **Status:** Indicates that the coverage status is a failure. + +Note: If this table or section is not present in the coverage report, it indicates that no changes exhibited failure. + +# + +**2.b. Failing coverages** + +### Failing coverage + +| File | Coverage | Lines Hit | Status | Min Required | +|------|:--------:|----------:|:------:|:------------:| +|
Fail1.ktscripts/src/java/org/oppia/android/scripts/coverage/Fail1.kt
| 51.38% | 148 / 288 | :x: | 70% | +|
Fail2.ktutility/src/main/java/org/oppia/android/util/Fail2.kt
| 77.78% | 7 / 9 | :x: | 80% _*_ | +|
Fail3.ktdomain/src/main/java/org/oppia/android/domain/classify/Fail3.kt
| 10.00% | 1 / 10 | :x: | 30% _*_ | + +>**_*_** represents tests with custom overridden pass/fail coverage thresholds + +# + +This section highlights files that have failed to meet the minimum coverage percentage. Any file that does not meet the minimum threshold percentage is considered to have a failing status. The minimum threshold is configured to a standard value, as specified in [CoverageReporter.kt](https://github.com/oppia/oppia-android/blob/develop/scripts/src/java/org/oppia/android/scripts/coverage/reporter/CoverageReporter.kt), and this value is shown alongside the status for each file. + +Files with overridden coverage thresholds are indicated by an asterisk (*) and include the specific required percentage. + +Note: Files listed may have met the standard minimum threshold but still fail if they do not achieve the required overridden coverage percentage. These files are strictly obligated to meet their specific required percentage and do not consider the minimum threshold. + +For a complete list of files with overridden coverage thresholds, refer to the [test_file_exemptions.textproto](https://github.com/oppia/oppia-android/blob/develop/scripts/assets/test_file_exemptions.textproto) file. + +The data presented includes: + +1. **File:** Name of the file that failed to meet the coverage threshold (Clicking the drop-down reveals the file's path in the codebase). +2. **Coverage Percentage:** Percentage of coverage achieved for the file. +3. **Lines Covered:** Number of lines covered by the test cases, shown in "lines hit / lines found" format. +4. **Status:** Indicates whether the coverage status is a pass or fail. +5. **Required Percentage:** Minimum percentage required to pass the coverage check for the file. + +# + +**2.c. Passing coverages** + +### Passing coverage + +
+Files with passing code coverage
+ +| File | Coverage | Lines Hit | Status | Min Required | +|------|:--------:|----------:|:------:|:------------:| +|
Pass.ktutility/src/main/java/org/oppia/android/util/Pass.kt
| 94.26% | 197 / 209 | :white_check_mark: | 70% | +
+ +# + +This section highlights files that have met the minimum coverage requirements. Files with overridden coverage thresholds are marked with an asterisk (*), and their specific required percentages are provided. + +These files have meet their overridden minimum coverage to achieve a passing status. + +For a complete list of files with overridden coverage thresholds, refer to the [test_file_exemptions.textproto](https://github.com/oppia/oppia-android/blob/develop/scripts/assets/test_file_exemptions.textproto) file. + +The data presented includes: + +1. **File:** Name of the file that met the coverage threshold (Clicking the drop-down reveals the file's path in the codebase). +2. **Coverage Percentage:** Percentage of coverage achieved for the file. +3. **Lines Covered:** Number of lines covered by the test cases, shown in "lines hit / lines found" format. +4. **Status:** Indicates whether the coverage status is a pass or fail. +5. **Required Percentage:** Minimum percentage required to pass the coverage check for the file. + +**2.d. Exempted coverages** + +### Exempted coverage +
Files exempted from coverage
+ +| File | Failure Reason | +|------|----------------| +|
TestExemptedFile.ktapp/src/main/java/org/oppia/android/app/activity/TestExemptedFile.kt
| This file is exempted from having a test file; skipping coverage check. | +|
SourceIncompatible.ktapp/src/main/java/org/oppia/android/app/activity/SourceIncompatible.kt
| This file is incompatible with code coverage tooling; skipping coverage check. | + +
+ +# + +Certain files are exempt from coverage checks. These exemptions include: + +1. **Test File Exemptions:** Files that are exempted from having corresponding test files are also exempted from coverage checks. Since no test files are available for these sources, coverage analysis cannot be performed, and these files are therefore skipped. + +2. **Source File Incompatibility Exemptions:** Some files are currently incompatible with Bazel coverage execution ([see tracking issue #5481](https://github.com/oppia/oppia-android/issues/5481)) and are temporarily excluded from coverage checks. + +You can find the complete list of exemptions in this file: [test_file_exemptions.textproto](https://github.com/oppia/oppia-android/blob/develop/scripts/assets/test_file_exemptions.textproto) + +This section appears at the bottom of the report, as a drop-down. It includes a table listing the exemptions with columns: + +1. **File:** Displays the file name that is exempted (Clicking the drop-down reveals the file's path in the codebase). +2. **Failure Reason:** Describes the specific reason for its exemption. + +With this report, you can review the coverage percentages for various files and identify which files pass or fail the coverage check. + + +## 2. Local Command-Line Interface (CLI) Tools + +While the CI check provides an overview of coverage, you might want to visualize how tests cover specific files locally, including which lines are covered and which are not. For this, Oppia's local command-line coverage tooling is useful. + +Note: Follow these [Bazel setup instructions](https://github.com/oppia/oppia-android/wiki/Oppia-Bazel-Setup-Instructions) if Bazel isn't yet set up in your local development environment. + +Oppia Android allows you to generate coverage reports in HTML format using the command: + +### Run Coverage + +```sh +bazel run //scripts:run_coverage -- +``` + +- : Your root directory. +- : Files you want to generate coverage reports for. + +For example, to analyze coverage for the file MathTokenizer.kt, use the relative path: + +```sh +bazel run //scripts:run_coverage -- $(pwd) utility/src/main/java/org/oppia/android/util/math/MathTokenizer.kt +``` + +By default, this will generate an HTML report in the coverage_reports directory. For the given file, the report will be saved as **coverage_reports/utility/src/main/java/org/oppia/android/util/math/MathTokenizer/coverage.html** + +A list of files can be provided as an input to generate coverage reports for each of the provided the files. An example of this is: + +```sh +bazel run //scripts:run_coverage -- $(pwd) +utility/src/main/java/org/oppia/android/util/math/MathTokenizer.kt +utility/src/main/java/org/oppia/android/util/parser/math/MathModel.kt +``` + +### Process Timeout + +The default wait time for a process to complete is 300 seconds (5 minutes). If the process does not finish within this period, you may encounter a "Process Did Not Finish Within Intended Time" error. To extend the wait time, you can modify the timeout setting by adding the flag --processTimeout=15 (The Time unit is minutes). + +```sh +bazel run //scripts:run_coverage -- $(pwd) +utility/src/main/java/org/oppia/android/util/parser/math/MathModel.kt --processTimeout=15 +``` + +## 2.1 Understanding the CI Coverage Report + +Let's examine the example coverage.html report (generated using the sample command in the previous section) to understand its contents. The report should be located at ``coverage_reports/utility/src/main/java/org/oppia/android/util/math/MathTokenizer/coverage.html``. + +The report is divided into two sections: +- Coverage Overview +- Line-by-Line Coverage Breakdown + +# + +### Coverage Overview + +![image](https://github.com/user-attachments/assets/e366af1f-2f64-4f14-a7e4-f7a592688c6c) + +In the top section of the report, you'll find the following details: + +- **File Path:** The relative path of the file to the oppia-android codebase. +- **Coverage Percentage:** The percentage of code covered by tests. +- **Line Coverage Data:** Details on the coverage of individual lines. +- **Main Content:** + +# + +### Line-by-Line Coverage Breakdown + +The subsequent section of the report visually represents the coverage status of each line of the source code. + +- **Red Lines:** Indicate lines not covered by test cases. +- **Green Lines:** Represent lines that are covered by test cases. + +![image](https://github.com/user-attachments/assets/c1d55cf8-94bf-4ab5-a02b-c95264e854db) + +These generated html reports are be used to identify areas of your code that may need additional testing. + +## Increasing Code Coverage Metrics + +To improve code coverage, start by identifying which lines are covered and which are not. To effectively increase coverage, trace the uncovered lines back to their calling functions, and ensure these are executed and tested in your test suites. The corresponding test files for the source code are usually located in the same package within the test directories. Add tests that cover all relevant behavioral scenarios for the uncovered lines to achieve comprehensive testing. + +For more guidance on best practices, refer to the [Writing Tests with Good Behavioral Coverage](https://github.com/oppia/oppia-android/wiki/Writing-Tests-With-Good-Behavioral-Coverage) wiki page. + +Note: Some files in the app module may have multiple test files, located in the sharedTest and test packages, all testing a single source file. For example: [StateFragment.kt](https://github.com/oppia/oppia-android/blob/develop/app/src/main/java/org/oppia/android/app/player/state/StateFragment.kt) has 2 test files [StateFragmentTest.kt](https://github.com/oppia/oppia-android/blob/develop/app/src/sharedTest/java/org/oppia/android/app/player/state/StateFragmentTest.kt) under ``app/src/sharedTest`` and [StateFragmentLocalTest.kt](https://github.com/oppia/oppia-android/blob/develop/app/src/test/java/org/oppia/android/app/player/state/StateFragmentLocalTest.kt) under ``app/src/test``. + +## Unit-Centric Coverage Philosophy + +Oppia Android's approach to code coverage is focused on analyzing each source unit within the context of its own tests. This means that only the tests specifically written for a particular source file are considered when determining that file's coverage. Incidental coverage—when a source file is indirectly tested through another test file—is not counted towards the coverage metrics of the original file. + +This approach ensures that coverage percentages are more meaningful, as they reflect deliberate and thorough testing of each source file. This makes it essential to write comprehensive tests for every source unit to achieve accurate and reliable coverage metrics. + +### Example Scenario + +To clarify the concept, consider the following example with source files and their corresponding test files: + +| **Source File** | **Tested By** | **Test File** | +|------------------------------|:-------------:|--------------------------------| +| `OppiaCommentBot.kt` | -> |`OppiaCommentBotTest.kt` | +| `UploadReviewComment.kt` | -> |`UploadReviewCommentTest.kt` | + +**OppiaCommentBot.kt (Source file):** + +It manages the logic for uploading comments and interacting with the Oppia project’s comment system. + +```kotlin +class OppiaCommentBot { + fun uploadComment(comment: String): String { + // Uploads a comment to a repository + return "Comment uploaded: $comment" + } +} +``` + +**OppiaCommentBotTest.kt (Test file for OppiaCommentBot):** + +```kotlin +import org.junit.Test +import com.google.common.truth.Truth.assertThat + +class OppiaCommentBotTest { + @Test + fun testUploadComment_returnsExpectedMessage() { + val bot = OppiaCommentBot() + val result = bot.uploadComment("Great job!") + assertThat(result).isEqualTo("Comment uploaded: Great job!") + } +} +``` + +**UploadReviewComment.kt (Another Source file):** + +It handles the creation and submission of review comments, utilizing features from OppiaCommentBot.kt. + +```kotlin +class UploadReviewComment { + fun createAndUploadReview(comment: String) { + // generates review + val bot = OppiaCommentBot() + bot.uploadComment(review) + } +} +``` + +**UploadReviewCommentTest.kt (Test file for UploadReviewComment):** + +```kotlin +import org.junit.Test + +class UploadReviewCommentTest { + @Test + fun testCreateAndUploadReview_callsUploadComment() { + val review = UploadReviewComment() + review.createAndUploadReview("Needs revision") + } +} +``` + +In this example, the `OppiaCommentBot` class is used in both `UploadReviewComment.kt` and `OppiaCommentBotTest.kt`. However, the code coverage tool only considers the tests in `OppiaCommentBotTest.kt` when calculating the coverage for `OppiaCommentBot.kt`. Tests in `UploadReviewCommentTest.kt` that indirectly call `OppiaCommentBot` do not count toward its coverage. + +It’s essential to ensure that each source file is directly tested by its own corresponding test file to accurately reflect the unit's coverage. This approach helps maintain a high standard of code quality and ensures that the coverage metrics genuinely represent the code’s reliability. + +## Limitations of the code coverage tool + +1. **Incompatibility with Code Coverage Analysis:** Certain test targets in the Oppia-Android codebase fail to execute and collect coverage using the Bazel coverage command. The underlying issues are still being investigated ([see tracking issue #5481](https://github.com/oppia/oppia-android/issues/5481)), and these files are currently exempt from coverage checks. However, it's expected that all new test files should work without needing this exemption. + +2. **Function and Branch Coverage:** The Oppia-Android code coverage tool currently provides only line coverage data. It does not include information on function or branch coverage. diff --git a/wiki/Writing-tests-with-good-behavioral-coverage.md b/wiki/Writing-tests-with-good-behavioral-coverage.md new file mode 100644 index 00000000000..7a0aa5caebc --- /dev/null +++ b/wiki/Writing-tests-with-good-behavioral-coverage.md @@ -0,0 +1,1599 @@ +## Table of Contents + +- [What is Behavioral Coverage?](#what-is-behavioral-coverage) +- [Understanding Behavioral Coverage and its Importance](#understanding-behavioral-coverage-and-its-importance) +- [Writing Effective Tests](#writing-effective-tests) + - [Understand the Requirements](#1-understanding-the-requirements) + - [Writing Clear and Descriptive Test Cases](#2-writing-clear-and-descriptive-test-cases) + - [Focusing on Specific Test Cases](#3-focusing-on-specific-test-cases) + - [Covering Different Scenarios](#4-covering-different-scenarios) + - [Covering All Branches, Paths, and Conditions](#5-covering-all-branches-paths-and-conditions) + - [Exception and Error Handling](#6-exception-and-error-handling) + - [Absence of Unwanted Output](#7-absence-of-unwanted-output) +- [Testing Public APIs](#testing-public-apis) +- [Structuring Test Bodies](#structuring-test-bodies) + - [When and How to Divide Responsibilities](#1-when-and-how-to-divide-responsibilities) + - [When Not to Divide Responsibilities](#2-when-not-to-divide-responsibilities) + - [Importance of Descriptive Test Names](#3-importance-of-descriptive-test-names) +- [How to Map a Line of Code to Its Corresponding Behaviors?](#how-to-map-a-line-of-code-to-its-corresponding-behaviors) + +# What is Behavioral Coverage? + +Behavioral coverage refers to the practice of testing various behaviors or functional aspects of your code to ensure it operates correctly under different scenarios. Instead of merely executing lines of code, it focuses on validating that all specified behaviors, edge cases, and conditions are correctly handled by the code. + +## Understanding Behavioral Coverage and its Importance + +Focusing on behavioral coverage is essential because: + +- Multiple Behaviors per Line: A single line of code can produce multiple behaviors depending on input values and conditions. +- Comprehensive Testing: High line coverage doesn’t guarantee all behaviors are tested. Behavioral coverage ensures every scenario, including edge cases and exceptions, is validated. +- Quality Over Quantity: Achieving high line coverage might give a false sense of security. Testing all possible behaviors ensures higher quality and robustness. + +Let's understand it better with a sample function. Consider this function that validates a name: + +```kotlin +fun getName(name: String? = " ") = + name?.takeIf { it.all { char -> char.isLetterOrWhitespace() } } + ?: throw IllegalArgumentException("Invalid name") +``` + +### Line Coverage Testing + +A basic test case for line coverage might look like this: + +```kotlin +@Test +fun testValidName() { + // Tests line coverage by hitting the line where name is accessed + assertThat(getName("Alice")).isEqualTo("Alice") +} +``` + +**Line Coverage Result:** This test covers the line of code but doesn’t test all scenarios. + +### Behavioral Coverage Testing + +To ensure behavioral coverage, the test needs to verify various conditions: + +```kotlin +@Test +fun testGetName_withDefaultValue_returnsEmptyValue() { + // Default value when no name is provided + assertThat(getName()).isEmpty() +} + +@Test +fun testGetName_withNullName_throwsException() { + // Exception for null value + assertThrows { getName(null) } +} + +@Test +fun testGetName_withSpecialCharacters_throwsException() { + // Exception for special characters + assertThrows { getName("!@#$%^&*()") } +} + +@Test +fun testGetName_withEmptyName_returnsEmptyValue() { + // Empty string should use default value + assertThat(getName("")).isEmpty() +} + +@Test +fun testGetName_withWhitespaceName_returnsWhitespaceValue() { + // Whitespace name + assertThat(getName(" ")).isEqualTo(" ") +} +``` + +**Behavioral Coverage Result:** These tests ensure that all potential behaviors, including edge cases and exceptions, are covered, providing a more thorough validation. + +### Quality > Percentage + +While line coverage might reach 100% with a single test, it doesn’t ensure that all scenarios are tested. Behavioral coverage ensures quality by validating all possible scenarios and edge cases, which is more important for reliable software. + +**Evaluation and Enhancement Flow:** + +```mermaid +block-beta +columns 2 + A[["Coverage Analysis"]]:2 + block:Coverage1 + B["100% Line Coverage
Does not guarantee full
behavioral coverage"] + end + block:Coverage2 + C["Less Than 100% Coverage
Guarantees at least one
important behavior is missing"] + end + block:Action1 + D["Add tests for lines that
miss behavioral coverages
even if stated as covered"] + end + block:Action2 + E["Find uncovered lines and add
tests that cover all behaviors
and not just line coverages"] + end + space + Coverage1 --> D + Coverage2 --> E + E --> D + + style A fill:#f3f3f3,stroke:#333,stroke-width:1px,color:#000000 + style B fill:#e8fcde,stroke:#333,stroke-width:1px,color:#000000 + style C fill:#ffdad4,stroke:#333,stroke-width:1px,color:#000000 + style D fill:#d8fac5,stroke:#333,stroke-width:1px,color:#000000 + style E fill:#e2fcd4,stroke:#333,stroke-width:1px,color:#000000 + +``` + +**Coverage and Behavior:** + +- 100% Line Coverage: Does not guarantee that all critical behaviors and edge cases are tested. It merely shows that each line of code has been executed. +- Less Than 100% Coverage: Guarantees that some part of the code is not covered by tests, which likely means that at least one important behavior or edge case is missing from the tests. + +**Line Coverage as a Starting Point:** + +- Use line coverage metrics as a starting point to identify which parts of the code have not been tested. +- Analyze these uncovered lines to determine which behaviors are associated with them. While line coverage helps locate untested code, it is crucial to ensure that all significant behaviors are tested, especially for volatile or complex scenarios. + +**Improving Coverage:** + +- Testing a specific line of code is useful, but it’s equally important to check for other missing behaviors or edge cases even if a line is stated as covered with coverage analysis. +- If a line of code is covered but does not account for all potential scenarios, add tests to cover those additional important behaviors. +- Ensure that tests are comprehensive by addressing various scenarios, including edge cases and error conditions, which might not be immediately obvious from line coverage alone. + +For more details on testing methodologies specific to Oppia Android, please refer to the [Oppia Android Testing](https://github.com/oppia/oppia-android/wiki/Oppia-Android-Testing). + +# Writing Effective Tests + +Writing effective tests is crucial for ensuring that your code behaves correctly under various conditions. Good testing practices help you identify issues early, maintain high code quality, and ensure that changes or new features do not break existing functionality. This guide focuses on how to write tests that not only check if your code runs but also verify that it performs as expected in different scenarios. + +## 1. Understanding the Requirements + +Before you start writing tests, it's essential to thoroughly understand the requirements or specifications for the functionality you are testing. This ensures that your tests accurately reflect what the code is supposed to do. + +# + +**Example User Story and Specification:** + +| **Aspect** | **Details** | +|--------------------------|-----------------------------------------------| +| **User Story** | As a shopper, I want to check if an item’s price is within my budget. If the price is above my budget, I shouldn’t buy it. If it’s within my budget, I should buy it. | +| **Function Name** | `shouldBuyItem` | +| **Inputs** | - `price`: A double representing the item's price.
- `budget`: A double representing the maximum amount you’re willing to spend. | +| **Output** | A boolean indicating whether the item should be bought. | +| **Behavior** | - Return `true` if the price is less than or equal to the budget.
- Return `false` if the price is greater than the budget. | + +**Requested Feature Code:** + +The function to determine if an item should be bought based on the price and budget is, + +```kotlin +fun shouldBuyItem(price: Double, budget: Double): Boolean { + return price <= budget +} +``` + +**Respective Test Case:** + +To ensure that the shouldBuyItem function works correctly, we can add the following test cases, + +```kotlin +@Test +fun testShouldBuyItem_withPriceWithinBudget_returnsTrue() { + assertThat(shouldBuyItem(50.0, 100.0)).isTrue() +} + +@Test +fun testShouldBuyItem_withPriceAboveBudget_returnsFalse() { + assertThat(shouldBuyItem(150.0, 100.0)).isFalse() +} +``` + +## 2. Writing Clear and Descriptive Test Cases + +Each test case should: + +- Clearly describe the scenario and expected outcome. +- Use descriptive names for your test methods. + +Naming Convention: +```kotlin +testAction_withOneCondition_withSecondCondition_hasExpectedOutcome +``` + +Example: +```kotlin +testCheckSign_forPositiveInput_returnsPositive() +testCheckSign_forNegativeInput_returnsNegative() +testCheckSign_forZeroInput_returnsNeitherPositiveNorNegative() +``` + +## 3. Focusing on Specific Test Cases + +When testing a function that performs multiple actions, it's crucial to write test cases that focus on individual aspects of the function. This approach allows you to isolate and identify issues more effectively if a test fails, rather than dealing with a more complex failure analysis. + +### Why Focus on Specific Test Cases? +Testing one thing at a time helps: + +- Identify Issues More Easily: If a test fails, you know precisely which aspect of the function is problematic. +- Improve Test Clarity: Each test case has a clear purpose, making tests easier to read and maintain. +- Isolate Failures: Helps in pinpointing issues related to a specific behavior or output. + +Consider a function that manages a food order process. This function does the following: + +1. Lists the food items. +2. Calculates the total price. +3. Displays the order. +4. Checks if the payment has been made and provides the corresponding message. + +```kotlin +fun processOrder(order: List, paymentMade: Boolean): String { + // List the food items + val itemList = order.joinToString(", ") + + // Calculate total price (mocked here for simplicity) + val totalPrice = order.size * 10.0 + + // Display order + println("Order: $itemList") + println("Total: $totalPrice") + + // Payment status + return if (paymentMade) "Payment successful" else "Payment pending" +} +``` + +**Potential output with payment made:** + +``` +Order: Pizza, Burger +Total: 20.0 +Result: Payment successful +``` + +### Testing the Entire Functionality + +**Single Test Case:** + +```kotlin +@Test +fun testProcessOrder_allSteps_returnsCorrectMessage() { + val result = processOrder(listOf("Pizza", "Burger"), paymentMade = true) + assertThat(result).isEqualTo("Payment successful") +} +``` + +**Difficulties in Testing All Aspects Together:** + +- Complex Failure Diagnosis: If this test fails, you need to diagnose whether the issue lies in listing items, calculating the total, displaying the order, or payment status. +- Less Focused: It does not target individual aspects of the function, making it harder to identify which specific action failed for cases when the test overall is failing. + +### Testing Specific Aspects + +**Test Case 1: Testing Listing items** + +```kotlin +@Test +fun testProcessOrder_providedWithList_displaysListOfItems() { + processOrder(listOf("Pizza", "Burger"), paymentMade = true) + val output = outContent.toString().trim() + assertThat(output).contains("Order: Pizza, Burger") +} +``` + +**Test Case 2: Calculates Total** + +```kotlin +@Test +fun testProcessOrder_forListItems_calculatesCorrectTotalPrice() { + processOrder(listOf("Pizza", "Burger"), paymentMade = true) + val output = outContent.toString().trim() + assertThat(output).contains("Total: 20.0") +} +``` + +**Test Case 3: Payment Success** + +```kotlin +@Test +fun testProcessOrder_whenPaymentMade_displaysPaymentSuccess() { + processOrder(listOf("Pizza", "Burger"), paymentMade = true) + val output = outContent.toString().trim() + assertThat(output).contains("Payment successful") +} +``` + +**Test Case 4: Payment Pending** + +```kotlin +@Test +fun testProcessOrder_whenNotPaymentMade_displaysPaymentPending() { + processOrder(listOf("Pizza", "Burger"), paymentMade = false) + val output = outContent.toString().trim() + assertThat(output).contains("Payment pending") +} +``` + +**Benefits of specific test cases:** + +- Clear Purpose: Each test case has a single, well-defined objective, making the tests more readable and maintainable. +- Easier Debugging: Focusing on one aspect makes it easier to pinpoint and fix issues. +- Improved Coverage: Ensures that each individual functionality of the method is tested thoroughly. + +## 4. Covering Different Scenarios + +To ensure robust testing, it's crucial to cover various scenarios your function might encounter. This involves testing the function with a range of inputs, including positive numbers, negative numbers, zero, and edge cases. Each scenario should be tested to verify that your function behaves correctly across different conditions. + +Consider a function `checkSign` that takes an integer and returns a string indicating whether the number is positive, negative, or zero. + +```kotlin +fun checkSign(number: Int): String { + return when { + number > 0 -> "Positive" + number < 0 -> "Negative" + else -> "Zero" + } +} +``` + +### Testing different scenerios + +Positive Number: Verifies that the function correctly identifies positive numbers. + +```kotlin +@Test +fun testCheckNumber_forPositiveInput_returnsPositive() { + assertThat(checkNumber(5)).isEqualTo("Positive") +} +``` + +Negative Number: Ensures that negative numbers are correctly classified. + +```kotlin +@Test +fun testCheckNumber_forNegativeInput_returnsNegative() { + assertThat(checkNumber(-3)).isEqualTo("Negative") +} +``` + +Zero: Checks that zero is handled correctly. + +```kotlin +@Test +fun testCheckNumber_forZeroInput_returnsZero() { + assertThat(checkNumber(0)).isEqualTo("Zero") +} +``` + +Maximum Value: Tests the function with Int.MAX_VALUE to ensure it handles the upper boundary. + +```kotlin +@Test +fun testCheckNumber_forMaxValue_returnsPositive() { + assertThat(checkNumber(Int.MAX_VALUE)).isEqualTo("Positive") +} +``` + +Minimum Value: Tests the function with Int.MIN_VALUE to ensure it handles the lower boundary. + +```kotlin +@Test +fun testCheckNumber_forMinValue_returnsNegative() { + assertThat(checkNumber(Int.MIN_VALUE)).isEqualTo("Negative") +} +``` + +## 5. Covering All Branches, Paths, and Conditions + +Testing all branches, paths, and conditions within your code is essential to ensure that every possible execution path is verified. This approach helps in identifying edge cases and logic errors that could otherwise go unnoticed. Effective testing covers all possible scenarios, including combinations of conditions and branching logic. + +Let's see the function to evaluate a user's access level based on their age and membership status. + +```kotlin +fun evaluateAccess(age: Int, isMember: Boolean): String { + var result: String + + if (age >= 18 && isMember) { + result = "Access granted" + } else if (age >= 18 && !isMember) { + result = "Membership required" + } else { + result = "Access denied" + } + + return result +} +``` + +The different scenarios and the expected outcomes are, + +| Scenario | Description | Expected Outcome | +|------------------|---------------------------------------|----------------------------| +| Adult Member | `age >= 18` and `isMember = true` | Returns "Access granted" | +| Adult Non-Member | `age >= 18` and `isMember = false` | Returns "Membership required" | +| Minor Member | `age < 18` and `isMember = true` | Returns "Access denied" | +| Minor Non-Member | `age < 18` and `isMember = false` | Returns "Access denied" | + +Testing needs to be performed to cover all branches, paths and conditions. + +```kotlin +@Test +fun testEvaluateAccess_forAdultMember_grantsAccess() { + assertThat(evaluateAccess(25, true)).isEqualTo("Access granted") +} + +@Test +fun testEvaluateAccess_forAdultNonMember_requiresMembership() { + assertThat(evaluateAccess(30, false)).isEqualTo("Membership required") +} + +@Test +fun testEvaluateAccess_forMinorMember_deniesAccess() { + assertThat(evaluateAccess(16, true)).isEqualTo("Access denied") +} + +@Test +fun testEvaluateAccess_forminorNonMember_deniesAccess() { + assertThat(evaluateAccess(15, false)).isEqualTo("Access denied") +} +``` + +Testing all branches and conditions ensures that your function can handle all possible scenarios, making it more reliable and easier to maintain. + +## 6. Exception and Error Handling + +### Exceptions + +Exceptions are unexpected events or errors that occur during the execution of a program, disrupting the normal flow of instructions. These are typically conditions that a program cannot anticipate or recover from easily, such as a division by zero, accessing an invalid index in an array, or trying to open a file that doesn’t exist. When an exception occurs, the program may terminate abruptly unless the exception is properly handled. + +```kotlin +fun divideNumbers(numerator: Int, denominator: Int): Int { + if (denominator == 0) throw IllegalArgumentException("Denominator cannot be zero") + return numerator / denominator +} +``` + +In this example, if the denominator is zero, an `IllegalArgumentException` is thrown. This is a standard way to handle situations where continuing the program as usual doesn’t make sense due to an error in the input. + +**Testing Exceptions:** + +The primary focus when testing exceptions is ensuring that the correct exception is thrown in the appropriate circumstances and that the program can handle it gracefully. + +**Test:** + +```kotlin +@Test +fun testDivideNumbers_forZeroDenominator_throwsIllegalArgumentException() { + val exception = assertThrows { + divideNumbers(10, 0) + } + assertThat(exception).hasMessageThat().contains("Denominator cannot be zero") +} +``` + +This test verifies that the divideNumbers function throws an IllegalArgumentException when the denominator is zero. It also checks that the exception message contains the expected text. Testing exceptions involves confirming that the application correctly identifies and handles these unexpected situations. + +### Domain Errors + +Domain errors are errors related to the logic or rules of the application, rather than technical issues like those covered by exceptions. These errors occur when the data or conditions within the domain do not meet the specific rules or constraints defined during application design. Domain errors are expected conditions and are handled within the normal flow of the application, rather than through exceptions. + +Let's understand this with a sample snippet from the source **[FractionParser.kt](https://github.com/oppia/oppia-android/blob/f9106d91297abd09b24da25d5485deb8473b0125/utility/src/main/java/org/oppia/android/util/math/FractionParser.kt#L7)** + +```kotlin +fun getSubmitTimeError(text: String): FractionParsingError { + if (text.isNullOrBlank()) { + return FractionParsingError.EMPTY_INPUT + } + if (invalidCharsLengthRegex.find(text) != null) { + return FractionParsingError.NUMBER_TOO_LONG + } + if (text.endsWith("/")) { + return FractionParsingError.INVALID_FORMAT + } + val fraction = parseFraction(text) + return when { + fraction == null -> FractionParsingError.INVALID_FORMAT + fraction.denominator == 0 -> FractionParsingError.DIVISION_BY_ZERO + else -> FractionParsingError.VALID + } +} +``` + +This function checks various conditions on the input string text to determine whether it meets the criteria for a valid fraction. Each condition that fails returns a specific domain error from the FractionParsingError enum. Unlike exceptions, these errors are expected as part of the application's logic and represent specific, recoverable conditions that the application can handle. + +**Testing Domain Errors:** + +The goal when testing domain errors is to ensure that the application correctly identifies and responds to these errors as part of its normal operation. + +**Test samples from [FractionParserTest.kt](https://github.com/oppia/oppia-android/blob/f9106d91297abd09b24da25d5485deb8473b0125/utility/src/test/java/org/oppia/android/util/math/FractionParserTest.kt#L19):** + +```kotlin +@Test +fun testSubmitTimeError_tenDigitNumber_returnsNumberTooLong() { + val error = getSubmitTimeError("0123456789") + assertThat(error).isEqualTo(FractionParsingError.NUMBER_TOO_LONG) +} + +@Test +fun testSubmitTimeError_nonDigits_returnsInvalidFormat() { + val error = getSubmitTimeError("jdhfc") + assertThat(error).isEqualTo(FractionParsingError.INVALID_FORMAT) +} + +@Test +fun testSubmitTimeError_divisionByZero_returnsDivisionByZero() { + val error = getSubmitTimeError("123/0") + assertThat(error).isEqualTo(FractionParsingError.DIVISION_BY_ZERO) +} + +@Test +fun testSubmitTimeError_ambiguousSpacing_returnsInvalidFormat() { + val error = getSubmitTimeError("1 2 3/4") + assertThat(error).isEqualTo(FractionParsingError.INVALID_FORMAT) +} +``` + +These tests check various inputs to ensure the getSubmitTimeError function correctly identifies and returns the appropriate FractionParsingError. Each test case focuses on a specific condition that is expected and handled within the domain logic, ensuring the application behaves as intended. + +### Handling Exceptions and Domain Errors + +- **Nature:** Exceptions are typically unforeseen disruptions in program execution, whereas domain errors are expected results of business logic conditions. + +- **Focus:** When testing exceptions, the focus is on ensuring proper handling and recovery from unexpected situations. In contrast, testing domain errors involves verifying that the application correctly identifies and manages these expected conditions. + +- **Handling and Recovery:** Exceptions often require special recovery mechanisms, such as try-catch blocks, while domain errors are managed through normal application logic and flow control. + +## Key Considerations for Testing Exception / Error Scenarios + +Testing exceptions and error handling is vital to ensure that applications behave correctly under error conditions, provide meaningful feedback, and maintain reliability. Without these tests, applications are prone to unpredictable failures, poor user experiences, and potential security issues. + +### 1. Verifying Correct Exception Types + +Ensure that the correct type of exception is thrown in response to error conditions. This confirms that your error handling logic is specific and accurate. + +```kotlin +@Test +fun testDivideNumbers_forZeroDenominator_throwsIllegalArgumentException() { + assertThrows { + divideNumbers(10, 0) + } +} +``` + +### 2. Checking for Proper Exception Messages + +When writing tests, it's crucial to ensure that exceptions thrown by your code contain meaningful and descriptive messages. These messages play a vital role in diagnosing issues, providing clarity on what went wrong and why it happened. + +Let's consider a TicketBooking class that has a function to reserve a seat. This function checks if seats are available using Kotlin's check() function. If no seats are available, it throws an exception with a specific message. + +**Functionality:** + +```kotlin +class TicketBooking { + fun reserveSeat(seatsAvailable: Int) { + check(seatsAvailable > 0) { + "No seats are available. Please check other bookings for available seats." + } + // Additional code to reserve a seat + } +} +``` + +In this case, when the **seatsAvailable** becomes 0, the `check()` function will throw an `IllegalStateException` with the message "No seats are available. Please check other bookings for available seats." + +**Test:** + +To verify that the exception is thrown with the correct message, we write a test case: + +```kotlin +@Test +fun testBookTickets_withUnavailableSeats_throwsException() { + val booking = TicketBooking() + val exception = assertThrows { + booking.reserveSeat(0) + } + assertThat(exception).hasMessageThat().contains("No seats are available. Please check other bookings for available seats.") +} +``` + +This test case checks that when no seats are available, the reserveSeat() function throws an `IllegalStateException` with the appropriate message. + +### Why verify Exception Messages? + +Verifying exception messages is crucial for ensuring the correctness and usefulness of your tests. This is especially important when dealing with generic exceptions, where multiple checks might throw the same type of exception. + +To better understand this, let's extend the `TicketBooking` class with an additional function that checks the payment status before confirming a booking. This function uses `check()` to verify if the payment was successful. If not, it throws an exception with a specific message. + +**Extended Functionality:** + +```kotlin +class TicketBooking { + fun reserveSeat(seatsAvailable: Int) { + check(seatsAvailable > 0) { + "No seats are available. Please check other bookings for available seats." + } + // Additional code to reserve a seat + } + + fun confirmPayment(isPaymentSuccessful: Boolean) { + check(isPaymentSuccessful) { + "Payment not successful. Please try again." + } + // Additional code to confirm payment + } +} +``` + +In this scenario, the `confirmPayment()` function throws an `IllegalStateException` if the payment is not successful, with the message "Payment not successful. Please try again." + +### Importance of Specific Error Messages + +Imagine if both checks in the `reserveSeat()` and `confirmPayment()` functions used generic messages like "Error occured" as: + +```kotlin +check(seatsAvailable > 0) { "Error occurred" } +check(isPaymentSuccessful) { "Error occurred" } +``` + +In this case, when an exception is thrown, it becomes very challenging to determine the exact cause of the error. Did the error occur because there were no seats available, or because the payment was not successful? This ambiguity can make debugging difficult and reduce the effectiveness of your tests. + +That is why it is necessary to test that each exception throws a specific error message relevant to its particular scenario—to help accurately diagnose where things went wrong. Consider the following test cases: + +**Test for Seat Availability:** + +```kotlin +@Test +fun testBookTickets_withUnavailableSeats_throwsException() { + val booking = TicketBooking() + val exception = assertThrows { + booking.reserveSeat(0) + } + assertThat(exception).hasMessageThat().contains("No seats are available. Please check other bookings for available seats.") +} +``` + +This test case ensures that when no seats are available, the correct exception with the appropriate message is thrown. + +**Test for Payment Status:** + +```kotlin +@Test +fun testConfirmPayment_withUnsuccessfulPayment_throwsException() { + val booking = TicketBooking() + val exception = assertThrows { + booking.confirmPayment(false) + } + assertThat(exception).hasMessageThat().contains("Payment not successful. Please try again.") +} + +``` + +### 3. Ensuring Exceptions Are Thrown at Correct Times + +Exceptions should be thrown only under specific conditions, not during normal operations. Verify that exceptions are correctly managed according to the context. + +```kotlin +@Test +fun testDivideNumbers_forValidInputs_returnsExpectedResult() { + val result = divideNumbers(10, 2) + assertThat(result).isEqualTo(5) +} +``` + +### 4. Testing Edge Cases + +Test edge cases where exceptions might be thrown, such as boundary values or extreme input scenarios. + +**Function with Edge Case Handling:** + +```kotlin +fun calculateDiscount(price: Double, discountPercent: Double): Double { + if (price < 0 || discountPercent < 0) { + throw IllegalArgumentException("Price and discount cannot be negative") + } + return price - (price * discountPercent / 100) +} +``` + +**Test Case:** + +```kotlin +@Test +fun testCalculateDiscount_forNegativePrice_throwsIllegalArgumentException() { + val exception = assertThrows { + calculateDiscount(-100.0, 10.0) + } + assertThat(exception).hasMessageThat().contains("Price and discount cannot be negative") +} + +@Test +fun testCalculateDiscount_forNegativeDiscount_throwsIllegalArgumentException() { + val exception = assertThrows { + calculateDiscount(100.0, -10.0) + } + assertThat(exception).hasMessageThat().contains("Price and discount cannot be negative") +} +``` + +## 7. Absence of Unwanted Output + +In addition to validating correct handling of valid and invalid files, it's also important to ensure that unwanted output or behavior does not occur. + +Let's use a simple `Pizza` class with an `orderPizza` function that has optional parameters like **addCheese** and **takeaway**. The idea is to test that when these options are set to **false**, no corresponding messages are printed. + +**Functionality:** + +```kotlin +class Pizza { + fun orderPizza(addCheese: Boolean = false, takeaway: Boolean = false): String { + var orderDetails = "Ordered a pizza" + + if (addCheese) { + orderDetails += " with extra cheese" + } + + if (takeaway) { + orderDetails += " for takeaway" + } + + println(orderDetails) + return orderDetails + } +} +``` + +**Test:** + +Ensure No Cheese Message When **addCheese** is **false** + +```kotlin +@Test +fun testOrderPizza_withNoCheese_doesNotPrintCheeseMessage() { + val pizza = Pizza() + val output = ByteArrayOutputStream() + System.setOut(PrintStream(output)) + + // Order pizza without cheese, default value is false + pizza.orderPizza(addCheese = false) + + // Verify that "with extra cheese" is not printed + assertThat(output.toString().trim()).doesNotContain("with extra cheese") + + System.setOut(System.out) +} +``` + +**Test:** + +Ensure No Takeaway Message When **takeaway** is **false** + +```kotlin +@Test +fun testOrderPizza_withNoTakeaway_doesNotPrintTakeawayMessage() { + val pizza = Pizza() + val output = ByteArrayOutputStream() + System.setOut(PrintStream(output)) + + // Order pizza without takeaway, default value is false + pizza.orderPizza(takeaway = false) + + // Verify that "with extra cheese" is not printed + assertThat(output.toString().trim()).doesNotContain("for takeaway") + + System.setOut(System.out) +} +``` + +These tests confirm that the class behaves as expected without producing unnecessary outputs. + +# Testing Public APIs + +A public API (Application Programming Interface) refers to the set of methods, properties, and functionalities exposed by a class or module for use by external code. It defines how other parts of a system or external systems can interact with the functionality provided by that class or module. + +Public APIs are essential because they provide a way to interact with the functionality of a class or module without exposing its internal workings. They define how external code can use the functionality offered by the class or module, ensuring that interactions are safe and predictable while keeping the internal implementation hidden and secure. + +Let's consider the following example for a public API to withdraw money from a BankAccount. + +```kotlin +class BankAccount( + private var balance: Double, + private val username: String, + private val password: String +) { + + // Public method to withdraw money + fun withdraw( + requestedUsername: String, // Username provided for the withdrawal + requestedPassword: String, // Password provided for the withdrawal + file: File? = null // Optional passbook file to upload to note transactions + amount: Double, // Amount to withdraw + needReceipt: Boolean = false // Flag to indicate if a receipt is needed, defaults to false + ) { + // Verify user credentials + // Validate withdrawal amount + // Perform the withdrawal operation + // Print a receipt if needed + println("Withdrawing $amount for $requestedUsername") + if (needReceipt) { + printReceipt(amount) + } + + // Process the file if provided + file?.let { + processFile(it) + } + } + + private fun isValidUser(requestedUsername: String, requestedPassword: String): Boolean { + return true + } + + private fun isValidWithdrawal(amount: Double): Boolean { + return true + } + + private fun performWithdrawal(amount: Double) { + println("Withdrew $amount. New balance is $balance") + } + + private fun printReceipt(amount: Double) { + println("Receipt: Withdrew $amount. Current balance: $balance") + } + + private fun processFile(file: File) { + println("Processing file: ${file.name}") + } +} +``` + +The **`withdraw`** method serves as the single public entry point for withdrawing money from the account. It handles user validation, amount checking, optional file upload and printing of the receipt. By keeping the internal methods private, the class ensures that the operations are performed in a controlled manner while hiding the complexity of these operations from the user. + +## How to Write Tests for a Public API + +```mermaid +graph LR + A[Analyze] --> B[Map Behaviors] + B --> C[Add Tests] + C --> D[Refine] + + style A fill:#f5f5f5,stroke:#333,stroke-width:1px,color:#000000 + style B fill:#f2fced,stroke:#333,stroke-width:1px,color:#000000 + style C fill:#e8fcde,stroke:#333,stroke-width:1px,color:#000000 + style D fill:#dcf5d0,stroke:#333,stroke-width:1px,color:#000000 + +``` + +### 1. Analyze the Public API + +Goal: Identify and map all possible behaviors and edge cases of the API method. + +**1. Identify Core Functionalities:** Break down the public method into its core functionalities. For the withdraw method, these include: + +- User authentication +- Amount validation +- Withdrawal execution +- Receipt printing +- File processing + +**2. Determine Expected Behaviors:** List the expected behaviors and outcomes for each core functionality. Consider both normal and edge cases. + +- Valid and invalid user credentials +- Valid and invalid withdrawal amounts +- Presence and absence of receipt +- File presence and absence + +### 2. Map Expected Behaviors to Test Cases + +- Goal: Create a comprehensive list of test cases based on the identified behaviors. +- Format: Use clear and descriptive test names to represent each behavior. + +Example Mappings: + +**1. User Authentication:** +- testWithdraw_validCredentials_outputsCorrectBalance +- testWithdraw_invalidUsername_throwsException +- testWithdraw_invalidPassword_throwsException +- testWithdraw_noBankData_initializationError + +**2. Amount Validation:** +- testWithdraw_validAmount_updatesBalance +- testWithdraw_negativeAmount_throwsException +- testWithdraw_amountGreaterThanBalance_throwsException + +**3. Receipt Printing:** +- testWithdraw_withNeedReceipt_receiptPrinted +- testWithdraw_withoutNeedReceipt_noReceiptPrinted +- testWithdraw_withDefaultReceipt_noReceiptPrinted + +**4. File Processing:** +- testWithdraw_withValidFile_processesFile +- testWithdraw_withInvalidFileFormat_throwsException +- testWithdraw_withAvailableFile_processesFile +- testWithdraw_withUnavailableFile_throwsException + +**5. Edge Cases:** +- testWithdraw_emptyUsername_throwsException +- testWithdraw_emptyPassword_throwsException +- testWithdraw_emptyAmount_throwsException +- testWithdraw_noBankData_initializationError + +### 3. Write the Tests Based on Mapped Behaviors + +Goal: Implement the test cases using your mapping as a guide. + +**Test Samples:** + +```kotlin +@Test +fun testWithdraw_validCredentials_outputsCorrectBalance() { + val account = BankAccount(1000.0, "user", "password") + + val output = ByteArrayOutputStream() + System.setOut(PrintStream(output)) + + account.withdraw("user", "password", 200.0) + + assertThat(output.toString().trim()).contains("Withdrew 200.0. New balance is 800.0") + System.setOut(System.out) +} + +@Test +fun testWithdraw_invalidUsername_throwsInvalidCredentialsException() { + val account = BankAccount(1000.0, "user", "password") + + val exception = assertThrows { + account.withdraw("invalidUser", "password", 200.0) + } + assertThat(exception).hasMessageThat().contains("Invalid credentials") +} + +@Test +fun testWithdraw_withNeedReceipt_receiptPrinted() { + val account = BankAccount(1000.0, "user", "password") + + val output = ByteArrayOutputStream() + System.setOut(PrintStream(output)) + + account.withdraw("user", "password", 200.0, needReceipt = true) + + assertThat(output.toString().trim()).contains("Receipt: Withdrew 200.0. Current balance: 800.0") + System.setOut(System.out) +} + +@Test +fun testWithdraw_withInvalidFileFormat_throwsInvalidFileFormatException() { + val account = BankAccount(1000.0, "user", "password") + val invalidFile = File("invalid.txt") + + val exception = assertThrows { + account.withdraw("user", "password", invalidFile, 200.0) + } + assertThat(exception).hasMessageThat().contains("Invalid file format") +} + +@Test +fun testWithdraw_withDefaultReceipt_noReceiptPrinted() { + val account = BankAccount(1000.0, "user", "password") + + val output = ByteArrayOutputStream() + System.setOut(PrintStream(output)) + + account.withdraw("user", "password", 200.0) + + assertThat(output.toString().trim()).doesNotContain("Receipt:") + System.setOut(System.out) +} + +``` + +### 4. Review and Refine + +Goal: Ensure all scenarios are covered and tests are effective. + +Once you have confirmed that all mappings are tested, proceed with the following steps. + +```mermaid +graph LR + A[Code Coverage] --> B[Map Lines] + B --> C[Add Tests] + C --> A + + style A fill:#e6e6e6,stroke:#333,stroke-width:1px,color:#000000 + style B fill:#ecfae3,stroke:#333,stroke-width:1px,color:#000000 + style C fill:#d8ffbf,stroke:#333,stroke-width:1px,color:#000000 +``` + +1. **Perform Code Coverage Analysis:** Evaluate code coverage to identify any uncovered lines. + +2. **Trace and map uncovered lines:** Investigate to identify lines not covered by existing tests. + +3. **Add Additional Cases:** Add behavioral tests for uncovered lines and include other missing tests even for covered lines. + + +Ensure tests are clear, maintainable, and provide meaningful results. Continue this process iteratively until all lines and behaviors are fully covered. + +By systematically analyzing the public API, mapping expected behaviors to test cases, and writing tests based on these mappings, you can ensure comprehensive and effective testing. This structured approach helps in covering all scenarios and provides clarity on how to test the API thoroughly. + +Note: For more information on how to utilize the code coverage analysis tool, please refer to the [Oppia Android Code Coverage](https://github.com/oppia/oppia-android/wiki/Oppia-Android-Code-Coverage) page. + +## Testing a Single Outcome in Multiple Ways + +When testing a single outcome like a successful withdrawal, you can use multiple approaches to verify the if the balance is updated correctly. Here are different ways to ensure the single outcome of withdrawal was processed correctly, each following a distinct approach. + +**a. To verify correctness of output:** + +Verifies that after withdrawing $200, the balance is updated to $800. This checks that the core functionality of updating the balance works correctly. + +```kotlin +@Test +fun testWithdraw_withSufficientBalance_updatesBalance() { + val output = ByteArrayOutputStream() + System.setOut(PrintStream(output)) + + account.withdraw("user", "password", 200.0) + + assertThat(output.toString().trim()).isEqualTo("Withdrew 200.0. New balance is 800.0") + System.setOut(System.out) +} +``` + +**b. To verify with receipt:** + +Ensures that when a receipt is requested, it includes the correct balance details of the withdrawal. + +```kotlin +@Test +fun testWithdraw_withReceipt_generatesReceipt() { + val output = ByteArrayOutputStream() + System.setOut(PrintStream(output)) + + account.withdraw("user", "password", 200.0, needReceipt = true) + + assertThat(output.toString().trim()).contains("Receipt: Withdrew 200.0. Current balance: 800.0") + System.setOut(System.out) +} +``` + +**c. To verify with passbook details:** + +Confirms balance statement with the passbook file being updated when a file is provided. + +```kotlin +@Test +fun testWithdraw_withPassbook_updatesPassbook() { + val passbookFile = File("passbook.pdf") + passbookFile.createNewFile() + val output = ByteArrayOutputStream() + System.setOut(PrintStream(output)) + + account.withdraw("user", "password", 200.0, file = passbookFile) + + // Read the passbook file and check its contents + val fileContents = passbookFile.readText() + assertThat(fileContents).contains("Withdrew 200.0. New balance is 800.0") + + System.setOut(System.out) +} +``` + +**d. To verify based on the log message:** + +Validates that the correct message about the withdrawal is logged. + +```kotlin +@Test +fun testWithdraw_withSufficientBalance_logsCorrectMessage() { + val output = ByteArrayOutputStream() + System.setOut(PrintStream(output)) + + account.withdraw("user", "password", 200.0) + + assertThat(output.toString().trim()).contains("New balance is 800.0") + System.setOut(System.out) +} +``` + +**e. To verify with the balance is updated correctly:** + +Ensures that the balance is updated correctly after a withdrawal. + +```kotlin +@Test +fun testWithdraw_withSufficientBalance_updatesBalanceCorrectly() { + account.withdraw("user", "password", 200.0) + assertThat(account.balance).isEqualTo(800.0) +} +``` + +These tests cover various aspects of the withdrawal functionality, ensuring that the balance updates correctly with receipts, passbooks and output messages. Although the core functionality of withdrawing funds and updating the balance is consistent, it can be observed in multiple ways. Each test focuses on a specific verification method while ultimately validating the same core functionality. + +# Structuring Test Bodies + +In testing, it's crucial to ensure that your tests verify implementation code while maintaining clarity and readability. Tests validate the correctness of the code, but it is humans who verify the correctness of the test code itself. Therefore, striking a balance between clarity and conciseness in test writing is essential. + +```mermaid +graph LR + A[  Tests ] -..->|verify| B[Source Code] + C[Humans] -..->|verify| D[  Tests  ] + + style A fill:#e8e8e8,stroke:#333,stroke-width:1px,color:#000000 + style B fill:#e0f7d7,stroke:#333,stroke-width:1px,color:#000000 + style C fill:#e8e8e8,stroke:#333,stroke-width:1px,color:#000000 + style D fill:#e0f7d7,stroke:#333,stroke-width:1px,color:#000000 +``` + +Tests should focus on verifying the behavior of the implementation code, while humans should be able to easily understand and verify the correctness of the test code itself. + +Let's use a Restaurant class as an example to explain how to structure test functionalities effectively. The Restaurant class manages orders, calculates bills, and applies discounts. + +```kotlin +class Restaurant( + private val menu: Map, + private val logHistoryPath: String = "LogHistory.txt" +) { + var items: List = listOf() + private var discount: Double = 0.0 + + fun placeOrder(orderItems: List) { + items = orderItems + println("Order placed: $items") + val totalBill = calculateBill() + println("Total bill after discount: ${totalBill - (totalBill * discount)}") + + val logDetails = "Items: $items, Total bill: ${totalBill - (totalBill * discount)}" + val file = loadLogHistory() + file.appendText(logDetails + "\n") + } + + fun applyDiscount(isMember: Boolean, code: String, quarter: YearQuarter) { + discount = when { + isMember && code == "SAVE10" -> 0.10 + code == "SAVE20" && quarter == YearQuarter.QUARTER1 -> 0.20 + code == "SUMMERSALE" && quarter == YearQuarter.QUARTER2 -> 0.15 + else -> 0.0 + } + println("Discount applied: ${discount * 100}%") + } + + private fun calculateBill(): Double { + return items.sumOf { menu[it] ?: 0.0 } + } + + private fun loadLogHistory(): File { + val file = File(logHistoryPath) + if (!file.exists()) { + file.createNewFile() + } + return file + } +} + +enum class YearQuarter { + QUARTER1, QUARTER2, QUARTER3, QUARTER4 +} +``` + +It's important to understand how to segment or split functionalities in your tests to maintain clarity and avoid confusion. Knowing when to use helper functions and `@Before` / `@After` annotations effectively, and when to keep logic within the test cases themselves, ensures that your tests are both clear and maintainable. Let’s explore these concepts with a Restaurant Ordering System example. + +## 1. When and How to Divide Responsibilities + +### a. Helper Functions + +Helper Functions are valuable for reducing redundancy in tests. They encapsulate **non-behavioral tasks**, ensuring that the focus remains on testing the core logic. + +With the above Restaurant class example, it can be seen that each order is logged to a provided file. While testing these functionalities it is crucial to ensure that each test case operates with its own unique log file. This is necessary to avoid cross-test interference and to verify that each test correctly logs its own data. To streamline this process, instead of creating a new log file manually within each test case, we use a utility function to handle the file creation. + +This approach keeps the test code clean and focused on testing the core logic, as the utility function purely deals with file management. By keeping this utility function separate from the core logic, it ensures that it doesn't affect the actual functionality being tested, making it an ideal candidate for a helper function. + +**Helper Function:** + +```kotlin +// Helper function to create a new log file and return its path +fun createLogFile(filePath: String): String { + val file = File(filePath) + if (file.exists()) { + file.delete() + } + file.createNewFile() + return filePath +} +``` + +**Test using the Helper Function:** + +```kotlin +@Test +fun testPlaceOrder_withValidItems_logsOrderDetails() { + val menu = mapOf("Burger" to 5.0, "Pizza" to 8.0, "Salad" to 4.0) + // Use the utility function to create a log file + val logFilePath = createLogFile("testLogHistory.txt") + val restaurant = Restaurant(menu, logHistoryPath = logFilePath) + + restaurant.placeOrder(listOf("Burger", "Pizza")) + + val logContent = File(logFilePath).readText() + + assertThat(logContent).contains("Items: [Burger, Pizza]") + assertThat(logContent).contains("Total bill: 13.0") +} +``` + +Using the `createLogFile` utility function helps maintain clean and focused test cases. It manages the file creation process without interfering with the actual logic being tested. + +### b. `@Before` and `@After` Annotations + +`@Before` and `@After` Annotations help in managing setup and teardown tasks, ensuring consistency and reducing redundancy in test cases. + +```kotlin +class RestaurantTests { + private lateinit var outputStream: ByteArrayOutputStream + + @Before + fun setUp() { + // Setup necessary resources + outputStream = ByteArrayOutputStream() + System.setOut(PrintStream(outputStream)) + } + + @After + fun tearDown() { + // Clean up resources after tests + System.setOut(System.out) + } + + @Test + fun testPlaceOrder_withValidItems_displaysCorrectTotalBill() { + val menu = mapOf("Burger" to 5.0, "Pizza" to 8.0, "Salad" to 4.0) + val logFilePath = createLogFile("LogHistory.txt") + val restaurant = Restaurant(menu, logHistoryPath = logFilePath) + + restaurant.placeOrder(listOf("Burger", "Pizza")) + assertThat(outputStream.toString().trim()).contains("Total bill: 13.0") + } +} +``` + +Use `@Before` and `@After` for tasks that need to be performed before and after every test case, such as setting up streams, initializing objects, or restoring states. These tasks should not contain logic that is part of the actual test behavior. + +## 2. When Not to Divide Responsibilities + +### Prioritizing Clarity Over Conciseness in Tests + +While it’s tempting to reduce code duplication in tests by using helper functions or annotations, **clarity should take precedence**. Tests are meant to verify that the code works correctly, but they also need to be clear enough for humans to understand and verify their correctness. + +**The Pitfalls of Complex Test Helper Implementations can be understood with the following case:** + +```kotlin +fun createDiscount(): Triple { + val isMember = true + val code = "SAVE10" + val quarter = YearQuarter.QUARTER1 + return Triple(isMember, code, quarter) +} + +@Test +fun testDiscountedBill_withCreateDiscountHelper_returnsDiscountedBill() { + val menu = mapOf("Burger" to 5.0, "Pizza" to 8.0, "Salad" to 4.0) + val logFilePath = createLogFile("LogHistory.txt") + val restaurant = Restaurant(menu, logHistoryPath = logFilePath) + + val discountDetails = createDiscount() + + restaurant.applyDiscount(discountDetails.first, discountDetails.second, discountDetails.third) + restaurant.placeOrder(listOf("Burger")) + + assertThat((outputStream.toString().trim()).contains("Total bill after discount: 4.5") +} +``` + +**The Drawbacks of This Approach** +- Hidden Logic: The helper function `createDiscount()` hides critical logic affecting the test outcome. This makes the test harder to understand and debug. +- Complexity: The helper function handles multiple scenarios, which should be tested separately. This introduces complexity and reduces clarity. +- Clarity: Hidden logic in a helper function makes the test less transparent and harder to interpret, compromising its readability. + +**Approach to write test with clarity:** + +In test code, being explicit often trumps being concise. This means defining the necessary conditions and actions directly within the test case, so the test's intent is immediately clear to anyone reading it. + +```kotlin +@Test +fun testDiscountedBill_withAppliedDicount_returnsDiscountedBill() { + val menu = mapOf("Burger" to 5.0, "Pizza" to 8.0, "Salad" to 4.0) + val logFilePath = createLogFile("LogHistory.txt") + val restaurant = Restaurant(menu, logHistoryPath = logFilePath) + + // Explicitly defining discount details in the test + val isMember = true + val code = "SAVE10" + val quarter = YearQuarter.QUARTER1 + + restaurant.applyDiscount(isMember, code, quarter) + restaurant.placeOrder(listOf("Burger")) + + assertThat((outputStream.toString().trim()).contains("Total bill after discount: 4.5") +} +``` + +Laying out the logic and conditions directly within the test makes it independent of external functions or files, which makes the test easier to understand, maintain, and debug. + +Unlike production code, where duplication is often avoided, in test code, it’s sometimes better to duplicate code if it leads to clearer, more understandable tests. This ensures that the behavior being tested is directly represented in the test case. + +## 3. Importance of Descriptive Test Names + +Naming test functions descriptively helps in identifying the purpose and scope of each test. Use names that reflect the specific behavior being tested. + +Oppia Android follows a naming convention where the test names should read like a sentence, and be consistent with other nearby test names to facilitate easily coming up with new tests. It's preferred that the following format be used for naming test functions: + +``` +testAction_withOneCondition_withSecondCondition_hasExpectedOutcome +``` + +```kotlin +@Test +fun testPlaceOrder_withValidItems_orderPlacedSuccessfully() { + // Test Logic: Order should be placed with given items +} + +@Test +fun testPlaceOrder_withEmptyItems_orderNotPlaced() { + // Test Logic: Handle empty order gracefully +} + +@Test +fun testCalculateBill_withValidItems_correctBillCalculated() { + // Test Logic: Calculate correct bill for ordered items +} + +@Test +fun testApplyDiscount_withMemberAndValidCode_discountApplied() { + // Test Logic: Apply 10% discount for valid code and membership +} + +@Test +fun testApplyDiscount_withNonMemberAndValidCode_noDiscountApplied() { + // Test Logic: No discount applied for non-member +} + +@Test +fun testApplyDiscount_withMemberAndValidCode_inQuarter2_discountApplied() { + // Test Logic: Apply discount for valid code in Quarter 2 +} +``` + +### Benefits of Descriptive Naming Conventions + +- **Clarity:** Specific names and conditions make tests easier to understand and manage. +- **Focus:** Helps pinpoint exact scenarios being tested, improving test coverage. +- **Debugging:** Clear names and conditions aid in quickly identifying the cause of failures. +- **Documentation:** Serves as self-documentation, providing insight into test purpose and scope. +- **Maintenance:** Simplifies updates and modifications by clearly defining what each test covers. + +# How to Map a Line of Code to Its Corresponding Behaviors + +Understanding how to map a line of code to its corresponding behaviors is essential for improving code coverage and writing effective tests. Here’s a structured approach to locate and cover lines of code: + +Let's use our Bank API code from previous examples to understand how to map uncovered lines of code to their corresponding behaviors and effectively write tests to cover them. + +Consider that our test suite covers the code as follows: + +**Test:** + +```kotlin +@Test +fun testWithdraw_withValidCredentials_printsWithdrawMessage() { + val account = BankAccount(1000.0, "user", "password") + val file = File("file.pdf") + + val output = ByteArrayOutputStream() + System.setOut(PrintStream(output)) + + account.withdraw("user", "password", 200.0, file = file) + + assertThat(output.toString().trim()).contains("Receipt: Withdrew 200.0. Current balance: 800.0") + System.setOut(System.out) +} +``` + +This validates the code behavior to function properly with valid inputs. + +### 1. Identify the Line of Code + +You can utilize the Oppia Android code coverage tool to assess the coverage for this code. This will generate an HTML report that helps you visualize the lines covered in green highlight and those not covered in red highlight. + +![image](https://github.com/user-attachments/assets/53bde5dd-3f96-4454-8ed8-159fb090f20e) + +Analyzing the report reveals that the line, + +```kotlin +println("Receipt: Withdrew $amount. Current balance: $balance") +``` + +and its function call `printReceipt` are marked in red, indicating that this line was never executed by the test case. This suggests that the functionality is not covered by the current tests, potentially exposing it to issues or regressions if the code is modified in the future. The green highlights indicate the lines of code that are covered by test cases. + +For more information on how to utilize the code coverage analysis tool, please refer to the [Oppia Android Code Coverage](https://github.com/oppia/oppia-android/wiki/Oppia-Android-Code-Coverage) page. + +### 2. Map to the Line of Code + +**1. Locate the Uncovered Line:** + +Locate to the corresponding line number of the uncovered line in the source file. That would locate to these lines: + +BankAccount.kt + +```kotlin +private fun printReceipt(amount: Double) { + println("Receipt: Withdrew $amount. Current balance: $balance") +} +``` + +Flow Diagram + +```mermaid +graph TD + F[printReceipt] --> G[print - Receipt: Withdrew $400] + + style F fill:#e0f7d7,stroke:#333,stroke-width:1px,color:#000000 + style G fill:#e0f7d7,stroke:#333,stroke-width:1px,color:#000000 +``` + +**2. Traceback to the calling point:** + +Next, trace the uncovered line back to where it is called in the code. This helps to understand why it wasn’t executed by the test case. + +```kotlin +fun withdraw( + requestedUsername: String, + requestedPassword: String, + amount: Double, + needReceipt: Boolean = false, // Defaults to false + file: File? = null +) { + // Other code here + + if (needReceipt) { --------------------. + printReceipt(amount) : + } : +} : + : +... : + < -----------------` +private fun printReceipt(amount: Double) { + println("Receipt: Withdrew $amount. Current balance: $balance") +} +``` + +2.1 The Conditional Call + +Identify the condition that controls whether the line of code is executed. Here it is the condition to have the value of **needReceipt** set to **true** to call the `printReceipt` method. + +```kotlin +if (needReceipt) { + printReceipt(amount) +} +``` + +Flow Diagram + +```mermaid +graph TD + D[Condition - if needReceipt is true] --> E[Method Call - printReceipt] + E --> F[printReceipt] + F --> G[print - Receipt: Withdrew $400] + + style D fill:#e0f7d7,stroke:#333,stroke-width:1px,color:#000000 + style E fill:#e0f7d7,stroke:#333,stroke-width:1px,color:#000000 + style F fill:#e0f7d7,stroke:#333,stroke-width:1px,color:#000000 + style G fill:#e0f7d7,stroke:#333,stroke-width:1px,color:#000000 +``` + +2.2 Determine the Origin of the Conditional Value + +Next, trace where this conditional value is set in the method. This helps to identify the requirement of the condition to set a passing value and access the method call. + +```kotlin +fun withdraw( + requestedUsername: String, + requestedPassword: String, + amount: Double, + needReceipt: Boolean = false, // Defaults to false ---------. + file: File? = null : +) { : + : + if (needReceipt) { <-----------------------` + printReceipt(amount) + } +} +``` + +Flow Diagram + +```mermaid +graph TD + B[needReceipt Default to false] + D[Condition - if needReceipt is true] --> E[Method Call - printReceipt] + E --> F[printReceipt] + F --> G[print - Receipt: Withdrew $400] + + style B fill:#FFB6C1,stroke:#333,stroke-width:1px,color:#000000 + style D fill:#e0f7d7,stroke:#333,stroke-width:1px,color:#000000 + style E fill:#e0f7d7,stroke:#333,stroke-width:1px,color:#000000 + style F fill:#e0f7d7,stroke:#333,stroke-width:1px,color:#000000 + style G fill:#e0f7d7,stroke:#333,stroke-width:1px,color:#000000 +``` + +It can be seen that the **needReceipt** value is passed as a parameter while having a **default value** of **false**. Since the value was **never set to true** in our test case, + +```kotlin +@Test +fun testWithdraw_withValidCredentials_printsWithdrawMessage() { + ... + + account.withdraw("user", "password", 200.0, file = file) + + ... +} +``` + +it defaulted to being false thereby never meeting the condition to perform the `printReceipt`. + +2.3 Trace Back to the Method Call + +Identify the method or function that influences the needReceipt parameter and trace it to the public API where it is used. By understanding this connection, you can modify the needReceipt parameter’s default value in withdraw to affect the behavior of the code. + +```kotlin +fun withdraw( + requestedUsername: String, + requestedPassword: String, + amount: Double, + needReceipt: Boolean = false, // Defaults to false + file: File? = null +) { } +``` + +Flow Diagram + +```mermaid +graph TD + A[Public API - withdraw] --> B[needReceipt Default to false] + A[Public API - withdraw] --> C[needReceipt set to true] + C --> D[Condition - if needReceipt is true] + D --> E[Method Call - printReceipt] + E --> F[printReceipt] + F --> G[print - Receipt: Withdrew $400] + + style A fill:#f5f5f5,stroke:#333,stroke-width:1px,color:#000000 + style B fill:#ffb6c1,stroke:#333,stroke-width:1px,color:#000000 + style C fill:#e0f7d7,stroke:#333,stroke-width:1px,color:#000000 + style D fill:#e0f7d7,stroke:#333,stroke-width:1px,color:#000000 + style E fill:#e0f7d7,stroke:#333,stroke-width:1px,color:#000000 + style F fill:#e0f7d7,stroke:#333,stroke-width:1px,color:#000000 + style G fill:#e0f7d7,stroke:#333,stroke-width:1px,color:#000000 +``` + +**3. Add Test Case to Cover the Line:** + +To ensure that the `printReceipt` method is covered, we need to add a test case that sets the **needReceipt** parameter to **true**. + +Test: + +```kotlin +@Test +fun testWithdraw_withReceipt_printsReceipt() { + val output = ByteArrayOutputStream() + System.setOut(PrintStream(output)) + + // Call withdraw with needReceipt set to true + account.withdraw("user", "password", 200.0, needReceipt = true) + + // Verify that receipt was printed + assertThat(output.toString().trim()).contains("Receipt: Withdrew 200.0. Current balance: 800.0") + System.setOut(System.out) +} +``` + +This test ensures that the `printReceipt` method is invoked and behaves as intended. By covering this line, you verify that the receipt functionality is properly executed, which is crucial for ensuring complete test coverage and the reliability of the feature. + +Performing a code coverage analysis with the added test case would generate a report as below: + +![image](https://github.com/user-attachments/assets/cc4b8ca5-5d10-48a1-893e-19e06c7bff95) + +By following these steps, you can effectively map an uncovered line of code to its calling point and understand why it was not covered. Adjusting your test cases to trigger the conditions required for the line to be executed will help ensure comprehensive test coverage and minimize the risk of regression issues. \ No newline at end of file diff --git a/wiki/_Sidebar.md b/wiki/_Sidebar.md index 16a5e8e93a1..ea5a254f2c6 100644 --- a/wiki/_Sidebar.md +++ b/wiki/_Sidebar.md @@ -25,6 +25,8 @@ * Testing * [Oppia Android Testing](https://github.com/oppia/oppia-android/wiki/Oppia-Android-Testing) * [End to End Testing Guide](https://github.com/oppia/oppia-android/wiki/End-to-End-Testing-Guide) + * [Oppia Android Code Coverage](https://github.com/oppia/oppia-android-workflow/wiki/Oppia-Android-Code-Coverage) + * [Writing Tests with Good Behavioral Coverage](https://github.com/oppia/oppia-android/wiki/Writing-Tests-With-Good-Behavioral-Coverage) * [Developing Skills](https://github.com/oppia/oppia-android/wiki/Developing-skills) * [Frequent Errors and Solutions](https://github.com/oppia/oppia-android/wiki/Frequent-Errors-and-Solutions) * [RTL Guidelines](https://github.com/oppia/oppia-android/wiki/RTL-Guidelines) From 2091fc30a44b2acbc2295ed7c5a7913946e46d99 Mon Sep 17 00:00:00 2001 From: Adhiambo Peres <59600948+adhiamboperes@users.noreply.github.com> Date: Sat, 24 Aug 2024 21:59:14 +0300 Subject: [PATCH 7/8] Fix lint issue --- .../java/org/oppia/android/app/activity/ActivityComponent.kt | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/src/main/java/org/oppia/android/app/activity/ActivityComponent.kt b/app/src/main/java/org/oppia/android/app/activity/ActivityComponent.kt index df7d722f1ab..402161f88ea 100644 --- a/app/src/main/java/org/oppia/android/app/activity/ActivityComponent.kt +++ b/app/src/main/java/org/oppia/android/app/activity/ActivityComponent.kt @@ -9,5 +9,3 @@ import org.oppia.android.app.utility.datetime.DateTimeUtil * Instances of this subcomponent should be created using [ActivityComponentFactory]. */ interface ActivityComponent : AppLanguageActivityInjector, DateTimeUtil.Injector - -// # Some test comment \ No newline at end of file From 54718ea051de4007c02f53b0d23f9fc921a1f64e Mon Sep 17 00:00:00 2001 From: Adhiambo Peres <59600948+adhiamboperes@users.noreply.github.com> Date: Sun, 25 Aug 2024 14:41:57 +0300 Subject: [PATCH 8/8] Revert test changes --- .../org/oppia/android/testing/espresso/TextInputActionTest.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/testing/src/test/java/org/oppia/android/testing/espresso/TextInputActionTest.kt b/testing/src/test/java/org/oppia/android/testing/espresso/TextInputActionTest.kt index a6b703c0427..34d7f816950 100644 --- a/testing/src/test/java/org/oppia/android/testing/espresso/TextInputActionTest.kt +++ b/testing/src/test/java/org/oppia/android/testing/espresso/TextInputActionTest.kt @@ -53,7 +53,7 @@ class TextInputActionTest { } } - /*@Test + @Test fun testTextExistsMatcher_descriptionMatchesExpectedDescription() { val errorText = "Incorrect Administrator PIN. Please try again." val expectedDescription = @@ -87,7 +87,7 @@ class TextInputActionTest { val result: Boolean = errorTextNotExisted.matches(textInputLayout) assertThat(result).isFalse() } - }*/ + } @Test fun testTextDoesNotExistMatcher_descriptionMatchesExpectedDescription() {