Fixes #976: Resolve ambiguous constructors #980

mureinik · 2017-03-07T10:02:15Z

With the current code (introduced in Mockito 2.7.14 by commit 6a82c03), calling MockSettings.useConstructor with an argument list that would be applicable to more than one constructor would fail with an org.mockito.internal.creation.instance.InstantiationException.

This behavior, however, is suboptimal, as described in issue #976, as it makes useConstructor less robust than the Java compiler, which is able to resolve such ambiguities.
With this patch, Mockito will attempt to match the constructor with the most specific parameter types. A constructor X is considered more specific than a constructor Y if:

They are both applicable to the given argument list
Constructor X has at least one parameter which is a further specialization of the corresponding parameter of constructor Y (i.e. paramX.isAssignableFrom(paramY)).
Constructor Y has no parameter which is a further specialization of the corresponding parameter of constructor X, as defined above.

E.g., consider the following class:

public class SomeClass {
    SomeClass(Object o) {}
    SomeClass(String s) {}
}

Without this patch, calling

mock(SomeClass.class, withSettings().useConstructor("string!"))

would fail. With this patch, such a call would invoke the SomeClass(String) constructor.

As noted above, this PR fixes issue #976.

codecov-io · 2017-03-07T10:16:47Z

Codecov Report

Merging #980 into release/2.x will increase coverage by 0.04%.
The diff coverage is 100%.

@@                Coverage Diff                @@
##             release/2.x     #980      +/-   ##
=================================================
+ Coverage           86.8%   86.84%   +0.04%     
- Complexity          2275     2284       +9     
=================================================
  Files                287      287              
  Lines               5769     5787      +18     
  Branches             675      682       +7     
=================================================
+ Hits                5008     5026      +18     
  Misses               571      571              
  Partials             190      190

Impacted Files	Coverage Δ	Complexity Δ
...nal/creation/instance/ConstructorInstantiator.java	`95.77% <100%> (+1.43%)`	`36 <9> (+9)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5815d4c...ed4e169. Read the comment docs.

mureinik · 2017-03-08T18:25:23Z

I wonder why Codecov has no previous report to compare against. Do any of the maintainers have a clue?

TimvdLippe · 2017-03-09T11:10:06Z

@mureinik Because commits by the delivery drone do [ci skip] and therefore do not trigger a code coverage report.

mureinik · 2017-03-09T11:13:19Z

@TimvdLippe That means that any PR that's rebased on top of a release won't have the benefit of a comparative CodeCov report. I get why the delivery drone should skip CI, but this is an unfortunate side effect. I'll bring this up on the mailing list - this PR probably isn't the right place to discuss it.

And thanks for the explanation!

TimvdLippe · 2017-03-09T11:16:28Z

@mureinik please see #493 for reference.

mureinik · 2017-03-16T16:51:05Z

@szczepiq ping? Will you have some time to take a look at this please?

mockitoguy · 2017-03-16T18:24:10Z

@mureinik, I'm very keen to review this change. Thank you for your submission!

I'm allocating most of my time to #911 currently but I promise to review (and hopefully merge) by the end of this weekend.

Thanks for patience with us :)

TimvdLippe

Implementation looks fine to me. Could you add 1 more test to verify this also works correctly for primitive types? Just to make sure that keeps on working.

Thanks for your effort!!

mureinik · 2017-03-17T12:43:03Z

Rebased on top of 2.7.17 and added CreatingMocksWithConstructorTest#can_spy_ambiguius_constructor_with_primitive as per @TimvdLippe's suggestion above.

Note that this API does not support implicit promotions (e.g., passing an int in useConstructors() varargs will autobox it to an Integer. This integer can't be matched to a constructor expecting a long primitive or a Long wrapper), in the same fashion that the java.lang.reflect APIs don't support it.

mockitoguy

Really nice changes, thank you!

I really appreciate how cleanly and thoroughly you have described the PR. It is linked from our release notes and it makes Mockito documentation really good.

I requested changes but I don't consider them blocking. If you can get them done, I'm happy to merge right away :)

Thank you very much for the contribution!!!

mockitoguy · 2017-03-19T03:29:47Z

src/main/java/org/mockito/internal/creation/instance/ConstructorInstantiator.java

@@ -130,4 +127,32 @@ private static boolean paramsMatch(Class<?>[] types, Object[] params) {
        }
        return true;
    }
+
+    private void evaluateConstructor(List<Constructor<?>> matchingConstructors, Constructor<?> constructor) {


Can you add a javadoc describing this method? It has non trivial complexity and it would be handy to understand why we are implementing the algorithm this way.

Good point, thanks.
I'll add some javadoc to explain this.

mockitoguy · 2017-03-19T03:30:37Z

src/test/java/org/mockitousage/constructor/CreatingMocksWithConstructorTest.java

@@ -137,45 +137,90 @@ public void exception_message_when_constructor_not_found() {
    static class ExtendsExtendsBase extends ExtendsBase {}


I love the new tests. Thank you for adding comprehensive coverage!!!

mockitoguy · 2017-03-19T03:34:40Z

src/main/java/org/mockito/internal/creation/instance/ConstructorInstantiator.java

-                "If you believe that Mockito could do a better join deciding on which constructor to use, please let us know.",
-                "Ticket 685 contains the discussion and a workaround for ambiguous constructors using inner class.",
-                "See https://github.com/mockito/mockito/issues/685"
+                join("", " - ", constructors)


Let's keep the extra information. I think it is useful to:

inform about the inner class pattern for working around the ambiguous constructors

ask for feedback - it does not hurt to do that

You can keep the original or modify it if you want. I'd like to keep at least the hint about inner class.

I originally took this message as a "workaround" until this PR is implemented, but you're right, let's keep it. Note there's a typo there ("join" instead of "job"). I'll fix that while I'm at it.

With the current code (introduced in Mockito 2.7.14 by commit 6a82c03), calling MockSettings#useConstructor with an argument list that would be applicable to more than one constructor would fail with an org.mockito.internal.creation.instance.InstantiationException. This behavior, however, is suboptimal, as described in issue mockito#976, as it makes useConstructor less robust than the Java compiler, which is able to resolve such ambiguities. With this patch, Mockito will attempt to match the constructor with the most specific parameter types. A constructor X is considered more specific than a constructor Y if: 1. They are both applicable to the given argument list 2. Constructor X has at least one parameter which is a further specialization of the corresponding parameter of constructor Y (i.e. paramX.isAssignableFrom(paramY)). 3. Constructor Y has no parameter which is a further specialization of the corresponding parameter of constructor X, as defined above. E.g., consider the following class: public class SomeClass { SomeClass(Object o) {} SomeClass(String s) {} } Without this patch, calling mock(SomeClass.class, withSettings().useConstructor("string!")) would fail. With this patch, such a call would invoke the SomeClass(String) constructor.

The text should read "a better JOB", not "a better JOIN".

mureinik · 2017-03-19T08:17:19Z

Pushed updated branch. Change from previous push:

Returned the error message regarding issue Support constructor parameters for spying on abstract classes #685, as per @szczepiq review above
Fixed a Freudian slip in the aforementioned error message ("a better join" -> "a better job")
Added javadoc explaining the logic of org.mockito.internal.creation.instance.ConstructorInstantiator#evaluateConstructor, as per @szczepiq review.

mureinik · 2017-03-19T11:39:29Z

The build failure seems to be a TravisCI issue, not an issue with the code itself, as this revision introduced only minor changes (added javadoc and fixed a typo) compared to the previous one, and some of the Travis checks are passing.

The error:

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
The build has been terminated

@szczepiq @TimvdLippe Is there any way to re-trigger Travis on this PR?

TimvdLippe · 2017-03-19T11:46:14Z

I have restarted the builds

TimvdLippe

Very nice pull request, thanks for your effort!

TimvdLippe mentioned this pull request Mar 9, 2017

Release commits do not trigger code coverage report #984

Closed

mureinik mentioned this pull request Mar 9, 2017

Code cov on releases #493

Merged

TimvdLippe approved these changes Mar 17, 2017

View reviewed changes

mureinik force-pushed the ambiguousConstructor branch from ae4cb3e to 63afde1 Compare March 17, 2017 12:40

mockitoguy reviewed Mar 19, 2017

View reviewed changes

mureinik force-pushed the ambiguousConstructor branch from 63afde1 to b353d97 Compare March 19, 2017 08:13

Fix Freudian slip in message regarding issue 685

ed4e169

The text should read "a better JOB", not "a better JOIN".

mureinik force-pushed the ambiguousConstructor branch from b353d97 to ed4e169 Compare March 19, 2017 08:15

TimvdLippe approved these changes Mar 19, 2017

View reviewed changes

TimvdLippe merged commit 9888504 into mockito:release/2.x Mar 19, 2017

		@@ -137,45 +137,90 @@ public void exception_message_when_constructor_not_found() {
		static class ExtendsExtendsBase extends ExtendsBase {}

Uh oh!

Fixes #976: Resolve ambiguous constructors #980

Fixes #976: Resolve ambiguous constructors #980

Uh oh!

Conversation

mureinik commented Mar 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Mar 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mureinik commented Mar 8, 2017

Uh oh!

TimvdLippe commented Mar 9, 2017

Uh oh!

mureinik commented Mar 9, 2017

Uh oh!

TimvdLippe commented Mar 9, 2017

Uh oh!

mureinik commented Mar 16, 2017

Uh oh!

mockitoguy commented Mar 16, 2017

Uh oh!

TimvdLippe left a comment

Choose a reason for hiding this comment

Uh oh!

mureinik commented Mar 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mockitoguy left a comment

Choose a reason for hiding this comment

Uh oh!

mockitoguy Mar 19, 2017

Choose a reason for hiding this comment

Uh oh!

mureinik Mar 19, 2017

Choose a reason for hiding this comment

Uh oh!

mockitoguy Mar 19, 2017

Choose a reason for hiding this comment

Uh oh!

mockitoguy Mar 19, 2017

Choose a reason for hiding this comment

Uh oh!

mureinik Mar 19, 2017

Choose a reason for hiding this comment

Uh oh!

mureinik commented Mar 19, 2017

Uh oh!

mureinik commented Mar 19, 2017

Uh oh!

TimvdLippe commented Mar 19, 2017

Uh oh!

TimvdLippe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mureinik commented Mar 7, 2017 •

edited

Loading

codecov-io commented Mar 7, 2017 •

edited

Loading

mureinik commented Mar 17, 2017 •

edited

Loading