Hilt: test Android components in isolation

francescocervone commented 4 years ago

Use case

I have this Gradle modules configuration:

            app
             |
             A
           /   \
          B     C
        /  \    / \
       D    E  F   G

Each one of these Gradle modules is an implementation project('...') dependency.

Each one of these Gradle modules has a Hilt module:

@Module
@InstallIn(ApplicationComponent::class)
internal object Module[X] { ... }
// for X in (A, B, C, D, E)

Some of these Dagger modules depend on values provided by some other Gradle modules included by app, or by app itself.

I have a MainActivity in the Gradle module A with some dependencies.

@AndroidEntryPoint
class MainActivity: AppCompatActivity() {
  @Inject lateinit var dep1: Dep1
  @Inject lateinit var dep2: Dep2
}

I want to test my MainActivity in isolation providing Dep1 and Dep2 manually in the test. Something like this:

@HiltAndroidTest
@UninstallModules(...)
class MainActivityTest {
  @get:Rule val hiltRule = HiltAndroidRule(this)
  @BindValue @JvmStatic val dep1 = mock<Dep1>()
  @BindValue @JvmStatic val dep2 = mock<Dep2>()
  @Before fun setup() { hiltRule.inject() }
}

As far as I understand, the only way I can do this is by uninstalling all dagger modules of the generated graph of Gradle module A. Without uninstalling all modules, dagger will not be able to resolve the dependencies provided by app unless I provide them in my test with @BindValue (which would be an overkill).

I should do something like this:

@UninstallModules(
  ModuleA::class, 
  ModuleB::class, 
  ModuleC::class, 
  ModuleD::class, 
  ModuleE::class, 
  ModuleF::class, 
  ModuleG::class
)

Is that right?

We have a pretty large project and this is a very common use case.

Sometimes we don't have access to the dagger modules we want to uninstall, they are in the classpath but they are not visible from the source code for several reasons:

Dagger modules can be internal
Some modules are an implementation dependency of another implementation dependency (see D, E, F and G)

We could make all modules public and all Gradle dependencies api but that wouldn't be a clean solution.

For large projects manually checking all indirect dagger modules to uninstall them would be really painful.

Is there a non-painful way to test Android components in isolation for this use case?

Does it make sense a @UninstallAllModules annotation that automatically clears the entire dagger graph? This looked the only way to solve this issue to me, but since I still have a little experience with Hilt, it could sound totally stupid.

This is the way we will probably proceed in our private project to solve this issue, but an "official" API or guidance would be highly appreciated 😄

We solved this issue with Dagger-Android in a really easy way. This is the only reason that stops us from migrating activities/fragment injection to Hilt.

Chang-Eric commented 4 years ago

A couple of clarifying questions:

Without uninstalling all modules, dagger will not be able to resolve the dependencies provided by app unless I provide them in my test with @BindValue (which would be an overkill).

I'm a bit confused why dependencies in app would be causing you problems. Isn't your test in gradle module A so it shouldn't have any dependencies on app? I understand Dep1 and Dep2 normally are provided from app, but I would assume that the modules in app aren't even pulled into the tests for module A because there's no dependency on app. So in this case you would just easily replace those with mocks as you wanted and then not have any dependency problems.

Also, does gradle module A actually use dependencies from B -> G? I don't quite understand why you want to uninstall all of B -> G if so. In general, we suggest trying to use the real bindings as much as possible (https://dagger.dev/hilt/testing-philosophy). So I'm not quite sure why you need to uninstall everything vs just the few things that need to be replaced.

francescocervone commented 4 years ago

I will try to create an example project.

I don't quite understand why you want to uninstall all of B -> G

I don't want to, but the test don't compile because Hilt is not able to resolve all the dependencies of the graph. Hilt tries to build a dependency graph with A as root, but there are some provided dependencies of B -> G that depends on objects provided by app or its own submodules. The fact that A-G don't depend on app doesn't mean that they cannot depend on some object provided by app.

A simple example:

db-api module

interface Database

db-impl module

class DatabaseImpl(...): Database

Modules don't need to depend on db-impl. They just need to depend on db-api, but the Database instance is provided by app.

Chang-Eric commented 4 years ago

Right, but what I mean is that, those things in B -> G are unused in your test? In a sense, the API of the module A is that in order to use module A, you have to provide some bindings like a Database. So if you are testing the A module, it makes sense you have to provide those Database items. Whether or not Database is used in B -> G (which are implementation details to the user of A), they add to this API.

If the API of A in that sense is too large or wide for all of the use cases (e.g. module A does several things each of which only need a small part of those bindings), then I think you may want to consider splitting A into more separate libraries/modules that aren't tied together.

francescocervone commented 4 years ago

Right, but what I mean is that, those things in B -> G are unused in your test? In a sense, the API of the module A is that in order to use module A, you have to provide some bindings like a Database. So if you are testing the A module, it makes sense you have to provide those Database items. Whether or not Database is used in B -> G (which are implementation details to the user of A), they add to this API.

You are right. Anyway, the idea of implementation and api configurations (or internal and public modifiers) is that we want to hide some implementation details. In this case Database is not used by A or by its direct dependencies. It's used by the dependencies of the dependencies of A. So, the implementation details are actually "leaked" by the dagger graph just because they are in the classpath. I don't think this is some Hilt's deficiency, but I'm just saying that it's very likely to have a use case like this in a large project that uses the dependency inversion principle.

I just want to test in isolation an android component, just like it was a unit test and Hilt makes it really hard. Dagger-Android instead is the exact opposite: there is no dependency graph at all in the specific module 😄 . With dagger-android we can write the "integration" test in app (where the entire graph is available), and the "unit" test in the specific module.

Anyway, Hilt's testing philosophy is pretty explanatory, so I would understand if this is a case you don't want to cover.

francescocervone commented 4 years ago

it makes sense you have to provide those Database items

It's just weird that you have to provide a dependency not used by the class under test and in order to do this you have to replace some implementation with api and some internal with public for testing purposes.

Chang-Eric commented 4 years ago

I think this is where I get confused though. In this example, is Database actually unused by the test? If it is, the only reason I can think of that you would be forced to provide it is if it is being pulled in through some other entry point. Like you have another activity or fragment that is unused in the test that requires the Database binding.

In these cases, Hilt/Dagger doesn't really know which activity or fragment you might try to use since they are entry points and usage is determined at runtime. It needs to potentially throw errors at you at compile time, so that's why this case becomes a little difficult. For this reason I often try to advise against bundling together too many unrelated/loosely related activities/fragments together. Not sure if this is your issue though?

francescocervone commented 4 years ago

In this example Database is not referenced by the test or the object under test.

I'm also pretty sure that there isn't some other entry point in the graph built with A as root since this is the very first @AndroidEntryPoint annotation I added.

There are just a bunch of @Module/InstallIn(ApplicationComponent) annotated modules.

Chang-Eric commented 4 years ago

Hm, can you trace the missing binding error message to the usage to double check? In Dagger, if you have a module with say an @Provides Foo provideFoo(Bar bar), even if Bar is not provided anywhere, if Foo is unused there won't be any error.

Chang-Eric commented 4 years ago

What I meant by "unused" above btw is unused even transitively.

francescocervone commented 4 years ago

> Task :feature-splash:kaptDebugAndroidTestKotlin
error: cannot access MyQualifier
  class file for com.example.MyQualifier not found
  Consult the following stack trace for details.
  com.sun.tools.javac.code.Symbol$CompletionFailure: class file for com.example.MyQualifier not found
warning: The following options were not recognized by any processor: '[dagger.hilt.disableModuluperclassValidation, kapt.kotlin.generated]'
> Task :feature-splash:compileDebugAndroidTestJavaWithJavac FAILED
/.../build/generated/source/kapt/debugAndroidTest/com/example/SplashActivityTest_ComponentDataHolder.java:18: error: cannot find symbol
    return new TestComponentData(false, testInstance -> injectInternal((SplashActivityTest) testInstance), Collections.emptySet(), Collections.emptySet(), (modules, testInstance, autoAddModuleEnabled) -> DaggerSplashActivityTest_HiltComponents_SingletonC.builder()
                                                                                                                                                                                                            ^
  symbol:   variable DaggerSplashActivityTest_HiltComponents_SingletonC
  location: class SplashActivityTest_ComponentDataHolder
1 error

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':feature-splash:compileDebugAndroidTestJavaWithJavac'.
> Compilation failed; see the compiler error output for details.

Following the original example:

MyQualifier is a qualifier defined in D.
MyQualifier is used by a @Module object ModuleB defined in B to provide a dependency (@Provides @MyQualifier fun dependency()).
SplashActivityTest is my test for SplashActivity
SplashActivity is the only class annotated with @AndroidEntryPoint
SplashActivity doesn't use any of the dependencies provided by ModuleB. It uses just one object from B that as the constructor @Inject annotated dependent on one object provided by app.
The only way to fix the issue is adding @UninstallModule(ModuleB::class) even if SplashActivity doesn't use even transitively any dependency provided by ModuleB.

Chang-Eric commented 4 years ago

Ah I'm sorry, I thought the original motivation of the problem was a missing binding issue. This looks like an api/implementation issue.

Basically, right now Hilt requires using api instead of implementation due to the aggregation methods (and in many cases Dagger also requires it if you read the issue linked within there, especially when using subcomponents).

We're looking at ways to fix this, but for now I wouldn't try to uninstall things but instead just use api.

francescocervone commented 4 years ago

Yes, I guess that's the issue. Using api for project D in module B fixes the compilation error in module A tests.

In Dagger, if you have a module with say an @Provides Foo provideFoo(Bar bar), even if Bar is not provided anywhere, if Foo is unused there won't be any error.

About this, thanks for the explanation, I was wrong. I thought Dagger would require the test to provide all unused dependencies because MyQualifier is used to provide an unused dependency (Foo in your example). I automatically thought that the annotation processor was trying to add all the @Module dependencies to the graph of SplashActivityTest but it isn't true. As a matter of fact, I don't need to bind a value for Foo after fixing the error as you suggested.

I still think that having some method to uninstall all modules or at least be able to test an Android component in isolation without requiring me to navigate all the dagger graph and try to understand what are the modules I have to uninstall one by one, is something that would help a lot in general. Doing this job is like doing manually something that Dagger already does automatically. But again we are clearly trying to break the "Testing philosophy" that states we shouldn't test components in isolation.

Chang-Eric commented 4 years ago

Yea in my mind it is really just a reversal of defaults. In Dagger, the default as you mention is modules aren't installed and you need to add modules. In Hilt, we've reversed that and the default is modules are installed and you need to subtract modules. This is because as you pointed out this default works better with the testing philosophy we're trying to support (and hence also works well with our other testing APIs).

I'm going to close this for now since I think adding a way to make modules not installed by default is going to mess with things. Part of the magic of things like @AndroidEntryPoint and @BindValue is that they can generate modules with @InstallIn and they will be picked up for you. Even if it were just an option to switch the default, it'd start to get painful if those things didn't work for you unless you manually installed the generated module. And I think it'd start to get complicated if we tried something where certain modules always are included despite the option and other modules instead follow it.

Hope that all makes sense, thanks!

francescocervone commented 4 years ago

Hope that all makes sense

Unfortunately, it doesn't from my point of view. I would be able to unit test in isolation an Android component and Hilt makes it even harder than before.

it'd start to get painful if those things didn't work for you

As you said, "In Dagger, the default as you mention is modules aren't installed", so I'd expect people to be used to write tests in this kind of environment.

Hope that all makes sense

It does from your point of view, I understand you don't want to develop and maintain a feature you don't believe in. This is totally acceptable.

Thank you for your time!

Chang-Eric commented 4 years ago

it'd start to get painful if those things didn't work for you

As you said, "In Dagger, the default as you mention is modules aren't installed", so I'd expect people to be used to write tests in this kind of environment.

Just to clarify, I was referring to new things like @BindValue where people aren't already using it with normal Dagger. In this case, if you did @BindValue Foo foo; and you used this option to flip the default, you'd also somehow need to reference the generated module somewhere like @InstallModulesIntoThisTest(BindValue_GeneratedFooModule.class) (class name is an example, I didn't actually look up what the generated name is). This would probably severely impact the usefulness of such a feature. Same thing would be for other libraries, like you'd probably need to pull in some internal ViewModel Hilt extension modules. So it'd be breaking a lot of encapsulation that Hilt is trying to make easier.

Thanks for understanding my point of view and for this discussion. Even though I closed this and your other pull request, I do really appreciate the contributions and effort.

francescocervone commented 4 years ago

I don't understand why you would need @InstallModulesIntoThisTest(BindValue_GeneratedFooModule.class).

I expected something like this:

@AndroidEntryPoint
class MyActivity: AppCompatActivity() {
  @Inject lateinit var dep1: Dep1
  @Inject lateinit var dep2: Dep2
  ...
}

@HiltAndroidTest
@UninstallAllModules // Instead of @UnistallModules(Module1::class, Module2::class,...)
class MyActivityTest {
  @get:Rule val hiltRule = HiltAndroidRule(this)
  @BindValue val dep1: Dep1 = FakeDep1()
  @BindValue val dep2: Dep2 = FakeDep2()
  ...
}

You think this is impossible to implement in Hilt?

Chang-Eric commented 4 years ago

In your scenario, are you only able to add bindings to your test via @BindValue?

It isn't impossible, but what I'm saying is that there are more modules in there that you do need for that test that aren't seen. There are Hilt internal modules (https://github.com/google/dagger/tree/master/java/dagger/hilt/android/internal/modules), Hilt generated modules, @BindValue generated modules, ViewModel extension modules, etc that are all technically necessary for your test. We would have to create a complex set of rules to say which modules are uninstalled and which ones aren't.

francescocervone commented 4 years ago

In your scenario, are you only able to add bindings to your test via @BindValue?

That's the idea. It's exactly like when you have a class with all its dependencies in its constructor. The only difference is that in Android components you don't have a constructor. Given this assumption you just have to provide the dependencies you need in the Activity in some way and @BindValue looks a good candidate to me. I figured there were some hidden dagger modules, but I just thought in case there was the @UninstallModules annotation, only @BindValue generated modules would be picked by dagger.

With this idea, do you still think we need any other module to make dagger/hilt work? It should be just like instantiating an object under test with the fakes/mocks you want.

Of course you have the knowledge of how Hilt works under the hood and only you will know if this is feasible and how much is expensive.

Chang-Eric commented 4 years ago

I think it'd be very difficult to set the expectations correctly on what should or shouldn't work in this case and it isn't even clear to me where that line should be drawn, especially with extensions like ViewModels or any other extension library someone might create. I also want @BindValue to be basically a syntactic sugar as opposed to anything different about using a module to keep things simpler for people so I wouldn't want to special case it.

I think I understand more what you are trying to do and I think I agree this is where it starts to involve a testing philosophy clash. Thanks for realizing that sooner than I did and still continuing to explain it to me =)

google / dagger