Fix part of #4: Introduce domain module #64
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
into both an activity and a fragment.
The domain module includes a user app history controller that provides instances of a new AsyncResult interface that's meant to be a potential bridge between Kotlin coroutines and LiveData. The exact design of how this should work needs to be determined as part of #6. This also includes a new testsupport module that's required due to robolectric/robolectric#4736. This is supporting a new test for the app history controller that leverages an AndroidX activity scenario to test the live data. Note that the test does not yet work since there's a race condition between the LiveData's coroutines completing and the test continuing to verify the state of the activity. This needs to be resolved, likely by waiting for test visual elements to change based on the LiveData result. Additional tests need to be added for other new components, and some slight cleaning up may be necessary.
for production configurations.
LiveData + kotlin coroutine system. This introduces a custom LiveData to mediate between Kotlin coroutines in a way that allows tests to actually ensure the coroutines resolve quickly, and ensure lifecycle safety through standard LiveData mechanisms. However, this doesn't actually fix the underlying tests (which have been changed in a number of ways in this commit in an effort to try and get them to pass). The kotlinx coroutine deps were downgraded to work around a NoSuchMethodError (see Kotlin/kotlinx.coroutines#1331). After introducing a runBlockingTest structure and updating the user app histoyr controller to use the test context, the operations seem to work correctly. However, one job is hanging which is causing the test to still fail (it appears to be deadlocked--no known amount of time allows it to resolve). This new NotifiableAsyncLiveData also appears to break the production behavior, as well. The LiveData result is no longer being observed by the data binding transformation function, so it seems the coroutines aren't being executed, aren't completing, or something is deadlocking somewhere. Further investigation is needed.
binder had the view model set properly. Fix resource binding issues in Robolectric tests by ensuring binary resources are provided to Robolectric. Fix the controller tests not working by introducing a temporary, custom CoroutineLiveData that ensures no long-living jobs continue running after the live data is completed.
build gradle files. Add missing newlines at end of various files.
|
Hello @seanlip @veena14cs @BenHenning @droidizer , This PR LGTM. Waiting for other reviewers to have a look and then we can merge this in master branch. |
veena14cs
approved these changes
Aug 22, 2019
|
@BenHenning Should I "Squash and merge" this PR? |
6 tasks
seanlip
approved these changes
Aug 22, 2019
.idea/codeStyles/Project.xml
Outdated
| <indentOptions> | ||
| <option name="INDENT_SIZE" value="2" /> | ||
| <option name="CONTINUATION_INDENT_SIZE" value="4" /> | ||
| <option name="TAB_SIZE" value="2" /> | ||
| </indentOptions> | ||
| </codeStyleSettings> | ||
| </code_scheme> | ||
| </component> | ||
| </component> |
There was a problem hiding this comment.
Note missing newline at EOF, ditto for misc.xml below.
| it.lifecycleOwner = this | ||
| } | ||
|
|
||
| // TODO(BenHenning): Mark that the user opened the app once it's persisted to disk. |
There was a problem hiding this comment.
Could you file an issue for this and use the format TODO(#issueNumber): ...? We are moving to this in the main repo too for all todos, becauseo otherwise they get stale.
|
|
||
| /** Controller for persisting and retrieving the previous user history of using the app. */ | ||
| class UserAppHistoryController(private val coroutineContext: CoroutineContext = EmptyCoroutineContext) { | ||
| // TODO(BenHenning): Persist this value. |
There was a problem hiding this comment.
Ditto for other TODOs, here and elsewhere.
This was referenced Aug 23, 2019
|
LGTM. I think it's fine to merge this? |
|
Yup, thanks all for the review! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is introducing the domain module for Oppia. At a high-level, the domain module includes logic for determining whether the user has already opened the app. This value is not being changed yet by the UI, or being persisted. The rest of this flow will be completed with the remaining work of #4: introducing the data source module.
This PR is has other noteworthy aspects, including:
Note that this PR is introducing a temporary CoroutineLiveData as a child of UserAppHistoryController. This is because the one that comes with AndroidX appears to have a bug that leads to a job not completing, causing the controller's tests to fail. This is a temporary workaround to unblock the module. We shouldn't ship this solution to users, and should work with the AndroidX team to fix the underlying problem and move our implementation back over to the official component rather than using our own.
This PR's history also includes a demonstration for a testsupport module that can contain test-only activities and fragments for testing purposes. The controller's tests were also using these for verifying LiveData correctness in a more production-similar situation, however this additional complexity ended up not being needed. It's quite likely we'll want to test with fake activities/fragments in the future, though, so this PR's history should be used as a reference.
Finally, this PR introduces Groovy syntax and reformats all of the build.gradle files for consistency.