SEPURI-SAI-KRISHNA commented on PR #12455: URL: https://github.com/apache/seatunnel/pull/12455#issuecomment-5794690994
Thanks @DanielLeens. You are right that the wrapper is unused in that test, and I checked rather than assuming: `MqttSinkWriter`'s constructor reads `qos` and throws at the fourth statement, while `new MqttClient(...)` is well below that, so the mock is never exercised on the path the test takes. It was deliberate but undocumented, which is the actual problem. The reasoning was that the qos check sits above the connect only by convention, and if it ever moved below it the test would stop failing on the assertion and start reaching `tcp://localhost:1883` from a unit test. Keeping the wrapper makes that reordering harmless. That belonged in a comment rather than in my head, so the criticism lands either way. On timing: this PR is green right now, which has been unusually hard to come by on Windows this week, so I would rather not reset CI for a comment. If you would like it changed I will fold it in the moment anything else needs a push here, and my preference would be to annotate rather than drop, for the reason above. Equally happy to drop it if you would rather the test only mock what it uses. -- 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]
