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

JUnit rule logs warnings about unsued / misused stubs #384

Closed
mockitoguy opened this issue Apr 7, 2016 · 15 comments
Closed

JUnit rule logs warnings about unsued / misused stubs #384

mockitoguy opened this issue Apr 7, 2016 · 15 comments
Assignees
Milestone

Comments

@mockitoguy
Copy link
Member

@mockitoguy mockitoguy commented Apr 7, 2016

Why

When test fails, the failure might be caused by misused stubs. Hence, it might be worth to log out debugging information to the System out (e.g. misused stubs, unused stubs). For more details see documentation for MockitoHint.

Plan

  • document 2.x change
  • tweak the warning message
  • add behavior to the runner
  • add silent runner
  • add silent setting to the rule (or warnings level for all / exception only / none)
  • deprecate/remove console spamming runner

Impl

The JUnit rule or the runner will potentially include following info in the output (somehow): https://gist.github.com/szczepiq/38619fd8766c66c46dff4dc2b6db06ec

I'm wondering if "Mockito.validateMockitoUsage();" should automatically print warnings. We could provide boolean parameter to control printing of the warnings.

@mockitoguy mockitoguy added this to the 2.0 milestone Apr 7, 2016
@mockitoguy mockitoguy changed the title Make JUnit rule more useful JUnit rule on test failure logs warnings about unsued / misused stubs Apr 13, 2016
mockitoguy added a commit that referenced this issue Apr 13, 2016
See issue #384

When test fails, it is useful to know about unused / misused stubs.
mockitoguy added a commit that referenced this issue Apr 18, 2016
mockitoguy added a commit that referenced this issue Apr 18, 2016
Making the output look better in real tests.

See issue #384
mockitoguy added a commit that referenced this issue Apr 18, 2016
Tweaked the output and improve test coverage (the case of multiple different kinds of stubbing)

See issue #384
mockitoguy added a commit that referenced this issue Apr 18, 2016
Tweaked the output again.

See issue #384
mockitoguy added a commit that referenced this issue Apr 18, 2016
Top level test now asserts on full content of the stubbing information. Added handy method for making assertions stable and the test easier to work with.

See issue #384
@TimvdLippe TimvdLippe added the ready label Jul 12, 2016
@TimvdLippe TimvdLippe modified the milestones: 2.1, 2.0 Jul 13, 2016
@mockitoguy
Copy link
Member Author

@mockitoguy mockitoguy commented Jul 24, 2016

On it.

@mockitoguy mockitoguy self-assigned this Jul 24, 2016
mockitoguy added a commit that referenced this issue Jul 31, 2016
I'm planning to rework the current behavior of JUnit rules, so that the rule by default:
 - on test failure print the stubbing mismatch
 - when test passes show unused stubs

It will use the new StubbingListener interface. WarningsCollector class and friends will be deleted.
mockitoguy added a commit that referenced this issue Jul 31, 2016
Added a new style of events that the StubbingListener can listen to.
I need information when the mock invocation was triggered but no stubbed behavior was registered for this invocation.
mockitoguy added a commit that referenced this issue Jul 31, 2016
mockitoguy added a commit that referenced this issue Jul 31, 2016
mockitoguy added a commit that referenced this issue Jul 31, 2016
mockitoguy added a commit that referenced this issue Jul 31, 2016
This class was only used in tests, so it's better if it lives in test source tree.

Issue #384
mockitoguy added a commit that referenced this issue Jul 31, 2016
Removed unnecessary conditional logic

Issue #384
mockitoguy added a commit that referenced this issue Jul 31, 2016
mockitoguy added a commit that referenced this issue Jul 31, 2016
mockitoguy added a commit that referenced this issue Jul 31, 2016
mockitoguy added a commit that referenced this issue Jul 31, 2016
Busted bigger class into something smaller

Issue #384
mockitoguy added a commit that referenced this issue Jul 31, 2016
Ensured the stubs are reported in the order they are declared. This is not unit testable cleanly.

Issue #384
mockitoguy added a commit that referenced this issue Jul 31, 2016
Reporting needs to be based on the actual invocation, not based on String representation of the location.

Issue #384
mockitoguy added a commit that referenced this issue Jul 31, 2016
- more cases covered
- more coverage

Issue #384
mockitoguy added a commit that referenced this issue Jul 31, 2016
mockitoguy added a commit that referenced this issue Aug 12, 2016
Issue #384
mockitoguy added a commit that referenced this issue Aug 13, 2016
Following great feedback from Christian.

Issue #384
mockitoguy added a commit that referenced this issue Aug 13, 2016
Following neat feedback from Christian.

Issue #384
@TimvdLippe TimvdLippe modified the milestones: 2.0, 2.1 Aug 14, 2016
mockitoguy added a commit that referenced this issue Aug 14, 2016
Following neat feedback from Brice.

Issue #384
mockitoguy added a commit that referenced this issue Aug 14, 2016
mockitoguy added a commit that referenced this issue Aug 14, 2016
Issue #384
mockitoguy added a commit that referenced this issue Aug 14, 2016
- this commit squashes 50+ commits
- introduces new MockitoListener API

Fixed issue #384
@TimvdLippe TimvdLippe removed the in progress label Aug 16, 2016
mockitoguy added a commit that referenced this issue Aug 19, 2016
 - unused stubbing detection logic was not thread safe in Mockito JUnit runner
 - stopped using the StubbingListener - by design it couldn't support thread safe scenarios unfortunately :(

Fixes #384
mockitoguy added a commit that referenced this issue Aug 19, 2016
 - unused stubbing detection logic was not thread safe in Mockito JUnit runner
 - stopped using the StubbingListener - by design it couldn't support thread safe scenarios unfortunately :(

Fixes #384
@t1
Copy link

@t1 t1 commented Aug 29, 2016

@szczepiq: I like this feature very much. Also that the MockitoJUnitRunner fails by default is good. I think a note in the javadoc of UnnecessaryStubbingException to use MockitoJUnitRunner.Silent would be helpful.

We often have quite complex test fixtures, and it's cumbersome to switch absolutely each and every stub on or off... some methods simply have to be there; it's also not necessary to verify if they've been invoked. So it would be great to be able to disable it for some stubs, but leave it on for others.

@TimvdLippe
Copy link
Contributor

@TimvdLippe TimvdLippe commented Aug 29, 2016

@t1 That seems like a reasonable idea. Want to make a PR for that?

@mockitoguy
Copy link
Member Author

@mockitoguy mockitoguy commented Aug 29, 2016

Good feedback. I think that MockitoHint class should also mention that it is possible to silence the new mechanism.

@mockitoguy
Copy link
Member Author

@mockitoguy mockitoguy commented Aug 29, 2016

@Sarev0k
Copy link

@Sarev0k Sarev0k commented Oct 26, 2016

The MockitoHint documentation seems to indicate that this will give you warnings about mismatched arguments, when using the MockitoJUnitRunner. However, from my experience this doesn't seem to work with Mockito 2.1.

Here's the source I'm trying:

Foo foo = mock(Foo.class);
when(foo.bar("baz")).thenReturn("bar");

foo.bar("baz");
foo.bar("test"); // expected to get a warning here

Is there something else I need to do to enable this functionality?

@mockitoguy
Copy link
Member Author

@mockitoguy mockitoguy commented Nov 1, 2016

Thank you for feedback!

Is there something else I need to do to enable this functionality?

This is how they were implemented. Runner should also report mismatches because it helps with debugging. The reason it does not do it today is because I wanted to limit the noise.

We will fix this issue. Also, we will consider always printing warning when stub args mismatch, even if one does not use runner / rule.

Do you want to open a separate ticket to track this improvement?

Thanks for reporting!

@Sarev0k
Copy link

@Sarev0k Sarev0k commented Nov 1, 2016

I've opened #725 to track this.

@mockitoguy mockitoguy mentioned this issue Jan 18, 2017
19 of 19 tasks complete
@sanjaypujare
Copy link

@sanjaypujare sanjaypujare commented Jan 8, 2020

Came across this after running into this in my test :

[MockitoHint] MyTest.checkSomething (see javadoc for MockitoHint):
[MockitoHint] 1. Unused -> at MyTest.test1(MyTest.java:584)

It is complaining about this (line 584):

   when(mockObject.getPeerHost()).thenReturn("peer-host-from-mock");

Now my issue is this: this particular method getPeerHost() is indeed not called when the code works as expected e.g. something like this:

   if (newMethod()) {
      ...
       String peerHost = mockObject.getPeerHost();
       // use peerHost in further processing
      ...
   }

newMethod() is the new code being added and should correctly return false. However if there is a bug in newMethod() and it returns true then I want getPeerHost() to return some value which will cause a failure that I can detect. So this requires me to mock a method even if it is not called in normal processing and there is no bug in the code. I think that should not generate a warning. Has this been addressed?

@sanjaypujare
Copy link

@sanjaypujare sanjaypujare commented Jan 8, 2020

Do I need to open a new issue as I cannot reopen this?

@mockitoguy
Copy link
Member Author

@mockitoguy mockitoguy commented Jan 9, 2020

Do I need to open a new issue as I cannot reopen this?

Yes, please.

Has this been addressed?

Yes. For example, you can use lenient().when(...) stubbing to avoid reporting an unused stubbing. However, in your test case, I would simply write verify(mockObject, never()).getPeerHost();.

Hope that helps!

@sanjaypujare
Copy link

@sanjaypujare sanjaypujare commented Jan 9, 2020

Yes. For example, you can use lenient().when(...) stubbing to avoid reporting an unused stubbing.

Thanks for the lenient tip - that helps.

However, in your test case, I would simply write verify(mockObject, never()).getPeerHost();.

That's more work and more white box testing than I want. Let's say this is for testing the SSL logic where peerHost (of a connection) is matched (or not matched) against "subject-name" in a cert under various conditions but I have common setup/initialization where I "mock" the peerHost of the connection. In one of the conditions the host-name match is suppressed (i.e. newMathod() returns false) but because of the common setup code I get the 'unused' warning from MockitoHint for that test.

@mockitoguy
Copy link
Member Author

@mockitoguy mockitoguy commented Jan 10, 2020

My recommendation is to use lenient() + strict stubbing for this use case.

If you feel we should still discuss the use case verify(never()) vs. when(...).thenThrow(...) then please open a separate ticket.

Hope that helps and thank you for feedback!

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.

None yet
7 participants
You can’t perform that action at this time.