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

Enable mocking static methods in Mockito #1013

Closed
mockitoguy opened this issue Mar 31, 2017 · 106 comments
Closed

Enable mocking static methods in Mockito #1013

mockitoguy opened this issue Mar 31, 2017 · 106 comments

Comments

@mockitoguy
Copy link
Member

@mockitoguy mockitoguy commented Mar 31, 2017

Static methods mocking with Mockito

This is a placeholder ticket for enabling mocking static methods in Mockito. Example action items that are totally negotiable and can ran in parallel. We're looking for someone who can lead this effort.

  • Research + discuss whether it is a good idea to enable static methods mocking in Mockito. The theory is that it is useful for legacy code, which is most code in the world.
  • Research on how other mocking frameworks do that and whether it is considered useful feature for their users.
  • Design and present for discussion an API for static mocking (slightly relevant ticket: #643)
  • Work with @raphw / ByteBuddy to come up with hacky prototype (the hackier, the better!)
  • Mold the prototype with the API, remove enough rough edges so that the feature is good enough for incubating rollout
  • SHIP IT!
@TimvdLippe
Copy link
Contributor

@TimvdLippe TimvdLippe commented Mar 31, 2017

I am torn on this one. If we provide this possibility, we might invite developers into bad practices. Every time a developer uses this feature, it should be very explicit that it is bad practice imo. Would it be possible to come up with a solution that is clear on this front, but still allows the developer to opt-in?

@mockitoguy
Copy link
Member Author

@mockitoguy mockitoguy commented Apr 1, 2017

Great feedback. That's the first thing to figure out when working on this ticket :)

@raphw
Copy link
Member

@raphw raphw commented Apr 1, 2017

the way I saw mocking of static methods for myself was to only mock calls to static methods that happen within a mock. This would be useful, for example, for implementing spies in a situation like:

class Foo {
  String bar() { return Util.makeString(); }
}

With Mockito, I would like to see something like:

Foo foo = spy(new Foo());
doReturn("foo").when(foo).invokesStatic(Util.class).makeString();
assertThat(foo.bar()).isEqualTo("foo");

What do you think? As for a hack, I am missing a component in Byte Buddy which I was supposed to write for a customer at some point what did however not pull through. I am not currently in a position to spend so much time to build this component, due to my personal situation, but please prototype away. I think the API and spec work is crucial to make this a success.

@mockitoguy
Copy link
Member Author

@mockitoguy mockitoguy commented Apr 3, 2017

Great feedback! Thank you Rafael for sharing your thoughts.

Is the component you mentioned a lot of work in ByteBuddy? Is this something that we can ask the community to help out or you're the best person to take on?

I'd rather see more robust API but your API idea is certainly workable. Mocking statics should be rare and our API should be optimized for common use cases and not edge/rare cases. We can always iterate based on feedback :)

Here's my preference as of today:

//setup
mockStatic(Util.class);

//then
doAnswer().when(() -> Util.foo());
when(Util.foo()).thenReturn("foo");
verify(() -> Util.foo());
verify(never(), () -> Util.foo());

If we choose to support mocking static methods, I'd rather offer API that is robust enough to support all kinds of use cases. I like when tools are separated from policies and are opinionated but not dogmatic :) If we think that mocking static is disgusting, we can ship it in a different jar called "mockito-for-crappy-code", loosely following @TimvdLippe idea.

The main use cases for mocking statics I see are:

  • legacy code (I really, really want to write a unit test but I don't dare to change some ancient ugly code)
  • dealing with some awkward static 3rd party APIs. This scenario is currently workable by developing some injectable API layer on top of 3rd party statics. However, the workaround could be cumbersome and can spoil the clarity of codebase.

Without addressing above 2 use cases, developers around the world will be looking for help in tools like Powermockito, and such.

The biggest downside of offering static mocking is making it too easy to test crappy, procedural code full of static methods. We would remove a motivation to refactor the code into clean OO / DI.

@TimvdLippe
Copy link
Contributor

@TimvdLippe TimvdLippe commented Apr 3, 2017

Should we also send out a Tweet to gather some community insights?

@tmurakami
Copy link
Contributor

@tmurakami tmurakami commented Apr 3, 2017

If this feature uses the Java Instrumentation API, it will not work on Android devices, so it might confuse Android developers.
If so, I think it should not be integrated into the mockito-core artifact.

@bencampion
Copy link

@bencampion bencampion commented Apr 3, 2017

I came across this issue by chance (I was just curious to know what you were planning for Mockito 3), but I'd like to share my experiences of working with static method mocking.

I used to work on several large codebases that made extensive use of static methods and used PowerMock (and PowerMockito) heavily for testing. My main memories of that time were that the tests were really slow to run (in some cases tests that use PowerMock were literally ten times slower than those that didn't) and that we ultimately had to remove all usages of PowerMock because it wasn't compatible with newer versions of Java. I also recall there being incompatibilities with some libraries were using that required special setup in the tests to make them pass, although I no longer remember the details of that.

I can understand concerns about promoting bad code, but as a Mockito user I'd be more worried about the impact on performance and robustness. Not all of the code we were using PowerMock for was legacy. Some of it was new code written by an inexperienced team (of which I was part of) and knowing that Mockito devs disapproved of our design patterns probably wouldn't have made any difference.

This was a few years ago now and techniques for mocking static methods may have improved significantly since then. If you think there's a performant and robust way to implement this feature then I'd welcome it (even though I'd hope I never have to use it).

@mockitoguy
Copy link
Member Author

@mockitoguy mockitoguy commented Apr 3, 2017

Really good feedback, thank you guys. I helps us make good decisions about the features/API we plan.

@szpak
Copy link
Member

@szpak szpak commented Apr 3, 2017

In my opinion in general testing static methods is a bad idea and it would be better to refactor the code, however, sometimes it's something that is needed. For instance, testing a default method given() in mockito-java8 interface delegating to a static method in BDDMockito.given() the easiest solution I see is to generate in runtime a list of static methods in BDDMockito and execute parameterized test for every of them verifying that a corresponding method in WithBDDMockito interface delegates to it with proper parameters. In that case static method mocking would be beneficial. Maybe I will wait for PowerMockito for Mockito v2 to test Mockito itself...

Alternatively I would need to play with AOP and load-time weaving, code generation in runtime or some other not very pleasant to use technique (unless you have some good ideas how to do it easier).

@mockitoguy
Copy link
Member Author

@mockitoguy mockitoguy commented Apr 4, 2017

@szpak, thank you for the feedback! Interesting use case.

I think we all agree that mocking statics is a bad practice and an anti-pattern in general. The question is whether we enforce the "no mocking statics" policy (e.g. not offer the feature in the tool) or let the user decide to enforce it or not (e.g. offer the feature).

@Krazybug
Copy link

@Krazybug Krazybug commented Apr 4, 2017

Please, consider the legacy code issue. Sometimes you don't have any choice.
Enforcing or not a practice shouldn't be the decision of a framework but of the team.
Also, for now, we're using Powermock as a workaround for this need but it's not compliant with the last versions of Mockito. We're blocked with 1.9.x version of Mockito.
It's another good reason to get this feature: Limit dependencies.

We already need all these libraries to get decent unit test in pure Java world:
JUnit,
JUnitParams as Junit Parametrized are awfull
AssertJ for expressive and clear assertions
Mockito ... and Powermock

and i shouldn't mention other peripheral libraries:
DBUnit,
SpringDBunit
JBehave, JGiven for BDD style ....

Then you can write all your testing stuf in Groovy and you only need ... Spock

Please! Let user choose. Othewise, why did you provide private method mocking in the recent versions athough it's considered as a "bad" practice for TDD purists ?

Finally, thanks for your great work. Mockito is really a nice framework

@jlink
Copy link

@jlink jlink commented Apr 4, 2017

Mocking static methods is a different use case, maybe that means it should be a different tool.

Answer the following questions (to yourself):

  • Do you want to support a double tool in the long term?
  • Does it use the same set of dependencies and only those?
  • Does mocking static methods fall nicely into the current design?

If you answer all 3 with "Yes", then go ahead. If at least one is a "No" consider the alternatives:

  • Build a Mockito extension
  • Build a new tool and call it Mackarita
  • Leave the other tool to someone else

Just my two cents.

@karollewandowski
Copy link

@karollewandowski karollewandowski commented Apr 4, 2017

I agree with most of you and think that mocking static methods is not a good idea.
If some method uses static method provided by legacy library/framework, then it can be easily wrapped in object and provided as mockable dependency. It's not perfect solution, but is educational - shows developers that writing too complex static util methods hurts and makes code hard to test. If Mockito had such feature, many of developers would consider it as acceptable practise and produce bad code.
Another issue is parallel tests execution. In my current project we used to use PowerMockito to mock static methods and after some time our tests started to fail because of concurrency issues:

  1. Thread A mocked static method X.y and stopped.
  2. Thread B mocked static method X.y and stopped.
  3. Thread A was awaken and run test - it failed because thread B overwritten static method's behaviour expected in test run by A.

We ended up with wrapping static methods in injectable objects and removed PowerMockito from project dependencies.

@rdicroce
Copy link

@rdicroce rdicroce commented Apr 4, 2017

I had asked about this at some point in the past and was told it was being considered. Glad to see that's actually happening.

In my opinion, support for mocking static methods is a good idea for the simple reason that the standard Java classes that ship with the JRE have tons of static methods. Consider the NIO.2 API as an example. If you want to read a file using that API, you might do something like:

Files.readAllLines(Paths.get("myfile"));

Now, is there a way to test this without mocking static methods? Obviously yes; you could make a IFiles interface and then make a JREFilesImpl that passes the calls through to the static methods. But that's a bunch of extra effort, it adds complexity to your code, and obscures the methods that are actually being called. In my opinion, I shouldn't need to do that to test my code's ability to properly read some file.

So +1 for the ability to mock static methods.

@karollewandowski
Copy link

@karollewandowski karollewandowski commented Apr 4, 2017

Now, is there a way to test this without mocking static methods?

Well, in my opinion file name/path from your example is good candidate for passing it to method/setter/constructor instead of hardcoding it and you can just create test file in filesystem. Such Java API methods are considered as simple and reliable and there is no need to mock them like you wouldn't mock java.util.List if your method operated on data from given list. You would just create actual list with test data.

@rdicroce
Copy link

@rdicroce rdicroce commented Apr 4, 2017

There are at least 2 problems with that argument: a) you can't test behavior when exceptions occur, and b) you can't test behavior if the path is for a different OS. I realize the latter is an esoteric use case, but I actually am working on a project that is developed on Windows but runs exclusively on Linux.

@TimvdLippe
Copy link
Contributor

@TimvdLippe TimvdLippe commented Apr 4, 2017

I think the recurring theme right now is: either you are pro or you are strictly against it. Personally, I am for clean tests and therefore consider static mocking very bad practice. However, our users apparently have usecases that require (intrusive?) solutions such as PowerMock. Given that these users opt for such solutions signals that the other solution would be no tests at all, and that is probably what we would never want.

Given that solutions like PowerMock are built not only on Mockito, but also other libraries, they can not be easily updated like Mockito can. This incentives Mockito to solve static mocking, if users opt-in for it.

Even though I inherently disagree with static mocking, I think it is only fair that we as Mockito developers offer users the option to opt-in a less intrusive and upgrade-compatible solution. Therefore, I would vouch for a different artifact (probably called mockito-legacy), which offers static mocking. This should signal our users that this is for legacy purposes and should be strictly discouraged, but at least gives them a way out. This particular solution would tick off all points of @jlink

In the end, no matter which outcome, there will always be disappointed developers. We will not be able to find a golden solution for this particular problem and I am fairly convinced it will never happen either.

We can always try it out with the new artifact and discontinue it later if it is significantly misused.

One sidenote: putting everything in a new artifact would scatter the mockito ecosystem. We should probably find a better solution for this, potentially in Mockito 3.

@marcingrzejszczak
Copy link
Collaborator

@marcingrzejszczak marcingrzejszczak commented Apr 5, 2017

I disagree with @TimvdLippe. I don't believe that in an open source world you can "try it out with the new artifact and discontinue it later if it is significantly misused.". Once it's there in the library users will require it to be there. You'll have to support that feature etc. Either it's there or not. Not often can you easily deprecate sth and roll it back afterwards. Typically it requires a couple of release cycles.

@TimvdLippe you're mentioning this " However, our users apparently have usecases that require (intrusive?) solutions such as PowerMock". I have a comment on that. As an exercise from time to time I'm trying to answer PowerMock questions on StackOverflow. Typically someone says that there's a static method somewhere that they're calling and it does some logic that they want to stub. The question I often ask to those people is "Why are you doing this?". Would you really want to stub a call to StringUtils ? Another frequent problem is that the design is flawed and that's why one is looking for hacks. (e.g. http://stackoverflow.com/questions/37059406/how-can-i-get-powermock-to-return-the-expected-value-from-a-static-method/37066436#37066436 , http://stackoverflow.com/questions/37052069/program-termination-during-quartz-scheduler-verification-using-power-mockito/37066521#37066521 , http://stackoverflow.com/questions/31840964/powermock-private-method-with-null-pointer-exception/37066400#37066400 or http://stackoverflow.com/questions/32195928/getting-powermockito-to-mock-a-static-method-on-an-interface/32537392#32537392)

That's why I fully agree with @karollewandowski . "Every problem can be solved with a layer of abstraction" ;) You can't mock a static method? Ok then, wrap it in a class which you can stub. Is it wrong? Quite the contrary. Rarely should you call static methods directly and if you do, most likely these are utils that you don't want to stub. You should encapsulate the logic of a static method in an object that makes business sense to use it.

Summing it up giving people a tool to stub static methods is making them even easier to write bad code. Instead of thinking of how to fix the design they'll be able to make the design even worse.

@jlink
Copy link

@jlink jlink commented Apr 5, 2017

@TimvdLippe Just one thing: If you create a new artifact like "mockito-legacy" communicate clearly if you're planning to support it mid/long-term or if it's just an experiment. Some people are willing to experiment together with you, others will be pissed off when you quit support after they have heavily used it.

@marcingrzejszczak
Copy link
Collaborator

@marcingrzejszczak marcingrzejszczak commented Apr 5, 2017

@rdicroce I completely disagree with this statement:

Now, is there a way to test this without mocking static methods? Obviously yes; you could make a IFiles interface and then make a JREFilesImpl that passes the calls through to the static methods. But that's a bunch of extra effort, it adds complexity to your code, and obscures the methods that are actually being called. In my opinion, I shouldn't need to do that to test my code's ability to properly read some file.

You're design is wrong. If you had

Files.readAllLines(Paths.get(myFilePath));

You could via a constructor inject myFilePath to point to your test resources. You don't even need to create any additional classes.

@ChristianSchwarz
Copy link
Contributor

@ChristianSchwarz ChristianSchwarz commented Apr 5, 2017

The Android community would like mock static methods, well at least me.
Why? : The Android SDK provides many static utility methods with classes like TextUtils. The downside is that there implementation is only provided on the device or emulator. When users run Unit-Test on there local machine they will get the famous "Method not mocked" exception.

The android.jar file that is used to run unit tests does not contain any actual code - that is provided by the Android system image on real devices. Instead, all methods throw exceptions (by default). This is to make sure your unit tests only test your code and do not depend on any particular behaviour of the Android platform (that you have not explicitly mocked e.g. using Mockito).

They are many workarounds for this issue like PowerMock or Roboelectric. Roboelectric is a great testing framework but it is damn slow and maintainance intensiv. PowerMock is not stable state to be used with Mockito 2. So I think at least Android users will love this feature.

@dbacinski
Copy link

@dbacinski dbacinski commented Apr 5, 2017

@ChristianSchwarz I do not agree, using Android static utils or Android framework in your business logic is a bad smell. Well structured Android app do not need mocking static methods, nor Roboelectric. Calls to Android framework must be abstracted and current Mockito implementation helps to enforce that. If you want to test view layer then go with Instrumentation tests and Espresso.

@mockitoguy
Copy link
Member Author

@mockitoguy mockitoguy commented Apr 5, 2017

@dbacinski, I am not an expert on Android so bear with me, please :) Adding extra layer introduces more method calls which can be problematic on large apps that hit dex method limit, right?

@dbacinski
Copy link

@dbacinski dbacinski commented Apr 5, 2017

@szczepiq you are right that you need additional methods to abstract Android apis, but this is a cost of good architecture. Dex method limit is not a huge problem anymore because now there is native platform support for multidex apps.

@rdicroce
Copy link

@rdicroce rdicroce commented Apr 5, 2017

@marcingrzejszczak Your response a) does not address the call to readAllLines(), and b) does not address either of the issues I raised in #1013 (comment)

To all: it seems to me there's a fundamental divide in this thread between people who are of the opinion that it's fine to just make a wrapper class for static calls, and people who are of the opinion that wrapper classes add bloat and shouldn't be necessary. I don't see either side convincing the other, so I'm not sure where that leaves us.

@marcingrzejszczak
Copy link
Collaborator

@marcingrzejszczak marcingrzejszczak commented Apr 5, 2017

Your response a) does not address the call to readAllLines(), and b) does not address either of the issues I raised in #1013 (comment)

@rdicroce I haven't explicitly but the answer is simple

a) does not address the call to readAllLines(),

If the path is a parameter you don't have to mock the call at all. You can pass

a) a path that exists - to test the positive scenario
b) a path that doesn't exist - that way it will blow up and you'll test an exception

. I realize the latter is an esoteric use case, but I actually am working on a project that is developed on Windows but runs exclusively on Linux.

You can write a couple of tests that are exclusively executed depending on the OS (example for Windows - http://stackoverflow.com/questions/23410738/run-unit-tests-only-on-windows ). Again, if you use parameters instead of magic values then you can do basically whatever you want.

To all: it seems to me there's a fundamental divide in this thread between people who are of the opinion that it's fine to just make a wrapper class for static calls, and people who are of the opinion that wrapper classes add bloat and shouldn't be necessary. I don't see either side convincing the other, so I'm not sure where that leaves us.

I'd say that the divide is between people who want to design their code properly and those who want to take the path of least resistance (which isn't a wrong choice sometimes).

@pioorg
Copy link

@pioorg pioorg commented Apr 5, 2017

I'd say that the divide is between people who want to design their code properly and those who want to take the path of least resistance.

Static members aren't something good, I hope it's quite obvious. But Java has them and will support them, whether we like it or not. So I'd say that educating and influencing is good, forcing might be not. It's like "oooh, your code is using statics, so it can't be tested with Mockito".
(Not to mention that some people would like to have nice unclebobish tests but aren't allowed to refactor the code they have to maintain. I know it's sick, but hey, this is reality sometimes.)

@raphw
Copy link
Member

@raphw raphw commented Jun 19, 2020

Alright, something bit me yesterday and I built a POC with a slightly different approach. What always bothered me about static mocks was their potential inferrence with parallel test cases and so forth, but all of this is solvable. I put the working prototype on a branch and suggest this API:

https://github.com/mockito/mockito/blob/static-mock/subprojects/inline/src/test/java/org/mockitoinline/StaticMockTest.java

I'll clean it up and add documentation. Feedback welcome!

@CharlieReitzel
Copy link

@CharlieReitzel CharlieReitzel commented Jun 19, 2020

Hi Rafael, I like the scoping of the mock via Closable interface. +1. Also, Java 8 method references make a lot of sense in this context.

Just out of interest, this looks a bit different than Christian's use of lambdas. Or does it work out the the same thing (w/ the Java compiler handing Mockito a Function to mock, either way)?

@TimvdLippe
Copy link
Contributor

@TimvdLippe TimvdLippe commented Jun 19, 2020

Thanks @raphw for that and glad to see some progress on it. I do want to reiterate that you should not take this project on if you do not have the mental/physical capacity to do so. As a fellow maintainer, I would prefer if you stay in the running, rather than overloading you with this project.

Let us know if we can assist you in any matter possible. Very glad to see an elegant solution this problem, so props for figuring this out! Evidence yet again we can be lucky we have you on our side working on these problems 😄

@ChristianSchwarz
Copy link
Contributor

@ChristianSchwarz ChristianSchwarz commented Jun 19, 2020

@raphw glad you took some inspiration and motivation out of my attempt. I had quick look into your branch, looks like you are going full steam ahead.

Some questions and toughts:

  • The scope meant to automatically reset the static mock after usage, right?

    • It is safe when used with "try with resource" but people might use it without, its not possible to enforce it.
    • The try with resource adds noise / ceremony to the test it self , an other option is to do the reset automatically after a test.
    • How to mock multiple static classes?
    • Can I stub a static method in a before-each so it is not required to stub it in every test?
  • On twitter you wrote the static mocks are thread local

    • What if I am testing a larger component that runs code in multiple threads, how to do that?
raphw added a commit that referenced this issue Jun 19, 2020
raphw added a commit that referenced this issue Jun 19, 2020
@raphw
Copy link
Member

@raphw raphw commented Jun 19, 2020

Hi Christian,

thanks for your prototype. It helped me to put the last bits of the puzzle on its place. The thing that bothers me the most with static method mocks is the inability to run tests in parallel. It can really add a lot of confusion since not everybody expects it, for example when adjusting a build script. This way, it is fully safe. I think for now, we leave it this way; it would however not be difficult to add a MockSettings option to include additional threads or to even make the mock global. I still think we leave it out of the first version and consider it for later.

Mocking multiple classes for static methods is fully possible:

try (
  MockedStatic<Foo> foo = Mockito.mockStatic(Foo.class);
  MockedStatic<Bar> bar = Mockito.mockStatic(Bar.class)
) {
  // ...
}

The only thing that is not allowed is to mock the same class twice but Mockito will detect this and throw an exception. I also adjusted the JUnit integration to make the ceremony superfluous. You can simply do:

@Mock 
MockedStatic<Foo> foo;

in a JUnit test when using the rule or extension and the same works in JUnit 5's extension for method parameters. Mockito will then take care of the lifecycle. If the explicit model is used and the mock is not closed, it is currently unsafe. We could however add functionality to clear all mocks on a thread within the runner and rules where I do however expect the annotations to be used mostly.

raphw added a commit that referenced this issue Jun 19, 2020
raphw added a commit that referenced this issue Jun 19, 2020
raphw added a commit that referenced this issue Jun 19, 2020
raphw added a commit that referenced this issue Jun 19, 2020
raphw added a commit that referenced this issue Jun 21, 2020
raphw added a commit that referenced this issue Jun 21, 2020
@ChristianSchwarz
Copy link
Contributor

@ChristianSchwarz ChristianSchwarz commented Jun 24, 2020

@raphw I know I am a little late to the party. Is it possible to simplify the API a bit so users are not forced to use the MockStatic instance like this:

try(MockedStatic<Dummy> ignore = Mockito.mockStatic(Dummy.class)) {
            when(Dummy::foo).thenReturn("bar");
            verify(Dummy::foo);
            
}

Once the Verification-Lambda is executed you know that Dummy.foo() was called. Then you can map the Dummy.class to the corresponding MockedStatic object. This way users don't need to reference the MockedStatic object. This also allows to simplify futher by obmitting the try-with-resource. E.g:

Mockito.mockStatic(Dummy.class, ()=>{  //Note: The lambda defines the scope of the static mock
  when(Dummy::foo).thenReturn("bar");
  verify(Dummy::foo);
});
@raphw
Copy link
Member

@raphw raphw commented Jun 25, 2020

I can see for myself that we add something like:

Mockito.mockStatic(Dummy.class).scope(() -> { 
  ... 
});

mockStatic is already overloaded, I would like to avoid another overload to keep it synchronous with mock. Not sure if I like the lambda syntax. It works nicely with no-arg methods but not so much otherwise. I think at least that introducing it into the standard API should be another ticket, if accepted.

@ChristianSchwarz
Copy link
Contributor

@ChristianSchwarz ChristianSchwarz commented Jun 25, 2020

Thank Rafael you for the constuctive discussion and considering other opinions and approches. Such a behaviour is rarly found !

Not sure if I like the lambda syntax.
Yes I know what you mean, it adds noise.

Talking about the scope: An other option is to enable static mocks via@StaticMock or staticMock(Class) and then reset/disable them automatically after each test. If required users can call resetStatic(Class) in the test.

@TimvdLippe
Copy link
Contributor

@TimvdLippe TimvdLippe commented Jun 25, 2020

I don't think I would support a lambda-solution or a solution that relies on a return type. If we would return a special interaction method, where you later have to call close() yourself, then you can assign this to any variable/field and make it longlived. This opens up a whole can of worms with working with static method mocking.

Therefore, for me, one requirement for static method mocking is that it is explicitly limited to scope and can't escape that scope. The try-solution addresses this point, but the lambda return call does not.

@ChristianSchwarz
Copy link
Contributor

@ChristianSchwarz ChristianSchwarz commented Jun 25, 2020

When you use the lambda you will be safe, the static mock will only life in the scope of the lambda.

Mockito.mockStatic(Dummy.class).scope(() -> { 
  ... 
});

The mock is generated when you call the scope(..) method, then the lambda/scope is executed.After that the mock will be closed/reset. There is no way to escape the scope. But this extra method and lamda add complexity, same goes for the overloaded mockStatic method that accepts the scope/lambda.


The MockedStatic approach don't give you guarantees if the user doesn't use try-with resource like this:

var dummy= Mockito.mockStatic(Dummy.class);
dummy.when(Dummy.foo()).thenXXX();

The mocking/stubbing/verifing will work as expected but leaves the Class in a mocked state.

The automatic close/reset at the end of a test is an option that can be considered.

@mkq
Copy link

@mkq mkq commented Jun 25, 2020

Replacing Mockito.mockStatic(klass, () -> { … }) with the fluent Mockito.mockStatic(klass).scope(() -> { … }) may look nice (to others, not necessarily to me :-)), but then the compiler won't complain when the scope is missing.
If you want to force a scope, please make it a parameter of (all overloaded) mockStatic.

@raphw
Copy link
Member

@raphw raphw commented Jun 25, 2020

The problem with only allowing it in a given lambda scope is that any alternartive life-cyle management approaches are not supported. Having the construct available for example allows for the use in Junit extensions and rules.

The try-with-resources construct can be forgotten but it is also the most flexible option. I suggest we stick to it for the first release.

raphw added a commit that referenced this issue Jul 2, 2020
raphw added a commit that referenced this issue Jul 2, 2020
raphw added a commit that referenced this issue Jul 9, 2020
raphw added a commit that referenced this issue Jul 10, 2020
@raphw raphw closed this in #1955 Jul 10, 2020
raphw added a commit that referenced this issue Jul 10, 2020
Mockito #1013: Defines and implements API for static mocking.
@kriegfrj
Copy link
Contributor

@kriegfrj kriegfrj commented Jul 31, 2020

I'm late to this conversation, and I'm glad to see that there is some movement on this. I am firmly in support of the addition of static method mocking to Mockito. It is true that there are workarounds - someone wrote: "Every problem can be solved with a layer of abstraction" - but then there is the corollary that seems to be forgotten: "...except for the problem of too many layers of abstraction." Simplicity should be a goal of code - we shouldn't add complexity without good reason, and layers of abstraction add complexity at all levels (in development and at runtime). In the case of mocking statics, it is certainly possible to get around this problem (to an extent) by adding a new layer of abstraction - but this adds complexity. We may often decide the trade-off is worth it to make our code testable, but wouldn't it be better if you didn't have to make the trade-off? If you could mock statics without paying the price of adding an extra level of indirection? Also, adding a layer of abstraction is only possible in code that I have control over. What if I'm using a third-party library? System.currentTimeMillis() is a classic example - there's no good way to reliably simulate code running at different system times without mocking this static method (you can use wrappers in your own code, but there's no guarantee that all the 3rd-party libraries you might want to use will).

I also wanted to throw a couple of things into the mix, in commentary on the current implementation-in-progress:

  1. constructors are effectively static methods. I think it would be great if whatever mechanism is implemented here to mock static methods can also be used to mock constructors. Doing so gives you a way to inject mock instances of dependencies into your class-under-test without requiring you to rewrite the class-under-test for this purpose. JMockit provides such a capability, and I would love to see Mockito have it too (as JMockit doesn't play will with OSGi, whereas Mockito does).
  2. There has been talk about the temporal scope of the static mock, but I think that consideration also needs to be given to the caller scope: which classes will see the mocked method and which will see the original. I'm thinking here in particular the case of mocking methods of common classes (eg, JDK classes) that might also be in use by the test framework. The test framework will rely on the normal behaviour of the mocked class while you will want the class-under-test (and its dependencies) to see the mocked behaviour. It wouldn't be a good idea to mock (eg) System.currentTimeMillis() if it was going to be seen by the test framework as it will mess up the running of the test, but it would be great to be able to mock it for the code-under-test.
@raphw
Copy link
Member

@raphw raphw commented Jul 31, 2020

Thanks, this is about my train of thought. Additionally, it is sometimes just not feasible to refactor code. Either the code is not yours or you cannot change a legacy code base (immediately). In these cases, it is very convenient to have the possibility to mock static methods, also to give you safety if you are changing code in intermediate steps to finally remove them.

Construction mocks are in the making and will be part of 3.5.0. You can have a look at my open PRs if you want to give it a test run. It's fully working and I am only waiting for the Mockito team members to return from their vacations to get some feedback from them before merging.

Finally note that Mockito forbids mocking the static methods of System (and Thread). Those methods are to much cemented into class loading which happens in the same thread. At some point, we might add instrumentation to class loading to temporarily disable the static mocks within it to make mocking these classes, too, where we also would need to disable their intensification properties. You can however easily mock Instant.now().

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

Successfully merging a pull request may close this issue.

You can’t perform that action at this time.