Fix #3813, #92, part of #4044: Refactor AsyncResult into a sealed class

[BenHenning]

Explanation

Fix #3813
Fix #92
Fix part of #4044 (see below for how this relates to the broader math expressions project)

This PR refactors AsyncResult to be a sealed class rather than using enums which has a distinct advantage: the strong typing guarantees from a sealed class ensure the presence of certain data is guaranteed (such as the Throwable from the failing AsyncResult). However, this refactor is very far-reaching since essentially every usage of AsyncResult needed to be updated (though not necessarily just references).

High-level AsyncResult pattern changes

Here are comparisons in how the common operations have changed:

Before:

val result: AsyncResult<String> = fetchOrReceiveSomeResult()

// Construction.
val pending = AsyncResult.pending<String>()
val failure = AsyncResult.failure<String>(IllegalStateException("Failure reason"))
val success = AsyncResult.success("success value")

// Check type.
val isPending = result.isPending()
val isFailure = result.isFailure()
val isSuccess = result.isSuccess()
val isCompleted = result.isCompleted()

// Retrieve success values.
val successValue1 = result.getOrDefault("default string")
val successValue2 = result.getOrThrow()

// Retrieve failure reason.
val failureReason = result.getErrorOrNull()

// Generic type check.
when (result) {
  // We typically ignore the pending case.
  result.isFailure() -> { /* Do something with result.getErrorOrNull() */ }
  result.isSuccess() -> { /* Do something with result.getOrDefault() or result.getOrThrow() */ }
  // The 'else' case is possible, and often not implemented.
}

After:

val result: AsyncResult<String> = fetchOrReceiveSomeResult()

// Construction.
val pending = AsyncResult.Pending<String>()
val failure = AsyncResult.Failure<String>(IllegalStateException("Failure reason"))
val success = AsyncResult.Success("success value")

// Check type.
val isPending = result is AsyncResult.Pending
val isFailure = result is AsyncResult.Failure
val isSuccess = result is AsyncResult.Success
val isCompleted1 = result !is AsyncResult.Pending
val isCompleted2 = result is AsyncResult.Failure || result is AsyncResult.Success

// Retrieve success values.
val success1 = success.value // Type is 'String'
val success2 = (result as? AsyncResult.Success)?.value // Type is 'String?' conditioned on if the result is a success.

// Retrieve failure reason.
val error1 = failure.error // Type is 'Throwable'
val error2 = (result as? AsyncResult.Failure)?.error // Type is 'Throwable?' conditioned on if the result is a failure.

// Generic type check.
when (result) {
  is AsyncResult.Pending { /* Do something in the pending case, such as showing a loading indicator. */ }
  is AsyncResult.Failure { /* Do something with result.error which is never null. */ }
  is AsyncResult.Success { /* Do something with result.value which is always defined. */ }
  // 'else' can't exist since all cases are exhausted, and Kotlin will eventually produce an error if sealed class 'when's aren't exhaustive.
}

// Note 'getOrThrow' is no longer needed since 'value' can only be retrieved if the result is confirmed to be a success.
// Similarly, 'getErrorOrNull' isn't needed since the error can similarly only be extracted from a Failure result.
// 'getOrDefault' is still possible using patterns as so (where the second is preferred to ensure all cases are considered):
val defaultExample1 = if (result is AsyncResult.Success) result.value else default
val defaultExample2 = when (result) {
  // Or, split out 'failure' to log an error message.
  is AsyncResult.Pending, is AsyncResult.Failure -> default
  is AsyncResult.Success -> result.value
}

// Note that result transformations haven't changed at all.

The main advantages of the new approach:

• All type cases must always be considered when using 'when' with the result (forcing consideration of both error and pending cases--this is useful since we often ignore pending cases today despite them existing and occurring)
• There's no longer any question about the presence of success values or failure error throwables (they are always present if the corresponding result is the correct type) which results in safer, more readable, and shorter code
• The implementation properly allows for null success values (see 'rationale' section below)

The only disadvantage is that some generic cases will conflate AsyncResult's sub-types, such as this case:

val results1 = mutableListOf(AsyncResult.Pending())
// results1 is of type MutableList<AsyncResult.Pending>, but this can be fixed by being explicit:
val results2 = mutableListOf<AsyncResult<String>>(AsyncResult.Pending())
// Or:
val results3: MutableList<AsyncResult<String>> = mutableListOf(AsyncResult.Pending())

Rationale for why this PR is needed & connection to algebraic expressions

#4239 introduces changes to QuestionAssessmentProgressController and ExplorationProgressController such that data providers with null payloads are transformed into others. However, that caused an issue when transforming results since 'null' can mean either the absence of 'success' (i.e. a failure) or a successful null value, and AsyncResult couldn't tell the difference between these without leveraging a sealed class.

See #4239 for why it's necessary for the broader algebraic expressions work (which is the same justification as why this PR is). This PR exists to make #4239 smaller since it brought in some additional refactoring & cleanup beyond the minimal migration to a sealed class. Furthermore, this included in the algebraic project chain for the same reason as #4239: to simplify merging and reviews.

Tests migration and DataProviderMonitorFactory

Rather than updating all tests which verified DataProvider results to use the new AsyncResultSubject, it seemed nicer to migrate them over to DataProviderMonitorFactory (which effectively fully fixed #3813). This resulted in a nice simplification in a lot of test suites by eliminating Mockito usage entirely (except in the monitor's implementation). I also replaced all toLiveData-esque calls to use DataProviders, instead (which required changing a bunch of controllers over to using DataProviders instead of LiveData (which is the preferred approach since LiveData should never be used in the domain layer to ensure good threading cohesion).

Test exemptions & changes

Note that there's a new AsyncResultSubject being introduced here to simplify testing (which consequently fixes #92), but tests aren't added for this subject for similar reasons to past algebraic expression PRs. #4236 was filed to track adding tests for this subject in the future. All existing cases that weren't migrated as part of the monitor factory refactor were moved over to using this new subject (except those explicitly testing AsyncResult).

A bunch of tests have been removed since they are no longer possible due to compile-time guarantees about the value of AsyncResult in different scenarios.

StateFragmentTest was updated to account for the content description for incorrect answers to not be computed correctly (see below section for details).

Changes to StateRecyclerViewAssembler & content descriptions

There was a bug in StateRecyclerViewAssembler where an answer wouldn't be indicated as incorrect correctly in explorations. I fixed this by introducing a more explicit 'correct_answer' signal in the answer response proto. This fix was needed since one of StateFragmentTest's tests started failing when another issue was fixed in ExplorationProgressControllerTest: in an old PR, one of the notifications for the current state DataProvider was dropped mid-answer submission. Restoring this notification triggered the test to correctly fail (due to it actually hitting the bug in the assembler; before, the content description wasn't being correctly defined in that test and it was falsely passing). The fix to the assmbler caused the test to start passing (but another to fail due to it not being supported on Robolectric; it's been subsequently ignored like others of its kind).

The new fix is intentionally designed to be robust against cases when an answer doesn't have a correct label (since that was assumed to be present before).

Essential Checklist

• 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: ...".)
• Any changes to scripts/assets files have their rationale included in the PR explanation.
• The PR follows the style guide.
• The PR does not contain any unnecessary code changes from Android Studio (reference).
• The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
• The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

N/A -- This should be a non-side effect change. Despite the UI changes, it shouldn't actually change user behaviors (possibly with the exception of answer notification being a bit faster which led to the UI test fix that was explained above) with one primary exception: this fixes the content description computation for incorrect answers. Reviewers: please let me know if you'd like this demonstrated, and how. I also didn't explicitly verify that affected shared tests pass on Espresso, but I highly doubt they wouldn't (or at least, would change their current pass/fail status) given that their Robolectric counterparts pass and this is a non-functional change.

I did not verify StateFragmentTest on Espresso since it's currently broken (see #4238). This is fixed in the next PR where the test suite is verified to pass.