EsMoX commented on PR #19041: URL: https://github.com/apache/kafka/pull/19041#issuecomment-2690914753
> Thanks for the PR. Seems there is some confusion about this test... > > First, the test was added like this 2 year ago and never modified. So it was missed on the original PR review [7c280c1](https://github.com/apache/kafka/commit/7c280c1d5f6267acbbebd10d3e58ea4b8fe7a4ef) > > But it seems there is some confusion what the test does... The `message` string, this PR changes, has nothing to do with what it actually tested... `assertThrows` only checks if the exception is `ConfigException` -- it does not verify the exception message. If `assertThrows` fails (ie, no exception or different exception thrown), it would use `message` as test result output, to given an informative message to the developer why the test fails (of course, this message would be misleading so yes, we should fix it.) > > Thus, there is no bug in the actual code, and because the test just passes always ever since it was added, the incorrect "test failed message" was never detected. > > I am happy to merge this PR as-is, as it does improve the test. But I want to point out, that it does not verify the actually error message from `ConfigException`. If we want to verify the error message, we would need to change the test further. Not sure if we want/need to verify the error message. Thoughts? > > Just let me know how you want to proceed. Thank you @mjsax for the detailed explanation and feedback! After considering the test behavior, I believe adding validation for the exception message would significantly improve the test for the following reasons: 1. **Promoting Shared Understanding** : Validating the exception message promotes shared knowledge among developers about the exact message that should be thrown when a ConfigException occurs. Since the error message is already defined when `throw new ConfigException()`, reusing it in the test ensures consistency, reduces duplication, and acts as a form of documentation for the expected behavior. This makes the codebase easier to understand and maintain. 2. **Improved Test Quality** : By validating both the exception type and its message, the test becomes stricter and more reliable. This ensures consistency and avoids issues like misleading failure messages, while providing greater confidence in the correctness of the code under test. That said, I agree with you in proceeding with the PR as-is for now, as it already improves the test by fixing the misleading failure message. Once the community decides how to handle exception message validation, I’ll be happy to revisit the test and make any necessary updates to align with the agreed approach. Let me know your thoughts! -- 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
