You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/12/19 19:39:26 UTC
[GitHub] [airflow] potiuk opened a new pull request, #28475: Fixed flaky Resource exception
potiuk opened a new pull request, #28475:
URL: https://github.com/apache/airflow/pull/28475
After #28047 the test_recover_from_resource_too_old started to fail in a flaky way. Turned out that - depend on some other test run the Singleton ResourceVersion could containt not one but two namespaces (including default namespace).
Also while fixing the tests it's been noticed that the test missed an assert - it did not assert that the Exception was in fact thrown, so the test could have succeeded even if the exception was not really thrown (there was assert in "except" clause but if the exception was not thrown, it would not have been called at all).
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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
[GitHub] [airflow] XD-DENG commented on a diff in pull request #28475: Fix flaky test_recover_from_resource_too_old exception
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on code in PR #28475:
URL: https://github.com/apache/airflow/pull/28475#discussion_r1052576617
##########
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:
The change in line 1258 is all good to me.
But is this change at line 1252 necessary? I don't fully get it yet . The watcher is already mocked and will fail
--
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
[GitHub] [airflow] potiuk merged pull request #28475: Fix flaky test_recover_from_resource_too_old exception
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #28475:
URL: https://github.com/apache/airflow/pull/28475
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #28475: Fix flaky test_recover_from_resource_too_old exception
Posted by 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