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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
