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/09/22 16:44:18 UTC
[GitHub] [kafka] divijvaidya opened a new pull request, #12677: KAFKA-14132: Replace PowerMock/Easymock with Mockito for WorkerMetricsGroupTest
divijvaidya opened a new pull request, #12677:
URL: https://github.com/apache/kafka/pull/12677
1. Note that `@RunWith(MockitoJUnitRunner.StrictStubs.class)` ensures that all declared stubs are called, otherwise it throws an exception, hence there is no requirement to `verify` stubbed methods.
2. Note that there is no need to stub the methods that return void, since they don't return anything! But we need to verify their invocation using `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 pull request #12677: KAFKA-14132: Replace PowerMock/Easymock with Mockito for WorkerMetricsGroupTest
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on PR #12677:
URL: https://github.com/apache/kafka/pull/12677#issuecomment-1255925164
@mimaison please review this when you get an opportunity. Thank you!
--
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 #12677: KAFKA-14132: Replace PowerMock/Easymock with Mockito for WorkerMetricsGroupTest
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #12677:
URL: https://github.com/apache/kafka/pull/12677#discussion_r978867978
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerMetricsGroupTest.java:
##########
@@ -83,167 +77,107 @@ public void setup() {
taskStartupAttempts = mockSensor(metricGroup, "task-startup-attempts");
taskStartupSuccesses = mockSensor(metricGroup, "task-startup-successes");
taskStartupFailures = mockSensor(metricGroup, "task-startup-failures");
-
- delegateConnectorListener = PowerMock.createStrictMock(ConnectorStatus.Listener.class);
- delegateTaskListener = PowerMock.createStrictMock(TaskStatus.Listener.class);
}
private Sensor mockSensor(ConnectMetrics.MetricGroup metricGroup, String name) {
- Sensor sensor = PowerMock.createMock(Sensor.class);
- metricGroup.sensor(eq(name));
- expectLastCall().andReturn(sensor);
-
- sensor.add(anyObject(CompoundStat.class));
- expectLastCall().andStubReturn(true);
-
- sensor.add(anyObject(MetricName.class), anyObject(CumulativeSum.class));
- expectLastCall().andStubReturn(true);
-
+ Sensor sensor = mock(Sensor.class);
+ when(metricGroup.sensor(name)).thenReturn(sensor);
+ when(sensor.add(any(CompoundStat.class))).thenReturn(true);
+ when(sensor.add(any(MetricName.class), any(CumulativeSum.class))).thenReturn(true);
return sensor;
}
@Test
public void testConnectorStartupRecordedMetrics() {
- delegateConnectorListener.onStartup(eq(connector));
- expectLastCall();
-
- connectorStartupAttempts.record(eq(1.0));
- expectLastCall();
- connectorStartupSuccesses.record(eq(1.0));
- expectLastCall();
- connectorStartupResults.record(eq(1.0));
- expectLastCall();
-
- PowerMock.replayAll();
-
WorkerMetricsGroup workerMetricsGroup = new WorkerMetricsGroup(new HashMap<>(), new HashMap<>(), connectMetrics);
final ConnectorStatus.Listener connectorListener = workerMetricsGroup.wrapStatusListener(delegateConnectorListener);
connectorListener.onStartup(connector);
- PowerMock.verifyAll();
+ verifyRecordConnectorStartupSuccess();
+ verify(delegateConnectorListener).onStartup(connector);
}
@Test
public void testConnectorFailureAfterStartupRecordedMetrics() {
- delegateConnectorListener.onStartup(eq(connector));
- expectLastCall();
-
- connectorStartupAttempts.record(eq(1.0));
- expectLastCall();
- connectorStartupSuccesses.record(eq(1.0));
- expectLastCall();
- connectorStartupResults.record(eq(1.0));
- expectLastCall();
-
- delegateConnectorListener.onFailure(eq(connector), eq(exception));
- expectLastCall();
-
- // recordConnectorStartupFailure() should not be called if failure happens after a successful startup
Review Comment:
Fixed in upcoming commit.
--
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 #12677: KAFKA-14132: Replace PowerMock/Easymock with Mockito for WorkerMetricsGroupTest
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #12677:
URL: https://github.com/apache/kafka/pull/12677#discussion_r978870789
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerMetricsGroupTest.java:
##########
@@ -83,167 +77,107 @@ public void setup() {
taskStartupAttempts = mockSensor(metricGroup, "task-startup-attempts");
taskStartupSuccesses = mockSensor(metricGroup, "task-startup-successes");
taskStartupFailures = mockSensor(metricGroup, "task-startup-failures");
-
- delegateConnectorListener = PowerMock.createStrictMock(ConnectorStatus.Listener.class);
- delegateTaskListener = PowerMock.createStrictMock(TaskStatus.Listener.class);
}
private Sensor mockSensor(ConnectMetrics.MetricGroup metricGroup, String name) {
- Sensor sensor = PowerMock.createMock(Sensor.class);
- metricGroup.sensor(eq(name));
- expectLastCall().andReturn(sensor);
-
- sensor.add(anyObject(CompoundStat.class));
- expectLastCall().andStubReturn(true);
-
- sensor.add(anyObject(MetricName.class), anyObject(CumulativeSum.class));
- expectLastCall().andStubReturn(true);
-
+ Sensor sensor = mock(Sensor.class);
+ when(metricGroup.sensor(name)).thenReturn(sensor);
+ when(sensor.add(any(CompoundStat.class))).thenReturn(true);
+ when(sensor.add(any(MetricName.class), any(CumulativeSum.class))).thenReturn(true);
return sensor;
}
@Test
public void testConnectorStartupRecordedMetrics() {
- delegateConnectorListener.onStartup(eq(connector));
- expectLastCall();
-
- connectorStartupAttempts.record(eq(1.0));
- expectLastCall();
- connectorStartupSuccesses.record(eq(1.0));
- expectLastCall();
- connectorStartupResults.record(eq(1.0));
- expectLastCall();
-
- PowerMock.replayAll();
-
WorkerMetricsGroup workerMetricsGroup = new WorkerMetricsGroup(new HashMap<>(), new HashMap<>(), connectMetrics);
final ConnectorStatus.Listener connectorListener = workerMetricsGroup.wrapStatusListener(delegateConnectorListener);
connectorListener.onStartup(connector);
- PowerMock.verifyAll();
+ verifyRecordConnectorStartupSuccess();
+ verify(delegateConnectorListener).onStartup(connector);
}
@Test
public void testConnectorFailureAfterStartupRecordedMetrics() {
- delegateConnectorListener.onStartup(eq(connector));
- expectLastCall();
-
- connectorStartupAttempts.record(eq(1.0));
- expectLastCall();
- connectorStartupSuccesses.record(eq(1.0));
- expectLastCall();
- connectorStartupResults.record(eq(1.0));
- expectLastCall();
-
- delegateConnectorListener.onFailure(eq(connector), eq(exception));
- expectLastCall();
-
- // recordConnectorStartupFailure() should not be called if failure happens after a successful startup
-
- PowerMock.replayAll();
-
WorkerMetricsGroup workerMetricsGroup = new WorkerMetricsGroup(new HashMap<>(), new HashMap<>(), connectMetrics);
final ConnectorStatus.Listener connectorListener = workerMetricsGroup.wrapStatusListener(delegateConnectorListener);
connectorListener.onStartup(connector);
connectorListener.onFailure(connector, exception);
- PowerMock.verifyAll();
+ verify(delegateConnectorListener).onStartup(connector);
+ verifyRecordConnectorStartupSuccess();
+ verify(delegateConnectorListener).onFailure(connector, exception);
}
@Test
public void testConnectorFailureBeforeStartupRecordedMetrics() {
- delegateConnectorListener.onFailure(eq(connector), eq(exception));
- expectLastCall();
-
- connectorStartupAttempts.record(eq(1.0));
- expectLastCall();
- connectorStartupFailures.record(eq(1.0));
- expectLastCall();
- connectorStartupResults.record(eq(0.0));
- expectLastCall();
-
- PowerMock.replayAll();
-
WorkerMetricsGroup workerMetricsGroup = new WorkerMetricsGroup(new HashMap<>(), new HashMap<>(), connectMetrics);
final ConnectorStatus.Listener connectorListener = workerMetricsGroup.wrapStatusListener(delegateConnectorListener);
connectorListener.onFailure(connector, exception);
- PowerMock.verifyAll();
+ verify(delegateConnectorListener).onFailure(connector, exception);
+ verifyRecordConnectorStartupFailure();
}
@Test
public void testTaskStartupRecordedMetrics() {
- delegateTaskListener.onStartup(eq(task));
- expectLastCall();
-
- taskStartupAttempts.record(eq(1.0));
- expectLastCall();
- taskStartupSuccesses.record(eq(1.0));
- expectLastCall();
- taskStartupResults.record(eq(1.0));
- expectLastCall();
-
- PowerMock.replayAll();
-
WorkerMetricsGroup workerMetricsGroup = new WorkerMetricsGroup(new HashMap<>(), new HashMap<>(), connectMetrics);
final TaskStatus.Listener taskListener = workerMetricsGroup.wrapStatusListener(delegateTaskListener);
taskListener.onStartup(task);
- PowerMock.verifyAll();
+ verify(delegateTaskListener).onStartup(task);
+ verifyRecordTaskSuccess();
}
@Test
public void testTaskFailureAfterStartupRecordedMetrics() {
- delegateTaskListener.onStartup(eq(task));
- expectLastCall();
-
- taskStartupAttempts.record(eq(1.0));
- expectLastCall();
- taskStartupSuccesses.record(eq(1.0));
- expectLastCall();
- taskStartupResults.record(eq(1.0));
- expectLastCall();
-
- delegateTaskListener.onFailure(eq(task), eq(exception));
- expectLastCall();
-
- // recordTaskFailure() should not be called if failure happens after a successful startup
Review Comment:
Fixed in upcoming commit.
--
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 #12677: KAFKA-14132: Replace PowerMock/Easymock with Mockito for WorkerMetricsGroupTest
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #12677:
URL: https://github.com/apache/kafka/pull/12677#discussion_r978863704
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerMetricsGroupTest.java:
##########
@@ -55,24 +54,19 @@ public class WorkerMetricsGroupTest {
private Sensor taskStartupSuccesses;
private Sensor taskStartupFailures;
- private ConnectorStatus.Listener delegateConnectorListener;
- private TaskStatus.Listener delegateTaskListener;
+ @Mock private ConnectorStatus.Listener delegateConnectorListener;
+ @Mock private TaskStatus.Listener delegateTaskListener;
@Before
public void setup() {
- connectMetrics = PowerMock.createMock(ConnectMetrics.class);
- ConnectMetricsRegistry connectMetricsRegistry = PowerMock.createNiceMock(ConnectMetricsRegistry.class);
- ConnectMetrics.MetricGroup metricGroup = PowerMock.createNiceMock(ConnectMetrics.MetricGroup.class);
+ ConnectMetricsRegistry connectMetricsRegistry = mock(ConnectMetricsRegistry.class);
+ ConnectMetrics.MetricGroup metricGroup = mock(ConnectMetrics.MetricGroup.class);
+ MetricName metricName = mock(MetricName.class);
Review Comment:
Primarily to keep the PR code reviewer friendly :) by minimising the refactoring and doing in-place changes as much as possible.
I will move these to annotations in upcoming commit.
--
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] C0urante commented on a diff in pull request #12677: KAFKA-14132: Replace PowerMock/Easymock with Mockito for WorkerMetricsGroupTest
Posted by GitBox <gi...@apache.org>.
C0urante commented on code in PR #12677:
URL: https://github.com/apache/kafka/pull/12677#discussion_r978651670
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerMetricsGroupTest.java:
##########
@@ -55,24 +54,19 @@ public class WorkerMetricsGroupTest {
private Sensor taskStartupSuccesses;
private Sensor taskStartupFailures;
- private ConnectorStatus.Listener delegateConnectorListener;
- private TaskStatus.Listener delegateTaskListener;
+ @Mock private ConnectorStatus.Listener delegateConnectorListener;
+ @Mock private TaskStatus.Listener delegateTaskListener;
@Before
public void setup() {
- connectMetrics = PowerMock.createMock(ConnectMetrics.class);
- ConnectMetricsRegistry connectMetricsRegistry = PowerMock.createNiceMock(ConnectMetricsRegistry.class);
- ConnectMetrics.MetricGroup metricGroup = PowerMock.createNiceMock(ConnectMetrics.MetricGroup.class);
+ ConnectMetricsRegistry connectMetricsRegistry = mock(ConnectMetricsRegistry.class);
+ ConnectMetrics.MetricGroup metricGroup = mock(ConnectMetrics.MetricGroup.class);
+ MetricName metricName = mock(MetricName.class);
Review Comment:
Any reason not to use `@Mock` to initialize these?
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerMetricsGroupTest.java:
##########
@@ -55,24 +54,19 @@ public class WorkerMetricsGroupTest {
private Sensor taskStartupSuccesses;
private Sensor taskStartupFailures;
- private ConnectorStatus.Listener delegateConnectorListener;
- private TaskStatus.Listener delegateTaskListener;
+ @Mock private ConnectorStatus.Listener delegateConnectorListener;
+ @Mock private TaskStatus.Listener delegateTaskListener;
@Before
public void setup() {
- connectMetrics = PowerMock.createMock(ConnectMetrics.class);
- ConnectMetricsRegistry connectMetricsRegistry = PowerMock.createNiceMock(ConnectMetricsRegistry.class);
- ConnectMetrics.MetricGroup metricGroup = PowerMock.createNiceMock(ConnectMetrics.MetricGroup.class);
+ ConnectMetricsRegistry connectMetricsRegistry = mock(ConnectMetricsRegistry.class);
+ ConnectMetrics.MetricGroup metricGroup = mock(ConnectMetrics.MetricGroup.class);
+ MetricName metricName = mock(MetricName.class);
- connectMetrics.registry();
- expectLastCall().andReturn(connectMetricsRegistry);
-
- connectMetrics.group(anyString());
- expectLastCall().andReturn(metricGroup);
-
- MetricName metricName = PowerMock.createMock(MetricName.class);
- metricGroup.metricName(anyObject(MetricNameTemplate.class));
- expectLastCall().andStubReturn(metricName);
+ when(metricGroup.metricName((MetricNameTemplate) isNull())).thenReturn(metricName);
Review Comment:
The [Javadocs](https://github.com/apache/kafka/blob/8e43548175db086cbedf1b990e17c80dc438d55e/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/ConnectMetrics.java#L270-L276) for this method specifically prohibit invoking it with a null argument:
```java
/**
* Create the name of a metric that belongs to this group and has the group's tags.
*
* @param template the name template for the metric; may not be null
* @return the metric name; never null
* @throws IllegalArgumentException if the name is not valid
*/
public MetricName metricName(MetricNameTemplate template) {
```
I can see how the way that we use the registry in the `WorkerMetricsGroup` class at places like [this](https://github.com/apache/kafka/blob/8e43548175db086cbedf1b990e17c80dc438d55e/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerMetricsGroup.java#L45) makes it difficult to mock out this part, and it's probably not worth a large refactoring to make mocking possible. Could we add a small comment explaining why we have this expectation, though? Maybe something like:
```java
// We don't expect this to be invoked with null in practice,
// but it's easier to test this way, and should have no impact
// on the efficacy of these tests
```
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerMetricsGroupTest.java:
##########
@@ -83,167 +77,107 @@ public void setup() {
taskStartupAttempts = mockSensor(metricGroup, "task-startup-attempts");
taskStartupSuccesses = mockSensor(metricGroup, "task-startup-successes");
taskStartupFailures = mockSensor(metricGroup, "task-startup-failures");
-
- delegateConnectorListener = PowerMock.createStrictMock(ConnectorStatus.Listener.class);
- delegateTaskListener = PowerMock.createStrictMock(TaskStatus.Listener.class);
}
private Sensor mockSensor(ConnectMetrics.MetricGroup metricGroup, String name) {
- Sensor sensor = PowerMock.createMock(Sensor.class);
- metricGroup.sensor(eq(name));
- expectLastCall().andReturn(sensor);
-
- sensor.add(anyObject(CompoundStat.class));
- expectLastCall().andStubReturn(true);
-
- sensor.add(anyObject(MetricName.class), anyObject(CumulativeSum.class));
- expectLastCall().andStubReturn(true);
-
+ Sensor sensor = mock(Sensor.class);
+ when(metricGroup.sensor(name)).thenReturn(sensor);
+ when(sensor.add(any(CompoundStat.class))).thenReturn(true);
+ when(sensor.add(any(MetricName.class), any(CumulativeSum.class))).thenReturn(true);
return sensor;
}
@Test
public void testConnectorStartupRecordedMetrics() {
- delegateConnectorListener.onStartup(eq(connector));
- expectLastCall();
-
- connectorStartupAttempts.record(eq(1.0));
- expectLastCall();
- connectorStartupSuccesses.record(eq(1.0));
- expectLastCall();
- connectorStartupResults.record(eq(1.0));
- expectLastCall();
-
- PowerMock.replayAll();
-
WorkerMetricsGroup workerMetricsGroup = new WorkerMetricsGroup(new HashMap<>(), new HashMap<>(), connectMetrics);
final ConnectorStatus.Listener connectorListener = workerMetricsGroup.wrapStatusListener(delegateConnectorListener);
connectorListener.onStartup(connector);
- PowerMock.verifyAll();
+ verifyRecordConnectorStartupSuccess();
+ verify(delegateConnectorListener).onStartup(connector);
}
@Test
public void testConnectorFailureAfterStartupRecordedMetrics() {
- delegateConnectorListener.onStartup(eq(connector));
- expectLastCall();
-
- connectorStartupAttempts.record(eq(1.0));
- expectLastCall();
- connectorStartupSuccesses.record(eq(1.0));
- expectLastCall();
- connectorStartupResults.record(eq(1.0));
- expectLastCall();
-
- delegateConnectorListener.onFailure(eq(connector), eq(exception));
- expectLastCall();
-
- // recordConnectorStartupFailure() should not be called if failure happens after a successful startup
-
- PowerMock.replayAll();
-
WorkerMetricsGroup workerMetricsGroup = new WorkerMetricsGroup(new HashMap<>(), new HashMap<>(), connectMetrics);
final ConnectorStatus.Listener connectorListener = workerMetricsGroup.wrapStatusListener(delegateConnectorListener);
connectorListener.onStartup(connector);
connectorListener.onFailure(connector, exception);
- PowerMock.verifyAll();
+ verify(delegateConnectorListener).onStartup(connector);
+ verifyRecordConnectorStartupSuccess();
+ verify(delegateConnectorListener).onFailure(connector, exception);
}
@Test
public void testConnectorFailureBeforeStartupRecordedMetrics() {
- delegateConnectorListener.onFailure(eq(connector), eq(exception));
- expectLastCall();
-
- connectorStartupAttempts.record(eq(1.0));
- expectLastCall();
- connectorStartupFailures.record(eq(1.0));
- expectLastCall();
- connectorStartupResults.record(eq(0.0));
- expectLastCall();
-
- PowerMock.replayAll();
-
WorkerMetricsGroup workerMetricsGroup = new WorkerMetricsGroup(new HashMap<>(), new HashMap<>(), connectMetrics);
final ConnectorStatus.Listener connectorListener = workerMetricsGroup.wrapStatusListener(delegateConnectorListener);
connectorListener.onFailure(connector, exception);
- PowerMock.verifyAll();
+ verify(delegateConnectorListener).onFailure(connector, exception);
+ verifyRecordConnectorStartupFailure();
}
@Test
public void testTaskStartupRecordedMetrics() {
- delegateTaskListener.onStartup(eq(task));
- expectLastCall();
-
- taskStartupAttempts.record(eq(1.0));
- expectLastCall();
- taskStartupSuccesses.record(eq(1.0));
- expectLastCall();
- taskStartupResults.record(eq(1.0));
- expectLastCall();
-
- PowerMock.replayAll();
-
WorkerMetricsGroup workerMetricsGroup = new WorkerMetricsGroup(new HashMap<>(), new HashMap<>(), connectMetrics);
final TaskStatus.Listener taskListener = workerMetricsGroup.wrapStatusListener(delegateTaskListener);
taskListener.onStartup(task);
- PowerMock.verifyAll();
+ verify(delegateTaskListener).onStartup(task);
+ verifyRecordTaskSuccess();
}
@Test
public void testTaskFailureAfterStartupRecordedMetrics() {
- delegateTaskListener.onStartup(eq(task));
- expectLastCall();
-
- taskStartupAttempts.record(eq(1.0));
- expectLastCall();
- taskStartupSuccesses.record(eq(1.0));
- expectLastCall();
- taskStartupResults.record(eq(1.0));
- expectLastCall();
-
- delegateTaskListener.onFailure(eq(task), eq(exception));
- expectLastCall();
-
- // recordTaskFailure() should not be called if failure happens after a successful startup
Review Comment:
Same thought RE keeping this comment in the test somewhere
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerMetricsGroupTest.java:
##########
@@ -83,167 +77,107 @@ public void setup() {
taskStartupAttempts = mockSensor(metricGroup, "task-startup-attempts");
taskStartupSuccesses = mockSensor(metricGroup, "task-startup-successes");
taskStartupFailures = mockSensor(metricGroup, "task-startup-failures");
-
- delegateConnectorListener = PowerMock.createStrictMock(ConnectorStatus.Listener.class);
- delegateTaskListener = PowerMock.createStrictMock(TaskStatus.Listener.class);
}
private Sensor mockSensor(ConnectMetrics.MetricGroup metricGroup, String name) {
- Sensor sensor = PowerMock.createMock(Sensor.class);
- metricGroup.sensor(eq(name));
- expectLastCall().andReturn(sensor);
-
- sensor.add(anyObject(CompoundStat.class));
- expectLastCall().andStubReturn(true);
-
- sensor.add(anyObject(MetricName.class), anyObject(CumulativeSum.class));
- expectLastCall().andStubReturn(true);
-
+ Sensor sensor = mock(Sensor.class);
+ when(metricGroup.sensor(name)).thenReturn(sensor);
+ when(sensor.add(any(CompoundStat.class))).thenReturn(true);
+ when(sensor.add(any(MetricName.class), any(CumulativeSum.class))).thenReturn(true);
return sensor;
}
@Test
public void testConnectorStartupRecordedMetrics() {
- delegateConnectorListener.onStartup(eq(connector));
- expectLastCall();
-
- connectorStartupAttempts.record(eq(1.0));
- expectLastCall();
- connectorStartupSuccesses.record(eq(1.0));
- expectLastCall();
- connectorStartupResults.record(eq(1.0));
- expectLastCall();
-
- PowerMock.replayAll();
-
WorkerMetricsGroup workerMetricsGroup = new WorkerMetricsGroup(new HashMap<>(), new HashMap<>(), connectMetrics);
final ConnectorStatus.Listener connectorListener = workerMetricsGroup.wrapStatusListener(delegateConnectorListener);
connectorListener.onStartup(connector);
- PowerMock.verifyAll();
+ verifyRecordConnectorStartupSuccess();
+ verify(delegateConnectorListener).onStartup(connector);
}
@Test
public void testConnectorFailureAfterStartupRecordedMetrics() {
- delegateConnectorListener.onStartup(eq(connector));
- expectLastCall();
-
- connectorStartupAttempts.record(eq(1.0));
- expectLastCall();
- connectorStartupSuccesses.record(eq(1.0));
- expectLastCall();
- connectorStartupResults.record(eq(1.0));
- expectLastCall();
-
- delegateConnectorListener.onFailure(eq(connector), eq(exception));
- expectLastCall();
-
- // recordConnectorStartupFailure() should not be called if failure happens after a successful startup
Review Comment:
Can we keep this comment somewhere? Seems like it'd fit after these lines below:
```java
verify(delegateConnectorListener).onStartup(connector);
verifyRecordConnectorStartupSuccess();
verify(delegateConnectorListener).onFailure(connector, exception);
```
--
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] C0urante merged pull request #12677: KAFKA-14132: Replace PowerMock/Easymock with Mockito for WorkerMetricsGroupTest
Posted by GitBox <gi...@apache.org>.
C0urante merged PR #12677:
URL: https://github.com/apache/kafka/pull/12677
--
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 #12677: KAFKA-14132: Replace PowerMock/Easymock with Mockito for WorkerMetricsGroupTest
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #12677:
URL: https://github.com/apache/kafka/pull/12677#discussion_r978873294
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerMetricsGroupTest.java:
##########
@@ -55,24 +54,19 @@ public class WorkerMetricsGroupTest {
private Sensor taskStartupSuccesses;
private Sensor taskStartupFailures;
- private ConnectorStatus.Listener delegateConnectorListener;
- private TaskStatus.Listener delegateTaskListener;
+ @Mock private ConnectorStatus.Listener delegateConnectorListener;
+ @Mock private TaskStatus.Listener delegateTaskListener;
@Before
public void setup() {
- connectMetrics = PowerMock.createMock(ConnectMetrics.class);
- ConnectMetricsRegistry connectMetricsRegistry = PowerMock.createNiceMock(ConnectMetricsRegistry.class);
- ConnectMetrics.MetricGroup metricGroup = PowerMock.createNiceMock(ConnectMetrics.MetricGroup.class);
+ ConnectMetricsRegistry connectMetricsRegistry = mock(ConnectMetricsRegistry.class);
+ ConnectMetrics.MetricGroup metricGroup = mock(ConnectMetrics.MetricGroup.class);
+ MetricName metricName = mock(MetricName.class);
- connectMetrics.registry();
- expectLastCall().andReturn(connectMetricsRegistry);
-
- connectMetrics.group(anyString());
- expectLastCall().andReturn(metricGroup);
-
- MetricName metricName = PowerMock.createMock(MetricName.class);
- metricGroup.metricName(anyObject(MetricNameTemplate.class));
- expectLastCall().andStubReturn(metricName);
+ when(metricGroup.metricName((MetricNameTemplate) isNull())).thenReturn(metricName);
Review Comment:
Fixed in the new commit. Thank you for suggesting the comment itself. I appreciate your detailed approach to code review.
--
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