dplavcic commented on code in PR #12459:
URL: https://github.com/apache/kafka/pull/12459#discussion_r933758267
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/metrics/RocksDBMetricsRecorderTest.java:
##########
@@ -36,22 +38,23 @@
import org.rocksdb.StatsLevel;
import org.rocksdb.TickerType;
-import static org.easymock.EasyMock.anyObject;
-import static org.easymock.EasyMock.eq;
-import static org.easymock.EasyMock.expect;
-import static org.easymock.EasyMock.mock;
-import static org.easymock.EasyMock.niceMock;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertThrows;
-import static org.powermock.api.easymock.PowerMock.reset;
-import static org.powermock.api.easymock.PowerMock.createMock;
-import static org.powermock.api.easymock.PowerMock.mockStatic;
-import static org.powermock.api.easymock.PowerMock.replay;
-import static org.powermock.api.easymock.PowerMock.verify;
-
-@RunWith(PowerMockRunner.class)
-@PrepareForTest({RocksDBMetrics.class, Sensor.class})
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.ArgumentMatchers.isA;
+import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.mockStatic;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoInteractions;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
+import static org.mockito.Mockito.when;
+
+@RunWith(MockitoJUnitRunner.class)
Review Comment:
What do you think we use `@RunWith(MockitoJUnitRunner.StrictStubs.class)`
which automatically detects stubbing argument mismatches instead of the
`@RunWith(MockitoJUnitRunner.class)` 🤔
-
https://javadoc.io/doc/org.mockito/mockito-core/latest/org/mockito/junit/MockitoJUnitRunner.html
-
https://javadoc.io/doc/org.mockito/mockito-core/latest/org/mockito/junit/MockitoJUnitRunner.StrictStubs.html
-
https://javadoc.io/doc/org.mockito/mockito-core/latest/org/mockito/quality/Strictness.html#STRICT_STUBS
With `StrictStubs` we get (c/p from the
[docs](https://javadoc.io/doc/org.mockito/mockito-core/latest/org/mockito/quality/Strictness.html#STRICT_STUBS)):
> - Improved productivity: the test fails early when code under test invokes
stubbed method with different arguments (see
[PotentialStubbingProblem](https://javadoc.io/static/org.mockito/mockito-core/4.6.1/org/mockito/exceptions/misusing/PotentialStubbingProblem.html)).
> - Cleaner tests without unnecessary stubbings: the test fails when unused
stubs are present (see
[UnnecessaryStubbingException](https://javadoc.io/static/org.mockito/mockito-core/4.6.1/org/mockito/exceptions/misusing/UnnecessaryStubbingException.html)).
> - Cleaner, more DRY tests ("Don't Repeat Yourself"): If you use
[Mockito.verifyNoMoreInteractions(Object...)](https://javadoc.io/static/org.mockito/mockito-core/4.6.1/org/mockito/Mockito.html#verifyNoMoreInteractions(java.lang.Object...))
you no longer need to explicitly verify stubbed invocations. They are
automatically verified for you.
Note that once we migrate this test to use JUnit5, and we start using
`@ExtendWith(MockitoExtension.class)` instead of the
`@RunWith(MockitoJUnitRunner.class)` we will get `STRICT_STUBS` behavior
out-of-the-box.
---
Considering this test for example:
```java
@Test
public void
shouldSetStatsLevelToExceptDetailedTimersWhenValueProvidersWithStatisticsAreAdded()
{
doNothing().when(statisticsToAdd1).setStatsLevel(StatsLevel.EXCEPT_DETAILED_TIMERS);
recorder.addValueProviders(SEGMENT_STORE_NAME_1, dbToAdd1,
cacheToAdd1, statisticsToAdd1);
verify(statisticsToAdd1).setStatsLevel(eq(StatsLevel.EXCEPT_DETAILED_TIMERS));
}
```
- with `StrictStubs`, if someone changes what is passed to the
`setStatsLevel()` method in production code, and hard-code it to, for example
`statistics.setStatsLevel(StatsLevel.ALL);`, this test would fail **even
without explicit verify** with following message:
```
Unnecessary stubbings detected in test class: RocksDBMetricsRecorderTest
Clean & maintainable test code requires zero unnecessary code.
Following stubbings are unnecessary (click to navigate to relevant line of
code):
1. -> at
org.apache.kafka.streams.state.internals.metrics.RocksDBMetricsRecorderTest.shouldSetStatsLevelToExceptDetailedTimersWhenValueProvidersWithStatisticsAreAdded(RocksDBMetricsRecorderTest.java:180)
Please remove unnecessary stubbings or use 'lenient' strictness. More info:
javadoc for UnnecessaryStubbingException class.
...
```
Mockito documentation provides a good explanation of what is happening (and
why verify step could be redundant):
-
https://javadoc.io/doc/org.mockito/mockito-core/latest/org/mockito/Mockito.html#when(T)
-
https://javadoc.io/doc/org.mockito/mockito-core/latest/org/mockito/Mockito.html#verify(T)
> Although it is possible to verify a stubbed invocation, usually it's just
redundant. Let's say you've stubbed foo.bar(). If your code cares what
foo.bar() returns then something else breaks(often before even verify() gets
executed). If your code doesn't care what get(0) returns then it should not be
stubbed.
Note:
- I'm not sure if Mockito docs links with `fragment/anchor` don't work just
for me or for all 🤔
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
