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.

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

Generic data provider infrastructure #6

Closed
Tracked by #3610
BenHenning opened this issue Aug 2, 2019 · 7 comments
Closed
Tracked by #3610

Generic data provider infrastructure #6

BenHenning opened this issue Aug 2, 2019 · 7 comments
Assignees
Labels
Priority: Essential This work item must be completed for its milestone. Z-ibt Temporary label for Ben to keep track of issues he's triaged.

Comments

@BenHenning
Copy link
Sponsor Member

BenHenning commented Aug 2, 2019

There needs to be a general-purpose data provider system that can be used to observe asynchronous data that can possibly change over time. Subscriptions to these data providers should either be only in background threads with no lifecycle dependencies, or should be transformed to LiveData for UI-bound subscriptions. The data provider system should be used for network, database, and cache I/O, as well as in-memory storage. This system will need a solution for combining or transforming data, as is needed by subscribers.

If an existing system exists to satisfy these core requirements, that should be used instead of implementing a custom system.

@BenHenning BenHenning added Type: Improvement Priority: Essential This work item must be completed for its milestone. labels Aug 2, 2019
@BenHenning BenHenning added this to the Proof of concept milestone Aug 2, 2019
@BenHenning BenHenning added this to Triage in Ship Oppia on Android via automation Aug 2, 2019
@BenHenning
Copy link
Sponsor Member Author

This also needs to interop with Retrofit to allow bridging with #5 and downstream domain services.

@BenHenning
Copy link
Sponsor Member Author

This is blocked on #4.

@BenHenning BenHenning moved this from Triage to Blocked in Ship Oppia on Android Aug 2, 2019
@BenHenning BenHenning changed the title Generic data source infrastructure Generic data source infrastructure [Blocked on #4] Aug 12, 2019
@BenHenning BenHenning changed the title Generic data source infrastructure [Blocked on #4] Generic data source infrastructure [Blocked: #4] Aug 12, 2019
BenHenning added a commit that referenced this issue Aug 15, 2019
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.
@BenHenning BenHenning self-assigned this Aug 23, 2019
BenHenning added a commit that referenced this issue Aug 23, 2019
* Migrate ListenableFuture to a Kotlin coroutine, and split HomeActivity
into both an activity and a fragment.

* Introduce the domain and testsupport modules.

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.

* Fix binary build: testsupport should only be depended on for tests, not
for production configurations.

* Further attempts to try and test the asynchronous behavior of the new
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.

* Fix the app not properly binding to the live data by ensuring the data
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.

* Clean up tests (we no longer need a testsupport module for this module
introduction).

* Add test for AsyncResult. Set code style for Groovy files & clean up all
build gradle files. Add missing newlines at end of various files.

* Reformat top-level build.gradle file, as well.

* Address review comments: replace name-based TODOs with issues.

* Address review comments: add EOF new lines.
@BenHenning BenHenning changed the title Generic data source infrastructure [Blocked: #4] Generic data source infrastructure [Blocked: #4, #87] Sep 1, 2019
@BenHenning
Copy link
Sponsor Member Author

This is blocked on #87.

@BenHenning
Copy link
Sponsor Member Author

Also: consider data & error channel elision after configuration changes for LiveData-converted providers.

@BenHenning BenHenning changed the title Generic data source infrastructure [Blocked: #4, #87] Generic data provider infrastructure [Blocked: #4, #87] Sep 1, 2019
@BenHenning
Copy link
Sponsor Member Author

RxJava could probably satisfy all of this, but the initial introduction for #87 seems reasonable so we'll probably continue with that approach for at least the prototype.

@BenHenning BenHenning modified the milestones: Proof of concept, Prototype Sep 1, 2019
@BenHenning
Copy link
Sponsor Member Author

This is now part of the prototype milestone since #87 is introducing an initial version of it for proof-of-concept.

@BenHenning BenHenning changed the title Generic data provider infrastructure [Blocked: #4, #87] Generic data provider infrastructure [Blocked: #87] Sep 5, 2019
@BenHenning BenHenning changed the title Generic data provider infrastructure [Blocked: #87] Generic data provider infrastructure Sep 5, 2019
@BenHenning BenHenning moved this from Blocked to Backlog in Ship Oppia on Android Sep 5, 2019
@BenHenning
Copy link
Sponsor Member Author

The core system has now been in place for a while, so considering this work item done rather than keeping it open. There will be a presentation to the team later explaining how this system works, and how to effectively use it in conjunction with Kotlin Coroutines. Adjustments to the system may be made over the next few months, and there are certainly improvements that will be needed (#89 and #90).

Ship Oppia on Android automation moved this from Backlog to Done Sep 17, 2019
@BenHenning BenHenning added this to Triage in Prototype Oppia on Android via automation Sep 20, 2019
@BenHenning BenHenning moved this from Triage to Done in Prototype Oppia on Android Sep 20, 2019
@BenHenning BenHenning added the Z-ibt Temporary label for Ben to keep track of issues he's triaged. label Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Essential This work item must be completed for its milestone. Z-ibt Temporary label for Ben to keep track of issues he's triaged.
Development

No branches or pull requests

1 participant