Skip to content
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

automatically detect class to mock #2779

Merged
merged 6 commits into from Nov 14, 2022
Merged

automatically detect class to mock #2779

merged 6 commits into from Nov 14, 2022

Conversation

asolntsev
Copy link
Contributor

this commit adds method Mockito.mock() as a shorter alternative for Mockito.mock(Class). When result of this call is assigned to a variable/field, java will detect the needed class automatically.

P.S. I had to ignore errorprone checks "DoNotMock" and "DoNotMockAutoValue" because they fail with IndexOutOfBoundException trying to get the first argument of Mockito.mock(). :)

this commit adds method `Mockito.mock()` as a shorter alternative for `Mockito.mock(Class)`. When result of this call is assigned to a variable/field, java will detect the needed class automatically.

P.S. I had to ignore errorprone checks "DoNotMock" and "DoNotMockAutoValue" because they fail with IndexOutOfBoundException trying to get the first argument of `Mockito.mock()`. :)
@TimvdLippe
Copy link
Contributor

This is a super interesting implementation and I am not sure what to think of it. What I like about @Mock is that we don't have to deal with the unchecked warnings. This method would fix that exact problem, but now for mock(), which is awesome. At the same time, it also feels like a bit of a hack and I wonder what the readability of this will be. The expressiveness of the .class argument makes it at least clear what we are attempting to mock and what you will get.

That said, I am not sure if there is any harm in shipping this method, given that it does resolve the unchecked warning problem for Mockito.mock(), which is valuable on its own. For that, I think we do need to fix these 2 ErrorProne checks, as they will be crashing otherwise. However, I think that needs to be done after this PR is merged.

@mockito/developers WDYT?

@raphw
Copy link
Member

raphw commented Nov 11, 2022

I do like the concept! I am however not sure if overloading might create some issues here and there, but as our tests compile, it does not seem to be a problem. We are already doing a lot with the Java language that is not intuitive at first glance, so I think this would be a good API addition.

Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a new section to the JavaDoc (section 54) that showcases the new feature. Don't forget to also link to the new section in the Contents section at the top.

@szpak
Copy link
Member

szpak commented Nov 11, 2022

This API is similar to the Spock one where it has been in common use for years. If there are no warnings in IDE, why not to leverage it (assuming no technical glitches are found).

var mock = mock(MyClass.class)

is a (current) alternative, but the new one is even more robust (and shorter).

Btw, Groovy allows to do that kind of tricks, but I didn't know about that one in Java, good idea @asolntsev!

@codecov-commenter
Copy link

codecov-commenter commented Nov 13, 2022

Codecov Report

Base: 86.18% // Head: 74.95% // Decreases project coverage by -11.22% ⚠️

Coverage data is based on head (7232399) compared to base (eb85518).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@              Coverage Diff              @@
##               main    #2779       +/-   ##
=============================================
- Coverage     86.18%   74.95%   -11.23%     
+ Complexity     2828     2549      -279     
=============================================
  Files           320      320               
  Lines          8583     8556       -27     
  Branches       1060     1044       -16     
=============================================
- Hits           7397     6413      -984     
- Misses          905     1814      +909     
- Partials        281      329       +48     
Impacted Files Coverage Δ
src/main/java/org/mockito/Mockito.java 92.85% <100.00%> (-3.25%) ⬇️
...ckito/exceptions/stacktrace/StackTraceCleaner.java 0.00% <0.00%> (-100.00%) ⬇️
...ito/internal/stubbing/UnusedStubbingReporting.java 0.00% <0.00%> (-100.00%) ⬇️
.../exceptions/misusing/PotentialStubbingProblem.java 0.00% <0.00%> (-100.00%) ⬇️
...ernal/matchers/apachecommons/ReflectionEquals.java 0.00% <0.00%> (-100.00%) ⬇️
.../misusing/CannotStubVoidMethodWithReturnValue.java 0.00% <0.00%> (-100.00%) ⬇️
...mockito/internal/debugging/InvocationsPrinter.java 3.57% <0.00%> (-96.43%) ⬇️
.../org/mockito/internal/junit/ArgMismatchFinder.java 18.75% <0.00%> (-81.25%) ⬇️
.../mockito/internal/stubbing/StrictnessSelector.java 20.00% <0.00%> (-80.00%) ⬇️
...aultanswers/RetrieveGenericsForDefaultAnswers.java 18.86% <0.00%> (-79.25%) ⬇️
... and 103 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@asolntsev
Copy link
Contributor Author

Btw, Groovy allows to do that kind of tricks, but I didn't know about that one in Java, good idea @asolntsev!

I also didn't know that Java can detect the class name.
I found this trick in Tagir Valeev post

... similarly to `Mockito.mock()`
@asolntsev
Copy link
Contributor Author

@szpak @TimvdLippe Similarly to Mockito.mock(), I have also added method Mockito.spy().
Also implemented all suggestions from code reviews.

Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 small nits. I wanted to fix them myself, but I didn't have permission to push to your fork. After that, this LGTM!

src/main/java/org/mockito/Mockito.java Outdated Show resolved Hide resolved
src/main/java/org/mockito/Mockito.java Outdated Show resolved Hide resolved
asolntsev and others added 2 commits November 14, 2022 22:08
Co-authored-by: Tim van der Lippe <TimvdLippe@users.noreply.github.com>
Co-authored-by: Tim van der Lippe <TimvdLippe@users.noreply.github.com>
@asolntsev
Copy link
Contributor Author

2 small nits. I wanted to fix them myself, but I didn't have permission to push to your fork. After that, this LGTM!

Thank you, merged.

Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@TimvdLippe TimvdLippe merged commit 3faa002 into mockito:main Nov 14, 2022
@asolntsev asolntsev deleted the automatically-detect-class-to-mock branch November 14, 2022 20:48
*
* @param reified don't pass any values to it. It's a trick to detect the class/interface you want to mock.
* @return mock object
* @since 4.9.0

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too late, I know, but shouldn't it have been:

Suggested change
* @since 4.9.0
* @since 4.10.0

?

@rdesgroppes
Copy link

P.S. I had to ignore errorprone checks "DoNotMock" and "DoNotMockAutoValue" because they fail with IndexOutOfBoundException trying to get the first argument of Mockito.mock(). :)

I filed google/error-prone#3609: feel free to adjust as per your own observations.

@ankurbajaj9
Copy link

This change has broken my Mockito.spy(object_to_mock) calls in a scala project. Basically Scala cant differenciate between the two spy methods.

@asolntsev
Copy link
Contributor Author

@ankurbajaj9 Can you share some reproducible example? I have just tried with Scala 2 and Scala 3 - both Mockito.mock(Class) and Mockito.mock() work as expected.

@ankurbajaj9
Copy link

ankurbajaj9 commented Dec 28, 2022

can you try the Mockito.spy(Object) and see if you can reproduce the same

@asolntsev
Copy link
Contributor Author

@ankurbajaj9 Yes, works for me:

  @Test def spyWithObject(): Unit = {
    val names: util.ArrayList[String] = Mockito.spy(util.ArrayList())
    assert(names.size == 0)
    names.add("tere")
    assert(names.size == 1)
    doReturn(42).when(names).size
    assert(names.size == 42)
  }

@ankurbajaj9 Once again: it would be much simpler if you shared some reproducible example.

@sijtsche
Copy link

sijtsche commented Jan 12, 2023

We upgraded to a newer version of Mockito (4.11.0, works with 4,9.0) and it breaks our integration test, because I think it takes this new mock method instead of the original one, while it should have executed the original one with this signature public static T mock(Class classToMock).

This mock

<bean id="conversionService" name="conversionService" class="org.mockito.Mockito" factory-method="mock" >
        <constructor-arg value="org.springframework.core.convert.ConversionService"/>
</bean>

Causes the following error when creating the applicationcontext for integration tests:
Caused by: org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'conversionService' defined in URL [file:/C:/sometestproject/target/test-classes/MyTest-context.xml]: Bean instantiation via factory method failed; nested exception is org.springframework.beans.BeanInstantiationException: Failed to instantiate [java.lang.Object]: Factory method 'mock' threw exception; nested exception is java.lang.IllegalArgumentException: Please don't pass any values here. Java will detect class automagically.

This is described as a working way of using Mockito by Spring in their docs:
https://docs.spring.io/spring-framework/docs/current/reference/html/testing.html
See snippet on that page:

 <bean id="accountService" class="org.mockito.Mockito" factory-method="mock">
    <constructor-arg value="org.example.AccountService"/>
</bean>

@asolntsev
Copy link
Contributor Author

@sijtsche Thank you for reporting the problem.

  1. I would say this problem should be solved on Spring side.
  2. I would recommend to avoid using such XMLs etc. This is very old-school way of using Spring. Are you aware of annotations, autowired dependencies etc? It's much more convenient, and it existed in Spring for 10+ years already.

Anyway, could you please share some simple project to reproduce the problem? Some trivial "hello world" project?

@sijtsche
Copy link

Yes, I am aware of all types of configuration in Spring. But not all companies use that and it is not even a deprecated way of configuring a Spring application. So IMHO we should be able to use XML configuration in Spring. I think large projects that exists for a long time can not rewrite their whole application wiring because of a Mockito update. So it would be great if we could at least find a workaround.

mockitodemo.zip

I have created a demo Spring Boot project that reproduces this issue. With a partial solution in XML config too. You will find two testclasses, one that gives the error I mentioned, the other with this workaround that now invokes the right mock method in the Mockito class by specifying the type java.lang.Class:

    <bean id="conversionService" name="conversionService" class="org.mockito.Mockito" factory-method="mock">
        <constructor-arg>
            <value type="java.lang.Class">org.springframework.core.convert.ConversionService</value>
        </constructor-arg>
    </bean>

But, if I add a class to the application context that uses this conversionService mock as an autowired dependency (see TestClass in the testproject) it can not instantiate the TestClass class and fails with this error: Error creating bean with name 'testclass': Unsatisfied dependency expressed through field 'conversionService'; nested exception is org.springframework.beans.factory.NoSuchBeanDefinitionException: No qualifying bean of type 'org.springframework.core.convert.ConversionService' available: expected at least 1 bean which qualifies as autowire candidate. Dependency annotations: {@org.springframework.beans.factory.annotation.Autowired(required=true)}

For that I do not have a workaround (yet).

As soon as I set the dependency of mockito-core back to 4.9.0, this works again without other changes. Anyway, you can try it now yourself with this testproject.

@asolntsev
Copy link
Contributor Author

@sijtsche Thank you for providing the working example!
Now I see that this declaration is not really correct:

        <constructor-arg value="org.springframework.core.convert.ConversionService"/>

Since Spring doesn't know the type of parameter "ConversionService", it just tries to find a static method with one parameter of any type in class Mockito. It occasionally worked before Mockito had only one static method mock with a single parameter.

Now Mockito has at least two public static methods mock with a single parameter.
So it's anyway a good idea to declare the type of the parameter explicitly:

        <constructor-arg>
            <value type="java.lang.Class">org.springframework.core.convert.ConversionService</value>
        </constructor-arg>

P.S. About your second problem: I think it's not related to the static method Mockito.mock. It's really caused by the order of dependencies in your xml: you need to declare bean testclass AFTER bean conversionService. It seems reasonable to me because testclass depends on conversionService.

@sijtsche
Copy link

Hi @asolntsev the typing issue is then solved. But the missing dependency works if I just set mockito back to 4.9.0 it works and if I set it to a higher version it does not work anymore. It indeed probably is not related to this particular change, but I think it is somehow related to Mockito.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants