tbonelee commented on PR #5316: URL: https://github.com/apache/zeppelin/pull/5316#issuecomment-5012286775
A couple of points: **1. (should address) An unrelated existing test was removed** `start_removesContainer_evenWhenKillFailsDuringCleanup` was deleted in this PR. That test covered a distinct rollback-resilience case (`removeContainer` must still fire even when both `startContainer` and `killContainer` throw during cleanup), which is unrelated to the `isAlive` / `getErrorMessage` change here. The remaining `start_removesContainer_whenContainerPreparationFails` only exercises the kill-succeeds path, so this scenario is no longer covered. Looks like it may have been dropped accidentally while adding the new tests. Could you restore it? **2. (minor) Test name is broader than what it verifies** `isAlive_falseWhenContainerExitedOrOomKilled` only sets `running=false` and never touches `oomKilled`. Since `isAlive` only looks at `running`/`paused` and does not distinguish exited vs OOMKilled, the behavior is fine, but the name promises more coverage than exists. Renaming to something like `isAlive_falseWhenContainerNotRunning` would match the actual assertion. The OOMKilled path is already covered by `getErrorMessage_reportsOomKilled`. -- 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]
