[GitHub] [airflow] potiuk commented on a diff in pull request #28475: Fix flaky test_recover_from_resource_too_old exception

[GitBox <gi...@apache.org>]

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-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org