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/19 19:55:22 UTC
[GitHub] [kafka] C0urante commented on a diff in pull request #12422: KAFKA-13982: Move WorkerConfigTransformerTest to use Mockito
C0urante commented on code in PR #12422:
URL: https://github.com/apache/kafka/pull/12422#discussion_r924890270
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerConfigTransformerTest.java:
##########
@@ -144,10 +149,6 @@ public void subscribe(String path, Set<String> keys, ConfigChangeCallback callba
throw new UnsupportedOperationException();
}
- public void unsubscribe(String path, Set<String> keys) {
- throw new UnsupportedOperationException();
- }
-
Review Comment:
Nice catch, took me a second to figure out the misalignment between the type signatures for this method and for `ConfigProvider::unsubscribe`.
Should we also remove the `subscribe` method, since it matches the [default implementation](https://github.com/apache/kafka/blob/693e283802590b724ef441d5bf7acb6eeced91c5/clients/src/main/java/org/apache/kafka/common/config/provider/ConfigProvider.java#L59-L61) for `ConfigProvider::subscribe`?
Also, should we annotate any remaining methods with `@Override` to help prevent future mistakes like the one you've caught here?
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerConfigTransformerTest.java:
##########
@@ -53,64 +50,72 @@ public class WorkerConfigTransformerTest {
public static final String TEST_RESULT_WITH_TTL = "testResultWithTTL";
public static final String TEST_RESULT_WITH_LONGER_TTL = "testResultWithLongerTTL";
- @Mock private Herder herder;
- @Mock private Worker worker;
- @Mock private HerderRequest requestId;
+ private final Herder herder = Mockito.mock(Herder.class);
+ private final Worker worker = Mockito.mock(Worker.class);
+ private final HerderRequest requestId = Mockito.mock(HerderRequest.class);
private WorkerConfigTransformer configTransformer;
@Before
public void setup() {
- worker = PowerMock.createMock(Worker.class);
- herder = PowerMock.createMock(Herder.class);
configTransformer = new WorkerConfigTransformer(worker, Collections.singletonMap("test", new TestConfigProvider()));
}
@Test
public void testReplaceVariable() {
+ // Execution
Map<String, String> result = configTransformer.transform(MY_CONNECTOR, Collections.singletonMap(MY_KEY, "${test:testPath:testKey}"));
+
+ // Assertions
assertEquals(TEST_RESULT, result.get(MY_KEY));
}
@Test
public void testReplaceVariableWithTTL() {
- EasyMock.expect(worker.herder()).andReturn(herder);
-
- replayAll();
+ // Setup
+ when(worker.herder()).thenReturn(herder);
+ // Execution
Map<String, String> props = new HashMap<>();
props.put(MY_KEY, "${test:testPath:testKeyWithTTL}");
props.put(CONFIG_RELOAD_ACTION_CONFIG, CONFIG_RELOAD_ACTION_NONE);
Map<String, String> result = configTransformer.transform(MY_CONNECTOR, props);
+
+ // Assertions
+ assertEquals(TEST_RESULT_WITH_TTL, result.get(MY_KEY));
Review Comment:
Good catch 👍
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerConfigTransformerTest.java:
##########
@@ -53,64 +50,72 @@ public class WorkerConfigTransformerTest {
public static final String TEST_RESULT_WITH_TTL = "testResultWithTTL";
public static final String TEST_RESULT_WITH_LONGER_TTL = "testResultWithLongerTTL";
- @Mock private Herder herder;
- @Mock private Worker worker;
- @Mock private HerderRequest requestId;
+ private final Herder herder = Mockito.mock(Herder.class);
+ private final Worker worker = Mockito.mock(Worker.class);
+ private final HerderRequest requestId = Mockito.mock(HerderRequest.class);
private WorkerConfigTransformer configTransformer;
@Before
public void setup() {
- worker = PowerMock.createMock(Worker.class);
- herder = PowerMock.createMock(Herder.class);
configTransformer = new WorkerConfigTransformer(worker, Collections.singletonMap("test", new TestConfigProvider()));
}
@Test
public void testReplaceVariable() {
+ // Execution
Map<String, String> result = configTransformer.transform(MY_CONNECTOR, Collections.singletonMap(MY_KEY, "${test:testPath:testKey}"));
+
+ // Assertions
assertEquals(TEST_RESULT, result.get(MY_KEY));
}
@Test
public void testReplaceVariableWithTTL() {
- EasyMock.expect(worker.herder()).andReturn(herder);
-
- replayAll();
+ // Setup
+ when(worker.herder()).thenReturn(herder);
+ // Execution
Map<String, String> props = new HashMap<>();
props.put(MY_KEY, "${test:testPath:testKeyWithTTL}");
props.put(CONFIG_RELOAD_ACTION_CONFIG, CONFIG_RELOAD_ACTION_NONE);
Map<String, String> result = configTransformer.transform(MY_CONNECTOR, props);
+
+ // Assertions
+ assertEquals(TEST_RESULT_WITH_TTL, result.get(MY_KEY));
}
@Test
public void testReplaceVariableWithTTLAndScheduleRestart() {
- EasyMock.expect(worker.herder()).andReturn(herder);
- EasyMock.expect(herder.restartConnector(eq(1L), eq(MY_CONNECTOR), notNull())).andReturn(requestId);
- replayAll();
+ // Setup
+ when(worker.herder()).thenReturn(herder);
+ when(herder.restartConnector(eq(1L), eq(MY_CONNECTOR), notNull())).thenReturn(requestId);
+ // Execution
Map<String, String> result = configTransformer.transform(MY_CONNECTOR, Collections.singletonMap(MY_KEY, "${test:testPath:testKeyWithTTL}"));
+
+ // Assertions
assertEquals(TEST_RESULT_WITH_TTL, result.get(MY_KEY));
}
@Test
public void testReplaceVariableWithTTLFirstCancelThenScheduleRestart() {
- EasyMock.expect(worker.herder()).andReturn(herder);
- EasyMock.expect(herder.restartConnector(eq(1L), eq(MY_CONNECTOR), notNull())).andReturn(requestId);
-
- EasyMock.expect(worker.herder()).andReturn(herder);
- EasyMock.expectLastCall();
- requestId.cancel();
- EasyMock.expectLastCall();
- EasyMock.expect(herder.restartConnector(eq(10L), eq(MY_CONNECTOR), notNull())).andReturn(requestId);
-
- replayAll();
+ // Setup
+ when(worker.herder()).thenReturn(herder);
+ when(herder.restartConnector(eq(1L), eq(MY_CONNECTOR), notNull())).thenReturn(requestId);
+ when(herder.restartConnector(eq(10L), eq(MY_CONNECTOR), notNull())).thenReturn(requestId);
+ // Execution
Map<String, String> result = configTransformer.transform(MY_CONNECTOR, Collections.singletonMap(MY_KEY, "${test:testPath:testKeyWithTTL}"));
+
+ // Assertions
assertEquals(TEST_RESULT_WITH_TTL, result.get(MY_KEY));
+ // Execution
result = configTransformer.transform(MY_CONNECTOR, Collections.singletonMap(MY_KEY, "${test:testPath:testKeyWithLongerTTL}"));
+
+ // Assertions
assertEquals(TEST_RESULT_WITH_LONGER_TTL, result.get(MY_KEY));
+ verify(requestId, times(1)).cancel();
Review Comment:
Same thought here RE verifying `herder::restartConnector`
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerConfigTransformerTest.java:
##########
@@ -53,64 +50,72 @@ public class WorkerConfigTransformerTest {
public static final String TEST_RESULT_WITH_TTL = "testResultWithTTL";
public static final String TEST_RESULT_WITH_LONGER_TTL = "testResultWithLongerTTL";
- @Mock private Herder herder;
- @Mock private Worker worker;
- @Mock private HerderRequest requestId;
+ private final Herder herder = Mockito.mock(Herder.class);
+ private final Worker worker = Mockito.mock(Worker.class);
+ private final HerderRequest requestId = Mockito.mock(HerderRequest.class);
private WorkerConfigTransformer configTransformer;
@Before
public void setup() {
- worker = PowerMock.createMock(Worker.class);
- herder = PowerMock.createMock(Herder.class);
configTransformer = new WorkerConfigTransformer(worker, Collections.singletonMap("test", new TestConfigProvider()));
}
@Test
public void testReplaceVariable() {
+ // Execution
Map<String, String> result = configTransformer.transform(MY_CONNECTOR, Collections.singletonMap(MY_KEY, "${test:testPath:testKey}"));
+
+ // Assertions
assertEquals(TEST_RESULT, result.get(MY_KEY));
}
@Test
public void testReplaceVariableWithTTL() {
- EasyMock.expect(worker.herder()).andReturn(herder);
-
- replayAll();
+ // Setup
+ when(worker.herder()).thenReturn(herder);
+ // Execution
Map<String, String> props = new HashMap<>();
props.put(MY_KEY, "${test:testPath:testKeyWithTTL}");
props.put(CONFIG_RELOAD_ACTION_CONFIG, CONFIG_RELOAD_ACTION_NONE);
Map<String, String> result = configTransformer.transform(MY_CONNECTOR, props);
+
+ // Assertions
+ assertEquals(TEST_RESULT_WITH_TTL, result.get(MY_KEY));
}
@Test
public void testReplaceVariableWithTTLAndScheduleRestart() {
- EasyMock.expect(worker.herder()).andReturn(herder);
- EasyMock.expect(herder.restartConnector(eq(1L), eq(MY_CONNECTOR), notNull())).andReturn(requestId);
- replayAll();
+ // Setup
+ when(worker.herder()).thenReturn(herder);
+ when(herder.restartConnector(eq(1L), eq(MY_CONNECTOR), notNull())).thenReturn(requestId);
+ // Execution
Map<String, String> result = configTransformer.transform(MY_CONNECTOR, Collections.singletonMap(MY_KEY, "${test:testPath:testKeyWithTTL}"));
+
+ // Assertions
Review Comment:
Should we add a verification here that `herder::restartConnector` was invoked?
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerConfigTransformerTest.java:
##########
@@ -53,64 +50,72 @@ public class WorkerConfigTransformerTest {
public static final String TEST_RESULT_WITH_TTL = "testResultWithTTL";
public static final String TEST_RESULT_WITH_LONGER_TTL = "testResultWithLongerTTL";
- @Mock private Herder herder;
- @Mock private Worker worker;
- @Mock private HerderRequest requestId;
+ private final Herder herder = Mockito.mock(Herder.class);
+ private final Worker worker = Mockito.mock(Worker.class);
+ private final HerderRequest requestId = Mockito.mock(HerderRequest.class);
Review Comment:
Why not use the Mockito `@Mock` annotation 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