DanielLeens commented on PR #11574: URL: https://github.com/apache/seatunnel/pull/11574#issuecomment-5251995859
Thanks for both notes — let me take them separately since they're really two different questions. On the "does this need a UT" question: I want to be precise about what I'm actually asking for, because I think "UT in E2E" undersells it and makes it sound like I'm asking you to unit-test E2E *scenarios*, which I agree wouldn't make architectural sense. That's not the ask. `ContainerTcpProxy` itself is new, non-trivial code — it opens a `ServerSocket`, accepts connections, and pumps bytes bidirectionally between two sockets, with its own lifecycle (bind, accept loop, `close()` cleanup). That's the same kind of logic any other utility class in the codebase would get a plain, Docker-free JUnit test for, regardless of which module it happens to live in. The fact that its *callers* are E2E tests doesn't change that the proxy class itself has real logic that can have real bugs (a leaked socket on `close()`, a proxy that only forwards one direction, two proxies stepping on the same loopback port). A lightweight test of the class in isolation — no Testcontainers, no Docker — is w hat I had in mind; if you'd rather it live under a different package than the E2E tree to make that distinction clearer structurally, I'm fine with that too. The point is coverage of the proxy's own correctness, not testing E2E-ness. On the merge-impact assessment: really solid work, thank you — the dev-diff scope check, the `mergeable=true` confirmation, and mapping out which downstream PRs (#11572, #11626, and others) will need a rebase afterward is exactly the kind of due diligence that makes a wide-blast-radius E2E infra change easy to say yes to. I agree with the "low risk to dev" conclusion on that specific question. To be clear about scope though: that assessment answers "is it safe to merge relative to dev," not "is the code itself complete" — my Issue 1 (the regression test) is a code-quality ask independent of the dev-sync question, so it still stands on its own. Once that test lands I don't have anything else outstanding on this PR. -- 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]
