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

Fixes #2548 : Makes InOrder able to verify static methods #2549

Conversation

mirkoalicastro
Copy link
Contributor

@mirkoalicastro mirkoalicastro commented Jan 24, 2022

This PR makes it possible to verify static methods calls in order. Fixes: #2548

Example of usage:

try (MockedStatic<Foo> mocked = mockStatic(Foo.class)) {
  InOrder inOrder = inOrder(Foo.class);
  mocked.when(Foo::firstMethod).thenReturn("first");
  assertEquals("first", Foo.firstMethod());

  inOrder.verify(mocked, Foo::firstMethod);
}

You can also pass a specific VerificationMode as third argument to the InOrder#verify method.

Checklist

  • Read the contributing guide
  • PR should be motivated, i.e. what does it fix, why, and if relevant how
  • If possible / relevant include an example in the description, that could help all readers
    including project members to get a better picture of the change
  • Avoid other runtime dependencies
  • Meaningful commit history ; intention is important please rebase your commit history so that each
    commit is meaningful and help the people that will explore a change in 2 years
  • The pull request follows coding style
  • Mention Fixes #<issue number> in the description if relevant
  • At least one commit should mention Fixes #<issue number> if relevant

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 for the PR! Overall, the implementation looks solid. I only have a couple of nits with regards to the tests.

My only concern is the difference in the class passed into inOrder and the class we are actually interacting with. I guess this currently works, as we are passing in a Class in the List<Object>, but that feels a bit weird to me. Not sure WDYT?

I think to fix that, we would need a similar method to objectIsMockToBeVerified, but in this case verify that the context is correct. However, we are only passing in the lambda, which doesn't necessarily match to the Class we put in.

Therefore, we maybe have to look into passing the mockedStatic in the inOrder method instead and omit it as an argument from the inOrder.verify call?

@mirkoalicastro
Copy link
Contributor Author

Thanks for the PR! Overall, the implementation looks solid. I only have a couple of nits with regards to the tests.

My only concern is the difference in the class passed into inOrder and the class we are actually interacting with. I guess this currently works, as we are passing in a Class in the List<Object>, but that feels a bit weird to me. Not sure WDYT?

I think to fix that, we would need a similar method to objectIsMockToBeVerified, but in this case verify that the context is correct. However, we are only passing in the lambda, which doesn't necessarily match to the Class we put in.

Therefore, we maybe have to look into passing the mockedStatic in the inOrder method instead and omit it as an argument from the inOrder.verify call?

Thanks for your feedback @TimvdLippe ! I have addressed the comments and pushed the changes related to the tests.

I see your concern, and actually, passing the mockedStatic to the inOrder method was my initial idea. Here is the reason why I moved away from that.
Let's assume we have two static mocks (two different classes), and we want to verify them in order. Then, we'd pass two MockedStatic instances to the inOrder method.
When it comes to the verification, we'd have InOrder#verify(Verification), where Verification is a lambda (no parameters, void return type) because MockedStatic#verify deals with a lambda. Given a lambda, we can't assert if it contains a static method reference, or contains just some statements. That is, we can't retrieve the Class instance. Without this, we can't select and use the appropriate MockedStatic instance to verify the lambda.
Moreover, the above assumes we have the Class instance of the MockedStatic somehow available in InOrderImpl, but currently, that is not the case. Of course, we could add a method to MockedStatic that accepts a Class and returns true if the Class is the same as what is mocking (package private maybe?).
I see your concern, but I think we have the same situation in the MockedStatic#verify methods, where we could pass things not relevant to the context. But I can't see a different way to solve it otherwise, if possible.
WDYT?

@codecov-commenter
Copy link

codecov-commenter commented Feb 16, 2022

Codecov Report

Merging #2549 (fd2b2b6) into main (8cdf0cc) will increase coverage by 0.04%.
The diff coverage is 100.00%.

❗ Current head fd2b2b6 differs from pull request most recent head 94153ec. Consider uploading reports for the commit 94153ec to get more accurate results

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #2549      +/-   ##
============================================
+ Coverage     86.71%   86.75%   +0.04%     
- Complexity     2784     2796      +12     
============================================
  Files           320      321       +1     
  Lines          8341     8368      +27     
  Branches       1021     1024       +3     
============================================
+ Hits           7233     7260      +27     
  Misses          840      840              
  Partials        268      268              
Impacted Files Coverage Δ
src/main/java/org/mockito/InOrder.java 100.00% <100.00%> (ø)
...rc/main/java/org/mockito/internal/InOrderImpl.java 100.00% <100.00%> (ø)
...kito/internal/stubbing/answers/InvocationInfo.java 100.00% <100.00%> (ø)

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 8cdf0cc...94153ec. Read the comment docs.

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.

Awesome, thanks for the explanation (makes sense to me) and the nice PR + tests! Merging 🎉

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.

[PR open] Add feature to verify static methods calls in order
3 participants