potiuk commented on code in PR #28475:
URL: https://github.com/apache/airflow/pull/28475#discussion_r1052611348
##########
tests/executors/test_kubernetes_executor.py:
##########
@@ -1249,12 +1249,13 @@ def effect():
try:
# self.watcher._run() is mocked and return "500" as last
resource_version
self.watcher.run()
+ assert False, "Should have raised Exception"
Review Comment:
This is just consistency change. The author actually expected the sentinel
exception (and it's the one that is raised form the watcher to break the loop)
but the path when (potentially) no exception is thrown would not run this
assert:
```
except Exception as e:
assert e.args == ("sentinel",)
```
Sure, we know what happens inside the run method and by inspecting it, we
know it is not going to happen most likely - because exception is the only way
to get out of the loop. But ... this might change in the future.
Not likely and stupid example but if someonedoes fast "return 0" in the
`watcher.run()` - the test would have succeeded as well (no exception and
resource = 0, `assert e.args == ("sentinel", )` would have not been called).
Adding the assert simply makes absolutely sure that the exception was thrown
(because otherwise assert would fail). That's the usual pattern :)
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
