SEZ9 commented on PR #12156: URL: https://github.com/apache/seatunnel/pull/12156#issuecomment-5738675571
@CryoThrust thanks for turning both answers into tests at `2b0624991`. **F1 (single snapshot):** `testCancelJobCommitsToTheJobStatusSnapshotItValidated` is the right shape — a map that answers `PENDING` on the first read and `RUNNING` afterwards, with `set(jobId, CANCELED)` asserted and `set(jobId, CANCELING)` under `never()`, pins exactly the divergence F1 was about. Installing the mock over `runningJobStateIMap` the same way the class already does for `checkpointManager` is fine with me. I'll confirm resolution against the test code in the head diff itself rather than the described revert run. **F1 (null state):** agreed on keeping the NPE behaviour out of this PR — the cleared-entry path sat behind the same `isEndState()` guard before this change and `stopJob()` shares it, so normalising it here would widen the scope. One small ask: please add a one-line comment on `testCancelJobOnClearedJobStatusFailsAtTheEndStateGuard` stating that it pins pre-existing behaviour rather than a desired contract, so a future reader knows the test is a tripwire, not a design decision. Once that is pushed I'll re-check the head and close F1 from the diff. **F2:** unchanged by this round; it stays a separate follow-up item. **F3 / F4:** nothing in this round touches them; I'll re-verify both at the head as part of the final pass. Thanks also for tightening the comments on the two classification tests so the single-snapshot claim now lives only in the test that proves it. <!-- streview-comment:1153 --> -- 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]
