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

Migrate codebase from Dagger to Hilt #505

Merged
merged 21 commits into from
Jun 30, 2020
Merged

Conversation

dturner
Copy link
Contributor

@dturner dturner commented Jun 27, 2020

Hilt is a new DI framework for Android built on top of Dagger. It provides the same features in a simplified way by removing much of the heavy lifting associated with Dagger.

This PR migrates the Ground codebase from Dagger and dagger.android to Hilt. I followed the migration guide here and had some help from Manuel Vivo.

Here's the summary of work:

  • Migrate from GndApplicationComponent to Hilt's ApplicationComponent
  • Use @AndroidEntryPoint to install activities and fragments into Hilt's corresponding components
  • Remove dagger.android dependencies
  • Migrate GndWorkerFactory to Jetpack's HiltWorkerFactory

@dturner dturner marked this pull request as draft June 27, 2020 10:18
@dturner dturner marked this pull request as ready for review June 27, 2020 10:43
Copy link
Collaborator

@gino-m gino-m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautiful! Finally, an opinionated standard on how to structure DI in Android. Taking a look now; in the meantime, please run gradle checkstyle, you'll see the following error:

Already have image (with digest): gcr.io/cloud-builders/gsutil
Reports uploaded to https://console.cloud.google.com/storage/browser/artifact_bucket/hilt-migration-0be56337-08b4-4bda-94e5-6667ee4e9522/
[ant:checkstyle] [ERROR] /workspace/gnd/src/main/java/com/google/android/gnd/ui/offlinearea/viewer/OfflineAreaViewerViewModel.java:49: Line is longer than 100 characters (found 119). [LineLength]

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':workspace:gnd:checkstyle'.
> Checkstyle rule violations were found. See the report at: file:///workspace/gnd/build/reports/checkstyle/checkstyle.html
  Checkstyle files with violations: 1
  Checkstyle violations by severity: [error:1]


* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights.

* Get more help at https://help.gradle.org

BUILD FAILED in 42s
/workspace/gnd/src/test/java/com/google/android/gnd/persistence/local/LocalDataStoreTest.java:26: error: cannot find symbol
import com.google.android.gnd.inject.DaggerTestComponent;
                                    ^
  symbol:   class DaggerTestComponent
  location: package com.google.android.gnd.inject
/workspace/gnd/src/test/java/com/google/android/gnd/persistence/local/LocalDataStoreTest.java:204: error: cannot find symbol
    DaggerTestComponent.create().inject(this);
    ^
  symbol:   variable DaggerTestComponent
  location: class LocalDataStoreTest
2 errors

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':gnd:compileDebugUnitTestJavaWithJavac'.
> Compilation failed; see the compiler error output for details.

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights.

* Get more help at https://help.gradle.org

BUILD FAILED in 49s

Copy link
Collaborator

@gino-m gino-m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Migration LGTM! Have you tested all flows end to end to ensure there are no regressions after this change?

@dturner
Copy link
Contributor Author

dturner commented Jun 28, 2020

Many thanks for the quick review. Is there an exhaustive list of flows somewhere which I can use to manually test? This would be v.useful for writing the UI/integration tests as well.

@shobhitagarwal1612
Copy link
Member

Thanks @dturner! I think the build is failing because of unit tests (looking at the above stack-trace).

@shobhitagarwal1612
Copy link
Member

Thanks for the massive cleanup. I tried running the app and thinks are working fine except when trying to sync data remotely.

The below crash happens after creating a new observation, filling some info and clicking "save" button.

    Process: com.google.android.gnd, PID: 27158
    java.lang.RuntimeException: LiveData does not handle errors. Errors from publishers should be handled upstream and propagated as state
        at androidx.lifecycle.LiveDataReactiveStreams$PublisherLiveData$LiveDataSubscriber$1.run(LiveDataReactiveStreams.java:264)
        at android.os.Handler.handleCallback(Handler.java:883)
        at android.os.Handler.dispatchMessage(Handler.java:100)
        at android.os.Looper.loop(Looper.java:214)
        at android.app.ActivityThread.main(ActivityThread.java:7356)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:492)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:930)
     Caused by: java.lang.IllegalStateException: WorkManager is not initialized properly.  The most likely cause is that you disabled WorkManagerInitializer in your manifest but forgot to call WorkManager#initialize in your Application#onCreate or a ContentProvider.
        at androidx.work.WorkManager.getInstance(WorkManager.java:162)
        at com.google.android.gnd.GndApplicationModule.workManager(GndApplicationModule.java:66)
        at com.google.android.gnd.GndApplicationModule_WorkManagerFactory.workManager(GndApplicationModule_WorkManagerFactory.java:27)
        at com.google.android.gnd.DaggerGndApplication_HiltComponents_ApplicationC.getWorkManager(DaggerGndApplication_HiltComponents_ApplicationC.java:396)
        at com.google.android.gnd.DaggerGndApplication_HiltComponents_ApplicationC.access$5100(DaggerGndApplication_HiltComponents_ApplicationC.java:185)
        at com.google.android.gnd.DaggerGndApplication_HiltComponents_ApplicationC$SwitchingProvider.get(DaggerGndApplication_HiltComponents_ApplicationC.java:1622)
        at com.google.android.gnd.persistence.sync.DataSyncWorkManager.getWorkManager(DataSyncWorkManager.java:69)
        at com.google.android.gnd.persistence.sync.DataSyncWorkManager.enqueueSyncWorkerInternal(DataSyncWorkManager.java:61)
        at com.google.android.gnd.persistence.sync.DataSyncWorkManager.lambda$enqueueSyncWorker$0$DataSyncWorkManager(DataSyncWorkManager.java:53)
        at com.google.android.gnd.persistence.sync.-$$Lambda$DataSyncWorkManager$JA1fPsDKyzyNOdIabshhlKX2jqE.run(Unknown Source:4)
        at io.reactivex.internal.operators.completable.CompletableFromRunnable.subscribeActual(CompletableFromRunnable.java:36)
        at io.reactivex.Completable.subscribe(Completable.java:2309)
        at com.akaita.java.rxjava2debug.extensions.CompletableOnAssembly.subscribeActual(CompletableOnAssembly.java:39)
        at io.reactivex.Completable.subscribe(Completable.java:2309)
        at io.reactivex.internal.operators.completable.CompletableAndThenCompletable$SourceObserver.onComplete(CompletableAndThenCompletable.java:67)
        at com.akaita.java.rxjava2debug.extensions.CompletableOnAssembly$OnAssemblyCompletableObserver.onComplete(CompletableOnAssembly.java:71)
        at io.reactivex.internal.operators.completable.CompletableAndThenCompletable$NextObserver.onComplete(CompletableAndThenCompletable.java:99)
        at com.akaita.java.rxjava2debug.extensions.CompletableOnAssembly$OnAssemblyCompletableObserver.onComplete(CompletableOnAssembly.java:71)
        at io.reactivex.internal.operators.completable.CompletableSubscribeOn$SubscribeOnObserver.onComplete(CompletableSubscribeOn.java:79)
        at com.akaita.java.rxjava2debug.extensions.CompletableOnAssembly$OnAssemblyCompletableObserver.onComplete(CompletableOnAssembly.java:71)
        at io.reactivex.internal.operators.completable.CompletablePeek$CompletableObserverImplementation.onComplete(CompletablePeek.java:115)
        at com.akaita.java.rxjava2debug.extensions.CompletableOnAssembly$OnAssemblyCompletableObserver.onComplete(CompletableOnAssembly.java:71)
        at io.reactivex.internal.operators.completable.CompletableFromCallable.subscribeActual(CompletableFromCallable.java:47)
        at io.reactivex.Completable.subscribe(Completable.java:2309)
        at com.akaita.java.rxjava2debug.extensions.CompletableOnAssembly.subscribeActual(CompletableOnAssembly.java:39)
2020-06-29 19:38:39.930 27158-27158/com.google.android.gnd E/AndroidRuntime:     at io.reactivex.Completable.subscribe(Completable.java:2309)
        at io.reactivex.internal.operators.completable.CompletablePeek.subscribeActual(CompletablePeek.java:51)
        at io.reactivex.Completable.subscribe(Completable.java:2309)
        at com.akaita.java.rxjava2debug.extensions.CompletableOnAssembly.subscribeActual(CompletableOnAssembly.java:39)
        at io.reactivex.Completable.subscribe(Completable.java:2309)
        at io.reactivex.internal.operators.completable.CompletableSubscribeOn$SubscribeOnObserver.run(CompletableSubscribeOn.java:64)
        at io.reactivex.Scheduler$DisposeTask.run(Scheduler.java:578)
        at io.reactivex.internal.schedulers.ScheduledRunnable.run(ScheduledRunnable.java:66)
        at io.reactivex.internal.schedulers.ScheduledRunnable.call(ScheduledRunnable.java:57)
        at java.util.concurrent.FutureTask.run(FutureTask.java:266)
        at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:301)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
        at java.lang.Thread.run(Thread.java:919)
     Caused by: com.akaita.java.rxjava2debug.extensions.RxJavaAssemblyException
     Caused by: com.akaita.java.rxjava2debug.extensions.RxJavaAssemblyException
     Caused by: com.akaita.java.rxjava2debug.extensions.RxJavaAssemblyException
     Caused by: com.akaita.java.rxjava2debug.extensions.RxJavaAssemblyException
     Caused by: com.akaita.java.rxjava2debug.extensions.RxJavaAssemblyException

gnd/build.gradle Outdated Show resolved Hide resolved
gnd/build.gradle Outdated Show resolved Hide resolved
gnd/build.gradle Outdated Show resolved Hide resolved
gnd/src/main/AndroidManifest.xml Show resolved Hide resolved
@shobhitagarwal1612
Copy link
Member

@dturner Is there a specific reason for creating separate TestSchedulersModule & TestLocalDatabaseModule for unit tests? Can we combine them into a single module?

Secondly, we need to specifically uninstall module (as done in LocalDataStoreTest). Is this the only possible way to do that? The motivation behind this question is to keep configuration of tests separate and prevent duplicate code when new tests arrive.

@dturner
Copy link
Contributor Author

dturner commented Jun 30, 2020

@shobhitagarwal1612 great questions.

Is there a specific reason for creating separate TestSchedulersModule & TestLocalDatabaseModule for unit tests? Can we combine them into a single module?

The original code had a separate testing application and much simplified Dagger dependency graph with a single module which injected the testing dependencies (the only real difference I can see is that the Room database allows queries on the main thread)

The strategy for testing in Hilt is different. You start with your production ApplicationComponent and uninstall modules which need to be replaced for testing. With this in mind I moved the bindings which needed to be replaced into separate modules (SchedulersModule and LocalDatabaseModule) so they could be uninstalled.

We could have a single "testing" module which satisfies the dependencies of both the above modules. I kept them separate to keep a 1:1 mapping between uninstalled modules and installed test modules so it's clear what's being swapped.

Secondly, we need to specifically uninstall module (as done in LocalDataStoreTest). Is this the only possible way to do that?

Yes, this is the Hilt way. There are downsides to this approach. Relevant issues/discussions:

google/dagger#1896
google/dagger#1923

In this case I think the bigger question is: can we avoid Dagger/Hilt/Robolectric entirely for unit tests? We could avoid these extra dependencies/complexity by changing our codebase to support manual DI. In this case we're testing RoomLocalDataStore which is currently using field injection when it seems like it could use constructor injection.

The architecture samples project has a good example of unit testing a local data store using manual DI.

I'd like to investigate this in a followup PR and would love to get your thoughts.

@dturner
Copy link
Contributor Author

dturner commented Jun 30, 2020

@gino-m ok to merge? Just need your approval.

Copy link
Collaborator

@gino-m gino-m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🥇

@gino-m gino-m merged commit fb29556 into google:master Jun 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants