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

false positive RV_RETURN_VALUE_IGNORED_NO_SIDE_EFFECT #872

Closed
damnms opened this issue Feb 11, 2019 · 6 comments · Fixed by #2550
Closed

false positive RV_RETURN_VALUE_IGNORED_NO_SIDE_EFFECT #872

damnms opened this issue Feb 11, 2019 · 6 comments · Fixed by #2550

Comments

@damnms
Copy link

damnms commented Feb 11, 2019

Mockito.verify(converter, times(1)).convert(isA(SFDLBean.class));

This code leads to:

Return value of method without side effect is ignored
This code calls a method and ignores the return value. However our analysis shows that the method (including its implementations in subclasses if any) does not produce any effect other than return value. > Thus this call can be removed.
We are trying to reduce the false positives as much as possible, but in some cases this warning might be wrong. Common false-positive cases include:

  • The method is designed to be overridden and produce a side effect in other projects which are out of > the scope of the analysis.
  • The method is called to trigger the class loading which may have a side effect.
  • The method is called just to get some exception.
    If you feel that our assumption is incorrect, you can use a @CheckReturnValue annotation to instruct SpotBugs that ignoring the return value of this method is acceptable.

Bug kind and pattern: RV - RV_RETURN_VALUE_IGNORED_NO_SIDE_EFFECT

The verify method does effect the code - it breaks the unit test if the method is not called 1 time. Its more or less like an assert.
I will workaround this now with @CheckReturnValue but for me, this is a very common use case. Is it possible to fix that in spotbugs? Thanks!

@ThrawnCA
Copy link
Contributor

ThrawnCA commented Feb 18, 2019

Are you sure that the bug detector is complaining about the verify call, not the convert call?

My workplace uses Mockito all the time and I haven't seen this cause problems.

@damnms
Copy link
Author

damnms commented Apr 20, 2019

Yes it is complaining about the convert() which is called with verify().
A verify() should never require to check the return value, as this is a verify() to check that this method has been called, not if there is any return value to that and if that value has been checked in any way.

I just have another place where i have exact the same problem.

verify(myDatabase).registrationPasswordMatches(eq("sha"));
This is in a class called SecurityHandlerTest.java
It makes absolutely no sense to test here any return value of the registrationPasswordMatches() method from myDatabase, as it is irrelevant. If i would test that (MyDatabase class) inside this test (SecurityHandlerTest), i would do a integration test, not a unit test.

@zqq90
Copy link

zqq90 commented Jul 29, 2019

+1

@benariss
Copy link

+1
I'm hitting this bug when using Mockito.verify() and an argument captor to test the value passed into a Map.get() call:

ArgumentCaptor<String> captor = ArgumentCaptor.forClass(String.class);
verify(mockMap).get(captor.capture());
assertEquals("testValue", captor.getValue());

@rafallezanko
Copy link

meet same with mockito.verify. do you have plans to solve it?

@cru5ader
Copy link

cru5ader commented Dec 21, 2022

Is there any update on the fix for this bug for mockito.verify use case ?

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 a pull request may close this issue.

6 participants