Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add tests for AsyncResultSubject #4236

Open
BenHenning opened this issue Mar 8, 2022 · 0 comments
Open

Add tests for AsyncResultSubject #4236

BenHenning opened this issue Mar 8, 2022 · 0 comments
Labels
enhancement End user-perceivable enhancements. Impact: Low Low perceived user impact (e.g. edge cases). Issue: Needs Clarification Indicates that an issue needs more detail in order to be able to be acted upon. Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet. Z-ibt Temporary label for Ben to keep track of issues he's triaged.

Comments

@BenHenning
Copy link
Member

BenHenning commented Mar 8, 2022 •
edited
Loading

TODO introduces a new custom Truth test subject: AsyncResultSubject. It would be advantageous to add thorough tests for this to ensure that:

  • Each of the subject methods correctly allow users to test specific properties
  • Each of the subject methods are verified to fail when expected with the correct failure reason being verified

See #4097 for a similar issue.
@BenHenning BenHenning mentioned this issue Mar 16, 2022
Merged
6 tasks
BenHenning added a commit that referenced this issue Mar 27, 2022
@BenHenning
Fix #3813, #92, part of #4044: Refactor AsyncResult into a sealed cla…
cc5ba50 
…ss (#4237)

## 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:
```kotlin
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:
```kotlin
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:

```kotlin
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 ``DataProvider``s, instead (which required changing a bunch of controllers over to using ``DataProvider``s 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
- [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 -- 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.

Commit history:

* Alphabetize test exemptions.

* Fix typo & add regex check.

The new regex check makes it so that all parameterized testing can be
more easily tracked by the Android TL.

* Add missing KDocs.

* Post-merge cleanups.

Also, fix text file exemption ordering.

* Add new test for negation with math symbol.

* Post-merge fixes.

* Add KDocs.

Also, add new regex exemption for new parameterized tests in this
branch.

* Refactor & simplify real ext impl.

Also, fix/clarify some KDocs.

* Lint fixes.

* Simplify operation list converter a lot.

This inlines three recursive operations to be done during the actual
computation to simplify the overall converter complexity (and to make
determining the test matrix easier).

* Prepare for new tests.

* Remove the ComparableOperationList wrapper.

* Change parameterized method delimiter.

* Use utility directly in test.

* Post-merge fixes.

This adjusts for the removal of ComparableOperationList (i.e. no wrapper
proto).

* Add first round of tests.

This includes fixes to the converter itself as it wasn't distributing
both product inversions and negation correctly in several cases. Tests
should now be covering these cases.

* Finish initial test suite.

Still needs to be cleaned up, but after converter refactoring attempts.

* Simplify operation sorting comparators.

* Remove old tests.

* Add remaining missing tests.

* KDocs & test exemption.

* Renames & lint fixes.

* Post-merge fixes.

* Add tests.

* KDocs + exemptions.

Also, clean up polynomial sorting.

* Lint fixes.

* Post-merge fixes.

Also, mark methods/classes that need tests.

* Add extension tests.

* Add classifier tests.

* Use more intentional epsilons for float comparing.

* Treat en-dash as a subtraction symbol.

* Add explicit platform selection for paramerized.

This adds explicit platform selection support rather than it being
automatic based on deps. While less flexible for shared tests, this
offers better control for tests that don't want to to use Robolectric
for local tests.

This also adds a JUnit-only test runner, and updates MathTokenizerTest
to use it (which led to an almost 40x decrease in runtime).

* Exemption fixes.

Also, fix name for the AndroidJUnit4 runner.

* Remove failing test.

* Fix unary expression precedence.

Also, use ParameterizedJunitTestRunner for MathExpressionParserTest.

* Fixes & add more test cases.

* Post-merge fixes & test changes.

Also, update RealExtensionsTest to use the faster JUnit runner.

* Use utility directly in LaTeX tests.

* Post-merge fixes.

Also, update ExpressionToComparableOperationConverterTest to use the
fast JUnit-only runner.

* Post-merge fixes.

Also, update PolynomialExtensionsTest to use fast JUnit-only runner.

* Post-merge fixes.

Also, update float interval per new tests.

* Lint & other check fixes.

* Replace deprecated term.

* Post-merge fixes.

* Add full test suites for alg exp classifiers.

* Lint & static check fixes.

* Fix test on Gradle.

* Fix test for Gradle.

* Add tests for math equations.

And, post-merge fixes.

* Static check & lint fixes.

* Post-merge fixes.

Verified CI checks & all unit tests are passing.

* Split up tests.

Also, adds dedicated BUILD.bazel file for new test.

* Add missing test in Bazel, and fix it.

* Correct order for genrule.

* Add full test suite.

* Clean up + KDocs + exemption.

* Lint fixes.

* Post-merge fix.

* Cache KotliTeX renders.

Directly rendering LaTeX through KotliTeX is way too slow, so this
introduces a custom flow through Glide that computes a PNG for the LaTeX
on a background thread and then caches it as part of Glide's cache to
speed up re-renders of the LaTeX. We may need to manage/prune the cache
over time, but for now we'll just rely on Glide's internal behaviors.

This also documents some new tests that should be added, but it's not
comprehensive.

* Add tests, docs, and exemptions.

* Update to fixed version of KotliTeX.

The newer version correctly computes the bounds for rendered LaTeX.

* Lint fixes.

* Add new dependency licenses.

This isn't done yet (some of the licenses still need to be fixed).

* Fix license links.

Note that the kxml one was tricky since its Maven entry says it's
licensed under BSD and CC0 1.0, and its SourceForge link says the same
plus LGPL 2.0. However, the source code and GitHub version of the
project license it under MIT, and this seems to predate the others so
it seems like the most correct license to use in this case and the one
that we're using to represent the dependency.

* Fix Gradle build.

This uses a version of KotliTeX that builds correctly on Jitpack for Gradle,
and fixes the StaticLayout creation to use an alignment constant that
builds on Gradle (I'm not sure why there's a difference here between
Gradle & Bazel, but the previous constant isn't part of the enum per
official Android docs).

* Create the math drawable synchronously.

This requires exposing new injectors broadly in the app since the math
model loader doesn't have access to the dependency injection graph
directly.

* Remove new deps from Maven list.

They were incorrectly pulled in by KotliTeX.

* Add argument partitioning.

This fixes cases where argument calls may be very large and fail to
execute due to exceeding system limitations.

* Make allowance for empty cases to fix tests.

These tests correspond to real scenarios.

* Lint fixes.

* Address reviewer comment.

Clarifies the documentation in the test runner around parameter
injection.

* Fix broken build.

* Fix broken build post-merge.

* Fix broken post-merge classifier.

* Address reviewer comment.

* Post-merge build fixes.

* Post-merge build fixes for new classifiers.

* Post-merge build fixes.

* Correct reference document link.

* Ensure LaTeX isn't stretched or cut-off.

The comments in-code have much more specifics on the approach.

* Add and fix missing test (was broken on Gradle).

* Refactor AsyncResult into a sealed class.

This also introduces an AsyncResultSubject, and more or less fully fixes
issue #3813 in all tests.

This is a cherry-pick from the fix-progress-controller-deadlock branch
since it ended up being quite large (it made more sense to split it into
a pre-requisite PR).

Conflicts:
	app/src/main/java/org/oppia/android/app/player/state/testing/StateFragmentTestActivityPresenter.kt
	domain/src/main/java/org/oppia/android/domain/exploration/ExplorationProgressController.kt
	domain/src/main/java/org/oppia/android/domain/question/QuestionAssessmentProgressController.kt
	domain/src/test/java/org/oppia/android/domain/exploration/lightweightcheckpointing/BUILD.bazel
	testing/src/main/java/org/oppia/android/testing/data/DataProviderTestMonitor.kt
	utility/src/main/java/org/oppia/android/util/data/DataProviders.kt

* Post-merge fixes and updates for consistency.

* Post-merge fixes.

* TODO has been addressed.

* Fix documentation & add tests.

* Lint fixes.

* Fix gradle tests.

I've verified in this commit that all Gradle tests build & run locally
(at least on Robolectric).

* Post-merge fix.

* More post-merge fixes.

* Fix TODO comment.

* Post-merge lint fixes.

* Post-merge fix.

* Fix exploration routing issue.

The underlying problem was that the PR inadvertently changed the
behavior of comparing two results wherein results of different times
would be considered different and be re-delivered (which happened to
break exploration routing, and likely a variety of other places).

This introduces a new method for checking equivelance rather than
confusingly assuming that users of AsyncResult don't care about the
result's time during comparison checking.

New tests have been added to verify the functionality works as expected
(and fails when expected), and I manually verified that the exploration
routing issue was fixed. More details on the specific user-facing issue
will be added to the PR as a comment.

* Update KotliTeX version.

This version doesn't have debug drawing enabled.
@Broppia Broppia added issue_type_dev_initiated Impact: Low Low perceived user impact (e.g. edge cases). labels Jun 2, 2022
@BenHenning BenHenning added Issue: Needs Clarification Indicates that an issue needs more detail in order to be able to be acted upon. Z-ibt Temporary label for Ben to keep track of issues he's triaged. labels Sep 16, 2022
@seanlip seanlip added enhancement End user-perceivable enhancements. and removed issue_type_infrastructure labels Mar 28, 2023
@MohitGupta121 MohitGupta121 added the Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet. label Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement End user-perceivable enhancements. Impact: Low Low perceived user impact (e.g. edge cases). Issue: Needs Clarification Indicates that an issue needs more detail in order to be able to be acted upon. Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet. Z-ibt Temporary label for Ben to keep track of issues he's triaged.
Projects
[Team] Developer Workflow & Infrastructure - Android
Status: Todo
Milestone
No milestone
Development

No branches or pull requests
4 participants
@seanlip @BenHenning @MohitGupta121 @Broppia