Support constructor parameters for spying on abstract classes

[mockitoguy]

Nice, self-contained enhancement that makes Mockito API more robust. The implementation does not require proficiency with Mockito codebase.

Feature

We already support spying on abstract classes by allowing parameterless constructor. However, there is no support for constructor parameters. This has been asked about long time ago. Mockito API is not robust enough if it supports mocking with constructor but not when one has any constructor parameters.

//current api:
Foo spy = mock(Foo.class, withSettings() .useConstructor().defaultAnswer(CALLS_REAL_METHODS));
//existing method (will remain):
MockSettings useConstructor();

//new api (change existing method):
Foo spy = mock(Foo.class, withSettings() .useConstructor("someArg").defaultAnswer(CALLS_REAL_METHODS));
//changed method:
MockSettings useConstructorArgs(Object ... args);

Open questions

• in case we find multiple matching constructors, do we just use the 1st matching (option 1) or throw an exception (option 2)?

I'd say we go for option 1 because it's simpler to implement and seems more convenient (works out of the box for certain use cases). If we go for option 2 we need to inform the user what to do to resolve the problem (for example document and suggest @fluentfuture idea of creating an inner implementation)

• do we add new method or add vararg to existing useConstructor() method?

We decided that using existing method is simpler, keeps the API small and is easy to discover.

Implementation notes

• the main complexity is to identify and detect the right constructor to use based on types of parameters supplied by the user
• we already deal with detecting constructors for the @Injectmocks functionality - there should be code to reuse

Test coverage

• see existing tests that cover "useConstructor" method for
• ensure decent, informative exception messages
  • if user supplies wrong constructor args (wrong types, we cannot find matching constructor)
  • if the constructor throws some exception (constructors of some types have code that can fail)
  • when one uses existing parameter-less "useConstructor" method but the mocked class requires constructor args, the exception message should tell the user about new "useConstructorArgs" method.
• what if arguments supplied by the user match more than 1 constructor - either we fail gracefully with decent message or we pick one of the constructors.
• update documentation to describe new feature. Update documentation for existing parameter-less "useConstructor" method. Update documentation in main Mockito class if it references “useConstructor”.
• other use cases?

[fluentfuture]

I've thought about some variant of this feature for a while, so here's some feedback.

It'd be nice if we have some concrete use cases to study.

In my own experience, I have mostly managed to avoid needing this feature by spying non-static abstract class.

For example, if the abstract class I'm spying on needs two constructor parameters, I do this:

public class FooTest {
  @Spy private MockAbstractFoo foo;
  private final Bar bar = ...;

  private Baz baz() {...}

  abstract class MockAbstractFoo extends AbstractFoo {
    MockAbstractFoo() {
      super(bar, baz());
    }
  }
}

Fwiw:

1. This is static type safe, and refactoring friendly. So I find it preferable when it meets my needs.
2. The constructor can use instance fields or instance methods of the enclosing test object.
3. The main drawback is that you can't easily construct difference instances with different constructor parameters. On the other hand, I strive to keep my abstract classes stateless so I rarely need to pass constructor parameters anyway.

[fluentfuture]

I forgot to mention that it is possible with today's API to pass different constructor parameters, through MockSettings. Like the following:

class MockFooFactory {
  private final Bar bar;
  private final Baz baz;

  MockFooFactory(Bar bar, Baz baz) {...}

  public AbstractFoo create() {
    return Mockito.mock(MockFoo.class, withSettings()
        .useConstructor().outerInstance(this).defaultAnswer(CALLS_READ_METHODS));
  }

  abstract class MockFoo extends AbstractFoo {
    MockFoo() {
      super(bar, baz);
    }
  }
}

Then just call it with:

new MockFooFactory(bar, baz).create();

Again, this is static type safe and IDE friendly compared to reflection-based API.

The withSettings() thing has a bit of discoverability problem and I suppose not many people know they can do this. Alternatively, I would love to have a simpler API, like:

Mockito.spy(outerInstance, MockFoo.class);

[mureinik]

I think that overloading useConstructor() is a much cleaner approach than adding a new useConstructorArgs(Object...) method.
It also makes the method much easier to discover, as it's right there, and the user's IDE will offer the argument list.

I've coded this approach in PR #935. Feedback is more than welcome!

[fluentfuture]

Agreed that useContructor(args...) reads nicer.

Personally, I'm not convinced that the dynamic type support is worth the effort. It seems to be a slippery slope toward defeating Java static type safety, which reminds me of the ancient JMock vs. EasyMock comparison where the former relied on type-less reflection while the latter is static type safe. If Java doesn't allow you to call new Foo(Object, Object), does Mockito have to open that back door (when the enclosing class trick could be used to achieve the goal, albeit slightly indirectly)?

That said, if you do decide to add this feature, there are a few implementation caveats that we need to be careful about:

• Overload resolution. It's not hard to imagine passing an arg whose static type suggests constructor overload1 to be invoked, but the runtime type actually invokes overload2. In other words, new Foo(a, b) {...} and useConstructor(a, b) result in different constructor being invoked.
• Generics. There is no way to verify that the passed in List<Something> is actually a List<String>, even at runtime, risking unchecked type errors.
• Visibility. What happens if the constructor has both public constructor and package-private constructor both could have been chosen for the given args?

[fluentfuture]

I happened to need to dig up the history of @SPY AbstractClass in #106. And I found that all my concerns against constructor-args were already stated in that thread. And it was clear that @szczepiq is fine with the trade-off.

So, while I disagree with the design decision, my apologies for repeating myself over again. :)

[mockitoguy]

Thank you very much for contribution. It's really nice work!