HDDS-12505. Use Mockito doReturn() in hadoop-hdds/client
#8034
Conversation
| <Match> | ||
| <Class name="org.apache.hadoop.hdds.scm.storage.DummyBlockInputStreamWithRetry"/> | ||
| <Bug pattern="RV_RETURN_VALUE_IGNORED_NO_SIDE_EFFECT" /> | ||
| </Match> | ||
| <Match> | ||
| <Class name="org.apache.hadoop.hdds.scm.storage.TestBlockInputStream"/> | ||
| <Bug pattern="RV_RETURN_VALUE_IGNORED_NO_SIDE_EFFECT" /> | ||
| </Match> | ||
| <Match> | ||
| <Class name="org.apache.hadoop.ozone.client.io.TestBlockInputStreamFactoryImpl"/> | ||
| <Bug pattern="RV_RETURN_VALUE_IGNORED_NO_SIDE_EFFECT" /> | ||
| </Match> |
There was a problem hiding this comment.
Please do not add these.
- It is completely non-scalable to add an exclusion for each test class.
- We should reduce the number of exclusions, not increase it (HDDS-12342).
- Only 3 classes are added here, but 5 classes are changed similarly. I'm guessing the others were not reported by spotbugs. The problem is that spotbugs analysis is not 100% accurate, and sometimes starts reporting existing problems only later, when making changes in other classes.
This is a spotbugs problem. @peterxcli reported it few weeks ago: #7963 (comment)
I don't know if the problem is fixed by more recent versions of spotbugs. Unfortunately we have to use an older version of spotbugs due to lots of new issues reported by newer one (HDDS-10150).
I think we should either
- defer mass conversion to
doReturnuntil we have a version of spotbugs where this is fixed, OR - add exclusion for this issue in all test classes using pattern matching, AND enable similar rule in PMD (subtask of HDDS-12338).
There was a problem hiding this comment.
Hi, @adoroszlai
Thanks for the point out, which one would be better? How about we hold on the doReturn() changes, but I would still create a subtask of HDDS-12338 for that? thanks!
There was a problem hiding this comment.
On quick glance I couldn't find similar rule in PMD.
|
Thanks again @chiacyu for the patch. Let's revisit this once we get spotbugs fixed. |
2 participants
What changes were proposed in this pull request?
Currently we're using
thenReturn()statements in the tests, we should change it withdoReturn().What is the link to the Apache JIRA
HDDS-12505
How was this patch tested?
Tested by CI.