mjsax commented on PR #19041: URL: https://github.com/apache/kafka/pull/19041#issuecomment-2689866165
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 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. -- 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
