Re: [PR] KAFKA-16084: Simplify and deduplicate standalone herder test mocking [kafka]
gharris1727 merged PR #15389: URL: https://github.com/apache/kafka/pull/15389 -- 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
Re: [PR] KAFKA-16084: Simplify and deduplicate standalone herder test mocking [kafka]
gharris1727 commented on PR #15389: URL: https://github.com/apache/kafka/pull/15389#issuecomment-2021497976 Test failures appear unrelated, and the connect:runtime tests pass for me locally. -- 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
Re: [PR] KAFKA-16084: Simplify and deduplicate standalone herder test mocking [kafka]
ahmedsobeh commented on PR #15389: URL: https://github.com/apache/kafka/pull/15389#issuecomment-2020172004 > Hi @ahmedsobeh This is looking great now! > > Since we're in the area, I want to try and clean up the warnings in this class, especially since some of them are on lines you've already changed. I think this will be the last round of review. > > Thanks! @gharris1727 All done! let me know if there's anything else -- 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
Re: [PR] KAFKA-16084: Simplify and deduplicate standalone herder test mocking [kafka]
gharris1727 commented on code in PR #15389:
URL: https://github.com/apache/kafka/pull/15389#discussion_r1538282799
##
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerderTest.java:
##
@@ -115,64 +115,61 @@ public class StandaloneHerderTest {
private static final String TOPICS_LIST_STR = "topic1,topic2";
private static final String WORKER_ID = "localhost:8083";
private static final String KAFKA_CLUSTER_ID = "I4ZmrWqfT2e-upky_4fdPA";
+private static final Long WAIT_TIME = 1000L;
Review Comment:
nit: 1 second is probably good enough, but it costs nothing to increase this
now.
Also the constant should probably include the unit, so `WAIT_TIME_MS`.
```suggestion
private static final Long WAIT_TIME = 15000L;
```
##
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerderTest.java:
##
@@ -227,59 +222,48 @@ public void testCreateConnectorAlreadyExists() throws
Throwable {
@Test
public void testCreateSinkConnector() throws Exception {
-connector = mock(BogusSinkConnector.class);
expectAdd(SourceSink.SINK);
Map config = connectorConfig(SourceSink.SINK);
-Connector connectorMock = mock(SinkConnector.class);
-expectConfigValidation(connectorMock, true, config);
+expectConfigValidation(SourceSink.SINK, config);
herder.putConnectorConfig(CONNECTOR_NAME, config, false,
createCallback);
-Herder.Created connectorInfo =
createCallback.get(1000L, TimeUnit.SECONDS);
+Herder.Created connectorInfo =
createCallback.get(WAIT_TIME, TimeUnit.MILLISECONDS);
assertEquals(createdInfo(SourceSink.SINK), connectorInfo.result());
}
@Test
public void testCreateConnectorWithStoppedInitialState() throws Exception {
-connector = mock(BogusSinkConnector.class);
Map config = connectorConfig(SourceSink.SINK);
Connector connectorMock = mock(SinkConnector.class);
Review Comment:
nit: unused
##
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerderTest.java:
##
@@ -1168,25 +1090,27 @@ private static Map
taskConfig(SourceSink sourceSink) {
}
private void expectConfigValidation(
-Connector connectorMock,
-boolean shouldCreateConnector,
+SourceSink sourceSink,
Map... configs
) {
// config validation
+Connector connectorMock = sourceSink == SourceSink.SOURCE ?
mock(SourceConnector.class) : mock(SinkConnector.class);
when(worker.configTransformer()).thenReturn(transformer);
final ArgumentCaptor> configCapture =
ArgumentCaptor.forClass(Map.class);
when(transformer.transform(configCapture.capture())).thenAnswer(invocation ->
configCapture.getValue());
when(worker.getPlugins()).thenReturn(plugins);
when(plugins.connectorLoader(anyString())).thenReturn(pluginLoader);
when(plugins.withClassLoader(pluginLoader)).thenReturn(loaderSwap);
-if (shouldCreateConnector) {
-when(worker.getPlugins()).thenReturn(plugins);
-when(plugins.newConnector(anyString())).thenReturn(connectorMock);
-}
+
+// Assume the connector should always be created
+when(worker.getPlugins()).thenReturn(plugins);
+when(plugins.newConnector(anyString())).thenReturn(connectorMock);
when(connectorMock.config()).thenReturn(new ConfigDef());
-for (Map config : configs)
+// Set up validation for each config
+for (Map config : configs) {
when(connectorMock.validate(config)).thenReturn(new
Config(Collections.emptyList()));
+}
}
// We need to use a real class here due to some issue with mocking
java.lang.Class
Review Comment:
nit: My IDE is also warning me that these should be `static`, which is fine
for how these are used. Could you also fix that?
##
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerderTest.java:
##
@@ -726,22 +668,21 @@ public void testAccessors() throws Exception {
doNothing().when(tasksConfigCb).onCompletion(any(NotFoundException.class),
isNull());
doNothing().when(connectorConfigCb).onCompletion(any(NotFoundException.class),
isNull());
-// Create connector
-connector = mock(BogusSourceConnector.class);
+
expectAdd(SourceSink.SOURCE);
-expectConfigValidation(connector, true, connConfig);
+expectConfigValidation(SourceSink.SOURCE, connConfig);
// Validate accessors with 1 connector
doNothing().when(listConnectorsCb).onCompletion(null,
singleton(CONNECTOR_NAME));
ConnectorInfo connInfo = new ConnectorInfo(CONNECTOR_NAME, connConfig,
Arrays.asList(new ConnectorTaskId(CONNECTOR_NAME, 0)),
Review Comment:
Re: [PR] KAFKA-16084: Simplify and deduplicate standalone herder test mocking [kafka]
ahmedsobeh commented on code in PR #15389:
URL: https://github.com/apache/kafka/pull/15389#discussion_r1538122992
##
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerderTest.java:
##
@@ -726,21 +683,21 @@ public void testAccessors() throws Exception {
doNothing().when(connectorConfigCb).onCompletion(any(NotFoundException.class),
isNull());
// Create connector
-connector = mock(BogusSourceConnector.class);
+Connector connector = mock(BogusSourceConnector.class);
Review Comment:
Thanks for the walkthrough! I now understand the full picture. I think I
addressed all your comments now, let me know if I missed anything
--
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
Re: [PR] KAFKA-16084: Simplify and deduplicate standalone herder test mocking [kafka]
gharris1727 commented on code in PR #15389:
URL: https://github.com/apache/kafka/pull/15389#discussion_r1535731883
##
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerderTest.java:
##
@@ -226,59 +223,51 @@ public void testCreateConnectorAlreadyExists() throws
Throwable {
@Test
public void testCreateSinkConnector() throws Exception {
-connector = mock(BogusSinkConnector.class);
expectAdd(SourceSink.SINK);
Map config = connectorConfig(SourceSink.SINK);
Connector connectorMock = mock(SinkConnector.class);
-expectConfigValidation(connectorMock, true, config);
+expectConfigValidation(connectorMock, config);
herder.putConnectorConfig(CONNECTOR_NAME, config, false,
createCallback);
-Herder.Created connectorInfo =
createCallback.get(1000L, TimeUnit.SECONDS);
+Herder.Created connectorInfo =
createCallback.get(WAIT_TIME, TimeUnit.MILLISECONDS);
assertEquals(createdInfo(SourceSink.SINK), connectorInfo.result());
}
@Test
public void testCreateConnectorWithStoppedInitialState() throws Exception {
-connector = mock(BogusSinkConnector.class);
Map config = connectorConfig(SourceSink.SINK);
Connector connectorMock = mock(SinkConnector.class);
-expectConfigValidation(connectorMock, false, config);
+expectConfigValidation(connectorMock, config);
when(plugins.newConnector(anyString())).thenReturn(connectorMock);
Review Comment:
No, you did the right thing removing the `false` argument. I was just trying
to say that it was a mistake that it was `false` instead of `true`, which broke
the test, so the previous author added this line to fix it:
```
when(plugins.newConnector(anyString())).thenReturn(connectorMock);
```
Now that the expectConfigValidation is mocking newConnector always, this
line in the test is duplicated and can be removed.
--
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
Re: [PR] KAFKA-16084: Simplify and deduplicate standalone herder test mocking [kafka]
gharris1727 commented on code in PR #15389:
URL: https://github.com/apache/kafka/pull/15389#discussion_r1535731883
##
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerderTest.java:
##
@@ -226,59 +223,51 @@ public void testCreateConnectorAlreadyExists() throws
Throwable {
@Test
public void testCreateSinkConnector() throws Exception {
-connector = mock(BogusSinkConnector.class);
expectAdd(SourceSink.SINK);
Map config = connectorConfig(SourceSink.SINK);
Connector connectorMock = mock(SinkConnector.class);
-expectConfigValidation(connectorMock, true, config);
+expectConfigValidation(connectorMock, config);
herder.putConnectorConfig(CONNECTOR_NAME, config, false,
createCallback);
-Herder.Created connectorInfo =
createCallback.get(1000L, TimeUnit.SECONDS);
+Herder.Created connectorInfo =
createCallback.get(WAIT_TIME, TimeUnit.MILLISECONDS);
assertEquals(createdInfo(SourceSink.SINK), connectorInfo.result());
}
@Test
public void testCreateConnectorWithStoppedInitialState() throws Exception {
-connector = mock(BogusSinkConnector.class);
Map config = connectorConfig(SourceSink.SINK);
Connector connectorMock = mock(SinkConnector.class);
-expectConfigValidation(connectorMock, false, config);
+expectConfigValidation(connectorMock, config);
when(plugins.newConnector(anyString())).thenReturn(connectorMock);
Review Comment:
No, you did the right thing removing the `false` argument. I was just trying
to say that it was a mistake that it was `false` instead of `true`, which broke
the test, so the previous author added this line to fix it:
```
when(plugins.newConnector(anyString())).thenReturn(connectorMock);
```
Now that the expectConfigValidation is mocking newConnector always, this
test in the line is duplicated and can be removed.
--
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
Re: [PR] KAFKA-16084: Simplify and deduplicate standalone herder test mocking [kafka]
gharris1727 commented on code in PR #15389:
URL: https://github.com/apache/kafka/pull/15389#discussion_r1535731883
##
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerderTest.java:
##
@@ -226,59 +223,51 @@ public void testCreateConnectorAlreadyExists() throws
Throwable {
@Test
public void testCreateSinkConnector() throws Exception {
-connector = mock(BogusSinkConnector.class);
expectAdd(SourceSink.SINK);
Map config = connectorConfig(SourceSink.SINK);
Connector connectorMock = mock(SinkConnector.class);
-expectConfigValidation(connectorMock, true, config);
+expectConfigValidation(connectorMock, config);
herder.putConnectorConfig(CONNECTOR_NAME, config, false,
createCallback);
-Herder.Created connectorInfo =
createCallback.get(1000L, TimeUnit.SECONDS);
+Herder.Created connectorInfo =
createCallback.get(WAIT_TIME, TimeUnit.MILLISECONDS);
assertEquals(createdInfo(SourceSink.SINK), connectorInfo.result());
}
@Test
public void testCreateConnectorWithStoppedInitialState() throws Exception {
-connector = mock(BogusSinkConnector.class);
Map config = connectorConfig(SourceSink.SINK);
Connector connectorMock = mock(SinkConnector.class);
-expectConfigValidation(connectorMock, false, config);
+expectConfigValidation(connectorMock, config);
when(plugins.newConnector(anyString())).thenReturn(connectorMock);
Review Comment:
No, you did the right thing removing the `false` argument. I was just trying
to say that it was a mistake that it was `false` instead of `true`, which broke
the test, so the previous author added the line
```
when(plugins.newConnector(anyString())).thenReturn(connectorMock);
```
Now that the expectConfigValidation is mocking newConnector always, this
test in the line is duplicated and can be removed.
--
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
Re: [PR] KAFKA-16084: Simplify and deduplicate standalone herder test mocking [kafka]
gharris1727 commented on code in PR #15389:
URL: https://github.com/apache/kafka/pull/15389#discussion_r1535727231
##
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerderTest.java:
##
@@ -726,21 +683,21 @@ public void testAccessors() throws Exception {
doNothing().when(connectorConfigCb).onCompletion(any(NotFoundException.class),
isNull());
// Create connector
-connector = mock(BogusSourceConnector.class);
+Connector connector = mock(BogusSourceConnector.class);
Review Comment:
> For tests like testPutConnectorConfig where the function is called twice,
the test fails as the mock is different for each function call. I'm yet to
debug why having the same mock is the right way but thought I'd ask you first
It would help if I actually told you what this mock is doing. The strange
two-calls, boolean argument, multiple configs is related to just this one
computeIfAbsent call:
https://github.com/apache/kafka/blob/159d25a7df25975694e2e0eb18a8feb125f7c39e/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/AbstractHerder.java#L755
Each time the `AbstractHerder` (superclass of `StandaloneHerder`) runs a
validation and processes a config it calls `computeIfAbsent`, and only calls
newConnector once. This single Connector instance then validates every
configuration for that worker.
So the first call to `expectConfigValidation` (with argument true) mock out
the creation, and the following calls (with argument false) just add more
config invocations to the existing mocked object. If you create a new mocked
object, those are never used because the original mocked object is re-used.
Rather than returning the mocked object, I think it would be better that a
single expectConfigValidation call mocked out all of the configs in-advance,
like `testCreateConnectorAlreadyExists` does with two `config` arguments. The
`Map` argument in the `false` calls can be moved to the `true` calls.
--
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
Re: [PR] KAFKA-16084: Simplify and deduplicate standalone herder test mocking [kafka]
ahmedsobeh commented on code in PR #15389:
URL: https://github.com/apache/kafka/pull/15389#discussion_r1535694321
##
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerderTest.java:
##
@@ -726,21 +683,21 @@ public void testAccessors() throws Exception {
doNothing().when(connectorConfigCb).onCompletion(any(NotFoundException.class),
isNull());
// Create connector
-connector = mock(BogusSourceConnector.class);
+Connector connector = mock(BogusSourceConnector.class);
Review Comment:
I just verified that it's actually the reason for the tests failing. I can
adjust expectConfigValidation to return the mocked connector and reuse it? or
would that defeat the purpose of the mock not being visible to the test?
--
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
Re: [PR] KAFKA-16084: Simplify and deduplicate standalone herder test mocking [kafka]
ahmedsobeh commented on code in PR #15389:
URL: https://github.com/apache/kafka/pull/15389#discussion_r1535665054
##
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerderTest.java:
##
@@ -773,40 +730,34 @@ public void testPutConnectorConfig() throws Exception {
Callback> connectorConfigCb = mock(Callback.class);
// Create
-connector = mock(BogusSourceConnector.class);
expectAdd(SourceSink.SOURCE);
Connector connectorMock = mock(SourceConnector.class);
-expectConfigValidation(connectorMock, true, connConfig);
+expectConfigValidation(connectorMock, connConfig);
// Should get first config
doNothing().when(connectorConfigCb).onCompletion(null, connConfig);
// Update config, which requires stopping and restarting
doNothing().when(worker).stopAndAwaitConnector(CONNECTOR_NAME);
final ArgumentCaptor> capturedConfig =
ArgumentCaptor.forClass(Map.class);
-final ArgumentCaptor> onStart =
ArgumentCaptor.forClass(Callback.class);
-doAnswer(invocation -> {
-onStart.getValue().onCompletion(null, TargetState.STARTED);
-return true;
-}).when(worker).startConnector(eq(CONNECTOR_NAME),
capturedConfig.capture(), any(),
-eq(herder), eq(TargetState.STARTED), onStart.capture());
+mockStartConnector(null, capturedConfig, TargetState.STARTED,
TargetState.STARTED, null);
// Generate same task config, which should result in no additional
action to restart tasks
when(worker.connectorTaskConfigs(CONNECTOR_NAME, new
SourceConnectorConfig(plugins, newConnConfig, true)))
-.thenReturn(singletonList(taskConfig(SourceSink.SOURCE)));
+.thenReturn(singletonList(taskConfig(SourceSink.SOURCE)));
-expectConfigValidation(connectorMock, false, newConnConfig);
+expectConfigValidation(connectorMock, newConnConfig);
Review Comment:
This is probably related to what I'm saying here
https://github.com/apache/kafka/pull/15389/files/57987425879488e89bd7fe91276e85a793baed80#r1535652744
But i think I'm missing something
--
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
Re: [PR] KAFKA-16084: Simplify and deduplicate standalone herder test mocking [kafka]
ahmedsobeh commented on code in PR #15389:
URL: https://github.com/apache/kafka/pull/15389#discussion_r1535654142
##
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerderTest.java:
##
@@ -226,59 +223,51 @@ public void testCreateConnectorAlreadyExists() throws
Throwable {
@Test
public void testCreateSinkConnector() throws Exception {
-connector = mock(BogusSinkConnector.class);
expectAdd(SourceSink.SINK);
Map config = connectorConfig(SourceSink.SINK);
Connector connectorMock = mock(SinkConnector.class);
-expectConfigValidation(connectorMock, true, config);
+expectConfigValidation(connectorMock, config);
herder.putConnectorConfig(CONNECTOR_NAME, config, false,
createCallback);
-Herder.Created connectorInfo =
createCallback.get(1000L, TimeUnit.SECONDS);
+Herder.Created connectorInfo =
createCallback.get(WAIT_TIME, TimeUnit.MILLISECONDS);
assertEquals(createdInfo(SourceSink.SINK), connectorInfo.result());
}
@Test
public void testCreateConnectorWithStoppedInitialState() throws Exception {
-connector = mock(BogusSinkConnector.class);
Map config = connectorConfig(SourceSink.SINK);
Connector connectorMock = mock(SinkConnector.class);
-expectConfigValidation(connectorMock, false, config);
+expectConfigValidation(connectorMock, config);
when(plugins.newConnector(anyString())).thenReturn(connectorMock);
Review Comment:
Can you explain this a bit further? do you mean we need to have the boolean
argument and the check back again?
--
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
Re: [PR] KAFKA-16084: Simplify and deduplicate standalone herder test mocking [kafka]
ahmedsobeh commented on code in PR #15389:
URL: https://github.com/apache/kafka/pull/15389#discussion_r1535652744
##
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerderTest.java:
##
@@ -726,21 +683,21 @@ public void testAccessors() throws Exception {
doNothing().when(connectorConfigCb).onCompletion(any(NotFoundException.class),
isNull());
// Create connector
-connector = mock(BogusSourceConnector.class);
+Connector connector = mock(BogusSourceConnector.class);
Review Comment:
Sorry for being late here, had a few hospital visits.
that makes sense . For tests like `testPutConnectorConfig` where the
function is called twice, the test fails as the mock is different for each
function call. I'm yet to debug why having the same mock is the right way but
thought I'd ask you first
--
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
Re: [PR] KAFKA-16084: Simplify and deduplicate standalone herder test mocking [kafka]
gharris1727 commented on code in PR #15389:
URL: https://github.com/apache/kafka/pull/15389#discussion_r1523493857
##
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerderTest.java:
##
@@ -726,21 +683,21 @@ public void testAccessors() throws Exception {
doNothing().when(connectorConfigCb).onCompletion(any(NotFoundException.class),
isNull());
// Create connector
-connector = mock(BogusSourceConnector.class);
+Connector connector = mock(BogusSourceConnector.class);
Review Comment:
You would have expectConfigValidation accept a sourceSink, and then
basically do this:
```
Connector connectorMock = sourceSink == SourceSink.SOURCE ?
mock(SourceConnector.class) : mock(SinkConnector.class);
```
--
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
Re: [PR] KAFKA-16084: Simplify and deduplicate standalone herder test mocking [kafka]
gharris1727 commented on code in PR #15389:
URL: https://github.com/apache/kafka/pull/15389#discussion_r1523479764
##
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerderTest.java:
##
@@ -773,40 +730,34 @@ public void testPutConnectorConfig() throws Exception {
Callback> connectorConfigCb = mock(Callback.class);
// Create
-connector = mock(BogusSourceConnector.class);
expectAdd(SourceSink.SOURCE);
Connector connectorMock = mock(SourceConnector.class);
-expectConfigValidation(connectorMock, true, connConfig);
+expectConfigValidation(connectorMock, connConfig);
// Should get first config
doNothing().when(connectorConfigCb).onCompletion(null, connConfig);
// Update config, which requires stopping and restarting
doNothing().when(worker).stopAndAwaitConnector(CONNECTOR_NAME);
final ArgumentCaptor> capturedConfig =
ArgumentCaptor.forClass(Map.class);
-final ArgumentCaptor> onStart =
ArgumentCaptor.forClass(Callback.class);
-doAnswer(invocation -> {
-onStart.getValue().onCompletion(null, TargetState.STARTED);
-return true;
-}).when(worker).startConnector(eq(CONNECTOR_NAME),
capturedConfig.capture(), any(),
-eq(herder), eq(TargetState.STARTED), onStart.capture());
+mockStartConnector(null, capturedConfig, TargetState.STARTED,
TargetState.STARTED, null);
Review Comment:
Oh, no that isn't important. You can use any(HerderConnectorContext.class)
on both branches.
--
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
Re: [PR] KAFKA-16084: Simplify and deduplicate standalone herder test mocking [kafka]
ahmedsobeh commented on code in PR #15389:
URL: https://github.com/apache/kafka/pull/15389#discussion_r1523007035
##
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerderTest.java:
##
@@ -773,40 +730,34 @@ public void testPutConnectorConfig() throws Exception {
Callback> connectorConfigCb = mock(Callback.class);
// Create
-connector = mock(BogusSourceConnector.class);
expectAdd(SourceSink.SOURCE);
Connector connectorMock = mock(SourceConnector.class);
-expectConfigValidation(connectorMock, true, connConfig);
+expectConfigValidation(connectorMock, connConfig);
// Should get first config
doNothing().when(connectorConfigCb).onCompletion(null, connConfig);
// Update config, which requires stopping and restarting
doNothing().when(worker).stopAndAwaitConnector(CONNECTOR_NAME);
final ArgumentCaptor> capturedConfig =
ArgumentCaptor.forClass(Map.class);
-final ArgumentCaptor> onStart =
ArgumentCaptor.forClass(Callback.class);
-doAnswer(invocation -> {
-onStart.getValue().onCompletion(null, TargetState.STARTED);
-return true;
-}).when(worker).startConnector(eq(CONNECTOR_NAME),
capturedConfig.capture(), any(),
-eq(herder), eq(TargetState.STARTED), onStart.capture());
+mockStartConnector(null, capturedConfig, TargetState.STARTED,
TargetState.STARTED, null);
Review Comment:
I use the value of captured config to determine whether to call any() or
any(HerderConnectorContext.class), is there anything else to decide based on?
--
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
Re: [PR] KAFKA-16084: Simplify and deduplicate standalone herder test mocking [kafka]
ahmedsobeh commented on code in PR #15389:
URL: https://github.com/apache/kafka/pull/15389#discussion_r1522985257
##
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerderTest.java:
##
@@ -726,21 +683,21 @@ public void testAccessors() throws Exception {
doNothing().when(connectorConfigCb).onCompletion(any(NotFoundException.class),
isNull());
// Create connector
-connector = mock(BogusSourceConnector.class);
+Connector connector = mock(BogusSourceConnector.class);
Review Comment:
Agreed on the connector being created internally, but wouldn't that mean
that we only need the config argument? I think I missed the usage of the new
SourceSink variable in your explanation
--
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
Re: [PR] KAFKA-16084: Simplify and deduplicate standalone herder test mocking [kafka]
gharris1727 commented on code in PR #15389:
URL: https://github.com/apache/kafka/pull/15389#discussion_r1517018424
##
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerderTest.java:
##
@@ -773,40 +730,34 @@ public void testPutConnectorConfig() throws Exception {
Callback> connectorConfigCb = mock(Callback.class);
// Create
-connector = mock(BogusSourceConnector.class);
expectAdd(SourceSink.SOURCE);
Connector connectorMock = mock(SourceConnector.class);
-expectConfigValidation(connectorMock, true, connConfig);
+expectConfigValidation(connectorMock, connConfig);
// Should get first config
doNothing().when(connectorConfigCb).onCompletion(null, connConfig);
// Update config, which requires stopping and restarting
doNothing().when(worker).stopAndAwaitConnector(CONNECTOR_NAME);
final ArgumentCaptor> capturedConfig =
ArgumentCaptor.forClass(Map.class);
-final ArgumentCaptor> onStart =
ArgumentCaptor.forClass(Callback.class);
-doAnswer(invocation -> {
-onStart.getValue().onCompletion(null, TargetState.STARTED);
-return true;
-}).when(worker).startConnector(eq(CONNECTOR_NAME),
capturedConfig.capture(), any(),
-eq(herder), eq(TargetState.STARTED), onStart.capture());
+mockStartConnector(null, capturedConfig, TargetState.STARTED,
TargetState.STARTED, null);
Review Comment:
Instead of capturing the config, pass in `newConnConfig` to use the equality
assertion. Then the capturedConfig argument is always null and we can eliminate
some of the code paths in mockStartConnector.
##
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerderTest.java:
##
@@ -1044,14 +988,14 @@ public void
testRequestTaskReconfigurationDoesNotDeadlock() throws Exception {
// Set new config on the connector and tasks
FutureCallback> reconfigureCallback =
new FutureCallback<>();
-expectConfigValidation(connectorMock, false, newConfig);
+expectConfigValidation(connectorMock, newConfig);
Review Comment:
See testPutConnectorConfig comment about `false`.
##
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerderTest.java:
##
@@ -773,40 +730,34 @@ public void testPutConnectorConfig() throws Exception {
Callback> connectorConfigCb = mock(Callback.class);
// Create
-connector = mock(BogusSourceConnector.class);
expectAdd(SourceSink.SOURCE);
Connector connectorMock = mock(SourceConnector.class);
-expectConfigValidation(connectorMock, true, connConfig);
+expectConfigValidation(connectorMock, connConfig);
// Should get first config
doNothing().when(connectorConfigCb).onCompletion(null, connConfig);
// Update config, which requires stopping and restarting
doNothing().when(worker).stopAndAwaitConnector(CONNECTOR_NAME);
final ArgumentCaptor> capturedConfig =
ArgumentCaptor.forClass(Map.class);
-final ArgumentCaptor> onStart =
ArgumentCaptor.forClass(Callback.class);
-doAnswer(invocation -> {
-onStart.getValue().onCompletion(null, TargetState.STARTED);
-return true;
-}).when(worker).startConnector(eq(CONNECTOR_NAME),
capturedConfig.capture(), any(),
-eq(herder), eq(TargetState.STARTED), onStart.capture());
+mockStartConnector(null, capturedConfig, TargetState.STARTED,
TargetState.STARTED, null);
// Generate same task config, which should result in no additional
action to restart tasks
when(worker.connectorTaskConfigs(CONNECTOR_NAME, new
SourceConnectorConfig(plugins, newConnConfig, true)))
-.thenReturn(singletonList(taskConfig(SourceSink.SOURCE)));
+.thenReturn(singletonList(taskConfig(SourceSink.SOURCE)));
-expectConfigValidation(connectorMock, false, newConnConfig);
+expectConfigValidation(connectorMock, newConnConfig);
Review Comment:
Here and in testPutConnectorConfig where the second argument was `false`,
the config argument should be moved up to the first call in the method.
##
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerderTest.java:
##
@@ -114,64 +114,62 @@ public class StandaloneHerderTest {
private static final String TOPICS_LIST_STR = "topic1,topic2";
private static final String WORKER_ID = "localhost:8083";
private static final String KAFKA_CLUSTER_ID = "I4ZmrWqfT2e-upky_4fdPA";
+private static final Long WAIT_TIME = 1000L;
private enum SourceSink {
SOURCE, SINK
}
private StandaloneHerder herder;
-private Connector connector;
@Mock
protected Worker worker;
-@Mock protected
[PR] Kafka 16084 Simplify and deduplicate standalone herder test mocking [kafka]
ahmedsobeh opened a new pull request, #15389: URL: https://github.com/apache/kafka/pull/15389 - Removed the connector field. - The class had a mix of Mock annotations and mock(Class) invocations, cleaned up one of them - The test doesn't stop the thread pool created inside the herder and might leak threads, added herder stopping in tearDown - Mocking for Worker#startConnector is 6 lines which are duplicated 8 times throughout the test, extracted to a method - Some waits are 1000 ms and others are 1000 s, and could be pulled out to constants or a util method. unified and set to a constant *Summary of testing strategy (including rationale) for the feature or bug fix. Unit and/or integration tests are expected for any behaviour change and system tests should be considered for larger changes.* ### 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
