Is @InjectMocks selective enough?

[David Wallace]

I've been mulling this over for a while now, and I think it's going to
lead to me making an enhancement request (or just making a patch for
it). The @InjectMocks annotation can perform either constructor
injection or setter/field injection, depending on what constructors
are available. It's trying to be helpful, and do the best injection
that it can.

However, a side effect of that helpfulness is a little bit of
brittleness in tests that use @InjectMocks. If you add or remove a
constructor from a class, a test for that class that uses @InjectMocks
can switch between using constructor injection, and using setter/field
injection. And either way round - for example, removing a constructor
could actually switch ON constructor injection. So you could write a
test with @InjectMocks, hoping to get setter/field injection; and have
the test fail after a constructor is added to or removed from the
class.

But I suspect that if you're using @InjectMocks, you KNOW whether you
want constructor injection or setter/field injection. So I think it
might be good if there were an annotation like @FieldInjectMocks, for
example, which ONLY does setter/field injection, and won't get
sabotaged by someone adding or removing a constructor. I'm not sure
that an annotation like @ConstructorInjectMocks would be quite as
useful, because if you want to use a particular constructor, you can
just CALL the required constructor, and pass the mocks. But it might
be good as a shortcut.

These are just my musings. I don't know how people actually use
@InjectMocks, so I'm not really sure whether my points are useful. I
avoid it in my own tests, because I prefer to see exactly what is/
isn't being mocked, and because I believe that explicitly mocking
whichever fields or constructor arguments need to be mocked leads to a
more stable test.

So does anyone have any thoughts here? What real use cases are there
for @InjectMocks? Would @InjectMocks be improved if you could select
between constructor injection and setter/field injection? What if the
syntax were something like @InjectMocks( strategy=InjectMocks.FIELD ),
instead of an entirely new annotation?

Or will I just be wasting my time if I expend any effort in this area?

[Szczepan Faber]

Hmmm, interesting. I kind of like the idea of a different annotation that would be using constructors and fail eagerly when no matching constructor can be found.

I'm also thinking if we shouldn't make the @InjectMocs work more leniently, e.g:

1. try constructor

2. if cannot be found or args not match, etc. try reflection instead of failing.

Thoughts?

--
You received this message because you are subscribed to the Google Groups "mockito" group.
To post to this group, send email to moc...@googlegroups.com.
To unsubscribe from this group, send email to mockito+u...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/mockito?hl=en.

--
Szczepan Faber
Principal engineer@gradleware
Lead@mockito

[marekdec]

I'd like the Mockito @InjectMocks mechanism to resemble my DI
framework injection service. Like for example I'd probably chose my
required dependencies to be injected via a ctor, and the optional ones
via setters.

I wrote a small utility to test out some popular frameworks to see how
do they behave when asked to inject collaborators.
Here is what happens when all of the dependencies are satisfied.

[report] Vanilla Java
[report] Collaborator injected by constructor IS NULL
[report] Collaborator injected by field IS NULL
[report] Collaborator injected by setter IS NULL

[report] Spring
[report] Collaborator injected by constructor is not null
[report] Collaborator injected by field is not null
[report] Collaborator injected by setter is not null

[report] Guice
[report] Collaborator injected by constructor is not null
[report] Collaborator injected by field is not null
[report] Collaborator injected by setter is not null

[report] CDI
[report] Collaborator injected by constructor is not null
[report] Collaborator injected by field is not null
[report] Collaborator injected by setter is not null

[report] Mockito
[report] Collaborator injected by constructor is not null
[report] Collaborator injected by field IS NULL
[report] Collaborator injected by setter IS NULL

Have a look at: https://github.com/marekdec/framework-injection-tester
The actual report generator can be found at
https://github.com/marekdec/framework-injection-tester/blob/master/framework-injection-tester/src/main/java/com/wordpress/marekdec/injectiontester/InjectionTester.java
The results are quite similar if some dependencies are missing i.e.
Spring, Weld and Guice just fail to boot, Mockito on the other hand,
does not report an error (which is understandable).

As the DI done by a test framework is sort of a controversial feature,
I guess you implemented it this way for some good reason.
Anyhow, why isn't the Mockito DI mechanism just like the others? What
was the rationale for this decision?

Marek

On Feb 1, 3:05 pm, Szczepan Faber <szcze...@gmail.com> wrote:
> Hmmm, interesting. I kind of like the idea of a different annotation that
> would be using constructors and fail eagerly when no matching constructor
> can be found.
>
> I'm also thinking if we shouldn't make the @InjectMocs work more leniently,
> e.g:
> 1. try constructor
> 2. if cannot be found or args not match, etc. try reflection instead of
> failing.
>
> Thoughts?
>

[David Wallace]

Szczepan - what do you mean by "try reflection instead of failing"?

[Szczepan Faber]

>Szczepan - what do you mean by "try reflection instead of failing"?

I guess I thought that it fails if it finds the constructor but cannot assign parameters correctly. Maybe that's not the case - sorry - I wasn't actively following the mockito DI space :)

Cheers!

[Szczepan Faber]

>Anyhow, why isn't the Mockito DI mechanism just like the others? >What was the rationale for this decision?

Seems to be working for most people. Mockito is not DI, it's a mocking framework with very simple DI mechanism. If that mechanism does not work for you (e.g. real use cases) please contribute a fix or use something else to inject mocks :)

Cheers!

[marekdec]

> Seems to be working for most people.

That's fair enough.

I just reread my yesterday's post and it looks like I abbreviated my
thoughts slightly too much.

I actually found a 'real-life' use case, though it's sort of out of
the strict context of testing (namely: an injection mechanism for the
Mockarro thing).

On the testing side, however, I happened to be happy with the manual
injection of the dependencies. To be honest, I had a close look at the
@InjectMocks capability for the first time this week. And my first
conclusion was that in spite of its simplicity, it's a fully featured
injection mechanism (i.e. it supports constructor, field, setter
injection; by type and by name).

My second thought was that it might be really useful if your
application already uses some injection automagic (I guess that those
who use manual dependencies definition using something like XMLs,
would keep on defining them manually in their tests).

So I'll rework my question a bit.
Don't you think it'd be a good idea to try injecting dependencies to
the injection points already defined by the metadata that comes along
with the class under test? [so that it resembles the DI
frameworks :) ]

it might look like: @InjectMocks(toElementsAnnotatedWith=Inject.class)

regards,
Marek

> >https://github.com/marekdec/framework-injection-tester/blob/master/fr...

[David Wallace]

Umm, kind of. If the constructor injection fails, for whatever
reason, then two things can happen.
(1) If there's a default constructor, then it will get used to create
the object; and setter/field injection will be tried next.
(2) If there's no default constructor, Mockito throws an exception.

But (1) is the behaviour that I dislike. Because it means that
whether setter/field injection occurs depends on what selection of
constructors is available, and that makes the behaviour of the test
dependent on unrelated stuff in the class, ie brittle. Whereas now
that @InjectMocks is out there, we can't really change its behaviour,
I'm floating the idea of having some less brittle options.

marekdec's idea is interesting, but I don't know enough about DI
frameworks to assess its merits. Perhaps I need to go away and do
some study, before we progress this issue.

Cheers,
David.

On Feb 3, 10:01 am, Szczepan Faber <szcze...@gmail.com> wrote:
> >Szczepan - what do you mean by "try reflection instead of failing"?
>
> I guess I thought that it fails if it finds the constructor but cannot
> assign parameters correctly. Maybe that's not the case - sorry - I wasn't
> actively following the mockito DI space :)
>
> Cheers!
>

> On Thu, Feb 2, 2012 at 12:56 AM, David Wallace <dmwallace...@gmail.com>wrote:
>
>
>
>
>
> > Szczepan - what do you mean by "try reflection instead of failing"?
>

> > On Thu, Feb 2, 2012 at 3:05 AM, Szczepan Faber <szcze...@gmail.com> wrote:
>
> >> Hmmm, interesting. I kind of like the idea of a different annotation that
> >> would be using constructors and fail eagerly when no matching constructor
> >> can be found.
>
> >> I'm also thinking if we shouldn't make the @InjectMocs work more
> >> leniently, e.g:
> >> 1. try constructor
> >> 2. if cannot be found or args not match, etc. try reflection instead of
> >> failing.
>
> >> Thoughts?
>

> Lead@mockito- Hide quoted text -
>
> - Show quoted text -

[Szczepan Faber]

I don't think there's a good solution here. We could be less brittle by:

1. not trying to create an object, e.g. force the user to create one. This is what we had in past and users wanted the object to be constructed for free by mockito... :)

2. if no matching constructor, try to create object via objenesis and use reflection to populate fields. This might work for 95% of the cases but the remaining 5% will lead to hard-to-debug problems.

Cheers!

[Brice Dutheil]

Hi there,

My second thought was that it might be really useful if your application already uses some injection automagic (I guess that those who use manual dependencies definition using something like XMLs, would keep on defining them manually in their tests).

Mockito's DI is not made to test dependencies, and it doesn't have to. We are in the context of a Unit Test. Testing that every dependencies fills in correctly is an Integration Test. I will never bootstrap a Spring context in a Unit Test. There should be a clear distinction on this matter.

it might look like: @InjectMocks(toElementsAnnotatedWith=Inject.class)

The idea is interesting, however I'm not keen on having the API this way. You already know that Mockito doesn't have any knowledge of DI's framework annotations (Autowired, Inject, etc). Instead I had in mind a way to give the InjectMocks annotation a way to give an injection strategy. This would look like 

@InjectMocks(using = CDIInjection.class)

Guice or for example looks for annotations on constructor parameters. If people want Mockito's injection to use this they could write such a strategy:

@InjectMocks(using = GuiceInjection.class)

The main drawback here is that the strategy itself is not configurable.

Also this approach could be used to force Mockito's own strategy like

@InjectMocks(using = ConstructorInjection.class)

@InjectMocks(using = BeanPropertyInjection.class)

@InjectMocks(using = FieldInjection.class)

But (1) is the behaviour that I dislike. Because it means that whether setter/field injection occurs depends on what selection of constructors is available, and that makes the behaviour of the test dependent on unrelated stuff in the class, ie brittle.

I don't agree, Mock injection is a helper utility, it fairly well documented on what it can and cannot do and ho it does it. The only purpose of this utility is to write less code. The same as Mockito as mock framework (before you had to write interfaces and mock implementations); also you can write mocks that mimics types you don't own, while the real implementation's behavior changes, your test passes but your production code will broke in fail.

I'd rather have failing tests than a failing production.

Also I find it quite normal that property/field injection depends on wether there's some constructor. If you have a parameterized constructor you most certainly want to use it for instantiation. Plus if you have a parameterized constructor you certainly want to avoid at all cost setters which involve mutability.

if no matching constructor, try to create object via objenesis and use reflection to populate fields. This might work for 95% of the cases but the remaining 5% will lead to hard-to-debug problems.

This idea is interesting. However I'm not sure we should push such changes. Objenesis uses JDK's serialization internals to instantiate an object, for mocks this is fine. However for production code this is more dangerous as we want to actually use the constructor in place for 2 reasons : 1. code coverage, 2. constructor might actually do stuff. 

These two reasons are linked, if we skip this test because we can with objenesis we don't report it to the user, this could lead to the Liar test anti-pattern of James Carr.

Some of the edge case that are not managed by the current constructor injection implementation : 

• For now it doesn't instantiate private nested classes for example. And that should stay like that for production code. Nested classes in production code could and have the right to use the reference to their owner.
• It cannot use constructors that have native fields. That could be changed to use default values, it already does that for references.

I think that if we implement a toggable strategy it could solve this potential brittleness in some edge cases. Maybe I could rewrite the reported error also.

This thread made me realize that these two feature should be avoided in favor of just changing the strategy. 

http://code.google.com/p/mockito/issues/detail?id=290

http://code.google.com/p/mockito/issues/detail?id=291

Cheers,

-- Brice

[marekdec]

Hi Brice,

> Mockito's DI is not made to test dependencies, and it doesn't have to.

I didn't mean that at all. That would be a huge misuse of Mockito.
Just wanted to say that if I ever use the helper utility called 'DI
done by a testing framework' I would be much simpler if it was done as
it is done by my application.
And I find this

@InjectMocks(using = CDIInjection.class)

just a perfect solution. That is what I'd be looking for.

It's an interesting discussion ahyhow, I'll try follow it introducing
less noise from now on :)

Thanks,
Marek

>    - For now it doesn't instantiate private nested classes for example. And

>    that should stay like that for production code. Nested classes in
>    production code could and have the right to use the reference to their
>    owner.

>    - It cannot use constructors that have native fields. That could be

>    changed to use default values, it already does that for references.
>
> I think that if we implement a toggable strategy it could solve this
> potential brittleness in some edge cases. Maybe I could rewrite the
> reported error also.
>
> This thread made me realize that these two feature should be avoided in

> favor of just changing the strategy.http://code.google.com/p/mockito/issues/detail?id=290http://code.google.com/p/mockito/issues/detail?id=291

>
> Cheers,
>
> -- Brice
>
>
>
>
>
>
>
> On Fri, Feb 3, 2012 at 13:20, Szczepan Faber <szcze...@gmail.com> wrote:
> > I don't think there's a good solution here. We could be less brittle by:
>
> > 1. not trying to create an object, e.g. force the user to create one. This
> > is what we had in past and users wanted the object to be constructed for
> > free by mockito... :)
> > 2. if no matching constructor, try to create object via objenesis and use
> > reflection to populate fields. This might work for 95% of the cases but the
> > remaining 5% will lead to hard-to-debug problems.
>
> > Cheers!
>

> ...
>
> read more »

[Brice Dutheil]

Oh ok :)
Well tanks to you, for your feedback, it's allways welcome.

Eventually with mackaroo you could craft some OnDemandCDIMockInjection.class though the problem would still be accessing the state of this strategy just like we could with à Visitor.

-- Brice

>> >> > ...
>>
>> read more »

>
> --
> You received this message because you are subscribed to the Google Groups "mockito" group.
> To post to this group, send email to moc...@googlegroups.com.
> To unsubscribe from this group, send email to mockito+u...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/mockito?hl=en.
>
>

--

-- 

Brice