akshar27 commented on PR #12129: URL: https://github.com/apache/seatunnel/pull/12129#issuecomment-5555623975
Thanks for the thorough review! Re: CI — GitHub Actions needs a one-time manual enable on my fork after forking (a browser-only consent step GitHub doesn't expose via API). I've asked for that to be done and will push a retrigger commit once it's confirmed enabled. Re: the non-blocking suggestion to add a `PhysicalPlan`-level unit test exercising `cancelJob()`/`stopJob()` directly against `NOT_YET_STARTED_STATES` membership — I looked into it, but a faithful test would need to mock a fairly deep chain of internal machinery beyond just the `IMap`s (`updateStateInfo`, `reportJobStateEvent`, `stateProcess()`, and by extension `JobMaster`/`EngineConfig`, none of which this PR touches or needs). I was worried that under time pressure that risks producing a test that mostly exercises my own mock setup rather than real behavior, or one that's fragile against unrelated future changes to that machinery. Given the change is already provably behavior-preserving (verified against the real enum + a standalone comparison across all 13 states, per the PR description) and the project's own PR-scope guidance favors keeping hardening PRs minimal and focused, I'd rather not add a broad integration-style test in this PR. Happy to do it as a follow-up, or here too if you'd still prefer it despite the above — let me know. -- 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]
