lucasbru commented on PR #14842: URL: https://github.com/apache/kafka/pull/14842#issuecomment-1829919586
> Hi @lucasbru - I just opened the PR for you to review. I'm not 100% happy with the way tests are setup therefore I made some changes around optionally disabling autocommit in the network thread. Also, I feel the tests here kind of become some sort of integration testing. I thought it kind of against the unit test philosophy. Yeah. I think the tests may be using `spy` too much, and should use `mock` instead. It's definitely running too much code, which makes it more like an integration test, but still introspect a lot, which make them more like a unit test. But it's not something that we are going to fix in this PR. I wonder if we can maybe create a ticket to migrate the `AsyncKafkaConsumerTest` to a more mocking based approach, is that something that would be feasible in a reasonable amount of time? cc @cadonna who always has strong opinions on unit testing. > 1. We will only auto commit if the configuration is enabled (by default) or if we've got anything to commit at all Makes sense > 2. We need to enforce finding a coordinator and send an autocommit regardless of the previous commit state because we need to make sure to record the progress before closing Makes sense > 3. Quite a bit of changes to the testing, because autocommit depends on the current progress, so I need to "seek" for some cases to ensure the test sends an autocommit Makes sense -- 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]
