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/07/28 04:15:56 UTC
[GitHub] [kafka] dplavcic opened a new pull request, #12449: KAFKA-12947 [WIP]: Replace PowerMock with Mockito in TableSourceNodeTest
dplavcic opened a new pull request, #12449:
URL: https://github.com/apache/kafka/pull/12449
Based on the [Replace EasyMock and PowerMock with Mockito Jira](https://issues.apache.org/jira/browse/KAFKA-7438
) ticket:
>Development of EasyMock and PowerMock has stagnated while Mockito continues to be actively developed. With the new Java cadence, it's a problem to depend on libraries that do bytecode generation and are not actively maintained. In addition, Mockito is also easier to use.
Since the original PR #10881 (KAFKA-12947 Replace EasyMock and PowerMock with Mockito for Streams…) author is no longer accessible, this PR is created to fix what was started more than a year ago.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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
[GitHub] [kafka] dplavcic commented on pull request #12449: KAFKA-12947: Replace EasyMock and PowerMock with Mockito for StreamsMetricsImplTest
Posted by GitBox <gi...@apache.org>.
dplavcic commented on PR #12449:
URL: https://github.com/apache/kafka/pull/12449#issuecomment-1242651358
@cadonna, @clolov can you please help me get this PR merged (if/when you have time, ofc)?
--
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
[GitHub] [kafka] cadonna commented on a diff in pull request #12449: KAFKA-12947: Replace EasyMock and PowerMock with Mockito for StreamsMetricsImplTest
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] clolov commented on pull request #12449: KAFKA-12947: Replace EasyMock and PowerMock with Mockito for StreamsMetricsImplTest
Posted by "clolov (via GitHub)" <gi...@apache.org>.
clolov commented on PR #12449:
URL: https://github.com/apache/kafka/pull/12449#issuecomment-1413745195
Hello @dplavcic and @cadonna, how is this pull request fairing? Is there something outstanding as a review or comments to be addressed?
--
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
[GitHub] [kafka] dplavcic commented on a diff in pull request #12449: KAFKA-12947 [WIP]: Replace EasyMock and PowerMock with Mockito for StreamsMetricsImplTest
Posted by GitBox <gi...@apache.org>.
dplavcic commented on code in PR #12449:
URL: https://github.com/apache/kafka/pull/12449#discussion_r935983777
##########
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);
```
--
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
Re: [PR] KAFKA-12947: Replace EasyMock and PowerMock with Mockito for StreamsMetricsImplTest [kafka]
Posted by "dplavcic (via GitHub)" <gi...@apache.org>.
dplavcic closed pull request #12449: KAFKA-12947: Replace EasyMock and PowerMock with Mockito for StreamsMetricsImplTest
URL: https://github.com/apache/kafka/pull/12449
--
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
[GitHub] [kafka] dplavcic commented on a diff in pull request #12449: KAFKA-12947 [WIP]: Replace EasyMock and PowerMock with Mockito for StreamsMetricsImplTest
Posted by GitBox <gi...@apache.org>.
dplavcic commented on code in PR #12449:
URL: https://github.com/apache/kafka/pull/12449#discussion_r935986979
##########
streams/src/test/java/org/apache/kafka/streams/kstream/internals/graph/TableSourceNodeTest.java:
##########
@@ -23,41 +23,39 @@
import org.apache.kafka.streams.kstream.internals.MaterializedInternal;
import org.apache.kafka.streams.kstream.internals.graph.TableSourceNode.TableSourceNodeBuilder;
import org.apache.kafka.streams.processor.internals.InternalTopologyBuilder;
-import org.easymock.EasyMock;
import org.junit.Test;
import org.junit.runner.RunWith;
-import org.powermock.api.easymock.PowerMock;
-import org.powermock.core.classloader.annotations.PrepareForTest;
-import org.powermock.modules.junit4.PowerMockRunner;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
-@RunWith(PowerMockRunner.class)
-@PrepareForTest({InternalTopologyBuilder.class})
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+
+@RunWith(MockitoJUnitRunner.class)
public class TableSourceNodeTest {
private static final String STORE_NAME = "store-name";
private static final String TOPIC = "input-topic";
- private final InternalTopologyBuilder topologyBuilder = PowerMock.createNiceMock(InternalTopologyBuilder.class);
+ @Mock private InternalTopologyBuilder topologyBuilder;
@Test
public void shouldConnectStateStoreToInputTopicIfInputTopicIsUsedAsChangelog() {
final boolean shouldReuseSourceTopicForChangelog = true;
- topologyBuilder.connectSourceStoreAndTopic(STORE_NAME, TOPIC);
Review Comment:
Hi @divijvaidya, tnx for the feedback. I decided to remove `TableSourceNodeTest` from this MR since it's out-of-the scope (Jira ticket says to update only `StreamsMetricsImplTest`).
--
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
Re: [PR] KAFKA-12947: Replace EasyMock and PowerMock with Mockito for StreamsMetricsImplTest [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on PR #12449:
URL: https://github.com/apache/kafka/pull/12449#issuecomment-1766496561
It's also worth mentioning that this is the last remaining Streams test that uses PowerMock.
--
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
[GitHub] [kafka] dplavcic commented on a diff in pull request #12449: KAFKA-12947 [WIP]: Replace EasyMock and PowerMock with Mockito for StreamsMetricsImplTest
Posted by GitBox <gi...@apache.org>.
dplavcic commented on code in PR #12449:
URL: https://github.com/apache/kafka/pull/12449#discussion_r935984902
##########
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:
`shouldGetNewTopicLevelSensor` line became `verify(sensor).record(endTime - startTime);`
--
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
[GitHub] [kafka] dplavcic commented on a diff in pull request #12449: KAFKA-12947 [WIP]: Replace EasyMock and PowerMock with Mockito for StreamsMetricsImplTest
Posted by GitBox <gi...@apache.org>.
dplavcic commented on code in PR #12449:
URL: https://github.com/apache/kafka/pull/12449#discussion_r936038290
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImplTest.java:
##########
@@ -1293,43 +1259,42 @@ 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)
+ .thenReturn(endTime);
Review Comment:
First `time.nanoseconds()` invocation will return `startTime` value. All other `time.nanoseconds()` method invocations will return `endTime` value.
--
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
Re: [PR] KAFKA-12947: Replace EasyMock and PowerMock with Mockito for StreamsMetricsImplTest [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on PR #12449:
URL: https://github.com/apache/kafka/pull/12449#issuecomment-1766429512
@dplavcic It would be nice to get this over the line. Are you still willing to work on this?
--
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
[GitHub] [kafka] dplavcic commented on a diff in pull request #12449: KAFKA-12947 [WIP]: Replace EasyMock and PowerMock with Mockito for StreamsMetricsImplTest
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [kafka] dplavcic commented on pull request #12449: KAFKA-12947 [WIP]: Replace EasyMock and PowerMock with Mockito for StreamsMetricsImplTest
Posted by GitBox <gi...@apache.org>.
dplavcic commented on PR #12449:
URL: https://github.com/apache/kafka/pull/12449#issuecomment-1204406842
@ableegoldman, @cadonna could you please help review this changes in the streams test domain?
--
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
[GitHub] [kafka] divijvaidya commented on a diff in pull request #12449: KAFKA-12947: Replace EasyMock and PowerMock with Mockito for StreamsMetricsImplTest
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #12449:
URL: https://github.com/apache/kafka/pull/12449#discussion_r982716173
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImplTest.java:
##########
@@ -452,25 +441,24 @@ public void shouldNotUseSameStoreLevelSensorKeyWithDifferentThreadIds() throws I
otherThread.start();
otherThread.join();
- assertThat(sensorKeys.getValues().get(0), not(sensorKeys.getValues().get(1)));
+ assertThat(sensorKeys.getAllValues().get(0), not(sensorKeys.getAllValues().get(1)));
}
@Test
public void shouldUseSameStoreLevelSensorKeyWithSameSensorNames() {
- final Metrics metrics = niceMock(Metrics.class);
- final Capture<String> sensorKeys = setUpSensorKeyTests(metrics);
+ final Metrics metrics = mock(Metrics.class);
+ final ArgumentCaptor<String> sensorKeys = setUpSensorKeyTests(metrics);
final StreamsMetricsImpl streamsMetrics = new StreamsMetricsImpl(metrics, CLIENT_ID, VERSION, time);
streamsMetrics.storeLevelSensor(TASK_ID1, STORE_NAME1, SENSOR_NAME_1, INFO_RECORDING_LEVEL);
streamsMetrics.storeLevelSensor(TASK_ID1, STORE_NAME1, SENSOR_NAME_1, INFO_RECORDING_LEVEL);
- assertThat(sensorKeys.getValues().get(0), is(sensorKeys.getValues().get(1)));
+ assertThat(sensorKeys.getAllValues().get(0), is(sensorKeys.getAllValues().get(1)));
}
- private Capture<String> setUpSensorKeyTests(final Metrics metrics) {
- final Capture<String> sensorKeys = newCapture(CaptureType.ALL);
- expect(metrics.getSensor(capture(sensorKeys))).andStubReturn(sensor);
- replay(metrics);
+ private ArgumentCaptor<String> setUpSensorKeyTests(final Metrics metrics) {
+ final ArgumentCaptor<String> sensorKeys = ArgumentCaptor.forClass(String.class);
Review Comment:
nit
It's a style choice but this could be moved as a member of the class with `@captor` annotation (for all scenarios in this test) and `@Mock` for mocks. I prefer the annotation as it helps me quickly understand the mocks used in this test file.
--
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
[GitHub] [kafka] dplavcic commented on pull request #12449: KAFKA-12947: Replace EasyMock and PowerMock with Mockito for StreamsMetricsImplTest
Posted by "dplavcic (via GitHub)" <gi...@apache.org>.
dplavcic commented on PR #12449:
URL: https://github.com/apache/kafka/pull/12449#issuecomment-1542350805
@clolov , apologies for a delayed response. I unfortunately don't have time to finish this anytime soon 😞
--
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
[GitHub] [kafka] dplavcic commented on a diff in pull request #12449: KAFKA-12947 [WIP]: Replace EasyMock and PowerMock with Mockito for StreamsMetricsImplTest
Posted by GitBox <gi...@apache.org>.
dplavcic commented on code in PR #12449:
URL: https://github.com/apache/kafka/pull/12449#discussion_r937067118
##########
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:
`shouldGetExistingThreadLevelSensor` test invokes `setupGetExistingSensorTest(metrics)` which does the following:
```java
private void setupGetExistingSensorTest(final Metrics metrics) {
when(metrics.getSensor(anyString())).thenReturn(sensor);
}
```
An explicit verify is added to be sure `metrics.getSensor()` is invoked with the exact string, and not `anyString()` as stated in stub in `setupGetExistingSensorTest` method.
The same reasoning applies to other explicit `verify(metrics).getSensor(...)` calls.
##########
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:
`shouldGetNewTaskLevelSensor` invokes `setupGetNewSensorTest` which internally creates a stub with exact values, providing implicit verify.
--
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
[GitHub] [kafka] divijvaidya commented on a diff in pull request #12449: KAFKA-12947 [WIP]: Replace EasyMock and PowerMock with Mockito for StreamsMetricsImplTest
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #12449:
URL: https://github.com/apache/kafka/pull/12449#discussion_r933473975
##########
streams/src/test/java/org/apache/kafka/streams/kstream/internals/graph/TableSourceNodeTest.java:
##########
@@ -23,41 +23,39 @@
import org.apache.kafka.streams.kstream.internals.MaterializedInternal;
import org.apache.kafka.streams.kstream.internals.graph.TableSourceNode.TableSourceNodeBuilder;
import org.apache.kafka.streams.processor.internals.InternalTopologyBuilder;
-import org.easymock.EasyMock;
import org.junit.Test;
import org.junit.runner.RunWith;
-import org.powermock.api.easymock.PowerMock;
-import org.powermock.core.classloader.annotations.PrepareForTest;
-import org.powermock.modules.junit4.PowerMockRunner;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
-@RunWith(PowerMockRunner.class)
-@PrepareForTest({InternalTopologyBuilder.class})
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+
+@RunWith(MockitoJUnitRunner.class)
public class TableSourceNodeTest {
private static final String STORE_NAME = "store-name";
private static final String TOPIC = "input-topic";
- private final InternalTopologyBuilder topologyBuilder = PowerMock.createNiceMock(InternalTopologyBuilder.class);
+ @Mock private InternalTopologyBuilder topologyBuilder;
@Test
public void shouldConnectStateStoreToInputTopicIfInputTopicIsUsedAsChangelog() {
final boolean shouldReuseSourceTopicForChangelog = true;
- topologyBuilder.connectSourceStoreAndTopic(STORE_NAME, TOPIC);
Review Comment:
You can add
`doNothing().when(topologyBuilder).connectSourceStoreAndTopic(STORE_NAME, TOPIC);` here.
--
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
[GitHub] [kafka] dplavcic commented on pull request #12449: KAFKA-12947: Replace EasyMock and PowerMock with Mockito for StreamsMetricsImplTest
Posted by "dplavcic (via GitHub)" <gi...@apache.org>.
dplavcic commented on PR #12449:
URL: https://github.com/apache/kafka/pull/12449#issuecomment-1542382627
> Hello @dplavcic and @cadonna, how is this pull request fairing? Is there something outstanding as a review or comments to be addressed?
@clolov , I will work on this during the next two weeks. Apologies for a delayed response.
--
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
Re: [PR] KAFKA-12947: Replace EasyMock and PowerMock with Mockito for StreamsMetricsImplTest [kafka]
Posted by "dplavcic (via GitHub)" <gi...@apache.org>.
dplavcic commented on PR #12449:
URL: https://github.com/apache/kafka/pull/12449#issuecomment-1793353918
I will reopen this PR once changes are implemented. I'm currently working on this.
--
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