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: ni
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 WorkerConfigTrans