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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
