Skip to content

Commit

Permalink
[RunAllTests] Fix #5303, #5304, #5305, #5306, #5309, part of #5307, p…
Browse files Browse the repository at this point in the history
…art of #5308: Fix a variety of dev platform-specific issues [Blocked: #5335] (#5138)

## Explanation
Fixes #5303
Fixes #5304
Fixes #5305
Fixes #5306
Fixes #5309
Fixes part of #5307
Fixes part of #5308

This PR fixes a variety of issues that were discovered while trying to
begin development on #4929 with a different workstation than I usually
build on.

Some of the issues were outright problems at the outset, but others were
discovered when trying to fix those issues (e.g. during test utility
upgrades).

Specific issues that were found and fixed:
- #5303: ``AssertionHelpers.assertThrows`` didn't handle cases when
trying to catch an ``AssertionError`` itself (such in the case of
``TestGitRepositoryTest``). This could result in false positives, and it
has since been fixed. Plus, the changes to ``AssertionHelpers`` also
removes the dependency on Kotlin reflection.
- #5304: ``LoggingIdentifierController`` could have ID generation
inconsistencies between environments due to how its ``Random`` instance
was initialized. This could also result in a test order issue if a
subset of tests were run, or run in a different order. The controller
has been updated to have better robustness against the order in which
IDs are generated which fixes the test and environment determinism
issues. \
\
The fix was essentially generating ID-specific seeds for ID-specific
randoms that all originate from the application seed, and are always
generated in the same order deterministically at the creation time of
the controller. This fixes the IDs being generated in different orders
leading to inconsistencies. Note that this is why ID changes are seen in
a variety of tests (since there are now new seeds being used for
generating these IDs as a by-product of the robustness improvements).
- #5305: ``model/src/main/proto/BUILD.bazel`` needed a fix due to a
build warning (that only occurs when trying to build all targets).
- #5306: ``TestGitRepository`` was made more robust by better managing
and setting the configured user & email used in its Git commands. The
utility now manages a locally set configuration for each rather than
relying on the global configuration (which may not be present on all
systems, hence the original issue). Some other improvements in error
detection were also added. Note that this likely wasn't discovered in CI
because it seems that GitHub CI _does_ set up a Git user & email
automatically, as do most people who work on the project.
- #5307 & #5308: ``ActivityTestRule`` is now deprecated in favor of
``ActivityScenarioRule`` since it can interact with activities outside
their lifecycle. However, the scenario rule is also not good to use
anymore since it can result in partial dependencies being initialized
before test-only platform parameter states can be overridden. This PR
introduces a new pattern to prevent using either of these rules, and the
mentioned issues can be used to track the remaining tests that need to
be cleaned up following this PR.
- #5309: ``ProfileAndDeviceIdFragmentTest`` is environment and
order-dependent due to out-of-order platform parameter initialization.
This issue ultimately arose from test parameters being initialized after
they're needed in the launched UI. It is necessary when there are
environment differences (e.g. local vs. CI) or when running certain
tests individually. This was fixed by removing the usage of
``ActivityScenarioRule`` and instead managing the fragment's test
activity lifecycle directly. \
\
Separately, the test performs proto comparisons that can be
machine-dependent. It seemed the test was suffering from some proto
encoding inconsistencies that seem to occur between some development
machines vs. on CI. The fix improves the test's robustness by extracting
the raw encoded string, verifying that the other outputs in the intent
message correctly correspond to that string, and that the string (as a
parsed proto) contains the correct values. As a result, the test no
longer depends on a hardcoded encoding value to be present for
verification. This does result in a slightly lengthier test with more
logic, but should be more stable in the long-term.

Some other miscellaneous notes:
- ``testCreateLearnerId_verifyCreatesCorrectRandomValue`` was removed
from ``LoggingIdentifierControllerTest`` because it wasn't actually
providing useful testing value. The application seed is not itself part
of the class's public API. Instead, the ID generation methods should be
ultimately verified.
- ``ComputeAffectedTestsTest`` had a shard increase to 24 and
``BazelClientTest`` has now been sharded to 4 (ditto for
``GenerateMavenDependenciesListTest`` and
``MavenDependenciesListCheckTest``). Both were done to help distribute
the more expensive tests in each suite across multiple runners to try
and reduce the long-tail of script test runs. More optimization will
probably be useful in the future. Note also that some of these tests are
*much* slower on certain systems (the one I've been working on, for
instance), hence the need for these adjustments.
-
``testEnsureNextResultIsSuccess_failureThenSuccess_notified_throwsException``
was removed from ``DataProviderTestMonitorTest`` since it *seems* that
it wasn't actually written correctly (and the issue wasn't discovered
until ``assertThrows`` was fixed). Per the existing test coverage, it
doesn't seem necessary to try and fix the test so it was instead
removed.
- ``ProfileAndDeviceIdFragmentTest`` changes led to a new
``EventLogSubject.hasNoProfileId`` being added.
- There are a few miscellaneous script infrastructure changes that were
pulled into this PR in preparation for downstream work. Specifically:
- ``ComputeAffectedTests`` now allows for its command executor to be
used for Bazel operations (which provides its tests with a bit more
execution flexibility). Its tests were also updated to use a longer
timeout for commands to try and improve test stability.
``BazelClientTest`` was similarly updated.
  - ``TestBazelWorkspace`` was updated to:
- Have better workspace initialization robustness (since the workspace
can actually be generated at several different points, not singularly
with a call to ``initEmptyWorkspace``).
- Add a ``.bazelrc`` file to disable bzlmod (since it causes network
stability issues in some environments when using newer Bazel versions
even if the project isn't upgraded, and we don't use bzlmod yet,
anyway).
- Add a ``.bazelversion`` file to ensure that tests are using the same
Bazel version as the project build.
- ``AuthenticationControllerTest`` was largely updated since the changes
to ``assertThrows`` behavior revealed that the failure test wasn't
actually correctly verifying an exception was being passed (since it
isn't actually being thrown; the test was a false positive possibly
because it was using ``Throwable``, but I didn't dig into exactly why it
was passing before). The changes ensure that the callbacks are actually
verified rather than the user result (since there's a separate test to
do that), as well as the exception's failure message.

## 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
N/A since all of the changes in this PR are infrastructural or only
affect tests.

---------

Co-authored-by: Adhiambo Peres <[email protected]>
  • Loading branch information
BenHenning and adhiamboperes authored Feb 14, 2024
1 parent 30ba44d commit a864d5d
Show file tree
Hide file tree
Showing 113 changed files with 1,636 additions and 1,459 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ app_test(
"//app/src/main/java/org/oppia/android/app/translation/testing:test_module",
"//testing",
"//testing/src/main/java/org/oppia/android/testing/junit:initialize_default_locale_rule",
"//testing/src/main/java/org/oppia/android/testing/logging:event_log_subject",
"//testing/src/main/java/org/oppia/android/testing/logging:sync_status_test_module",
"//testing/src/main/java/org/oppia/android/testing/platformparameter:test_module",
"//testing/src/main/java/org/oppia/android/testing/robolectric:test_module",
Expand Down

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -418,7 +418,7 @@ class DeveloperOptionsFragmentTest {
createDeveloperOptionsTestActivityIntent(internalProfileId)
).use {
testCoroutineDispatchers.runCurrent()
val exception = assertThrows(RuntimeException::class) {
val exception = assertThrows<RuntimeException>() {
scrollToPosition(position = 2)
onView(withId(R.id.force_crash_text_view)).perform(click())
}
Expand All @@ -433,7 +433,7 @@ class DeveloperOptionsFragmentTest {
).use {
testCoroutineDispatchers.runCurrent()
onView(isRoot()).perform(orientationLandscape())
val exception = assertThrows(RuntimeException::class) {
val exception = assertThrows<RuntimeException>() {
scrollToPosition(position = 2)
onView(withId(R.id.force_crash_text_view)).perform(click())
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ class StringToRatioParserTest {

@Test
fun testParser_parseRatioOrThrow_ratioWithInvalidRatio_throwsException() {
val exception = assertThrows(IllegalArgumentException::class) {
val exception = assertThrows<IllegalArgumentException>() {
stringToRatioParser.parseRatioOrThrow("a:b:c")
}
assertThat(exception)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ class AppLanguageLocaleHandlerTest {

@Test
fun testGetDisplayLocale_initialState_throwsException() {
val exception = assertThrows(IllegalStateException::class) {
val exception = assertThrows<IllegalStateException>() {
appLanguageLocaleHandler.getDisplayLocale()
}

Expand Down Expand Up @@ -103,7 +103,7 @@ class AppLanguageLocaleHandlerTest {
fun testInitializeLocale_twice_throwsException() {
appLanguageLocaleHandler.initializeLocale(computeNewAppLanguageLocale(ENGLISH))

val exception = assertThrows(IllegalStateException::class) {
val exception = assertThrows<IllegalStateException>() {
appLanguageLocaleHandler.initializeLocale(retrieveAppLanguageLocale())
}

Expand All @@ -117,7 +117,7 @@ class AppLanguageLocaleHandlerTest {
fun testUpdateLocale_uninitialized_throwsException() {
setAppLanguage(ENGLISH)

val exception = assertThrows(IllegalStateException::class) {
val exception = assertThrows<IllegalStateException>() {
appLanguageLocaleHandler.updateLocale(retrieveAppLanguageLocale())
}

Expand Down Expand Up @@ -186,7 +186,7 @@ class AppLanguageLocaleHandlerTest {
fun testInitializeLocaleForActivity_uninitialized_throwsException() {
val configuration = Configuration()

val exception = assertThrows(IllegalStateException::class) {
val exception = assertThrows<IllegalStateException>() {
appLanguageLocaleHandler.initializeLocaleForActivity(configuration)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ class AppLanguageResourceHandlerTest {
updateAppLanguageTo(OppiaLanguage.ENGLISH)
val handler = retrieveAppLanguageResourceHandler()

assertThrows(Resources.NotFoundException::class) { handler.getStringInLocale(-1) }
assertThrows<Resources.NotFoundException>() { handler.getStringInLocale(-1) }
}

@Test
Expand Down Expand Up @@ -279,7 +279,7 @@ class AppLanguageResourceHandlerTest {
updateAppLanguageTo(OppiaLanguage.ENGLISH)
val handler = retrieveAppLanguageResourceHandler()

assertThrows(Resources.NotFoundException::class) { handler.getStringInLocaleWithWrapping(-1) }
assertThrows<Resources.NotFoundException>() { handler.getStringInLocaleWithWrapping(-1) }
}

@Test
Expand Down Expand Up @@ -316,7 +316,7 @@ class AppLanguageResourceHandlerTest {
updateAppLanguageTo(OppiaLanguage.ENGLISH)
val handler = retrieveAppLanguageResourceHandler()

assertThrows(Resources.NotFoundException::class) {
assertThrows<Resources.NotFoundException>() {
handler.getStringInLocaleWithoutWrapping(-1)
}
}
Expand All @@ -336,7 +336,7 @@ class AppLanguageResourceHandlerTest {
updateAppLanguageTo(OppiaLanguage.ENGLISH)
val handler = retrieveAppLanguageResourceHandler()

assertThrows(Resources.NotFoundException::class) { handler.getStringArrayInLocale(-1) }
assertThrows<Resources.NotFoundException>() { handler.getStringArrayInLocale(-1) }
}

@Test
Expand Down Expand Up @@ -366,7 +366,7 @@ class AppLanguageResourceHandlerTest {
updateAppLanguageTo(OppiaLanguage.ENGLISH)
val handler = retrieveAppLanguageResourceHandler()

assertThrows(Resources.NotFoundException::class) { handler.getQuantityStringInLocale(-1, 0) }
assertThrows<Resources.NotFoundException>() { handler.getQuantityStringInLocale(-1, 0) }
}

@Test
Expand Down Expand Up @@ -397,7 +397,7 @@ class AppLanguageResourceHandlerTest {
updateAppLanguageTo(OppiaLanguage.ENGLISH)
val handler = retrieveAppLanguageResourceHandler()

assertThrows(Resources.NotFoundException::class) {
assertThrows<Resources.NotFoundException>() {
handler.getQuantityStringInLocaleWithWrapping(-1, 0)
}
}
Expand Down Expand Up @@ -432,7 +432,7 @@ class AppLanguageResourceHandlerTest {
updateAppLanguageTo(OppiaLanguage.ENGLISH)
val handler = retrieveAppLanguageResourceHandler()

assertThrows(Resources.NotFoundException::class) {
assertThrows<Resources.NotFoundException>() {
handler.getQuantityStringInLocaleWithoutWrapping(-1, 0)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,15 @@ class LoggingIdentifierController @Inject constructor(
private val persistentCacheStoreFactory: PersistentCacheStore.Factory,
private val oppiaLogger: OppiaLogger
) {
private val learnerIdRandom by lazy { Random(applicationIdSeed) }
// Use a base random to compute the per-ID random IDs in order so that test behaviors are
// consistent and deterministic when fixing the application ID seed.
private val baseRandom = Random(applicationIdSeed)
private val installationRandomSeed = baseRandom.nextLong()
private val sessionRandomSeed = baseRandom.nextLong()
private val learnerRandomSeed = baseRandom.nextLong()
private val installationIdRandom by lazy { Random(installationRandomSeed) }
private val sessionIdRandom by lazy { Random(sessionRandomSeed) }
private val learnerIdRandom by lazy { Random(learnerRandomSeed) }

private val sessionId by lazy { MutableStateFlow(computeSessionId()) }
private val sessionIdDataProvider by lazy {
Expand Down Expand Up @@ -113,12 +121,12 @@ class LoggingIdentifierController @Inject constructor(
sessionId.value = computeSessionId()
}

private fun computeSessionId(): String = learnerIdRandom.randomUuid().toString()
private fun computeSessionId(): String = sessionIdRandom.randomUuid().toString()

private fun computeInstallationId(): String {
return machineLocale.run {
MessageDigest.getInstance("SHA-1")
.digest(learnerIdRandom.randomUuid().toString().toByteArray())
.digest(installationIdRandom.randomUuid().toString().toByteArray())
.joinToString("") { "%02x".formatForMachines(it) }
.substring(startIndex = 0, endIndex = 12)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -435,7 +435,7 @@ class AudioPlayerControllerTest {
@Test
fun testController_notInitialized_releasePlayer_fails() {
setUpMediaReadyApplication()
val exception = assertThrows(IllegalStateException::class) {
val exception = assertThrows<IllegalStateException>() {
audioPlayerController.releaseMediaPlayer()
}

Expand All @@ -446,7 +446,7 @@ class AudioPlayerControllerTest {
@Test
fun testError_notPrepared_invokePlay_fails() {
setUpMediaReadyApplication()
val exception = assertThrows(IllegalStateException::class) {
val exception = assertThrows<IllegalStateException>() {
audioPlayerController.play(isPlayingFromAutoPlay = false, reloadingMainContent = false)
}

Expand All @@ -456,7 +456,7 @@ class AudioPlayerControllerTest {
@Test
fun testError_notPrepared_invokePause_fails() {
setUpMediaReadyApplication()
val exception = assertThrows(IllegalStateException::class) {
val exception = assertThrows<IllegalStateException>() {
audioPlayerController.pause(isFromExplicitUserAction = true)
}

Expand All @@ -466,7 +466,7 @@ class AudioPlayerControllerTest {
@Test
fun testError_notPrepared_invokeSeekTo_fails() {
setUpMediaReadyApplication()
val exception = assertThrows(IllegalStateException::class) {
val exception = assertThrows<IllegalStateException>() {
audioPlayerController.seekTo(500)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,21 @@ import dagger.Module
import dagger.Provides
import kotlinx.coroutines.CoroutineDispatcher
import org.junit.Before
import org.junit.Rule
import org.junit.Test
import org.junit.runner.RunWith
import org.mockito.ArgumentCaptor
import org.mockito.Captor
import org.mockito.Mock
import org.mockito.Mockito.verify
import org.mockito.Mockito.verifyNoInteractions
import org.mockito.junit.MockitoJUnit
import org.mockito.junit.MockitoRule
import org.oppia.android.domain.oppialogger.analytics.ApplicationLifecycleModule
import org.oppia.android.testing.TestLogReportingModule
import org.oppia.android.testing.assertThrows
import org.oppia.android.testing.firebase.FakeFirebaseAuthWrapperImpl
import org.oppia.android.testing.firebase.TestAuthenticationModule
import org.oppia.android.testing.mockito.capture
import org.oppia.android.testing.robolectric.RobolectricModule
import org.oppia.android.testing.time.FakeOppiaClockModule
import org.oppia.android.util.data.DataProvidersInjector
Expand All @@ -36,66 +44,66 @@ import javax.inject.Singleton
@LooperMode(LooperMode.Mode.PAUSED)
@Config(application = AuthenticationControllerTest.TestApplication::class)
class AuthenticationControllerTest {
@Inject
lateinit var firebaseAuthWrapper: FirebaseAuthWrapper
@field:[Rule JvmField] val mockitoRule: MockitoRule = MockitoJUnit.rule()

@Inject
lateinit var fakeFirebaseAuthWrapperImpl: FakeFirebaseAuthWrapperImpl
@Inject lateinit var firebaseAuthWrapper: FirebaseAuthWrapper
@Inject lateinit var fakeFirebaseAuthWrapperImpl: FakeFirebaseAuthWrapperImpl
@Inject lateinit var authenticationController: AuthenticationController
@field:[Inject BackgroundDispatcher] lateinit var backgroundDispatcher: CoroutineDispatcher

@Inject
lateinit var authenticationController: AuthenticationController

@field:[Inject BackgroundDispatcher]
lateinit var backgroundDispatcher: CoroutineDispatcher
@Mock lateinit var mockFakeSuccessCallback: FakeSuccessCallback
@Mock lateinit var mockFakeFailureCallback: FakeFailureCallback
@Captor lateinit var throwableCaptor: ArgumentCaptor<Throwable>

@Before
fun setUp() {
setUpTestApplicationComponent()
}

@Test
fun testAuthentication_getCurrentUser_userSignedIn_returnsInstanceOfFirebaseUserWrapper() {
fun testAuthentication_signInAnonymously_onlyOnSuccessCalled() {
fakeFirebaseAuthWrapperImpl.simulateSignInSuccess()

firebaseAuthWrapper.signInAnonymously(
onSuccess = {},
onFailure = {}
onSuccess = mockFakeSuccessCallback::onSuccess,
onFailure = mockFakeFailureCallback::onFailure
)

val user = authenticationController.currentFirebaseUser

assertThat(user).isInstanceOf(FirebaseUserWrapper::class.java)
// onSuccess should be called.
verify(mockFakeSuccessCallback).onSuccess()
verifyNoInteractions(mockFakeFailureCallback)
}

@Test
fun testAuthentication_signInAnonymously_succeeds() {
fakeFirebaseAuthWrapperImpl.simulateSignInSuccess()
fun testAuthentication_signInAnonymously_onlyOnFailureCalled() {
fakeFirebaseAuthWrapperImpl.simulateSignInFailure()

firebaseAuthWrapper.signInAnonymously(
onSuccess = {},
onFailure = {}
onSuccess = mockFakeSuccessCallback::onSuccess,
onFailure = mockFakeFailureCallback::onFailure
)

val user = authenticationController.currentFirebaseUser

assertThat(user).isInstanceOf(FirebaseUserWrapper::class.java)
// onFailure should be called with the failure details.
verify(mockFakeFailureCallback).onFailure(capture(throwableCaptor))
verifyNoInteractions(mockFakeSuccessCallback)
assertThat(throwableCaptor.value).hasMessageThat().contains("Sign-in failure")
}

@Test
fun testAuthentication_signInAnonymously_failure_returnsException() {
fakeFirebaseAuthWrapperImpl.simulateSignInFailure()
fun testAuthentication_getCurrentUser_userSignedIn_returnsInstanceOfFirebaseUserWrapper() {
fakeFirebaseAuthWrapperImpl.simulateSignInSuccess()
firebaseAuthWrapper.signInAnonymously(
onSuccess = mockFakeSuccessCallback::onSuccess,
onFailure = mockFakeFailureCallback::onFailure
)

assertThrows(Throwable::class) {
firebaseAuthWrapper.signInAnonymously(
onSuccess = {},
onFailure = {}
)
}
val user = authenticationController.currentFirebaseUser

assertThat(user).isInstanceOf(FirebaseUserWrapper::class.java)
}

private fun setUpTestApplicationComponent() {
ApplicationProvider.getApplicationContext<TestApplication>()
.inject(this)
ApplicationProvider.getApplicationContext<TestApplication>().inject(this)
}

@Module
Expand Down Expand Up @@ -141,4 +149,8 @@ class AuthenticationControllerTest {

override fun getDataProvidersInjector(): DataProvidersInjector = component
}

interface FakeSuccessCallback { fun onSuccess() }

interface FakeFailureCallback { fun onFailure(failure: Throwable) }
}
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ class AnswerClassificationControllerTest {
fun testClassify_forUnknownInteraction_throwsException() {
val interaction = Interaction.getDefaultInstance()

val exception = assertThrows(IllegalStateException::class) {
val exception = assertThrows<IllegalStateException>() {
answerClassificationController.classify(
interaction,
TEST_STRING_0,
Expand All @@ -170,7 +170,7 @@ class AnswerClassificationControllerTest {
.addAnswerGroups(AnswerGroup.newBuilder().addRuleSpecs(RuleSpec.getDefaultInstance()))
.build()

val exception = assertThrows(IllegalStateException::class) {
val exception = assertThrows<IllegalStateException>() {
answerClassificationController.classify(
interaction,
TEST_STRING_0,
Expand Down
Loading

0 comments on commit a864d5d

Please sign in to comment.