You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/08/02 20:11:57 UTC
[GitHub] [kafka] dplavcic commented on a diff in pull request #12449: KAFKA-12947 [WIP]: Replace EasyMock and PowerMock with Mockito for StreamsMetricsImplTest
dplavcic commented on code in PR #12449:
URL: https://github.com/apache/kafka/pull/12449#discussion_r935981217
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImplTest.java:
##########
@@ -192,23 +180,23 @@ public boolean matches(final Object argument) {
}
@Override
- public void appendTo(final StringBuffer buffer) {
- buffer.append(message);
+ public String toString() {
+ return "<eqMetricConfig>";
Review Comment:
[Mockito docs ](https://javadoc.io/static/org.mockito/mockito-core/4.6.1/org/mockito/ArgumentMatcher.html)suggests to:
> Use toString() method for description of the matcher - it is printed in verification errors.
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImplTest.java:
##########
@@ -736,42 +705,34 @@ public void shouldProvideCorrectStrings() {
assertThat(ROLLUP_VALUE, is("all"));
}
- private void setupRemoveSensorsTest(final Metrics metrics,
- final String level) {
- final String fullSensorNamePrefix = INTERNAL_PREFIX + SENSOR_PREFIX_DELIMITER + level + SENSOR_NAME_DELIMITER;
- resetToDefault(metrics);
- metrics.removeSensor(fullSensorNamePrefix + SENSOR_NAME_1);
- metrics.removeSensor(fullSensorNamePrefix + SENSOR_NAME_2);
Review Comment:
`setupRemoveSensorsTest` was called only inside the `shouldRemoveThreadLevelSensors`.
Since these two lines are used only to verify things, they were moved to the `shouldRemoveThreadLevelSensors` where verification should indeed happen:
```java`
metrics.removeSensor(fullSensorNamePrefix + SENSOR_NAME_1);
metrics.removeSensor(fullSensorNamePrefix + SENSOR_NAME_2);
```
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImplTest.java:
##########
@@ -1293,43 +1252,41 @@ private void verifyMetric(final String name,
public void shouldMeasureLatency() {
final long startTime = 6;
final long endTime = 10;
- final Sensor sensor = createMock(Sensor.class);
- expect(sensor.shouldRecord()).andReturn(true);
- expect(sensor.hasMetrics()).andReturn(true);
- sensor.record(endTime - startTime);
+ final Sensor sensor = mock(Sensor.class);
+ when(sensor.shouldRecord()).thenReturn(true);
+ when(sensor.hasMetrics()).thenReturn(true);
final Time time = mock(Time.class);
- expect(time.nanoseconds()).andReturn(startTime);
- expect(time.nanoseconds()).andReturn(endTime);
- replay(sensor, time);
+ when(time.nanoseconds()).thenReturn(startTime);
+ when(time.nanoseconds()).thenReturn(endTime);
StreamsMetricsImpl.maybeMeasureLatency(() -> { }, time, sensor);
- verify(sensor, time);
+ verify(sensor).record(endTime - startTime);
}
@Test
public void shouldNotMeasureLatencyDueToRecordingLevel() {
- final Sensor sensor = createMock(Sensor.class);
- expect(sensor.shouldRecord()).andReturn(false);
+ final Sensor sensor = mock(Sensor.class);
+ when(sensor.shouldRecord()).thenReturn(false);
final Time time = mock(Time.class);
- replay(sensor);
StreamsMetricsImpl.maybeMeasureLatency(() -> { }, time, sensor);
- verify(sensor);
+ verify(sensor, never()).record(anyDouble());
+ verifyNoInteractions(time);
Review Comment:
Additional verification steps are added to confirm `record()` method is never called.
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImplTest.java:
##########
@@ -1293,43 +1252,41 @@ private void verifyMetric(final String name,
public void shouldMeasureLatency() {
final long startTime = 6;
final long endTime = 10;
- final Sensor sensor = createMock(Sensor.class);
- expect(sensor.shouldRecord()).andReturn(true);
- expect(sensor.hasMetrics()).andReturn(true);
- sensor.record(endTime - startTime);
+ final Sensor sensor = mock(Sensor.class);
+ when(sensor.shouldRecord()).thenReturn(true);
+ when(sensor.hasMetrics()).thenReturn(true);
final Time time = mock(Time.class);
- expect(time.nanoseconds()).andReturn(startTime);
- expect(time.nanoseconds()).andReturn(endTime);
- replay(sensor, time);
+ when(time.nanoseconds()).thenReturn(startTime);
+ when(time.nanoseconds()).thenReturn(endTime);
StreamsMetricsImpl.maybeMeasureLatency(() -> { }, time, sensor);
- verify(sensor, time);
+ verify(sensor).record(endTime - startTime);
}
@Test
public void shouldNotMeasureLatencyDueToRecordingLevel() {
- final Sensor sensor = createMock(Sensor.class);
- expect(sensor.shouldRecord()).andReturn(false);
+ final Sensor sensor = mock(Sensor.class);
+ when(sensor.shouldRecord()).thenReturn(false);
final Time time = mock(Time.class);
- replay(sensor);
StreamsMetricsImpl.maybeMeasureLatency(() -> { }, time, sensor);
- verify(sensor);
+ verify(sensor, never()).record(anyDouble());
+ verifyNoInteractions(time);
}
@Test
public void shouldNotMeasureLatencyBecauseSensorHasNoMetrics() {
- final Sensor sensor = createMock(Sensor.class);
- expect(sensor.shouldRecord()).andReturn(true);
- expect(sensor.hasMetrics()).andReturn(false);
+ final Sensor sensor = mock(Sensor.class);
+ when(sensor.shouldRecord()).thenReturn(true);
+ when(sensor.hasMetrics()).thenReturn(false);
final Time time = mock(Time.class);
- replay(sensor);
StreamsMetricsImpl.maybeMeasureLatency(() -> { }, time, sensor);
- verify(sensor);
+ verify(sensor, never()).record(anyDouble());
+ verifyNoInteractions(time);
Review Comment:
Additional verification steps are added to confirm record() method is never called.
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImplTest.java:
##########
@@ -87,10 +74,11 @@
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertNotNull;
-import static org.powermock.api.easymock.PowerMock.createMock;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.*;
-@RunWith(PowerMockRunner.class)
-@PrepareForTest({Sensor.class, KafkaMetric.class})
+@RunWith(MockitoJUnitRunner.StrictStubs.class)
Review Comment:
`StrictStubs` docs: https://javadoc.io/doc/org.mockito/mockito-core/latest/org/mockito/quality/Strictness.html#STRICT_STUBS
>
public static final [Strictness](https://javadoc.io/static/org.mockito/mockito-core/4.6.1/org/mockito/quality/Strictness.html) STRICT_STUBS
Ensures clean tests, reduces test code duplication, improves debuggability. Offers best combination of flexibility and productivity. Highly recommended. Planned as default for Mockito v4. Enable it via our JUnit support ([MockitoJUnit](https://javadoc.io/static/org.mockito/mockito-core/4.6.1/org/mockito/junit/MockitoJUnit.html)) or [MockitoSession](https://javadoc.io/static/org.mockito/mockito-core/4.6.1/org/mockito/MockitoSession.html).
Adds following behavior:
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.
For more information see [Strictness](https://javadoc.io/static/org.mockito/mockito-core/4.6.1/org/mockito/quality/Strictness.html).
Since:
2.3.0
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImplTest.java:
##########
@@ -1293,43 +1252,41 @@ private void verifyMetric(final String name,
public void shouldMeasureLatency() {
final long startTime = 6;
final long endTime = 10;
- final Sensor sensor = createMock(Sensor.class);
- expect(sensor.shouldRecord()).andReturn(true);
- expect(sensor.hasMetrics()).andReturn(true);
- sensor.record(endTime - startTime);
Review Comment:
This line became `verify(sensor).record(endTime - startTime);`
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImplTest.java:
##########
@@ -736,42 +705,34 @@ public void shouldProvideCorrectStrings() {
assertThat(ROLLUP_VALUE, is("all"));
}
- private void setupRemoveSensorsTest(final Metrics metrics,
- final String level) {
- final String fullSensorNamePrefix = INTERNAL_PREFIX + SENSOR_PREFIX_DELIMITER + level + SENSOR_NAME_DELIMITER;
- resetToDefault(metrics);
- metrics.removeSensor(fullSensorNamePrefix + SENSOR_NAME_1);
- metrics.removeSensor(fullSensorNamePrefix + SENSOR_NAME_2);
- replay(metrics);
- }
-
@Test
public void shouldRemoveClientLevelMetricsAndSensors() {
- final Metrics metrics = niceMock(Metrics.class);
+ final Metrics metrics = mock(Metrics.class);
final StreamsMetricsImpl streamsMetrics = new StreamsMetricsImpl(metrics, CLIENT_ID, VERSION, time);
- final Capture<String> sensorKeys = addSensorsOnAllLevels(metrics, streamsMetrics);
- resetToDefault(metrics);
-
- metrics.removeSensor(sensorKeys.getValues().get(0));
- metrics.removeSensor(sensorKeys.getValues().get(1));
- expect(metrics.removeMetric(metricName1)).andStubReturn(null);
- expect(metrics.removeMetric(metricName2)).andStubReturn(null);
- replay(metrics);
+ final ArgumentCaptor<String> sensorKeys = addSensorsOnAllLevels(metrics, streamsMetrics);
+ reset(metrics);
+
+
+ when(metrics.removeMetric(metricName1)).thenReturn(null);
+ when(metrics.removeMetric(metricName2)).thenReturn(null);
+
streamsMetrics.removeAllClientLevelSensorsAndMetrics();
- verify(metrics);
+ verify(metrics).removeSensor(sensorKeys.getAllValues().get(0));
+ verify(metrics).removeSensor(sensorKeys.getAllValues().get(1));
}
- @Test
+ @Test
public void shouldRemoveThreadLevelSensors() {
- final Metrics metrics = niceMock(Metrics.class);
+ final Metrics metrics = mock(Metrics.class);
final StreamsMetricsImpl streamsMetrics = new StreamsMetricsImpl(metrics, CLIENT_ID, VERSION, time);
addSensorsOnAllLevels(metrics, streamsMetrics);
- setupRemoveSensorsTest(metrics, THREAD_ID1);
streamsMetrics.removeAllThreadLevelSensors(THREAD_ID1);
- verify(metrics);
+ final String fullSensorNamePrefix = INTERNAL_PREFIX + SENSOR_PREFIX_DELIMITER + THREAD_ID1 + SENSOR_NAME_DELIMITER;
+ verify(metrics).removeSensor(fullSensorNamePrefix + SENSOR_NAME_1);
+ verify(metrics).removeSensor(fullSensorNamePrefix + SENSOR_NAME_2);
Review Comment:
Additional verification step is added to confirm `removeSensor` is indeed called twice.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org