lhotari commented on PR #24100: URL: https://github.com/apache/pulsar/pull/24100#issuecomment-2741072800
> I don't want to waste your time. I believe I've already added tests to cover it. That's true, that there's a test, but the test is not from the viewpoint of the user of our software. The flow described in the description isn't performed in the test case. That would be an optimal "failing test case". In many software teams, this is a well known concept. However, in the Pulsar code base, most of our tests are not following usual best practices and don't appear to be good examples of how to write good tests. > You'll see that I've covered in the tests that the consumer will be removed(closed) after this PR fix. That's not "Cannot close the consumer since user will get InterruptedException and can't get consumer object", what was in the description. I agree that the internal behavior change is covered in the test, but not the original bug that appeared. > In this scenario, not getting the consumer object is expected. Why would we add a test case for it? A method throws an exception, and you want to cover that it returns null? Simply test the scenario "Cannot close the consumer since user will get InterruptedException and can't get consumer object" and assume that it results in whatever is the expected result. It's not more harder than that. > If you can review the code directly and let me know where improvements are needed, I'd be happy to optimize it. As long as the mentioned scenario is handled in a separate test case, I'm fine with this PR. I know that it's unusual to knit pick on such details in our PRs, but unless attention is paid to details we won't be able to learn from each other and improve as a whole. -- 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]
