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

Mixing Java and Spring annotations. #103

Closed
slowikps opened this Issue Nov 14, 2014 · 11 comments

Comments

3 participants
@slowikps

slowikps commented Nov 14, 2014

Whenever one mix Java and Spring annotations:

  • org.springframework.beans.factory.annotation.Autowired
  • javax.annotation.Resource

and create a test:

public class MyTest {

    @Tested
    private TestedClass testedClass;

    @Injectable
    @Mocked
    private SomeService annotatedWithAutowired

    @Injectable
    @Mocked
    private SomeService annotatedWithResource
}

class TestedClass {
    @Autowired
    private SomeService1 annotatedWithAutowired;

    @Resource
    private SomeService2 annotatedWithResource;
}

only fields with Java annotations are injected to @tested object.

What I think need to be fixed/redesign is a discardFieldsNotAnnotatedWithJavaxInjectIfAtLeastOneIsAnnotated method inside
mockit.internal.expectations.injection.FieldInjection class

@rliesenfeld

This comment has been minimized.

Show comment
Hide comment
@rliesenfeld

rliesenfeld Nov 14, 2014

Member

JMockit does it this way by design, based on the assumption that the code under test would either use standard CDI/Java EE annotations, or proprietary (Spring, Guice) annotations, but not both at the same time.

In this case, is there a reason for the SUT to use both @resource and @Autowired? Shouldn't it just use @Autowired, or (ideally) the standard javax.inject.Inject annotation rather than @Autowired? Note that Spring does support @Inject.

Member

rliesenfeld commented Nov 14, 2014

JMockit does it this way by design, based on the assumption that the code under test would either use standard CDI/Java EE annotations, or proprietary (Spring, Guice) annotations, but not both at the same time.

In this case, is there a reason for the SUT to use both @resource and @Autowired? Shouldn't it just use @Autowired, or (ideally) the standard javax.inject.Inject annotation rather than @Autowired? Note that Spring does support @Inject.

@slowikps

This comment has been minimized.

Show comment
Hide comment
@slowikps

slowikps Nov 14, 2014

Hi @rliesenfeld,
Thanks for answer. I ideal world it make perfect sense, but I think that there are many projects which mix especially those two annotations @resource and @Autowired.
Then when one upgrade jmockit version it is really hard to figure out why some of dependencies are not injected and there are so many NullPointerExceptions in tests which were working before.
I would like to be able to somehow turn off this functionality, or at least see some hints in my logs.

slowikps commented Nov 14, 2014

Hi @rliesenfeld,
Thanks for answer. I ideal world it make perfect sense, but I think that there are many projects which mix especially those two annotations @resource and @Autowired.
Then when one upgrade jmockit version it is really hard to figure out why some of dependencies are not injected and there are so many NullPointerExceptions in tests which were working before.
I would like to be able to somehow turn off this functionality, or at least see some hints in my logs.

@rliesenfeld rliesenfeld added enhancement and removed question labels Nov 14, 2014

@rliesenfeld rliesenfeld self-assigned this Nov 14, 2014

@rliesenfeld

This comment has been minimized.

Show comment
Hide comment
@rliesenfeld

rliesenfeld Nov 14, 2014

Member

Support will be added for @Autowired.

Member

rliesenfeld commented Nov 14, 2014

Support will be added for @Autowired.

@slowikps

This comment has been minimized.

Show comment
Hide comment
@slowikps

slowikps Nov 17, 2014

Hi,
Thanks for replaying to my comment, but I think this Enhancement can not be closed as of yet.
Two problems:

  1. This implementations is very restrictive. It is throwing java.lang.IllegalStateException when one not provide all fields marked as @Injectable, @Autowired... for tested service. I think this should be logged only and exceptions should not be thrown. This can break a lot of legacy tests.
  2. If you are adding support to @Autowired annotation support for org.springframework.beans.factory.annotation.Value annotation should be added as well.

Thanks,
Paweł

slowikps commented Nov 17, 2014

Hi,
Thanks for replaying to my comment, but I think this Enhancement can not be closed as of yet.
Two problems:

  1. This implementations is very restrictive. It is throwing java.lang.IllegalStateException when one not provide all fields marked as @Injectable, @Autowired... for tested service. I think this should be logged only and exceptions should not be thrown. This can break a lot of legacy tests.
  2. If you are adding support to @Autowired annotation support for org.springframework.beans.factory.annotation.Value annotation should be added as well.

Thanks,
Paweł

@rliesenfeld

This comment has been minimized.

Show comment
Hide comment
@rliesenfeld

rliesenfeld Nov 17, 2014

Member

Hi,

You're right about @value; I will add support for it too.

About the IllegalStateException, I think this is necessary to avoid unexpected NullPointerExceptions from annotated injection points left uninitialized. Simply printing a warning message to the console will just get ignored by most users. If an existing test breaks, the user can fix it by adding a suitable @Injectable field or parameter. I think this is the right approach because DI containers don't allow required dependencies to remain null either, throwing an exception at startup for any unresolved dependency; also, this prevents a test that passes now to start failing with an NPE months later, when someone changes the code to use the dependency in some new place.

Member

rliesenfeld commented Nov 17, 2014

Hi,

You're right about @value; I will add support for it too.

About the IllegalStateException, I think this is necessary to avoid unexpected NullPointerExceptions from annotated injection points left uninitialized. Simply printing a warning message to the console will just get ignored by most users. If an existing test breaks, the user can fix it by adding a suitable @Injectable field or parameter. I think this is the right approach because DI containers don't allow required dependencies to remain null either, throwing an exception at startup for any unresolved dependency; also, this prevents a test that passes now to start failing with an NPE months later, when someone changes the code to use the dependency in some new place.

@rliesenfeld rliesenfeld reopened this Nov 17, 2014

@slowikps

This comment has been minimized.

Show comment
Hide comment
@slowikps

slowikps Nov 17, 2014

Hi Rogério,
I see your point, but for me it is a bit too much that I need to provide all dependencies. With this requirements unittests are very similar to integration tests, so they loose a bit of unittess beauty.

Furthermore lets consider this example

class A { 
  @Autowired Service1 s1
  @Autowired Service2 s2
  @Autowired Service3 s3
}

Lets assume that A is nicely covered by unit tests.
Then we have B:

class B extends A { 
  @Autowired Service4 s4
}

I would like to test B in isolation, but in order to do this I need to provide s1 and s2 and s3.

Does it make any sense for you?

Regards,
Paweł

slowikps commented Nov 17, 2014

Hi Rogério,
I see your point, but for me it is a bit too much that I need to provide all dependencies. With this requirements unittests are very similar to integration tests, so they loose a bit of unittess beauty.

Furthermore lets consider this example

class A { 
  @Autowired Service1 s1
  @Autowired Service2 s2
  @Autowired Service3 s3
}

Lets assume that A is nicely covered by unit tests.
Then we have B:

class B extends A { 
  @Autowired Service4 s4
}

I would like to test B in isolation, but in order to do this I need to provide s1 and s2 and s3.

Does it make any sense for you?

Regards,
Paweł

@slowikps slowikps closed this Nov 17, 2014

@slowikps slowikps reopened this Nov 17, 2014

@rliesenfeld

This comment has been minimized.

Show comment
Hide comment
@rliesenfeld

rliesenfeld Nov 17, 2014

Member

Most classes with injected dependencies shouldn't have more than three or four of them, so it shouldn't be that difficult to declare the necessary @Injectable's. Classes with more than four dependencies are asking to be broken up into smaller classes, so we can argue that the tests are encouraging a better design.

For the "B extends A" example, I would say that 1) such cases (where a class with its own tests has a subclass) tend to be rare; 2) testing B in isolation from A implies that A would be mocked away, and that doesn't sound good - either B should be tested as if it doesn't extend A, or object composition should have been used rather than inheritance (then it would be perfecly fine to mock A); and 3) a B instance will have all four dependencies when used in production, so why do differently from the tests?

Member

rliesenfeld commented Nov 17, 2014

Most classes with injected dependencies shouldn't have more than three or four of them, so it shouldn't be that difficult to declare the necessary @Injectable's. Classes with more than four dependencies are asking to be broken up into smaller classes, so we can argue that the tests are encouraging a better design.

For the "B extends A" example, I would say that 1) such cases (where a class with its own tests has a subclass) tend to be rare; 2) testing B in isolation from A implies that A would be mocked away, and that doesn't sound good - either B should be tested as if it doesn't extend A, or object composition should have been used rather than inheritance (then it would be perfecly fine to mock A); and 3) a B instance will have all four dependencies when used in production, so why do differently from the tests?

@slowikps

This comment has been minimized.

Show comment
Hide comment
@slowikps

slowikps Nov 19, 2014

Ok, I am fine with this.

slowikps commented Nov 19, 2014

Ok, I am fine with this.

@pberlowski

This comment has been minimized.

Show comment
Hide comment
@pberlowski

pberlowski Nov 25, 2014

@rliesenfeld Hi Rogerio and thanks for looking into this. I have a few thoughts on the issue.
I agree with the conclusion that all dependencies should be mocked into the tested class, however, I have a very important case in which I need to mix @resource and @Autowired and that is a self-injection.

Spring will not autowire an instance of the object into itself unless that field is annotated with @resource. We tend to stick to @Autowired (with possible @qualifier - will you support that?) in most of our services, so that @resource really stands out as self-injection. There's two major cases where do this: @Cacheable proxy and @trace proxy.

Now, we've been through all the discussions about the pros and cons of this approach and we stuck to this pattern in certain applications. Our question is really if JMockit will support this.

Also, while I agree that mocking framework for Unit Tests should encourage good practices and discourage bad ones, my opinion is that is not its place to enforce them. By encumbering a framework like this with a role of a policeman, you'd be actually discouraging possible new users from adopting it into their legacy applications. There are other tools to enforce good coding practices, some of them exquisitely good.

pberlowski commented Nov 25, 2014

@rliesenfeld Hi Rogerio and thanks for looking into this. I have a few thoughts on the issue.
I agree with the conclusion that all dependencies should be mocked into the tested class, however, I have a very important case in which I need to mix @resource and @Autowired and that is a self-injection.

Spring will not autowire an instance of the object into itself unless that field is annotated with @resource. We tend to stick to @Autowired (with possible @qualifier - will you support that?) in most of our services, so that @resource really stands out as self-injection. There's two major cases where do this: @Cacheable proxy and @trace proxy.

Now, we've been through all the discussions about the pros and cons of this approach and we stuck to this pattern in certain applications. Our question is really if JMockit will support this.

Also, while I agree that mocking framework for Unit Tests should encourage good practices and discourage bad ones, my opinion is that is not its place to enforce them. By encumbering a framework like this with a role of a policeman, you'd be actually discouraging possible new users from adopting it into their legacy applications. There are other tools to enforce good coding practices, some of them exquisitely good.

@rliesenfeld

This comment has been minimized.

Show comment
Hide comment
@rliesenfeld

rliesenfeld Nov 25, 2014

Member

@resource was already supported, and now (the next release) it can be used in the same class where @Autowired is used; both annotations, plus @Inject, are supported.

I don't know of any use case which requires support for @qualifier, so there isn't any. It shouldn't matter, though, as qualifiers affect only the implementation that would get injected in production, not the type of the mocked dependency used in tests.

Member

rliesenfeld commented Nov 25, 2014

@resource was already supported, and now (the next release) it can be used in the same class where @Autowired is used; both annotations, plus @Inject, are supported.

I don't know of any use case which requires support for @qualifier, so there isn't any. It shouldn't matter, though, as qualifiers affect only the implementation that would get injected in production, not the type of the mocked dependency used in tests.

@pberlowski

This comment has been minimized.

Show comment
Hide comment
@pberlowski

pberlowski Nov 25, 2014

Thanks, that confirms that everything we need is going out in the new release.

pberlowski commented Nov 25, 2014

Thanks, that confirms that everything we need is going out in the new release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment