cadonna commented on code in PR #12449:
URL: https://github.com/apache/kafka/pull/12449#discussion_r969376097
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImplTest.java:
##########
@@ -287,7 +279,7 @@ public void shouldGetExistingThreadLevelSensor() {
final Sensor actualSensor =
streamsMetrics.threadLevelSensor(THREAD_ID1, SENSOR_NAME_1, recordingLevel);
- verify(metrics);
+ verify(metrics).getSensor("internal.test-thread-1.s.sensor1");
Review Comment:
Good reasoning!
Why not pass the sensor name to `setupGetExistingSensorTest()` and remove
the explicit `verify()`?
Please use `INTERNAL_PREFIX + SENSOR_PREFIX_DELIMITER + THREAD_ID1 +
SENSOR_NAME_DELIMITER + ...` for the sensor names.
From a migration point of view the verification is not strictly needed.
Maybe you want to do the additional verifications in a separate PR and there
you could also add the verifications for `setupGetNewSensorTest()` by using the
captured names. Maybe you can even get rid of the capture thingy there since we
only use it to verify if the sensor name is the same in both calls which could
probably also be done by passing the sensor name to `setupGetNewSensorTest()`.
All of this also applies to the other verifications you added.
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImplTest.java:
##########
@@ -1419,4 +1387,5 @@ public void shouldCleanupThreadLevelImmutableMetric() {
);
assertThat(metrics.metric(name), nullValue());
}
+
Review Comment:
```suggestion
```
##########
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:
Why do you not return `message`?
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImplTest.java:
##########
@@ -400,50 +389,50 @@ public void shouldGetExistingStoreLevelSensor() {
recordingLevel
);
- verify(metrics);
+ verify(metrics).getSensor("internal.Test
worker.task.test-task-1.store.store1.s.sensor1");
Review Comment:
This does not work for me locally. I guess because the name of the thread is
`main` for me locally and not `Test worker`. We should avoid using literal
thread names in tests. It is not portable.
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImplTest.java:
##########
@@ -305,7 +297,6 @@ public void shouldGetNewTaskLevelSensor() {
recordingLevel
);
- verify(metrics);
Review Comment:
This is not completely true, since the sensor name is not verified. However,
you are completely right with removing the verify from here.
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImplTest.java:
##########
@@ -192,23 +187,23 @@ public boolean matches(final Object argument) {
}
@Override
- public void appendTo(final StringBuffer buffer) {
- buffer.append(message);
+ public String toString() {
+ return "<eqMetricConfig>";
Review Comment:
```suggestion
return message.toString();
```
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImplTest.java:
##########
@@ -192,23 +187,23 @@ public boolean matches(final Object argument) {
}
Review Comment:
I discovered that the "quota" part in the message needs a `null`-check. It
should be:
```
if (argument.quota() != null) {
message.append(", ");
message.append("quota=");
message.append(argument.quota().toString());
}
```
(Note: I just used the parameter `argument` instead of variable
`otherMetricConfig` since with the correct type parameter in `ArgumentMatcher`
you do not need `otherMetricConfig` anymore).
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImplTest.java:
##########
@@ -736,42 +715,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);
+
+
Review Comment:
nit: remove empty line
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImplTest.java:
##########
@@ -736,42 +715,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);
Review Comment:
Why do we need this `reset()`?
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImplTest.java:
##########
@@ -147,7 +142,7 @@ public class StreamsMetricsImplTest {
private final StreamsMetricsImpl streamsMetrics = new
StreamsMetricsImpl(metrics, CLIENT_ID, VERSION, time);
private static MetricConfig eqMetricConfig(final MetricConfig
metricConfig) {
- EasyMock.reportMatcher(new IArgumentMatcher() {
+ Mockito.argThat(new ArgumentMatcher<Object>() {
Review Comment:
```suggestion
return Mockito.argThat(new ArgumentMatcher<MetricConfig>() {
```
With this change you can remove the type check on line 150.
--
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
