square / dagger

A fast dependency injector for Android and Java.
https://square.github.io/dagger/
Apache License 2.0
7.31k stars 3.07k forks source link

Requiring 'injects' make unit-test injection cumbersome #269

Closed diwakergupta closed 10 years ago

diwakergupta commented 11 years ago

We use junit4 for tests and most of our tests require injection for some fields. With Guice, we can simply have a TestRule that injects the members:

// module is a test module that overrides production settings
Guice.createInjector(module).injectMembers(target);

Unfortunately, Dagger requires modules to declare entry points via the 'injects' clause and there doesn't seem any way to relax that. This effectively makes it impossible to use a Rule based approach like above. Instead, each test has to specify it's own module:

class MyTest {
  @Module(injects = MyTest.class, includes = TestModule.class)
  static class MyTestModule {
  }
}

This results in a lot of boilerplate code across all tests. I understand the 'injects' clause allows Dagger to flag errors up front, but there should be an easier way to inject members in unit tests.

cgruber commented 11 years ago

It's not just "allowing flagging of errors" - it's the basis for roots of the full graph analysis. I think there might be value in finding some testable approach.

What I would say is, firstly, consider mocking out the injector for all but end-to-end integration tests, and second, in an actual unit test you shoudn't ever be creating an injector. You should be testing your components in isolation.

But for full-scale integration tests, I can see the value of being able to assert state on arbitrary objects to more finely tune error detection.

diwakergupta commented 11 years ago

While this is a perfectly valid ideological standpoint, there are plenty of practical use-cases for using injectors in a unit test. Mocks are incredibly useful, but are not suited everywhere. As one example, we use @Named bindings to generate random port numbers for tests. What you choose to call "unit" vs. "integration" test is often subjective -- in my case, many unit tests depend on some other components for testing, hence the need for injection.

swankjesse commented 11 years ago

Agreed that injects is annoying. I'm planning to do something different for Dagger 2.0, when we can do full-application analysis.

cgruber commented 11 years ago

Greg and I are exploring some alternatives as well around entry points, including some nice pre-linking that should remove all loading except the first load of the entry point's adapter. I think we need to throw together some shared docs on ideas for how to approach it, and hash out what 2.0 might look like.

diwakergupta commented 11 years ago

@swankjesse, @cgruber awesome, look forward to it! I guess it's premature but I'll still ask -- any timeline for a 2.0 release? :-)

cgruber commented 11 years ago

now() + N minutes. :) No idea as we don't even know what we want in it.

jloisel commented 11 years ago

From my point of view, unit tests should test components isolated from each other by using mocks, or tracking down a test failure can really become painful.

We do test objects being injected by providing instances manually, this avoids that unit tests to fail when injection is not working.

We have tests on modules checking that wiring is working fine. These are the only tests we have that use injection. This allows use to quickly figure out if our code is failing because of wiring or broken code inside an object.

Objects should be simple enough to be mocked easily. We try to have mostly single method interfaces with obvious contracts.

Maybe, there should be something dedicated to integration unit tests in Dagger allowing to relax some settings, although this is pretty dangerous as it may be misused in production code.

diwakergupta commented 11 years ago

On Mon, Jun 24, 2013 at 3:06 AM, Jerome notifications@github.com wrote:

From my point of view, unit tests should test components isolated from each other by using mocks, or tracking down a test failure can really become painful.

This is a fine point of view but in practice, I've seen most code bases that use injection also relying on injection for testing. Existence of projects such as GuiceBerry (https://code.google.com/p/guiceberry/) and Jukito (https://github.com/ArcBees/Jukito) further support my anecdotal observations.

Anyways, just wanted to raise this so we can have a discussion. If the Dagger community doesn't find this useful, feel free to close as won't fix.

jloisel commented 11 years ago

There are also libraries like PowerMock (http://code.google.com/p/powermock/) but that does not mean it's a good practice to use them. I understand your point of view, working with legacy code often requires to make compromise. I'm open to having something in Dagger satisfying both worlds.

christopherperry commented 10 years ago

You only run into this problem where you aren't doing proper dependency injection (passing dependencies via constructor). If you are testing your class in isolation, and passing all dependencies in your constructor then you have zero need for dagger to be involved at all in your test class.

Sometimes though, it's unavoidable because you don't have control over the creation of the class (i.e. you can't provide your own constructor to do proper DI). Activity, Fragment, Service, View, etc are all problematic in this way, so these are really the only classes where you are forced into using field injection and Dagger in your test class.

cgruber commented 10 years ago

In these cases, however, you can provide a test module (often with overrides=true) that narrowly defines the types you need to pull out to examine in the test, or even better, a TestEntryPoint class that simply injects into itself bits of graph state for the test.

christopherperry commented 10 years ago

@cgruber That's exactly what I'm doing, and I've got something that hooks into the Activity/Fragment creation process to allow me to define mocked behaviors at the right time (I'm using Robolectric.buildActivity().create().etc().etc() to start my Activities in test, which presents timing issues for defining mocked behaviors for things that run in onCreate() etc). I could share if you guys would like, I'm open to comments/suggestions.

JakeWharton commented 10 years ago

injects= is dead in v2 and there's recommendations above for workarounds for v1.